Allow control for which image sizes to generate additional MIME type versions - #415
Conversation
Sorry, something went wrong.
… not allowed to have additional mime types.
jjgrainger
left a comment
There was a problem hiding this comment.
@eugene-manuilov Looks good, just added a comment following on from @peterwilsoncc review
Sorry, something went wrong.
…w-control-for-which-sizes-to-generate-webp
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.
Sorry, something went wrong.
| ); | ||
|
|
||
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
…w-control-for-which-sizes-to-generate-webp
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.
Sorry, something went wrong.
felixarntz
left a comment
There was a problem hiding this comment.
@eugene-manuilov Looks great, thank you!
Sorry, something went wrong.
|
@adamsilverstein @peterwilsoncc Can you please give this a pass too? We need at least a second approval :) |
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
|
+1 looks good to me, I left one request to comment on the $GLOBAL setting line in the tests which shouldn't be needed once add_image_size is updated. |
Sorry, something went wrong.
|
@adamsilverstein Added the TODO comment, back to you for approval. |
Sorry, something went wrong.
adamsilverstein
left a comment
There was a problem hiding this comment.
Excellent!
Sorry, something went wrong.
|
FYI: WordPress/wordpress-develop#3166 is the new PR that implements a slightly modified version of this for WordPress core. |
Sorry, something went wrong.
Summary
Fixes #413
Relevant technical choices
Checklist