feat: added enqueue_frontend_assets_for_content static function #3634
Conversation
WalkthroughSimplified several hook callback signatures to accept only Changes
Sequence Diagram(s)sequenceDiagram
participant PostRenderer as Post Renderer
participant Init as Stackable_Init (static)
participant Hook as action: stackable/enqueue_scripts
participant Enqueuer as WP Enqueue System
Note over PostRenderer, Init: New flow for enqueuing assets based on post content
PostRenderer->>Init: call enqueue_frontend_assets_for_content(post_content)
Init->>Hook: do_action('stackable/enqueue_scripts', block_content)
Hook->>Init: stackable_load_*_frontend_script(block_content)
Init->>Enqueuer: wp_enqueue_script/style (tracks via self::$scripts_loaded)
Enqueuer-->>Init: assets registered/enqueued
Note right of Init: static properties track loaded state
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)src/compatibility/ewww.php (1)
🪛 PHPMD (2.15.0)src/compatibility/ewww.php9-9: Avoid unused parameters such as '$block_content'. (undefined) (UnusedFormalParameter) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/compatibility/ewww.php (1)
9-24: Unused parameter indicates potential optimization opportunity.The
$block_contentparameter is never used (correctly flagged by PHPMD). The function enqueues the polyfill unconditionally on the first block render. Consider either:
- Using
$block_contentto conditionally enqueue only when needed (e.g., checking for specific image-related classes)- Registering this on a different, non-block-specific hook if content inspection isn't necessary
Based on static analysis hints.
🧹 Nitpick comments (2)
src/lightbox/index.php (1)
35-35: Update the accepted argument count to match function signature.The
add_actioncall specifies 2 accepted arguments, but the function now only accepts 1 parameter. While PHP will ignore the extra argument, updating this to10, 1improves consistency.Apply this diff:
- add_action( 'stackable/enqueue_scripts', 'stackable_load_lightbox_frontend_script', 10, 2 ); + add_action( 'stackable/enqueue_scripts', 'stackable_load_lightbox_frontend_script', 10, 1 );src/compatibility/ewww.php (1)
29-29: Update the accepted argument count to match function signature.The
add_actioncall specifies 2 accepted arguments, but the function now only accepts 1 parameter. Update this to10, 1for consistency.Apply this diff:
- add_action( 'stackable/enqueue_scripts', 'stackable_load_image_optimizer_polyfill_frontend_script', 10, 2 ); + add_action( 'stackable/enqueue_scripts', 'stackable_load_image_optimizer_polyfill_frontend_script', 10, 1 );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/compatibility/ewww.php(1 hunks)src/init.php(9 hunks)src/lightbox/index.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/compatibility/ewww.php
9-9: Avoid unused parameters such as '$block_content'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 7.3 and WP 6.5.5
🔇 Additional comments (7)
src/lightbox/index.php (1)
13-13: LGTM! Function signature simplified appropriately.The removal of the unused
$blockparameter streamlines the function signature.src/init.php (6)
23-23: LGTM! Static properties are appropriate for tracking enqueue state.Converting these properties to static is correct since they track global state to prevent duplicate script loading across the entire request lifecycle.
Also applies to: 30-30
95-95: LGTM! Static method conversions enable flexible asset loading.Converting these methods to static is appropriate since they don't rely on instance state and allows them to be called directly (e.g., from the new
enqueue_frontend_assets_for_contentmethod).Also applies to: 258-267
84-86: LGTM! Static property references are consistent throughout.All references to
$scripts_loadedand$is_main_script_loadedhave been correctly updated to useself::$propertysyntax.Also applies to: 174-174, 189-190, 219-225, 234-236, 240-243, 277-281, 293-295
248-248: LGTM! Hook signature simplified consistently.The
do_actioncall now passes only$block_content, which aligns with the simplified function signatures in the other files. Note this is a breaking change for any external code that hooks intostackable/enqueue_scriptsexpecting two parameters.
301-301: Clarify the behavioral difference: full post content vs individual block content.This method calls
do_action( 'stackable/enqueue_scripts', $post_content )with the full post content, whereasload_frontend_scripts_conditionally(line 248) passes individual$block_contentto the same action.Handlers like
stackable_load_lightbox_frontend_scriptcheck for specific classes (e.g.,stk--has-lightbox) in the content. Passing the full post content means these checks scan a larger haystack, which still works but may have different performance characteristics.Is this behavioral difference intentional? Consider documenting when each method should be used and how handlers should accommodate both scenarios.
275-302: LGTM! New static method enables programmatic asset loading.The
enqueue_frontend_assets_for_contentmethod provides a clean static interface for enqueuing frontend assets based on post content. The implementation correctly:
- Checks for Stackable blocks before enqueuing
- Extracts unique block names to trigger per-block enqueue actions
- Updates static state to prevent duplicate loading
- Maintains consistency with existing asset loading patterns
| preg_match_all( '/<!-- wp:stackable\/([a-zA-Z_-]+)/', $post_content, $stackable_blocks ); | ||
| // Go through each unique block name. | ||
| foreach ( $stackable_blocks[1] as $_block_name ) { | ||
| // Clean up the block name, trailing "-" from the end since it may have "--" in the end if the post content is compressed. | ||
| $block_name = trim( $_block_name, '-' ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the regex pattern and trailing dash trimming logic.
The regex /<!-- wp:stackable\/([a-zA-Z_-]+)/ captures block names, and line 290 trims trailing dashes "since it may have '--' in the end if the post content is compressed." This seems like a workaround for a specific edge case.
Ensure that:
- The regex pattern correctly handles all valid Stackable block names
- The trimming logic doesn't inadvertently truncate valid block names with trailing hyphens
- Compressed content scenarios are properly tested
You can verify the block name extraction logic with this script:
🏁 Script executed:
#!/bin/bash
# Description: Find all Stackable block names in the codebase to verify the regex pattern covers them
# Search for block registrations to find all valid block names
rg -n --type=php -C2 'register_block_type.*stackable/' | head -50Length of output: 0
Now I need to search the codebase directly to find the actual block names and verify the regex pattern:Let me run shell scripts to search the codebase directly for block definitions and usage:
🏁 Script executed:
#!/bin/bash
# Find block HTML comments in the codebase to see actual block names
rg -n --type=php '<!-- wp:stackable' | head -30Length of output: 442
🏁 Script executed:
#!/bin/bash
# Look for where blocks might be defined or registered in PHP
rg -n --type=php "stackable/" | head -50Length of output: 3397
🏁 Script executed:
#!/bin/bash
# Search for any file that might define block names
find . -name "*.json" -o -name "block*.php" | head -20Length of output: 572
🏁 Script executed:
#!/bin/bash
# Look for the enqueue_frontend_assets_for_content method implementation context
rg -n -A20 "enqueue_frontend_assets_for_content" --type=php | head -60Length of output: 1463
Let me verify if there are any block names with underscores or uppercase letters:
🏁 Script executed:
#!/bin/bash
# Search all block.json files to find actual block names
find . -name "block.json" -type f -exec grep -h '"name"' {} \; | sort -u | head -50Length of output: 1576
🏁 Script executed:
#!/bin/bash
# Look for block names in stk-block-types.php to get a complete picture
rg -n "'name' => 'stackable/" src/stk-block-types.php | head -100Length of output: 1926
🏁 Script executed:
#!/bin/bash
# Check if there's any test data or documentation about block names
rg -n "stackable/[a-zA-Z0-9_-]+" --type=php src/ | grep -E "(stackable/[A-Z]|stackable/.*_)" | head -20Length of output: 2369
🏁 Script executed:
#!/bin/bash
# Look at the actual HTML comment context in tests or examples
rg -B2 -A2 "<!-- wp:stackable/" --type=php --type=json | head -80Length of output: 1280
Improve regex pattern to match WordPress block naming conventions.
The regex /<!-- wp:stackable\/([a-zA-Z_-]+)/ at line 286 includes uppercase letters [A-Z] and underscores [_] which don't appear in any actual Stackable block names and violate WordPress block naming standards. All 50+ Stackable blocks use only lowercase letters and hyphens (e.g., button-group, call-to-action, icon-button).
The trimming at line 290 is a valid workaround but indicates the regex lacks proper boundary detection. Recommended fix:
preg_match_all( '/<!-- wp:stackable\/([a-z0-9-]+)(?:\s|-->)/', $post_content, $stackable_blocks );This uses the correct character class [a-z0-9-] and adds an explicit boundary (?:\s|-->) to eliminate reliance on trim as a corrective measure. The trim call becomes redundant but can remain for safety.
🤖 Prompt for AI Agents
In src/init.php around lines 286 to 290, the current regex /<!--
wp:stackable\/([a-zA-Z_-]+)/ is too permissive (allows uppercase and
underscores) and relies on trimming; replace it with a pattern that matches
WordPress block names only (lowercase letters, numbers and hyphens) and enforces
a boundary (e.g., use a pattern equivalent to /<!--
wp:stackable\/([a-z0-9-]+)(?:\s|-->)/ ), and then remove the redundant trim call
(you may keep it as a harmless safety net).
2 participants
Summary by CodeRabbit