Support retry mechanism for generating sub-sizes in additional MIME types on constrained environments. #188
Conversation
Replace the values in place without the need of extra variables, in order to update the metadta as soon as an image was uploaded. This `in place` mechanism allow us to remove the need for additional variables.
In constrained environments when the image fails to upload a retry process would be trigger, this process only should be executed on Ajax or REST endpoint.
mitogh
added
[Type] Feature
…e/142-multiple-mimes-for-image-sizes-with-retry
In order to be consistent with the rest of the code and format to detect key presence.
There was a problem hiding this comment.
@mitogh Left a few minor points of initial feedback, specifically to simplify the code for readability.
In any case, we should either always use $metadata['sizes'][ $size_name ] or always use $properties, but not mixed like it is currently here, sometimes checking one, other times the other.
felixarntz
changed the base branch from
feature/142-multiple-mimes-for-image-sizes
to
trunk
There was a problem hiding this comment.
@mitogh This looks great. The main thing that needs iteration though is using a more reliable approach for the "hack" here - we shouldn't be checking for whether this is an AJAX or REST request as that would mean the code doesn't support any other calls to wp_update_image_subsizes(). While this is a hack, it should still be reliable.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Make sure the same variable is used across when referencing the data from the sources.
Make sure the hook is placed closer to the function definition.
Update logic to retry under constrained mechanism with the `debug_backtrace` approach instead.
|
Looking good, nice work @mitogh & @felixarntz! I'll give this a test in my constrained environment. |
|
@mitogh & @felixarntz I tested this locally with a 5 second timeout and a large image upload. The retries continued until all webp images were generated, even though it took a few tries (a total of 4 retries in my case). I tested uploading in both the media library and directly in the block editor and the retry mechanism worked correctly in both. One difference I noticed is that in the editor, the image shows up greyed out immediately when the upload starts with a spinner over it: The media editor on the other hand shows this sad grey box with a "progress" bar that stays full all throughout the regeneration process (I think it shows upload percent as the file uploads, but then remains full until subsize gerneration completes): We may have a trac ticket for it already, if not I will create one: two small improvements for the media library uploader would be:
I also tested what happens if you navigate away during the file regeneration. Neither the media library or the post screen warn about navigating away while images are still being processed (after you have hit publish or save draft in the editor), so if you start an upload with a constrained system, then navigate away, the retries will not occur and you get left with an incomplete set of images. For now I think we should at least warn users who try to navigate away that image processing isn't complete (similar to how we warn users they have unsaved changes on a post if they try to navigate away) - I can create a follow up trac ticket for that if there isn't one already. In addition, when we build out image regeneration, we can make sure any missing images get filled in. |
There was a problem hiding this comment.
@mitogh This is pretty much good to go, all my comments are about documentation and for a few details worth improving IMO.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
That's a great catch! I think it's definitely okay for now, but especially for the WP core patch we should investigate a solution. I'm curious how the individual "steps" of the progress bar are being determined in core today. Right now it makes total sense that it sticks around at 100% until our process is completed, since basically our logic happens at the end, and the client-side has no idea that these additional sizes are being generated, so I assume it doesn't take them into account when "computing" the progress bar steps. |
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Remove indentation in the code to remove the complexity when reading the code. For the most part early return.
mitogh
deleted the
feature/142-multiple-mimes-for-image-sizes-with-retry
branch
I'm pretty sure the progress bar is showing the upload status & progress, then shows full while image processing competes, finally the thumbnail is shown on completion. in my local testing without the plugin, it behaves the same way, the progress bar goes to "full" but the box still shows gray until all of the image processing is complete (screencast) Dragging multiple files, the "progress" bar is partially full for each image as the processing and uploading happens for each image (screencast). Also, each upload causes the async upload callback to be called (so essentially images are uploaded one at a time) |
felixarntz
changed the title
|
testing on a remote site I can see the progress bar at work, showing the upload. then it stays 100% while the processing happens (screencast). |
Summary
Fixes:
Requires:
sourcesfor storage. #147Relevant technical choices
The following code uses the filter
wp_get_missing_image_subsizesas an action in order to trigger the execution of additional images when those images do not exist yet when the image is created under constrained environments. If the server timeouts WordPress will retry until a maximum of 5 times. Every retry would only process non-existing images on the metadata.Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.