Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create image sub-sizes in additional MIME types using
sources
for storage. #147Create image sub-sizes in additional MIME types using
sources
for storage. #147Changes from 30 commits
0706e81
9c43b89
25479a1
0f560d8
410c124
7a8b0c5
f71a897
4b4cbc2
b01be99
4ba9409
5321483
2bf68f6
de38c2d
dd13025
70eeb33
107db40
975fddd
e3c2e3a
6e3afee
76f1c2f
fba8fab
7da54dc
2c37d19
2c6307f
5b5ff6d
0e94c15
a5b63df
e0b48c4
e996230
d708dfd
061ef31
81ea713
411efe4
a2c6815
a2c655c
38224b0
acd12f1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code here is still missing the critical check for whether we actually need to generate the format still. Since the process may fail at any point, we need to always make sure we only generate the sizes which are missing, see also my feedback in #147 (comment).
Also, why a cron event? WordPress core doesn't use a cron event to generate its image sizes, and I think we should follow how it generates its default sub-sizes, which happens in a single request, just allowing for failures as far as I can tell, and then it retries until everything has been completed (see the
_wp_make_subsizes()
function and how it is used).We also need to make sure then that we keep using the latest version of
$sources
, since it could be updated within the function, so yourwebp_uploads_generate_image_size()
function below should probably return the source array that it has added.How about this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is already covered here:
https://github.com/WordPress/performance/pull/147/files/6e3afeef0648073c2615aa15f4d0e090f0b3b93f#diff-71ed188a16b79d179868ee80e76f775462a451e871fc3d6214fcacca3edcea8cR47
As if the size has not been created yet the additional mime types for that image won't be created.
Once the image is retried and finally generated it would trigger this hook again, which at that point would contain the key for that particular image and then the image would be scheduled for creation.
To split the work for every single size in a different process, and only use as minimal resources as possible per image size so we can decrease the number of potential failures when creating an image in a constrained environment.
It does not because it has to process the image in a single request and display the image right away, we as progressive enchantment can do that as we know a fallback image was already created. And we can create a background process (Cron) for any additional mime images.
This mechanism would ensure existing users would continue to use the same number of resources when uploading an image as they currently do, without the overhead of additional resources introduced by a new mime type.
The failures would be handled by this hook as described at the beginning every time an image fails won't be added into the image sizes, and every time a new image size is added it would trigger this hook, ensuring that the image is only scheduled for creation when the main image was created.
This is already handled here by just returning the updated metadata and the hook dispatched by this function would update the meta once is executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the
wp_schedule_single_event
approach works in my testing, I wonder if it will work across WordPress installs or might create problems for some users, for example does this work ifdefine( 'DISABLE_WP_CRON', true );
is set?I think we should test Felix's suggestion of trying to generate all images in one go, tracking the completed ones in meta . Core's upload process (in both media and the block editor) will catch incomplete generation runs (a timeout error) and send a retry request (which we may need to hook into?). I will try testing this approach with a low server timeout to verify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i encountered a big with the callback approach: after uploading an image, I deleted it before WebP creation completed. A final WebP was generated after the deletion.
testing with a short server timeout, WordPress's retry mechanism worked as expected, and the cron callback was set up after the core image process completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually when this is the case, is to run the corn with an actual CRON via the server which will still execute any remaining cron job. However, if the CRON is completely disabled from the server any additional functionality from WordPress won't work either like:
Can you share your setup PHP.ini so I can try to replicate it?
Oh, great point I think this can be an edge case but still would be ideal to make sure any remaining cron is removed when the image is removed as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mitogh
While I get the thinking behind this, we have to keep in mind we're building this for WordPress core, which already has an existing working solution for generating sub-sizes in a scalable way. WP core does not use cron for this, and if we wanted to change that, it would open up a big discussion on whether that makes sense or not, which is not the point we're trying to make here - we want to add support for adding sub-sizes also in WebP.
We should really minimize any sort of "refactoring" that is unrelated to the problem we're trying to solve. We can tie into the existing logic and follow the same approach that WP core has here without running into problems - and as @adamsilverstein has pointed out, using a different approach here potentially causes other problems and it will require a lot of extra testing - in contrast to using WP core's existing approach, which we know works.
For the above reasons, I still think it would be better to drop the cron usage here and just call the function right away, as that follows the existing approach in WP core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some testing with a low php timeout threshold using both approaches. Unfortunately, the way we hook in now is after core completes its retries, so our generation code only gets one run. With a low timeout, this means not all sizes will be generated. We could either change how we hook in so it happens earlier (before completion so the error is caught), or we could hook in later and return an error if mime/sizes are still missing:
For Ajax hooking in with the
wp_prepare_attachment_for_js
filter, which is the last thing the ajax callback calls (returning false when mime/sizes are missing would trigger another retry)I found the same issue in Gutenberg, the retry 'create-image-subsizes' was only called until all the jpg images are generated, then one attempt at webp images, the retry did not continue because the code only retries until a success return. We may be able to use the
rest_prepare_attachment
filter (in `src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php::L843). Returning an Error when the attachment has missing mime/sizes woulds trigger the retry.Maybe we can also bump these retries up on the trac ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine merging Felix's suggested code, then working on ensuring error catching (and retries) work correctly for secondary types as well.
It might be worth looking at hooking on
wp_update_attachment_metadata
, which is called for each image core generates, passing the full meta with each call, with the last item in the sizes array the most recently generated image. If our image generation threw a timeout error here it could trigger the retry.wp_update_attachment_metadata
is also called by image edits which could be handy if we want to handle those in alternate mime types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamsilverstein Based on your feedback, I agree we may need to hook into
wp_update_attachment_metadata
. That would require some tweaks for our callback though, since that hook is fired every time a single new sub-size is generated in core. We had also discussed the potential usage of this hook in our call last week - the tricky part would be to figure out whether it is used for an image size being generated so that our code is only run in that case. Also, the logic for which sub-sizes to generate may need to be reworked.Before we go towards doing that though, are we sure the current implementation is not working in a scalable way? I understand your argumentation @adamsilverstein, but why is that actually failing? Looking at the code, WP core uses
wp_update_image_subsizes()
, both via AJAX and the REST API. Before that function succeeds, it fires thewp_generate_attachment_metadata
filter, which we are using here, so I don't follow how the code would return a success response without running our added filter callback 🤔In any case, I think it makes sense to drop the cron hook, replace it with a direct call, and explore further from there on whether the
wp_generate_attachment_metadata
filter is okay or whether we need an alternative solution using thewp_generate_attachment_metadata
filter. cc @mitoghThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summarizing a chat with @mitogh and @adamsilverstein here:
wp_update_image_subsizes()
will no longer trigger thewp_generate_attachment_metadata
filter in any subsequent requests after the WP core sizes have been generated (sincewp_get_missing_image_subsizes()
returns an empty array, it short-circuits) - that's why the implementation will only make one more request after a failure in our code, but that won't trigger the hook, so WP thinks the sub-sizes generation has succeeded.wp_get_missing_image_subsizes
filter, and essentially misusing it as an action, as follows:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above, we should return the new source here so that it can also be processed outside, so that the data in the above function remains intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this behavior is to allow an
async
mechanism rather than a single request.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above - I think we should change that to use a single request while supporting retry attempts, thus following how WP core does it.