Discard WebP image if it is larger than corresponding JPEG image

[mukeshpanchal27]

Summary

After these changes, it affects many current unit tests because those tests are expecting .webp extensions but the images are bigger, so they end up with .jpg extensions. To handle the tests I use the filter approach to pass tests. thanks, @peterwilsoncc for that review.

After implementing this change we no longer need the smaller file size filter and related code. The filter $prefer_smaller_image_file = apply_filters('webp_uploads_prefer_smaller_image_file', false); and code for full size image https://github.com/WordPress/performance/blob/trunk/modules/images/webp-uploads/load.php#L477-L485 and thumbnail size image https://github.com/WordPress/performance/blob/trunk/modules/images/webp-uploads/load.php#L533-L541 because now images have only files that have a smaller size.

Introduce a new filter webp_prioritise_filesize_over_mime_type to handle old unit tests.

The new file size changes affect some old tests to handle it a new filter webp_prioritise_filesize_over_mime_type introduce, that allow users to disable to the new behavior. by default its value is true in code base. For the unit tests we don't need this filter so added add tests_add_filter( 'webp_prioritise_filesize_over_mime_type', '__return_false' ); in the tests bootstrap file so old tests don;t affect with new changes. For new tests, we include add_filter( 'webp_prioritise_filesize_over_mime_type', '__return_true' );.

Fixes #372

Checklist

• PR has either [Focus] or Infrastructure label.
• PR has a [Type] label.
• PR has a milestone or the no milestone label.

[mehulkaklotar]

@mukeshpanchal27 Please have a look at my comments below.

[adamsilverstein]

document @return

[mukeshpanchal27]

Thanks for the feedback, Adam.

I think it should be a param for the filter, as I checked the similar filters in the Performance plugin and WordPress code, and there it uses param for the filters.

https://github.com/WordPress/performance/blob/trunk/modules/images/webp-uploads/load.php#L430-L442
https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/options.php#L198-L208
https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/class-wp-rest-server.php#L267-L274

[adamsilverstein]

This documentation can be improved a bit - the filter controls whether to retain WebP images even if they are larger than the jpegs, right?

Maybe something like
"Filter whether WebP images that are larger than the matching JPEG should be discarded. Default is true"

Also makes me think the filter name should reflect this as well, maybe something like webp_uploads_discard_larger_generated_images, curious what @felixarntz thinks since he is great at naming things.

[mukeshpanchal27]

Thanks for the feedback. I have updated the documentation for the filter and updated the filter name to webp_uploads_discard_larger_generated_images for now. Will update it when Felix reviews this and suggests the changes regarding the filter name.

[adamsilverstein]

Overall approach seems solid, nice work! I left a few suggestions mostly around filter naming and documentation.

[eugene-manuilov]

Thanks, @mukeshpanchal27. This is a good start. Left some comments for you to address. Please, take a look.

[felixarntz]

@mukeshpanchal27 Mostly looks solid to me. I have one suggestion about avoiding duplicate code.

[felixarntz]

@mukeshpanchal27 Thank you for the updates. The production code looks good now, except for the small problem with the doc block for the filter.

Now that we have this function, can you add test coverage for it? Probably a good one to use a @dataProvider with different values for the $original and $additional, plus I think it would be good to also have one test to ensure that setting the filter to false disables that behavior.

[mukeshpanchal27]

@felixarntz Thank you for the feedback. I have addressed the feedback and updated the PR and unit tests accordingly.

[spacedmonkey]

Stupid question, what about pngs and gifs?

[felixarntz]

@mukeshpanchal27 Pretty much LGTM! Two minor nit-picks that aren't blockers though.

[felixarntz]

@spacedmonkey

Stupid question, what about pngs and gifs?

The scope of the current project is only to generate WebP for JPEG. For PNG and even more so GIF, there are additional complexities to consider. If someone wanted to generate WebP from PNG (or even JPEG from PNG), they could easily do so using the available filters, and it all would be subject to the same behavior as here. This is in fact a very powerful feature of this "additional MIME type support" feature, that it can also satisfy lots of other potential use-cases with few lines of code. We can always think about changing the defaults and expanding support inside of core later.

[felixarntz]

@adamsilverstein @eugene-manuilov Can you please re-review this PR since you previously requested changes?

[adamsilverstein]

Left one nit pick spacing feedback item on a doc block. Nice work!

[mukeshpanchal27]

Assign it to @felixarntz. It is now ready to merge.

[bethanylang]

@mukeshpanchal27 I still see unaddressed changes from @eugene-manuilov and it sounds like @felixarntz wanted him to do a final review, as well – can you take another look?

[mukeshpanchal27]

@bethanylang All the changes are addressed, which were raised by @eugene-manuilov. and resolved all conversations.

[eugene-manuilov]

LGTM

[felixarntz]

@mukeshpanchal27 Looks like there is a merge conflict here now that something else got merged. Can you please address this? Then we can merge it.

[mukeshpanchal27]

@felixarntz Merge conflict resolved.

The foreach loop added for the https://github.com/WordPress/performance/blob/trunk/modules/images/webp-uploads/load.php#L625-L633 to get $target_mime.

[felixarntz]

Excellent, thanks @mukeshpanchal27!