Media: Cover client-side big image scaling and orphaned files - #81061
Media: Cover client-side big image scaling and orphaned files#81061adamsilverstein wants to merge 4 commits into
Conversation
The client owns every derivative when generate_sub_sizes is false, so the server must not scale the upload. Use the same callback and priority core uses for this so the two registrations collapse into one instead of stacking.
The existing coverage used images below the threshold, so nothing caught the server scaling an upload the client also scales: the client's file lands as -scaled-1, sub-sizes inherit the number, and the server's full-size file is orphaned. Assert the exact file names and that the attachment leaves no untracked files behind.
|
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: 0 B Total Size: 7.81 MB |
|
This is already fixed in core, no backport required. |
andrewserong
left a comment
There was a problem hiding this comment.
Thanks for porting this to GB! Left a tiny nit-picky comment, feel free to ignore 😄
Tested in Playground and confirmed no unexpected -1 in the scaled filename in the media object returned by the REST API.
LGTM 👍
| /* | ||
| * Disable server-side "big image" downscaling; the client supplies | ||
| * its own scaled version through the sideload endpoint. Scaling | ||
| * here would create a conflicting `-scaled` file and orphan the | ||
| * full-size upload. Uses the same callback and priority as | ||
| * WP_REST_Attachments_Controller::create_item() so the two | ||
| * registrations collapse into one on WordPress versions that | ||
| * apply it themselves. | ||
| */ | ||
| add_filter( 'big_image_size_threshold', '__return_false', 100 ); |
There was a problem hiding this comment.
Totally optional: since we already use the exact same code in core, should we also re-use the same code comment?
| /* | |
| * Disable server-side "big image" downscaling; the client supplies | |
| * its own scaled version through the sideload endpoint. Scaling | |
| * here would create a conflicting `-scaled` file and orphan the | |
| * full-size upload. Uses the same callback and priority as | |
| * WP_REST_Attachments_Controller::create_item() so the two | |
| * registrations collapse into one on WordPress versions that | |
| * apply it themselves. | |
| */ | |
| add_filter( 'big_image_size_threshold', '__return_false', 100 ); | |
| // Disable server-side "big image" downscaling; the client supplies its | |
| // own scaled version via the sideload endpoint. Scaling here would | |
| // create a conflicting "-scaled" file and orphan the full-size upload. | |
| add_filter( 'big_image_size_threshold', '__return_false', 100 ); |
Related: https://core.trac.wordpress.org/ticket/65708
The Gutenberg plugin already suppresses server-side "big image" downscaling while the client owns the derivatives (added in #75817), but nothing tested it. Every existing test in this area uses an image below the threshold, so a regression here would have gone unnoticed - which is roughly what happened in core, where the same filter never made it into
WP_REST_Attachments_Controller. That gap is what @ianmjones ran into on the Trac ticket.When the server scales the upload as well, three things go wrong at once:
-scaled-1-150x150-1and so on)This PR adds coverage for all three, and aligns the filter callback with the one core is adding in WordPress/wordpress-develop#12689 (
__return_false, which is what thebig_image_size_thresholddocblock documents as the way to disable scaling). Same callback and priority means the plugin's registration and core's collapse into one instead of stacking.I verified the tests catch the bug: with the plugin's filter removed, the e2e assertion fails on
-scaled-1.jpegand the PHP tests fail onbig-photo-scaled.jpg/big-photo-150x150-1.jpg. With the filter in place they pass.Note that 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. The tests use a plain JPEG so they can run in Chromium, which has no native HEIC decode.
Why not a client-side change
The client is doing the right thing here.
generate_sub_sizes: falseis the contract that says the client owns every derivative including the scaled full-size copy, so the fix belongs on the server side of that contract. Core's PR does the same thing.Testing instructions
Automated:
To see them fail, comment out the
big_image_size_thresholdfilter inGutenberg_REST_Attachments_Controller::create_item()and re-run.Manual:
wp post meta get <id> _wp_attachment_metadata.fileshould be<name>-scaled.jpgandoriginal_imageshould be<name>.jpg, with sub-sizes named<name>-WxH.jpg. No-1anywhere.wp-content/uploads.Types of changes
big_image_size_thresholdfilter callback with core's.