Boost: Fix is_opened_script() regex to interpolate ignore attribute#47847
Boost: Fix is_opened_script() regex to interpolate ignore attribute#47847
Conversation
The regex in is_opened_script() had literal %s placeholders instead of being interpolated via sprintf(), making the negative lookahead dead code. This meant ignored scripts were counted as open scripts, which could cause the output buffer to hold content unnecessarily.
github-actions
bot
added
[Boost Feature] Defer JS
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
There was a problem hiding this comment.
Pull request overview
Fixes a bug in Jetpack Boost’s Render Blocking JS optimization where is_opened_script() attempted to exclude “ignored” scripts but used a regex with uninterpolated %s placeholders, making the negative lookahead ineffective.
Changes:
- Interpolate the ignore-attribute/value into the
is_opened_script()regex viasprintf()+preg_quote(). - Add a Boost changelog fragment describing the fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| projects/plugins/boost/app/modules/optimizations/render-blocking-js/class-render-blocking-js.php | Fixes the ignore-attribute negative lookahead in is_opened_script() by properly interpolating the regex. |
| projects/plugins/boost/changelog/boost-versioning-issues | Adds a changelog entry for the Render Blocking JS fix. |
| public function is_opened_script( $buffer ) { | ||
| $opening_tags_count = preg_match_all( '~<\s*script(?![^>]*%s="%s")([^>]*)>~', $buffer ); | ||
| $opening_tags_count = preg_match_all( | ||
| sprintf( '~<\s*script(?![^>]*%s="%s")([^>]*)>~', preg_quote( $this->ignore_attribute, '~' ), preg_quote( $this->ignore_value, '~' ) ), | ||
| $buffer | ||
| ); |
There was a problem hiding this comment.
There are no PHPUnit tests covering the Render Blocking JS optimization or is_opened_script() behavior (including the ignored-script negative lookahead). Since this regex is easy to regress and directly affects output buffering behavior, add a focused unit test to assert that ignored scripts are not counted as “opened” (and that non-ignored partial scripts are).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Replace the Copilot-suggested regex with the established pattern from get_script_tags() — using named group + backreference for quote matching, requiring =value to be present. The Copilot version made =value optional, which would have excluded bare attributes. Add 11 unit tests for is_opened_script() covering empty buffers, matched/unmatched tags, ignored scripts with double/single/no quotes, wrong values, bare attributes, and mixed scenarios.
ReflectionProperty::setAccessible() has been a no-op since PHP 8.1 and is deprecated in PHP 8.5. Private properties are accessible via reflection without it.
setAccessible() is required for private property access before PHP 8.1, a no-op from 8.1-8.4, and deprecated in 8.5. Guard the calls so they only run where needed.
See #47744
Proposed changes
is_opened_script()regex in the Render Blocking JS module — the pattern had literal%splaceholders instead of being interpolated viasprintf(), making the negative lookahead for ignored scripts dead code.Other information
Does this pull request change what data or activity we track or use?
No.
Testing instructions
sprintf()+preg_quote()pattern used inget_script_tags()on line 217 of the same file.jp test php plugins/boost— all 102 tests pass.