Repository navigation
Don't take a data: placeholder in a lazy-load attribute as the image URL - #1190
Merged
Merged
Conversation
…mage URL The image regex matches the `src=` at the end of any `*-src=` attribute, so in `<img data-src="data:image/gif;..." src="photo.jpg">` it captured the placeholder. The tag replacer then ran against `data:image/gif`: no profiler srcset or dimensions, and a data-opt-id shared by every image with that placeholder. A lazy-load attribute whose value starts with data: is now skipped, so the real src is used. A data: value in a plain src is still matched, so images owned by another lazy-load script are handled as before. Fixes #1186 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WPwFxvA5VPq8sd4EzsNFFA
Collaborator
pirate-bot
requested changes
Oct 20, 2026
Collaborator
There was a problem hiding this comment.
🟡 Changes requested
The parser fix works, but add tests for the tag shapes it now changes, including the new lazy-load result.
Validation details
- Files reviewed: 3/3 changed files.
- An isolated run of the base and HEAD regex used the real character class
cp /repo/inc/config.php /repo/inc/app_replacer.php /repo/inc/manager.php /workspace/checks/LC1/; cp /workspace/review/re…printedDIFFER=1,2,3,4,5,6,7,9,11(run R3). HEAD never took the placeholder and never matched across tags. - On PHP 8.3 with PHPUnit 9.6, two of the three new tests fail on
pr-baseand all pass on HEAD. Test_ReplacerandTest_Dimension_Replacementpass on HEAD.Test_Lazyload::test_dam_lazyloadingfails on HEAD and onpr-basealike in the sandbox.- A lazy-load probe showed no duplicate attributes and one
<noscript>per tag `cat > /workspace/checks/RUN1b/Integration_Lazy_Example.php <<'EOF'
Untested areas
- The full suite and PHP 7.4 were not run. - No browser check covered a tag that keeps `data-src="data:…"` next to `data-opt-src`. - Profiler `srcset` and crop handling for the fixed tag shape were not asserted.Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WPwFxvA5VPq8sd4EzsNFFA
pirate-bot
approved these changes
Oct 20, 2026
Collaborator
There was a problem hiding this comment.
🟢 Approved
The PR passes. The new tests cover the requested tag shapes and fail without the parser fix.
Validation details
- Files reviewed: 4/4 changed files. Only the tests changed since the prior review.
- With the pr-base parser, the new lazy-load test fails
set -o pipefail; cd /workspace/checks/RUN2a/base && timeout 600 vendor/bin/phpunit --do-not-cache-result --filter 'Test_…printedFailures: 1(run R5). - On HEAD, the same test passes
set -o pipefail; cd /repo && timeout 600 vendor/bin/phpunit --do-not-cache-result --filter 'Test_Lazyload::test_lazyload…printedOK (1 test, 6 assertions)(run R7). - With the pr-base parser, the new replacer test fails
set -o pipefail; cd /workspace/checks/RUN2a/base && timeout 600 vendor/bin/phpunit --do-not-cache-result --filter 'Test_…printedFailures: 1(run R6). - On HEAD,
Test_Lazyload,Test_ReplacerandTest_Dimension_Replacementfail onlytest_dam_lazyloadingset -o pipefail; cd /repo; timeout 900 vendor/bin/phpunit --do-not-cache-result --filter 'Test_(Lazyload|Replacer|Dimens…(run R4, exit 0). - The parser and security checks come from run code-review-agent_6ad78946a17627.14574941 and were not rerun.
Untested areas
- The suite did not run on PHP 7.4.
- No live site with the page profiler was checked.
- This run did not check
test_dam_lazyloadingon pr-base.
🤖 Automated review · run code-review-agent_6ad7909ba62382.76864001.
🤖 Review agent — review posted ✅ on 6f5f8b14 · approved · 0 findings · 9 min
Run code-review-agent_6ad7909ba62382.76864001 · trail
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WPwFxvA5VPq8sd4EzsNFFA
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Marius · project thread
All Submissions:
Changes proposed in this Pull Request:
Before: in
'<img data-src="data:image/gif;base64,…" src="photo.jpg">', the image parser tookdata:image/gifas the image URL. The tag replacer then worked on the placeholder: the image got no profiler srcset or dimensions, and itsdata-opt-idcame from the placeholder, so every image with that placeholder shared one ID.After: the parser skips a
data:value in a lazy-load attribute and uses the realsrc. The image gets its dimensions, srcset and its owndata-opt-id.Why it happened: by default the attribute alternation in
parse_images_from_html()is justsrc, and the\s?before it is optional. So it also matches thesrc=at the end ofdata-src=,data-lazy-src=and so on, and takes whichever comes first.How: the regex now has two branches. A plain
src=is matched as before, including adata:value. Any other*src=attribute (and theoptml_possible_src_attributesones) is skipped when its value starts withdata:(case-insensitive).Why a plain
data:src is still matched: when another lazy-load script owns the image (src="data:…" data-lazy-src="photo.jpg"), Optimole currently leaves the tag alone and only rewrites URLs. Skippingdata:everywhere would make it process those tags and add a second<noscript>.Test_Lazyload::test_check_with_no_scriptandtest_check_with_multiple_images_in_no_scriptfailed that way with the broader version, so this PR keeps that behaviour.What changes for other markup: a tag whose only source attributes are
data:lazy attributes and nosrc(for example'<img data-src="data:…">') is no longer picked up by the tag replacer. Before, it was processed against the placeholder URL, which did nothing useful.Closes #1186.
How to test the changes in this Pull Request:
Test_Replacer::test_parse_images_skips_data_uri_in_lazy_attribute: double quotes, single quotes,data-lazy-src,srcfirst, and upper-caseDATA-SRC/DATA:all give the real URL and the whole tag.Test_Replacer::test_parse_images_keeps_data_uri_src:src="data:…" data-lazy-src="…"still givesdata:image/gif.Test_Dimension_Replacement::test_dimensions_injected_when_data_src_placeholder_comes_first: with profiler data seeded for the real URL, the tag getswidth="1920",height="1080"and the real URL'sdata-opt-id. Ondevelopmentit getsdata-opt-id=771715633, which comes from the placeholder, and no dimensions.Other information:
development, 2 of 3 fail as expected (the data-uri src test passes on both, as intended)srcbranchtest_parse_images_keeps_data_uri_srcfailsinc/manager.phpNot proven: the full suite on PHP 7.4 (my local 7.4 container fails at bootstrap on
developmenttoo), and a live site with the page profiler, since that needs the Optimole service. The Codex review step was replaced by a self-review because the Codex CLI isn't available in this environment.Generated by Claude Code