Media: Skip server-side scaling during client-side media processing - #12689
Media: Skip server-side scaling during client-side media processing#12689itzmekhokan wants to merge 5 commits into
Conversation
When an attachment is uploaded with `generate_sub_sizes` set to false the client generates all derivatives itself, including the scaled full-size image it later supplies through the sideload endpoint. The upload handler already suppressed thumbnail generation and EXIF rotation in this case, but not the "big image" downscaling gated by the `big_image_size_threshold` filter. As a result a large client-converted image (for example a HEIC converted to JPEG in the browser) was still scaled server-side to a `-scaled` file, which became the attached file while the untouched upload was recorded as `original_image`. The client's later scaled sideload then collided with that `-scaled` file and was renamed `-scaled-1`, the thumbnails inherited the numbered name, and the server-generated full-size file was left orphaned on disk. Disable `big_image_size_threshold` alongside the existing client-side processing filters so the uploaded full-size image is stored untouched and the client's scaled sideload records it as `original_image` as intended. Fixes #65708.
There was a problem hiding this comment.
Pull request overview
This PR updates the REST attachments upload flow to better support client-side media processing when generate_sub_sizes=false, ensuring the server does not also perform “big image” downscaling (which can create -scaled filename collisions and leave stray files on disk).
Changes:
- Disable server-side big-image downscaling (
big_image_size_threshold) during REST uploads whengenerate_sub_sizesis explicitlyfalse. - Ensure the new filter is removed alongside the existing client-side-processing-related filters.
- Add a PHPUnit REST controller test that reproduces the
-scaledcollision scenario and asserts the corrected behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/phpunit/tests/rest-api/rest-attachments-controller.php | Adds coverage verifying big-image scaling is skipped on upload when the client owns derivative generation, preventing -scaled collisions. |
| src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php | Disables big_image_size_threshold during create_item() when generate_sub_sizes=false, and removes the filter during cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Thanks for the PR @itzmekhokan - I'll take a look. |
There was a problem hiding this comment.
Thanks for picking this up @itzmekhokan - this is the right fix and I verified it locally. Your test fails on trunk ('33772-scaled.jpg' does not contain "-scaled") and passes with the change, and the full WP_Test_REST_Attachments_Controller suite stays green.
Worth noting this finishes an earlier backport rather than changing behavior. WordPress/gutenberg#75817 made four changes to the plugin's controller; #11015 brought over three of them - the scaled enum value, the filter_wp_unique_filename regex, and the sideload_item branch - but not the create_item filter this PR adds. So Core got the half that accepts a client-supplied -scaled file without the half that stops the server making its own.
One suggestion on the test. The ticket has three symptoms and this covers one. The other two are the sub-sizes inheriting the numeric suffix (-150x150-1) and the server's full-size file being orphaned, which is what survives "Delete Permanently". I wrote a second test for both - it fails on trunk with 'big-photo-150x150-1.jpg' and passes with your fix. The equivalent is in WordPress/gutenberg#81061 if you want to lift it, or I am happy to push it here. Which do you prefer?
Also for anyone testing: this is not HEIC specific. Any image over the threshold hits it once the client owns the sub-sizes, so a large JPEG reproduces it and is easier to test with.
|
Adding testing steps for anyone picking this up. Note that this one needs client-side media processing to be active, which means Chrome 137+. Automated, fails on trunk and passes with the patch: Manually, with the site on https or localhost so client-side processing kicks in:
Step 4 is the orphan @ianmjones reported. Step 3 is worth eyeballing in the Media Library too, since the The ticket describes this with a HEIC upload but it is not HEIC specific - any image over the threshold hits it once the client owns the sub-sizes, so a large JPEG is easier to test with. |
…-side big image scaling. Walks the full client-side flow for an image over the "big image" threshold: upload with generate_sub_sizes false, sideload the thumbnail and the scaled full-size image, then finalize. Asserts that only the files the metadata tracks are written and that deleting the attachment removes all of them. On trunk the server's own scaling renames the client's sideloads to "-150x150-1" and "-scaled-1" and leaves its full-size file orphaned after "Delete Permanently". See #65708.
|
Thanks for the review @adamsilverstein - I have lifted your test, adapted to the Core test class ( I checked each symptom lands on its own by temporarily relaxing the earlier assertions: on trunk the thumbnail comes back as Good catch on the incomplete backport from #11015 - that explains why the sideload half was already in place. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
When an attachment is uploaded with
generate_sub_sizesset to false, the client generates all derivatives itself — including the scaled full-size image it later supplies through the sideload endpoint.create_item()already suppressed thumbnail generation and EXIF rotation in this case, but not the "big image" downscaling gated by thebig_image_size_thresholdfilter.What the problem was:
-scaledfile, which became the attached file while the untouched upload was recorded asoriginal_image.scaledsideload then collided with that-scaledfile and was renamed-scaled-1; the thumbnails inherited the numbered name.What the fix does:
big_image_size_thresholdalongside the existing client-side processing filters increate_item(), and removes it again inremove_client_side_media_processing_filters().original_imageand keeps the plain-scaledname.Approach and why:
generate_sub_sizes = falseis the contract that the client owns all derivative generation, which includes the scaled full-size image. Server-side scaling in that mode duplicates the client's work and causes the collision. Suppressing it is the minimal, consistent change — it sits directly beside the three filters already applied for the same purpose.Trac ticket: https://core.trac.wordpress.org/ticket/65708
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.8
Used for: Ticket analysis, code implementation, and tests. All changes were reviewed and validated by me.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.
Proposed commit message