Fix #1234: optimize once the client side sub sizes have arrived - #1240
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
🟢 Coverage 63.33% diff coverage
Metric Results Coverage variation Report missing for 0e1ea851 Diff coverage ✅ 63.33% diff coverage (50.00%) Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (0e1ea85) Report Missing Report Missing Report Missing Head commit (738bd7e) 20373 1605 7.88% 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 (#1240) 30 19 63.33% 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.
WordPress 7.1 can hand an upload to the browser. The attachment is then created with no sub sizes, each one is sent through the sideload endpoint afterwards, and a final request stores the whole metadata at once. Auto optimization ran on that first, empty metadata, so it only ever covered the full size and every thumbnail was left untouched while the media still reported success. The finalize pass then re-optimized the full size, spending a second API credit, or bailed out entirely. Wait for the request that brings the sub sizes in, treat it as the new upload it still is, and let it run after the metadata has been stored, since the sizes to optimize cannot be read before that write.
bacd1fd to
a497b17
Compare
|
Overall verdict: Changes requested Nice fix for a tricky timing bug — the "defer until subsizes actually arrive" approach is the right shape, and the manual testing in the PR description is thorough. Before merging, I'd like the core assumption behind the fix nailed down, since it determines whether the bug is truly fixed or just moved. Blocking
Non-blocking follow-ups
Happy to re-review once the multi-call question is settled — that's the one thing standing between this and a solid fix. |
Correct the docblock: WordPress has passed the context argument since 5.3, not 7.1. Note on the meta hooks why they are now registered for every version, and skip flagging an attachment Imagify cannot optimize. Register the new transient in InternalStateList so a reset and uninstall clear it, and derive the reset test's query count from that list. Add an integration test that fires the real hook chain rather than calling the methods directly: it proves nothing is optimized on the create phase, and that the pass which does run happens after the metadata is stored, so it reads the complete set of sub sizes.
get_media_files() reads width, height and mime type for every size, so the optimization it queues errored on the stub entries.
|
On the multi-call question: it fires once. |
…after-client-side-subsizes # Conflicts: # Tests/Unit/classes/Tools/InternalStateList/sharedList.php # Tests/Unit/classes/Tools/ResetInternalState/reset.php # classes/Tools/InternalStateList.php
Description
Fixes #1234
WordPress 7.1 can hand an upload over to the browser. The attachment is then created with no sub sizes at all, each one is sent afterwards through a new sideload endpoint, and a last request stores the complete metadata in one go.
Auto optimization ran on that first, empty metadata. Only the full size was ever optimized, every thumbnail was left untouched, and the media still reported success. Users saw "Optimize 6 missing thumbnails" on the media, or worse, on smaller images, "WELL DONE. This media file is already optimized, no further optimization is required." with no way to recover. Next-Gen images had the same gap, so the front end served WebP for the main image and untouched JPEG for every thumbnail.
Uploads made from the block editor now get every size optimized, in a single pass.
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 and auto optimization on. A small mu-plugin was used during testing to log when the metadata was generated and when auto optimization fired, and to read the optimization data back. It is not part of this PR.
Reproduced the bug first. Uploaded a 4000x3000 JPEG from the block editor on an unpatched build. The optimization data came back with
fullandfull@imagify-webponly, while the media had six sub sizes on disk, and the media row offered "Optimize 6 missing thumbnails". Repeated with an image below 2560px: four sub sizes unoptimized, and the row read "WELL DONE. This media file is already optimized, no further optimization is required." with no recovery link at all. The log showed auto optimization firing twice, once on the empty metadata and once at the end.Verified the fix on the same flow. Uploaded a 3800x2500 JPEG from the block editor and read the optimization data back:
fullonlyfulland all 6 sub sizesfullonlyThe log showed the create phase no longer triggering anything, and the finalize pass seeing every size:
Confirmed it in the interface, not only in the data. Opened "View Details" on the media in the Media Library list view: "Thumbnails Optimized: 7", "Next-Gen generated: Yes", overall saving 52.58%, and no "Optimize N missing thumbnails" link.
Checked that ordinary uploads still work, which matters most here because the hook registration changed for every WordPress version. Uploaded through Media, Add Media File: auto optimization fired exactly once, as a new upload, with all sizes already present, and the media ended with all 5 sizes and all 5 Next-Gen versions optimized. No second pass.
Re-ran the whole upload test after the last code change. The final commit tightened the input guards on the new hook, so the manual upload test was repeated against it rather than assumed: all 7 sizes plus all 7 Next-Gen versions optimized, one pass, flag cleared, 50.17% saving.
Automated checks on top of the manual work: 6 new unit tests (
composer test-unit -- --group AutoOptimization, 8 including the 2 that already covered this class), full suite green at 490,composer run-stanclean, PHPCS clean on both changed files.How to test
Setup:
localhost. Client side processing needs a secure context, and it is enabled by default there in any browser.Test the main case:
/imageand pressing Enter.Check the result:
Test the case that used to be worse:
Check nothing regressed on ordinary uploads:
Affected Features & Quality Assurance Scope
media-create-image-subsizes, which shares this class.Technical description
Documentation
Two separate problems had to be solved.
The trigger fired too early.
WP_REST_Attachments_Controller::create_item()generates and stores metadata with no sub sizes, then the browser sideloads them one by one, thenfinalize_item()stores the complete set. Imagify keyed its "generate" step off the first of those, andstore_ids_to_optimize()launched from there.There is no public way to ask WordPress whether the browser is handling an upload, so the state is taken from where WordPress itself declares it:
rest_after_insert_attachmentfires just before the metadata is generated and carries the request, whosegenerate_sub_sizesparameter isfalseexactly when the browser owns the sub sizes. That gets recorded in a one hour transient, because the sub sizes arrive in later requests.While that flag is set, the
createphase stores no step at all, so nothing is optimized. When the finalize request comes through with theupdatecontext, the flag is cleared and the "upload" step is restored. That step was set onadd_attachment, in the request that created the attachment, and is held in memory only, so it did not outlive it. Restoring it is what makes the media count as a new upload instead of falling into the "already optimized?" branch, which would either re-optimize the full size for a second credit or bail out and leave the media untouched.The size list was read too early. Fixing the timing alone was not enough, and this is the part worth reviewing closely.
optimize()resolves the sizes to work on when it queues the job, and it reads them from the stored metadata. Duringwp_update_attachment_metadatathe new value has not been written yet, so on the finalize pass it still saw zero sub sizes and queued the full size alone. This was found during testing: the trigger was firing at the right moment, as a new upload, and the thumbnails were still not optimized.Ordinary uploads never hit this because WordPress writes metadata progressively as it builds sub sizes, so by the last call the rows are already there. The client side flow writes everything once, at the end.
So for this path the optimization is deferred:
store_ids_to_optimize()sets its "update" step and returns without firing, anddo_auto_optimization_after_meta_update(), hooked onadded_post_metaandupdated_post_meta, picks it up once_wp_attachment_metadatais actually stored. That method already existed for WordPress versions below 5.3, and already guards on the "update" step, so it is now registered for every version. It cannot double fire:do_auto_optimization()clears the steps as its first action, so after an immediate run the meta hooks find nothing to do.New dependencies
None.
Risks
The
added_post_metaandupdated_post_metahooks are now registered on WordPress 5.3 and above, where previously they were not. They are guarded by the "update" step, which onlystore_ids_to_optimize()sets and whichdo_auto_optimization()clears before doing anything, so an ordinary upload cannot be optimized twice. This was verified on a real upload: one pass, one set of results.If the browser never sends its sub sizes, because the tab was closed mid upload, the flag expires after an hour and the media stays unoptimized until something else touches it. WordPress is in the same position, since the sub sizes never arrive either, and it exposes a resume path of its own for that case. Optimizing a partial media instead would be the worse outcome.
The flag is stored per attachment, only for uploads WordPress flagged as browser processed, and it is deleted as soon as the sub sizes arrive, so nothing accumulates.
maybe_store_generate_step()now takes a third parameter. It is optional and defaults to null, which is what WordPress versions before 7.1 pass, so third party code calling it directly keeps working.Mandatory Checklist
Code validation
Code style
Unticked items justification
Nothing unticked.
Additional Checks
The flow is observable through the existing
imagify_before_auto_optimizationhook, whose$is_new_uploadargument now reports true on the finalize pass, and through theimagify_awaiting_subsizes_<id>transient which exists between the two. No filesystem or HTTP calls were added.