#40439 closed enhancement (fixed)
Save progress of intermediate image creation after upload
Reported by: | kirasong | Owned by: | kirasong |
---|---|---|---|
Milestone: | 5.3 | Priority: | high |
Severity: | normal | Version: | |
Component: | Media | Keywords: | needs-unit-tests has-dev-note |
Focuses: | ui | Cc: |
Description
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.
Attachments (6)
Change History (77)
This ticket was mentioned in Slack in #core-media by mike. View the logs.
8 years ago
#3
@
8 years ago
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
This ticket was mentioned in Slack in #core-media by mike. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
7 years ago
#7
@
7 years ago
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. :)
This ticket was mentioned in Slack in #core-media by mike. View the logs.
7 years ago
#9
@
6 years ago
- Milestone changed from Future Release to 5.0
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.
#10
@
6 years ago
- Focuses ui added
- Keywords has-patch needs-testing needs-unit-tests added; needs-patch removed
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!
This ticket was mentioned in Slack in #core-media by azaozz. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-media by mike. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-media by mike. View the logs.
6 years ago
#16
@
6 years ago
- Milestone changed from Future Release to 5.2
- Priority changed from normal to high
Moving to 5.2 to support #43524
#18
@
6 years ago
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).
This ticket was mentioned in Slack in #core-media by mike. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
6 years ago
#22
@
6 years ago
- Milestone changed from 5.2 to 5.3
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.
This ticket was mentioned in Slack in #core-media by mike. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-media by mike. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-media by azaozz. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-media by azaozz. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
6 years ago
#28
@
6 years ago
- Keywords early added
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.
@
6 years ago
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.
#29
@
6 years ago
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.
This ticket was mentioned in Slack in #core-media by mike. View the logs.
6 years ago
#31
follow-up:
↓ 34
@
6 years ago
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.
#32
@
6 years ago
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:
- Update
WP_Image_Editor_GD
andWP_Image_Editor_Imagick
(andWP_Image_Editor
if needed) to handle saving of a single sub-size (this is already in the patch, may need review/refresh). - Review
_wp_create_image_subsizes()
and related new functions. - 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.). - 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. - 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.
This ticket was mentioned in Slack in #core-media by mike. View the logs.
6 years ago
#34
in reply to:
↑ 31
@
6 years ago
Replying to mikeschroder:
...in this pass, I switched to using
_wp_create_image_subsizes
rather than havemulti_resize
update meta because theWP_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.
#35
@
6 years ago
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 :)
#36
@
6 years ago
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()
, andwp_create_image_subsizes()
. - Fix the
$duplicate
test when creating new sub-size in bothWP_Image_Editor_Imagick
andWP_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 newmake_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.
This ticket was mentioned in Slack in #core by azaozz. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
6 years ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
6 years ago
#40
@
6 years ago
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:
- 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 extendsWP_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.
- Pass
attachment_ID
andimage_meta
as optional params when making new instance. This will let us add few actions inside the loop inmulti_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.
- 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 themulti-resize()
orresize()
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? :)
#43
follow-up:
↓ 44
@
5 years ago
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.
#44
in reply to:
↑ 43
@
5 years ago
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...
#48
@
5 years ago
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++ )
This ticket was mentioned in Slack in #core-media by mike. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by azaozz. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
5 years ago
#53
follow-up:
↓ 55
@
5 years ago
@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.
This ticket was mentioned in Slack in #core-restapi by joemcgill. View the logs.
5 years ago
#55
in reply to:
↑ 53
@
5 years ago
- Keywords has-patch needs-testing early removed
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 :)
This ticket was mentioned in Slack in #core-media by azaozz. View the logs.
5 years ago
#59
@
5 years ago
- Keywords needs-patch added
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.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
5 years ago
#61
@
5 years ago
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?
#63
@
5 years ago
- Keywords needs-patch removed
- Resolution set to fixed
- Status changed from assigned to closed
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!
This ticket was mentioned in Slack in #core by azaozz. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
5 years ago
#66
follow-up:
↓ 67
@
5 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
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?
#67
in reply to:
↑ 66
;
follow-up:
↓ 68
@
5 years ago
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.
#68
in reply to:
↑ 67
@
5 years ago
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 :)
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.