Fix #1235: stop overriding core's resize opt-out on client side uploads - #1239
Conversation
WordPress 7.1 switches its own downscaling off while the browser handles an upload, because the browser supplies the scaled file itself. Imagify's filter ignored the incoming value and always won, so the server produced a second scaled file that nothing referenced and `original_image` ended up pointing at it instead of the real upload. Hand a false threshold straight back, and remember the attachment so the later asynchronous optimization does not shrink the original the browser left intact.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
🟢 Coverage 69.57% diff coverage
Metric Results Coverage variation Report missing for cd27ae41 Diff coverage ✅ 69.57% diff coverage (50.00%) Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (cd27ae4) Report Missing Report Missing Report Missing Head commit (9800a9d) 20333 1556 7.65% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#1239) 23 16 69.57% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Check the flag before delegating to the parent, so a media the browser already scaled is refused without the rest of the work, and the branch can be exercised directly.
Honemo
left a comment
Honemo
left a comment
There was a problem hiding this comment.
Lead Review — CHANGES REQUESTED
Nice fix overall — the root-cause writeup and the reasoning around the transient flag are clear and well tested manually. Two things to address before merge:
- HIGH —
CanResizeTestdoesn't actually exercise the new guard;is_valid()being stubbedfalsemakes it pass regardless of whether the fix exists (see inline comment). - MEDIUM — the new
imagify_client_side_scaled_{attachment_id}transient (set inContext\WP::flag_client_side_scaling()) isn't registered inclasses/Tools/InternalStateList.php::get_locked_transient_patterns(). That file's own docblock asks for new transients to be added there as the single source of truth, soResetInternalState::reset()and uninstall won't clean this one up — it'll just sit until its 1-hour TTL expires. Could you add_transient_imagify_client_side_scaled_%(and the matching_transient_timeout_pattern) there?
Everything else checks out: PHPCS clean, full suite green (498 tests), all callers of the renamed filter callback updated consistently, and the scope correctly limited to Process\WP.
One optional thought for later: the transient is never cleared after it's read, only left to expire — worth a short comment noting that tradeoff, or a delete_transient() call if it doesn't conflict with checks across multiple sizes.
| */ | ||
| $process = Mockery::mock( WP::class )->makePartial(); | ||
| $process->shouldReceive( 'get_media' )->andReturn( $media ); | ||
| $process->shouldReceive( 'is_valid' )->andReturn( false ); |
There was a problem hiding this comment.
[HIGH] This test always stubs is_valid() to return false, so can_resize() short-circuits before it ever reaches the new client-side-scaling guard. I confirmed this by removing the guard entirely and re-running the suite — all 3 tests still passed, so right now this file wouldn't catch a regression that deletes the fix.
Suggest stubbing is_valid() to true (plus whatever else parent::can_resize() needs to reach the real decision), then adding a "flag not set → true" case alongside the existing "flag set → false" one. That way the test actually proves the guard is doing the work.
The can_resize test satisfied everything the parent checks except the guard, so it passed whether or not the guard existed. Every condition is now stubbed to answer true, which makes the guard the only thing that can return false, and a case covers the media the browser did not scale. Register the new transient in InternalStateList so a reset and uninstall clear it, and derive the reset test's query count from that list rather than repeating the number. Note on the flag why it is left to expire instead of deleted.
|
Both fixed — you were right that the test was vacuous: I deleted the guard entirely and all 3 tests still passed, so it's rewritten to stub everything the parent checks as true, making the guard the only thing that can return false (verified it now fails when the guard is removed), plus a case for the media the browser didn't scale. The transient is registered in |
Treating every false on the filter as "already scaled" changed behaviour for any site where a third party returns false for its own reasons: WordPress would skip the downscale because it was told to, and Imagify would skip its own because it believed the work was done, leaving an image nobody resized and no scaled file anywhere. Take the state from where WordPress declares it instead. rest_after_insert_attachment runs just before the metadata is generated and carries generate_sub_sizes, which is false exactly when the browser owns the sub sizes. A false from any other source is overridden with Imagify's value, as it was before, so nothing changes outside the 7.1 browser flow.
The callback only took four arguments to reach the attachment ID, leaving two declared and never read. WordPress notes the upload just before the metadata is generated, in the same request as the filter, so a property carries that instead and the callback is back to the single argument it uses. The transient stays: the optimization that consults it runs in a later request. This also removes the need for the argument count on both registrations.
2 participants
Description
Fixes #1235
WordPress 7.1 lets the browser process an upload, and when it does, it switches its own "big image" downscaling off because the browser already supplies the scaled file. Imagify's filter ignored that and always imposed its own value, so the server scaled the image a second time. Users were left with a scaled file and a Next-Gen companion that nothing referenced, and WordPress pointed
original_imageat that stray file instead of the real upload.After this change the upload produces a single scaled file, sub sizes keep their normal names, and
original_imagepoints at the untouched upload again.Type of change
Detailed scenario
What was tested
Everything below was done by hand on a real WordPress 7.1-RC3-63235 build, in Chrome 151, with Imagify connected, auto optimization on and "Resize larger images" on at 2560. A small mu-plugin was used during testing to log what
big_image_size_thresholdreturned and when the metadata was written. It is not part of this PR.Reproduced the bug first. Uploaded a 4000x3000 JPEG from the block editor on an unpatched build and confirmed the reported symptoms on disk: two scaled files (
big-4000px-scaled.jpgandbig-4000px-scaled-1.jpg), 673KB of them referenced by nothing, every sub size written with a-1suffix, andoriginal_imagenaming the stray file rather than the upload.Verified the fix on the same flow. Uploaded a 3800x2500 JPEG from the block editor and inspected the uploads folder and the attachment metadata:
-scaled.jpgand-scaled-1.jpg-scaled.jpg-1024x674-1.jpg-1024x674.jpgoriginal_imageThe log showed the filter standing down exactly once, during the upload, and keeping Imagify's value on all 52 other calls in the same run:
Checked that ordinary uploads still resize. Uploaded a 4000px PNG through Media, Add Media File. It still produced
big-4000px-scaled.pngat 2560 with the 4000px original intact, so nothing changed for server side uploads.Checked the premise itself, because the fix only makes sense if the browser scales to Imagify's value rather than a hardcoded one. Read
bigImageSizeThresholdout of the editor settings in the console: it returned2560, Imagify's configured value, and the file the browser produced measured 2560x1684, the same dimensions the server used to produce. Nothing is lost by standing down.Confirmed the settings page still behaves, since the reset value for the resize width is computed by temporarily unhooking this same callback. Opened the settings page and read the rendered values back: unchanged.
Automated checks on top of the manual work: 12 new unit tests across
--group ContextWPand--group ProcessWP, covering the browser upload, afalsefrom any other source, and the resize guard both ways. Full suite green at 502,composer run-stanclean, PHPCS clean on every changed file.How to test
Setup:
localhost. Client side processing needs a secure context, and it is enabled by default there in any browser.Reproduce the fixed behaviour:
/imageand pressing Enter.Check the files:
wp-content/uploads/<year>/<month>/on the server.-scaledfile. A second one ending in-scaled-1means the bug is still present.<name>-1024x674.jpgand so on, with no-1suffix before the extension.Check the metadata:
_wp_attachment_metadatafor that attachment, for example withwp post meta get <ID> _wp_attachment_metadata --format=json.original_imagemust name the file you uploaded, not a-scaledone.Check nothing regressed on ordinary uploads:
-scaledfile at 2560 must be created, and the original must be kept beside it. This confirms Imagify still resizes on the server side path.Affected Features & Quality Assurance Scope
can_resize()override.Technical description
Documentation
WP_REST_Attachments_Controller::create_item()adds__return_falseonbig_image_size_thresholdwhen the browser handles the sub sizes, and its own comment says why:Imagify hooked the same filter at
IMAGIFY_INT_MAXwithContext\WP::get_resizing_threshold(), a getter that takes no arguments and therefore could not see, let alone respect, the incoming value.Three things changed.
A dedicated filter callback.
Context\WP::filter_big_image_size_threshold()hands the threshold back untouched for an upload the browser scaled, and otherwise delegates to the existing getter, including when thefalsecame from somewhere else. The getter keeps its old contract, so the direct callers inAbstractContext::can_resize()andAbstractProcess::maybe_resize()are unaffected, andContextInterfacedoes not change, which matters becauseCustomFolders,NoopandNGGimplement it too.Standing down loses nothing, because the threshold the browser scales to is produced by this very filter, in
WP_REST_Server::get_index(). Imagify's setting is still what governs the result.The state is taken from where WordPress declares it.
rest_after_insert_attachmentruns just before the metadata is generated and carries the request, whosegenerate_sub_sizesparameter isfalseexactly when the browser owns the sub sizes. That is what identifies the upload, rather than thefalseon the filter, which says nothing about who sent it or whether a scaled file exists.A flag, so the original is left alone. Optimization does not run inside the upload request. It is pushed onto a queue and runs later, asynchronously, by which point the filters WordPress set up during the upload are gone. Without a record of what happened,
maybe_resize()would see the full size original at 3800px, compare it against the 2560 threshold and shrink the very file WordPress keeps aside asoriginal_image. So the attachment ID is recorded in a one hour transient, andProcess\WP::can_resize()declines to resize while that flag is present.A latent bug in the options class.
Imagify_Options::__construct()temporarily unhooks this callback to read WordPress's pristine value, then hooks it back withadd_filter( ..., IMAGIFY_INT_MAX )and no argument count, which silently re-registers it as accepting a single argument. That was harmless while the callback only took one. It is not harmless now: the callback stopped receiving the attachment ID, so the flag was never written. This was caught during testing, when the flag turned out to be missing even though the filter was clearly returningfalse. The argument count is now repeated there.New dependencies
None.
Risks
Nothing changes outside the WordPress 7.1 browser upload. Imagify stands down only for an
attachment WordPress itself flagged, by sending
generate_sub_sizesasfalseon the RESTrequest that created it. A
falsearriving on the filter from any other source is overriddenwith Imagify's value exactly as before, because only the browser flow leaves a scaled file
behind: treating every
falseas "already scaled" would produce an image nobody resized, sinceWordPress was told not to and Imagify would believe the work was done.
Known limitation: the flag is a one hour transient. On a site that purges transients
aggressively, or with an optimization queue backed up for more than an hour, it can be gone by
the time the queued optimization runs. The consequence is the behaviour that shipped before this
PR, the original being resized, with the untouched upload still in the backup folder. Nothing
errors, and there is no data loss.
The flag is written only for those uploads, and it expires on its own, so no cleanup routine is
needed and nothing is stored permanently. The pattern is registered in
classes/Tools/InternalStateList.phpso a reset and an uninstall clear it.One interaction is worth stating plainly. #1234 makes auto optimization run before the browser's sub sizes arrive, which means it still runs against the full size original during the upload. This PR stops that pass from resizing the original, and the original keeps its dimensions, but the file is still compressed in place, with the untouched upload preserved in the backup folder. Fixing #1234 removes that early pass entirely and the original stops being touched at all. This was verified: with both branches merged locally, the uploaded file came out byte identical to the source. Both are improvements on their own and neither depends on the other to be safe.
Mandatory Checklist
Code validation
Code style
Unticked items justification
Nothing unticked.
Additional Checks
The flag is observable: the
imagify_client_side_scaled_<id>transient exists for the lifetime of an upload that the browser scaled. No filesystem or HTTP calls were added, so there is nothing new that can throw.