Automattic / regenerate-thumbnails

WordPress plugin for regenerating thumbnails of uploaded images. Over 1 million active users and counting.
https://alex.blog/wordpress-plugins/regenerate-thumbnails/
GNU General Public License v2.0
133 stars 54 forks source link

Keep reference to metadata before regenerating images. #143

Closed aaronfc closed 1 year ago

aaronfc commented 1 year ago

Fixes #136

Description of the change

Starting from WP 5.3 "sizes" attribute on metadata is reset to empty when passed to intermediate_image_sizes_advanced filter. The fix saves temporarily the metadata for the original file before calling the intermediate_image_sizes_advanced filter, so we can keep the same logic that was being applied before.

Alternatives

I have been tempted to just remove the check for metadata sizes completely. I don't quite understand why it was introduced and doesn't seem really needed to me. But I finally decided to keep logic the same.

Testing instructions

Extra details on build step (troubleshooting)

I had some trouble setting up the development environment – here are the details I followed:

MarcGuay commented 1 year ago

My understanding of the need for checking the metadata sizes is that is how we know which sizes the image is supposed to have matching files for. Once we know what sizes we should have, then we can know which ones already have existing files and don't need to be regenerated.

aaronfc commented 1 year ago

My understanding of the need for checking the metadata sizes is that is how we know which sizes the image is supposed to have matching files for. Once we know what sizes we should have, then we can know which ones already have existing files and don't need to be regenerated.

Thanks for your input, @MarcGuay. Let me develop a bit further my concern below.

If I am not missing anything, the sizes we "expect" are passed to the intermediate_image_sizes_advanced hook as its first parameter. Then, as you mentioned, we check the sizes in the metadata here and here.

But the thing is that, we are already doing something similar in the get_thumbnail method called here. Since we are generating the filename based on the expected sizes and later it's checked if the file_exists.

It's not exactly the same, but I see it just as an extra measure in case the file existed (with the expected image dimensions in the file name) but having different real dimensions.

Anyway, I think that explaining it out loud here made me convince that it's not that bad to keep the extra check. Thanks for commenting on it!

ddur commented 1 year ago

There is another regenerate thumbnails issue created by WP 5.3 disaster changes. You have opportunity to fix it now. Issue https://github.com/Automattic/regenerate-thumbnails/issues/102

aaronfc commented 1 year ago

There is another regenerate thumbnails issue created by WP 5.3 disaster changes. You have opportunity to fix it now. Issue https://github.com/Automattic/regenerate-thumbnails/issues/102

Hey @ddur, thanks for your feedback. I would rather have the issues separated. Hopefully we will be able to work on that one soon. Meanwhile, just a reminder that you can raise a PR yourself if you have a clear solution in mind.