Upload Media: stop propagating -scaled to sub-size filenames#78038
Conversation
When an image exceeded `big_image_size_threshold`, every sub-size inherited the `-scaled` suffix (e.g. `IMG_2300-scaled-150x150.jpg`) because `prepareItem()` queued a `ResizeCrop` step before `Upload`, making the main upload itself the `-scaled` file. The server-returned `attachment.filename` then carried `-scaled` and was used as the basename for every thumbnail and the redundant scaled sideload — producing duplicates like `-scaled-scaled.jpg` once collisions kicked in. Move threshold scaling to the existing scaled-sideload path in `generateThumbnails()`, matching the HEIC flow and core's `wp_create_image_subsizes()` behavior: the original is uploaded unchanged, sub-sizes are derived from its un-suffixed basename, and a single `-scaled` file is sideloaded with `image_size: 'scaled'`. Also defensively strip a stray `-scaled` from `attachment.filename` before deriving sub-size names, via a new `stripScaledSuffix()` util. Adds unit tests covering the helper, the thumbnail/scaled sideload naming, and the prepareItem operation list. Fixes #78037
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -24 B (0%) Total Size: 7.97 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in cbbd0b8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25924610900
|
It looks like you tested the latter when you had already uploaded |
| : null; | ||
| const file = subSizeFilename | ||
| ? renameFile( thumbnailSource, subSizeFilename ) | ||
| : thumbnailSource; |
There was a problem hiding this comment.
Not a fan of removing suffixes here. What if my original image was actually named foo-scaled.jpeg (see my other comment)?
In media-experiments I avoided modifying the filename client-side like this, and instead provided the original filename without suffix via the REST API. Could this be an option here too?
There was a problem hiding this comment.
Hmm, good point. We probably should have and use the original name here, instead of adding and then removing the -scaled suffix. let me take a look at the approach you took in the plugin and work on a cleaner approach here that still passes the tests.
There was a problem hiding this comment.
Good point. I went with the simpler fix and removed stripScaledSuffix() entirely (eca40fc).
With the prepareItem() change in place, the upload uses the original basename and the server is told generate_sub_sizes: false (which the controller translates to big_image_size_threshold => 0), so the server can't produce a -scaled filename in the normal flow. The strip was unnecessary defense in depth and would have mangled a legitimate foo-scaled.jpg upload.
generateThumbnails() now uses attachment.filename verbatim, and I added a regression test covering the foo-scaled.jpg case.
There was a problem hiding this comment.
I'm going to test this manually to make sure the names are still generated as expected.
Yes, for that one I uploaded an image named |
The defensive stripScaledSuffix() utility removes a trailing '-scaled' from any attachment.filename, but that incorrectly mangles legitimately named files like foo-scaled.jpg. With the prepareItem() fix already in place, the server is told generate_sub_sizes: false (which disables big_image_size_threshold), so attachment.filename is never -scaled in the normal flow — the strip is unnecessary defense in depth that introduces a real regression. Use the server-returned filename verbatim. Add a regression test that covers the foo-scaled.jpg case.
@swissspidy - I eliminated this bit. |
Replace the two ad-hoc sub-size naming tests with parameterized tables covering the cases swissspidy raised about filenames that already contain '-scaled', plus other edge cases: - foo-scaled.jpg (legitimate trailing -scaled suffix) - scaled.jpg (literal basename) - my-scaled-image.jpg (-scaled mid-name) - IMG_2300-1.jpg (server conflict-resolution suffix) - IMG-scaled-2.jpg (-scaled mid-name with conflict suffix) - image.with.dots.jpg (multi-dot) - FOO-SCALED.JPG (mixed case) - photo.jpeg (alternate extension) Both the thumbnail and the scaled-sideload paths are exercised. 198 tests pass (up from 186).
andrewserong
left a comment
There was a problem hiding this comment.
Thanks for the updates! This is still testing well for me, and nice job simplifying things 👍
Resolve CHANGELOG conflict by keeping the Unreleased bug-fix entry above the 0.31.0 release header added on trunk.
|
@andrewserong unfortunately this introduced a bug I missed. I have a fix and test coverage in place in a follow up and would appreciate a review - #78358 / #78359 |
|
Thanks for sorting out a quick fix! |
…78359) PR #78038 moved big-image threshold scaling out of prepareItem() into a post-upload sideload, so the create-item response now carries the URL of the unscaled original. The block stores that URL via onChange, and the scaled-sideload step's server-side update_attached_file() never makes its way back to the client — mediaFinalize() is typed as Promise<void> and discards the REST response. The block then persists the unscaled URL, so on the front end wp_calculate_image_srcset() compares the block's src (IMG_0441.jpg) to each size file (IMG_0441-225x300.jpg, IMG_0441-768x1024.jpg, …) via str_contains() and matches none of them. $src_matched stays false and the function returns false — no srcset attribute is emitted. Make mediaFinalize() return the REST attachment (transformed so source_url maps to url), and have finalizeItem() forward it to finishOperation. The reducer merges it into item.attachment, and the next processItem pass fires onChange with the scaled URL, which is what the block needs to land a src that matches a known size. Adds a unit test for the new mediaFinalize return value, a regression test for finalizeItem forwarding the attachment to the reducer, and an e2e test that publishes an oversized CSM upload and asserts the rendered <img> ships with a srcset of at least two width descriptors. Fixes #78358
…ision Reflect four client-side media PRs merged after these docs were last revised, keeping the architecture and how-to guides in sync with current behavior: - #76707: document the disabled libvips operation cache (Cache.max(0)) and the 50-operation worker recycling that bound WASM linear-memory growth, plus the globalThis.__vipsDebug hook for capturing the suppressed vips output. - #74903: document the sideload endpoint's image-dimension validation and its 400 error responses (rest_upload_dimension_mismatch and friends). - #78359: note that the finalize endpoint returns the refreshed attachment, which the editor uses to keep the block URL (and front-end srcset) in sync. - #78038: clarify that sub-size filenames derive from the original basename and only the scaled full-size copy carries the -scaled suffix.






What
Fixes #78037 — when client-side media processing scales an image down (because it exceeds
big_image_size_threshold), every sub-size filename inherits the-scaledsuffix instead of just the scaled full-size copy.Before
After
This matches WordPress core's
wp_create_image_subsizes()naming, where only the scaled full-size copy carries-scaledand the original is preserved alongside it.Why
prepareItem()queued aResizeCropstep beforeUpload, so the main upload was already a-scaledfile. The server then returnedattachment.filename=IMG_2300-scaled.jpg, whichgenerateThumbnails()used as the basename for every thumbnail (producing-scaled-150x150.jpgetc.) and re-ran the scaled-sideload code path (producing a redundant-scaled-scaled.jpg).How
ResizeCropfromprepareItem(). The main upload now carries the original basename, matching the existing HEIC flow.generateThumbnails()becomes the single source of-scaledand runs only when the image actually exceeds the threshold.stripScaledSuffix()util and apply it ingenerateThumbnails()as defense-in-depth, so any stray-scaledinattachment.filename(e.g., from a server that scaled despitegenerate_sub_sizes: false) is stripped before sub-size derivation.Testing
stripScaledSuffixunit tests.generateThumbnailstests asserting:attachment.filenamecarries-scaledvipsResizeImageproduces a single-scaled.jpg(not-scaled-scaled.jpg)attachment.filenameunchangedprepareItemtest asserting noResizeCropis queued whenbigImageSizeThresholdis set.@wordpress/upload-mediatests pass.Test plan
big_image_size_threshold(default 2560px) with client-side media processing enabled. Confirm filenames in the uploads dir match the "After" pattern: original preserved, single-scaled.jpg, sub-sizes without-scaled.-scaledfiles are produced and sub-sizes are named normally.-scaled.jpgplus clean sub-size names.