fix: prevent ToWebp filename collisions between same-basename images - #3285
Open
si-kui-a wants to merge 8 commits into
Open
fix: prevent ToWebp filename collisions between same-basename images#3285si-kui-a wants to merge 8 commits into
si-kui-a wants to merge 8 commits into
Conversation
…images ToWebp::filename() ignored its $src_extension parameter entirely, always returning "$src_filename.webp" regardless of the source format. Two source images sharing a basename but differing only in extension (image0001.jpg vs image0001.jpeg, or "velo 3.jpg" vs "velo 3.png") therefore resolved to the identical destination path. ImageHelper::_operate() treats an existing destination file as a cache hit and returns early without calling run(), so whichever image converted first "won" and the second image's towebp filter call silently served the first image's webp content instead of converting its own -- a data-corruption bug, not cosmetic. Reported independently by two users in timber#2850, with the exact same collision mechanism in both reports. Fixed by folding the source extension into the generated filename (e.g. image0001-jpg.webp / image0001-jpeg.webp), so distinct source formats can no longer collide. Left the already-webp case (ToWebp converting a .webp file) untouched -- filename() still returns the bare name there, preserving the existing self-collision-as-no-op behavior for that case (verified via ImageHelper::_operate()'s destination-already-exists branch: since a webp source's own path already equals its own would-be destination, this treats re-processing an already-webp file as a no-op, avoiding a wasted GD re-encode/write on every unrelated conversion run against a directory that also contains already-webp images). Updated the six existing filename assertions in ToWebpTest.php to match the new naming (all confirmed via reading ImageHelper::analyze_url()'s pathinfo()-based extension parsing and ImageHelper::get_sideloaded_file_loc() for the two sideloaded-image cases), and added testCollidingBasenamesProduceDistinctWebp(), reproducing the exact scenario from timber#2850 using copyImageToUploads()'s $destName parameter to give two different fixture images the same basename. Fixes timber#2850. No PHP/Composer toolchain is available in this environment (same constraint disclosed on the earlier Yoast SEO PR from this session), so the test suite was not executed locally -- every filename change was hand-traced against ImageHelper::_operate()/analyze_url()/get_sideloaded_file_loc() instead. Disclosed in the PR body's Test instructions per CONTRIBUTING.md; CI will run the full suite. Also noted for maintainer awareness (not fixed in this PR to keep it scoped to the reported issue, per CONTRIBUTING.md's "Small > big" guidance): ToJpg::filename() in src/Image/Operation/ToJpg.php has the exact same ignored-$src_extension pattern and is likely affected by the identical collision bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HG5wiw3Z8QdjNWb5bFDLiA
Member
|
This is a duplicate of #2556. |
ToWebp::filename() now folds the source extension into the generated name (pic.png -> pic-png.webp) to avoid colliding with a different-format source of the same basename. delete_generated_files() didn't know about that name yet, so the derivative became an orphaned file nothing ever removed once its source attachment was deleted. The added glob pattern uses the deleted source's own extension literally (no wildcard), since a wildcard here could delete an unrelated real file sharing the same basename with a different suffix.
The unconditional rename (fold the source extension into the generated name) fixes the pseudo-duplicate collision from timber#2850, but changes the generated filename for every non-webp source going forward, not just colliding ones - there's no way to tell ahead of time which will collide. timber#2876 proposed exactly this unconditional version in Jan 2024 and it was turned down: same regeneration/orphan-file cost, deferred to the bigger image-handling rework tracked in timber#2866. Gate it behind timber/image/collision_safe_filenames instead, off by default: existing sites keep today's behavior (bug included) until they opt in; new projects can enable it from the start with no migration cost. Matches the filter-gated approach gchtr sketched on timber#2876 in Jan 2024 (minus the wp_options auto-migration/admin-notice part of that sketch, which is a bigger, separate piece of work if wanted). Reverted the everyday conversion tests back to asserting the bare-name (default) output, added a test pinning that the collision still happens by default on purpose, and moved the fixed-behavior assertion into a new test that enables the filter. Updated the delete_generated_files() cleanup test to enable the filter too, since that derivative only exists when it's on.
…h the filter on ImageHelper::get_url_components() hands filename() an empty $src_extension when the source path has no extension at all (falls back to "" - see timber#2773 / commit 028f6ac). Without this guard, enabling timber/image/collision_safe_filenames would fold that empty string in literally, producing "name-.ext" instead of falling back to the bare name the way the already-target-format case does. Not a hypothetical: ImageHelper has its own prior fix for exactly this extensionless-source case, in the same file this PR touches.
Direct call rather than through Timber::compile_string() like the rest of this file: an extensionless source cannot be round-tripped through the full towebp filter, since ToWebp::run() derives its GD decoder from wp_check_filetype($load_filename), which needs a real extension - the pipeline would fail before reaching the point this test needs to check.
…ame() already uses
Resize::filename() in the same Image/Operation directory already treats a
falsy $src_extension as "nothing to append" (`if ($src_extension) { ... }`)
rather than a real value. Switch from the stricter === "" comparison to
match that existing sibling-class idiom instead of inventing a new one.
…nless-source test comment
The empty-extension guard sits before the apply_filters() check, so it returns unconditionally regardless of timber/image/collision_safe_filenames. Toggling the filter in this test asserted nothing - it would pass identically with the filter left at its default. Dropped the dead setup call and renamed the test to stop implying the filter state is what makes this case work.
Author
|
Thanks for flagging that, @gchtr — you're right the version you saw here on 2026-09-03 was shaped just like #2556, hitting the same objection you've raised before (on #2876 in 2024, and on #2556 itself in Feb 2025). Since then this has been reworked to be filter-gated off by default — an attempt at the basic PR you asked on #2556 whether anyone had capacity for — plus a fix for an extensionless-source edge case I'd missed initially. I've updated the PR description above to match the current diff rather than the original commit; details are there rather than repeated here, including a filter-naming question for you. Would appreciate a re-look when you get a chance. |
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
2 participants
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.
Related:
Update (2026-09-09): originally submitted as an unconditional rename (first commit,
241c8c2a). @gchtr pointed out this was effectively the same shape as #2556 — thanks for catching that. #2556 has been stuck on exactly this objection since 2022 (raised again on #2876 in 2024, and on #2556 itself in Feb 2025, where a concrete opt-in filter —timber/image/advanced_file_names— was sketched as the way forward). This PR has since been reworked to be filter-gated per that guidance, and the sections below are rewritten to match the current diff rather than the original commit.Issue
ToWebp::filename()ignores its$src_extensionparameter entirely, always returning"$src_filename.webp". Two source images that share a basename but differ only in extension —image0001.jpgvsimage0001.jpeg, orvelo 3.jpgvsvelo 3.png(both reported independently in #2850) — therefore resolve to the exact same destination filename.ImageHelper::_operate()treats an existing destination file as a cache hit and returns the existing URL without callingrun()again. So whichever source image gets converted first "wins," and every subsequent same-basename image'stowebpfilter call silently serves the first image's webp content instead of converting its own. This is a data-corruption bug (wrong image served), not a cosmetic one.Solution
ToWebp::filename()folds the source extension into the generated name (image0001-jpg.webp/image0001-jpeg.webp), but only when a site opts in via thetimber/image/collision_safe_filenamesfilter — off by default (see Impact below for why).Two cases keep the bare name regardless of the filter:
ImageHelper::_operate()'s destination-already-exists branch: for an already-webp source, the would-be destination path is identical to the source's own path, so this is already treated as an implicit no-op.testWEBPtoWEBP(unmodified) still covers this.ImageHelper::get_url_components()falls back to an empty$src_extensionwhen a source path has no extension at all (see [BUG] Missing extension for image #2773 / commit028f6ac0'sisset($parts['extension']) ? ... : ''fallback). The siblingResize::filename()already treats a falsy$src_extensionas nothing to append rather than a real value, so this PR does the same, avoiding a strayname-.webp.Impact
Off by default: existing sites keep today's behavior (collision bug included) until they explicitly opt in —
— so there's no forced regeneration or URL churn for sites that don't enable it. Sites that do opt in get the new naming for every non-webp source going forward (e.g.
flag.png→flag-png.webpinstead offlag.webp), not just the colliding case, since there's no way to distinguish "will this actually collide" ahead of time. Existing already-convertedname.webpfiles are untouched either way (nothing deletes them).Also noted for maintainer awareness, not fixed in this PR to keep the change scoped to the reported issue:
ToJpg::filename()insrc/Image/Operation/ToJpg.phphas the exact same pattern and is very likely affected by the identical collision bug — see the sibling #3286.Usage Changes
No change for sites that don't opt in. Sites that enable
timber/image/collision_safe_filenames: generatedtowebpfilenames for non-webp sources now include the source extension (see Impact above). No API/signature changes.Considerations
timber/image/collision_safe_filenames, chosen before I'd found @gchtr'stimber/image/advanced_file_namessuggestion on imagehelper - filename (towebp, tojpg) #2556. Happy to rename to match exactly if that's still preferred.-rather than.(image0001-jpg.webprather thanimage0001.jpg.webp), to avoid the output looking like a legitimate double extension. No strong opinion if maintainers prefer the dot form.Testing
Updated the six existing filename-based assertions in
tests/Image/Operation/ToWebpTest.phpto expect the new naming, reverted them back for the default (filter-off) case, and added a test pinning the default-off collision as intentional. AddedtestCollidingBasenamesProduceDistinctWebpWhenFilterEnabled(), reproducing the exact scenario from #2850. AddedtestFilenameKeepsBareNameForExtensionlessSource()for the extensionless-source guard above (direct call toToWebp::filename()rather than throughTimber::compile_string()— an extensionless source can't be round-tripped through the full pipeline, sinceToWebp::run()derives its GD decoder fromwp_check_filetype($load_filename), which needs a real extension).Disclosure: earlier revisions of this PR were hand-traced only (no PHP toolchain in the environment they were prepared in). Since then, actually ran this branch's tests: PHP 8.2 in a container (matching CI's baseline matrix entry), GD with WebP support, and Mantle's SQLite-backed WordPress test scaffolding (no MySQL needed, same as what
phpunit.xml/CI use).ToWebpTestalone: 11/11 passed, 27 assertions. Fullimagetestsuite (all oftests/Image/, covering the untouched sibling operations too): 178 passed / 3 skipped (Imagick-only tests — Imagick isn't installed in this container, GD is) / 0 failed. CI will still run the full matrix (multiple PHP/WP versions) on top of this.Co-authored with Claude Code (Anthropic); reviewed by @si-kui-a before submission.