Save progress of intermediate image creation after upload

[kirasong]

When uploads fail with an HTTP Error, a user currently has no recourse other than to re-upload media and hope that it makes it the next time.

Currently, WordPress only saves data about intermediate sizes in meta after all of the sizes have been completed on upload. This means that every time a user attempts to re-upload, WordPress is required to resize *every* intermediate size necessary over again during this retry.

WordPress should save the progress of each successful image resize/intermediate so that it can be resumed on retry.

See #39647 for the global ticket on solving HTTP Error issues on upload.

[enshrined]

If it's of any interest I had a play with this issue as a plugin here ​https://github.com/darylldoyle/WP-Background-Image-Processing.

It stops creating image sizes on upload and then uses the background processing library by deliciousbrains to create intermediate sizes in the background.

[nosilver4u]

Be careful with WPBIP if you have any batch image import processes on your site. I use the background lib from delicious brains in EWWW Image Optimizer, and it has a rather large issue with dispatching the queue. It doesn't reset the $data variable after saving it to the db, so you can end up with something like this:
queue 1: 1
queue 2: 1,2
queue 3: 1,2,3
queue 4: 1,2,3,4
queue 5: 1,2,3,4,5
...
queue 10: 1,2,3,4,5,6,7,8,9,10
and so on. Import a couple hundred images, and you'll have a couple hundred queues with 1-200 items in them. There were some other issues with batch imports that I had to fix as well. You might be able to drop-in the background processing libs with the ones from EWWW IO: ​https://github.com/nosilver4u/ewww-image-optimizer/tree/master/vendor

[blobfolio]

An alternative to a queue- or batch-type approach would be to alter image_get_intermediate_size() (and probably a few related functions) so that it consults the global $_wp_additional_image_sizes to see if a size exists (independently of the stored image meta), and run a basic file_exists()-type check to see if the file does indeed exist on the system, and if not, build it then and there.

The above would help WP recover from upload timeouts, and as an added bonus, help old media transition to new themes.

The main downside is that file_exists() lookup times can add up, particularly on Windows hosts. Another factor to consider is that this would potentially push thumbnail generation overhead to the frontend, which could lead to slow first loads (if i.e. a lot of thumbnails were missing).

To clarify, the Core wouldn't really be changing its default upload behaviors with the above approach. These changes would serve as more of a failsafe, catching and creating images that should exist but don't for whatever reason. :)

[azaozz]

This seems simple enough to implement: instead of generating all image sizes and saving them first, then updating the image_meta in the db, we should be updating the meta after each size is created. That way if a size creation fails for some reason, we will be able to re-run that loop and create any missing sizes (and add the meta for them).

It's true, this will write to the db a few times more, but that is negligible overhead compared to timeouts of 30 seconds :)

The main downside is that file_exists() lookup times can add up

True, but still negligible (compared with 30 sec. running). Also, not sure we should do file_exists() for each newly created sub-size. If the file was saved successfully, it does exist at that point :)

Thinking we should try this now. After this is implemented we can think of a good UI/UX to expose the ability to regenerate image sizes when some are missing, see #43525.

[azaozz]

In 40439.patch:

• Abstract/expose creating of an image sub-size in class-wp-image-editor-imagick.php and class-wp-image-editor-gd.php. Note that I didn't change the signature of Class_WP_Image_Editor as "something" may be extending it.
• Refactor wp_generate_attachment_metadata() a bit, move the creation of image sub-sizes to a new function: _wp_create_image_subsizes().
• Add couple more helper functions for creating and checking of image sub-sizes.
• Add wp_update_image_attachment_sizes() that tries to create all missing sub-sized of an image.
• Add wp_check_image_attachment_sizes() that is used to check whether all sub-sizes were created successfully.
• Use the above function in the Media Library list-table to check for missing image sizes and add a button if any are found, so the user can create them.
• Add AJAX for the above.

This started as a "quick test" to see exactly how difficult it may be to add that functionality (I know, it's Sunday, but...) :)

TODO: needs UI/UX review and perhaps some design for how to add the messages and the button to the Media Library grid view (and the media modal?). Also probably needs some cleanup, and of course: testing!

[kirasong]

Moving to 5.2 to support #43524

[joemcgill]

#43525 was marked as a duplicate.

[joemcgill]

From @azaozz via #43525

Generate only one subsize on image upload, add meta for it and return "success". Then do another request (from PHP) and generate another. Continue until all subsizes are created.
On uploading first create the attachment post, then update the attachment meta after each image subsize is created. If the upload fails to create all subsizes, send another (AJAX) request from the browser to continue creating subsizes (we will need some UI for that).

[desrosj]

5.2 beta is in a little over a day. Since this still needs testing, I am going to punt it to 5.3 in unison with #43525. @mikeschroder if you are confident in this for 5.2, feel free to move it back.

[kirasong]

Refresh of 40439.diff.

[desrosj]

This was discussed in the #core-media weekly meeting this week. The goal is to test this so it can land as early as possible in 5.3.

[kirasong]

In progress patch. Replaces multi_resize for images in wp_generate_attachment_metadata with _wp_create_image_subsizes so that metadata gets updated as sizes are created.

[kirasong]

I noticed that when I forced image creation to crash with a low timeout, it didn't save the sizes generated in meta so far, and the image would also end up without a width and height (which causes the detection of sizes to create to fail -- this probably needs a check of its own as well). This is because multi_resize() was being called before _wp_create_image_subsizes(), so the crash would happen in code that does not save meta during resizing.

In this patch, I moved up _wp_create_image_subsizes() in image.php, replacing multi_resize() for images, while also getting the default image metadata earlier so that the saved meta is more useful when it is saved successfully.

This will have to be done for the other instances of calls to multi_resize() as well, but since I have to break, I wanted to document the partial creation working as expected in 40439.3.diff​.

[kirasong]

There was some discussion about this here: ​https://wordpress.slack.com/archives/C02SX62S6/p1558720942005200?thread_ts=1558688701.004800&cid=C02SX62S6

To copy my feedback here -- in this pass, I switched to using _wp_create_image_subsizes rather than have multi_resize update meta because the WP_Image_Editor abstraction is based around modification of a single image file rather than a WordPress attachment.

Open to conversation on it, of course, but just so that bit of history lives in this ticket.

[azaozz]

Been looking at the patches for the last few days. It's getting big and need to add few more functions, at least. Starting to think it may help if we split this in several tasks/tickets perhaps?

As far as I see the main tasks are:

1. Update WP_Image_Editor_GD and WP_Image_Editor_Imagick (and WP_Image_Editor if needed) to handle saving of a single sub-size (this is already in the patch, may need review/refresh).
2. Review _wp_create_image_subsizes() and related new functions.
3. Retire multi_resize() (stop using it in core), and keep it for back-compat. Wondering if it should call the external (to the class) _wp_create_image_subsizes() or still attempt to do all sub-sizes at once. Seems better to use the new functionality, but may run into some problems perhaps (will be over multiple requests so not all image sub-sizes will be available after it runs, etc.).
4. Add function(s) to resize images over multiple requests. Wondering if we should try something like the alternate cron redirecting here, not sure if it works well. Perhaps just do a chain of wp_remote_post() until there are no more sub-sizes to be created.
5. Revisit the UI to create missing sub-sizes. TBD: do we want it or need it in core or shall we leave it for plugins? If yes, it needs adding to the media modal, and perhaps other places images can be uploaded (custom header, etc.).

The 4) is not in the patch yet and seems it will need most testing. It should handle uploading of multiple images, so we are looking at temporarily storing a queue of image sub-sizes that need to be created, and chaining multiple requests until that queue is emptied. Of course this will need safeguards against circular dependencies, retry on fail, etc.

Thinking we can split (at least) 4) and 5) into separate tickets. Seems 1), 2) and 3) can be handled here and committed soon, so we can continue with the rest.

[azaozz]

Replying to mikeschroder:

...in this pass, I switched to using _wp_create_image_subsizes rather than have multi_resize update meta because the WP_Image_Editor abstraction is based around modification of a single image file rather than a WordPress attachment.

Yes, that works better, thanks @mikeschroder.

Was also thinking we should probably move these functions to a WP_Image_Resize or perhaps WP_Image_Create_Subsizes class? Can probably be just static. Then we can bring over other/existing functions that deal with resizing and generating the image meta, and just leave stubs outside the class. That would let us organize and name the new additions better, have some private vars and methods, etc.

I'm almost done reviewing the patch. The last remaining thing to "grok" is how to handle (in a compatible way) the fact that wp_generate_attachment_metadata() actually creates the image sub-sizes rather than (only) generating the meta data :)

Then we'll add the "multi-request" resizing to the same class. There we can look into the elapsed time since the current run started, similarly to how Site Health calculates file sizes, and trigger the next request when it is (perhaps) over 20 seconds? Would be a nice optimization.

[azaozz]

Moved the UI (js, ajax, etc.) bits into a small plugin to make it easier to see and test image meta: ​https://gist.github.com/azaozz/91101d686127137b0f5b850f0d269972. New (slimmer) patch coming up :)

[azaozz]

In 40439.4.diff:

• Take out the (test) UI including the ajax action, etc.
• Clean up some function and var names, mark the functions non-private, add inline docs.
• Review, improve, account for edge cases in wp_get_missing_image_subsizes(), wp_get_defined_image_subsizes(), and wp_create_image_subsizes().
• Fix the $duplicate test when creating new sub-size in both WP_Image_Editor_Imagick and WP_Image_Editor_GD. It only checks whether the just resized image matches the source image.
• Add fallback when a plugin extends WP_Image_Editor and doesn't have the new make_subsize() method that returns the image size (for including in the meta) after each sub-size is created.

There are also some unrelated WPCS fixes in the patch.

This seems to work well here. Please get the ​testing plugin and give it a go :)

The testing plugin shows the image meta on the Media screen, list mode (switch modes with the buttons at the top/left of the screen). It adds few large image sizes. After it is enabled you should see these new sizes listed as missing for all images that are large enough to support them. When all files for an uploaded image are present, it would let you selectively delete a few, and then re-create them.

After testing it would be good to check the uploads directory where the uploaded image is, to ensure all sizes were created properly.

[azaozz]

Been going back and forth on how to best make the changes to the WP_Image_Editor_Imagick and WP_Image_Editor_GD classes.

The problem is that the multi_resize() function there wasn't designed for this. It "locks" the resizing and saving of image sub-sizes inside the loop and silences all errors. That prevents incremental saving of the image meta (the purpose of this ticket), and prevents showing "meaningful" errors in the UI.

Furthermore the (public) resize() and save() functions cannot be used for the same image multiple times. The way it works is that after resize() is used, all will have to be reset and then a new instance created to be able to re-use the original image as source. (This is the reason the private _save() and _resize() are used in the loop in multi_resize().)

Other "not great" thing there is that the attachment ID and image meta data are "unknown" while processing images. This removes the context and generally prevents any meaningful possibility of extending the functionality.

The ways to fix that:

1. Introduce another function similar to multi_resize() but without the loop. This is in the current patch. That function returns the new dimensions after each sub-size is created or returns the relevant error when it fails. The downside is that if a plugin extends WP_Image_Editor directly and replaces one of the default classes, it will be missing the new function. I did a quick search and seems there are few plugins that do this.

2. Pass attachment_ID and image_meta as optional params when making new instance. This will let us add few actions inside the loop in multi_resize() and pass errors to the UI and incrementally update sub-size dimensions. This is not as straightforward as adding a specialized method but is still a workable solution. Again, it suffers from the same problem as 1, existing plugins will need to be updated to work properly.

3. Another alternate possibility is to (try to) replace the functionality of wp_get_image_editor(), i.e. use _wp_image_editor_choose() and then extend the class that was chosen and overload the multi-resize() or resize() methods. This is somewhat risky as we will need to replace low-level functions like _resize() without knowing the exact PHP library used or its configuration.

From these three options I'm thinking the first sounds best. The current patch has a fall-back in cases where one of the image editor classes is replaced by a plugin. It will take some time but plugins that do this will get updated and add the new create_subsize() method.

What do you think? Any other ideas? :)

[azaozz]

In 45538:

Save progress of intermediate image creation after upload. First run.

• Introduces wp_get_missing_image_subsizes() and wp_update_image_subsizes() to generate image sub-sizes that are missing or were not created after the upload.
• Adds a way to display errors that happened while creating sub-sizes.
• Introduces wp_create_image_subsizes() intended for use after an image was uploaded. It saves/updates the image metadata immediately after each sub-size is created. This fixes the (long standing) problem when some of the sub-size image files were created but there was a timeout or an error and the metadata was not saved. Until now such uploads were considered "failed" which usually resulted in the user trying to upload the same image again, creating even more "orphan" image files.

Note that the patch also includes some unrelated WPCS fixes.

Props mikeschroder, azaozz.
See #40439.

[azaozz]

In 45539:

After [45538]: fix a WPCS fix and make couple of var names consistent.

See #40439.

[birgire]

Great addition that will hopefully help many users.

Regarding

Note that the patch also includes some unrelated WPCS fixes.

I noticed the patch changes:

true == ini_get( 'allow_url_fopen' )

to:

true === ini_get( 'allow_url_fopen' )

I tested the output of ini_get( 'allow_url_fopen' ) on my test install

When I define in php.ini:

allow_url_fopen = On

or

allow_url_fopen = 1

or

allow_url_fopen = true

the output is the string '1'.

When I define it as:

allow_url_fopen = Off

or

allow_url_fopen = 0

or

allow_url_fopen = false

the output is the empty string ''.

So it seems the new strict true check will not work as the previous check did.

[azaozz]

Replying to birgire:

Ah, good catch, thanks for looking! :)

I'm actually having some "second thoughts" on this type of WPCS fixes. The best way to do them is to have full unit test coverage. I ran the tests several times...

[azaozz]

In 45540:

After [45538]: fix another WPCS "strict comparison" fix.

Props birgire.
See #40439.

[azaozz]

In 45541:

Media: fix support for arrays for the crop setting for registered image sub-sizes in wp_get_registered_image_subsizes().

See #40439.

[azaozz]

In 45543:

Media: Ignore errors coming from image_resize_dimensions() when creating sub-sizes (for now). It returns false when the requested size is larger than the original image and should be skipped. This triggers new WP_Error in WP_Image_Editor::resize().

See #40439.

[azaozz]

Updated the testing plugin (​https://gist.github.com/azaozz/91101d686127137b0f5b850f0d269972) to try and overload the test server. Try uploading a larger image, over 2000px :)

If it's not enough, increase the 42 number of sub-sizes in the loop:

for ( $i = 0; $i < 42; $i++ )

[azaozz]

In 45645:

Media: Sort the new sizes array by priority when creating image sub-sizes.

See #40439.

[kirasong]

@azaozz: Just wanted to check in -- do you think there's anything left here in the backend portions?

Wondering if we should close this ticket and open something (if there's not already a good ticket) for the UX and UI decisions around making this work well for users.

[azaozz]

Replying to mikeschroder:

...do you think there's anything left here in the backend portions?

This is pretty much done. Been testing it (on and off) for a while and haven't seen any problems.

The only thing left is (perhaps) some tests. Thinking to leave it open for a bit longer in case somebody makes a patch :)

[SergeyBiryukov]

In 45871:

Docs: Fix typo in _wp_make_subsizes() DocBlock.

Props itowhid06.
Fixes #47913. See #40439.

[azaozz]

In 45892:

Docs: Improve and update the description of multi_resize() to explain changes and expected use.

See #40439.

[kirasong]

I noticed recently that this should probably stay open to handle the PDF thumbnailing/resizing case, since it still uses multi_resize().

I think a quick audit to make sure core always uses the new method would be a good idea.

[kirasong]

Did an audit of places still using multi_resize() instead of using the new wp_create_image_subsizes() and found two:

• In the ​image editor (src/wp-admin/includes/image-edit.php).
• In ​PDF Thumbnails/Fallback Thumbnail Support (src/wp-admin/includes/image.php).

I'm not sure if I'll be able to get to both before beta or not, but I'd love to see them supported before release.

@azaozz Do you think we should open new tickets for each of those?

[kirasong]

Chatted with @azaozz about this in Slack, who indicated it was fine to go ahead and close this one, and open a new one for anything else necessary.

I'd otherwise convert the ticket into a task, but due to ticket length, I think splitting will make things easier to follow.

Thanks everyone!

[azaozz]

I'm somewhat wary about including error handling code here that is not used yet. Perhaps best to remove it for now and add it when these errors are used or displayed in the UI. Also the "cleanup" of old errors stored in meta is not implemented that well.

In 40439.5.diff: remove storing of individual errors while making image sub-sizes as it's currently unused.

@mikeschroder @joemcgill thinking to commit that asap, any thoughts?

[kirasong]

Replying to azaozz:

I'm somewhat wary about including error handling code here that is not used yet. Perhaps best to remove it for now and add it when these errors are used or displayed in the UI. Also the "cleanup" of old errors stored in meta is not implemented that well.

I don't want to lose the ability to tell what might be causing errors or users, but tend to agree that WordPress should be using data it logs. Does anyone think it's worthwhile to leave for users (via plugins or database access) or hosts as debugging data?

As another option, we could choose to save information on errors only when WP_DEBUG is enabled for this release.

[azaozz]

Replying to mikeschroder:

I don't want to lose the ability to tell what might be causing errors or users, but tend to agree that WordPress should be using data it logs.

Yeah, that's exactly what I'm thinking.

As another option, we could choose to save information on errors only when WP_DEBUG is enabled for this release.

Think that saving error messages in image meta wasn't a good idea. Seemed good at the time but not so good now :)

The idea was to be able to show these errors in the UI. As post-processing of images is always async, the errors (including the messages) would need to be stored "somewhere". That could also be a transient (to make cleanup easier).

Ideally these errors should be logged to an appropriate place. They can be hard to understand and are valuable mostly for (low level) debugging, perhaps not so much to show to users. Thinking this would fit better in the "Site Health" context. Also ideally we should be logging all errors encountered while trying to edit image files (all the TODOs). Currently all of the errors are logged in the server error logs, but would be nice to be able to access them from WP.

Thinking to commit 40439.5.diff as it fixes the "persistent data not used anywhere" problem. If you think we should add transients to store them, we still have time to do that before RC tomorrow :)

[azaozz]

In 46507:

Media: Do not store error messages in the image meta. The initial idea was to (be able to) display these errors in the UI but it wasn't implemented as these errors are mostly helpful for low-level bedugging.

Fixes #40439.

[justinahinon]

Mentioned in ​https://make.wordpress.org/core/2019/10/11/updates-to-image-processing-in-wordpress-5-3