Allow control for which image sizes to generate additional MIME type versions#415
Conversation
… not allowed to have additional mime types.
eugene-manuilov
added
[Type] Enhancement
eugene-manuilov
requested review from
adamsilverstein,
felixarntz and
mitogh
as code owners
jjgrainger
left a comment
jjgrainger
left a comment
There was a problem hiding this comment.
@eugene-manuilov Looks good, just added a comment following on from @peterwilsoncc review
…w-control-for-which-sizes-to-generate-webp
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@eugene-manuilov This mostly looks solid. I have some thoughts on the data format chosen here which is overly complicated IMO. It makes sense to be able to use isset over array_search, but we should not make such very "minor" performance enhancements at the cost of developer experience (the duplication of data there right now is problematic to me). We can probably find a way to do it in a way that is both performant and more clear for developers.
felixarntz
changed the title
modules/images/webp-uploads/load.php
Outdated
| ); | ||
|
|
||
| foreach ( $additional_sizes as $size => $size_details ) { | ||
| if ( ! empty( $size_details['provide_additional_mime_types'] ) ) { |
There was a problem hiding this comment.
where does provide_additional_mime_types property come from or this something you were thinking we would add later?
There was a problem hiding this comment.
This would be the field that can later be populated via add_image_size's additional parameter. So having this here prepares us for that.
Technically, one could already set it now by directly touching the $_wp_additional_image_sizes global - which of course is discouraged, but I think it makes sense supporting this here already since we'll need it anyway and it's technically possible.
…w-control-for-which-sizes-to-generate-webp
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@eugene-manuilov Production code looks good, however we should remove all the additional sizes again. The remaining feedback is exclusively some minor things about tests.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@eugene-manuilov Looks great, thank you!
|
@adamsilverstein @peterwilsoncc Can you please give this a pass too? We need at least a second approval :) |
| add_image_size( 'allowed_size_400x300', 400, 300, true ); | ||
| add_image_size( 'not_allowed_size_200x150', 200, 150, true ); | ||
|
|
||
| $GLOBALS['_wp_additional_image_sizes']['allowed_size_400x300']['provide_additional_mime_types'] = true; |
There was a problem hiding this comment.
I assume this could be removed once we update add_image_size to accept the new "output_additional_mimes" parameter, right? Can you add a TODO comment to remove this once that is in place?
|
+1 looks good to me, I left one request to comment on the |
|
@adamsilverstein Added the |
felixarntz
deleted the
enhancement/413-allow-control-for-which-sizes-to-generate-webp
branch
|
FYI: WordPress/wordpress-develop#3166 is the new PR that implements a slightly modified version of this for WordPress core. |
6 participants
Summary
Fixes #413
Relevant technical choices
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.