Fix Modern Image Format not cropping image if crop is an array #1887
Conversation
b1ink0
added
[Type] Bug
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1887 +/- ##
=======================================
Coverage 69.59% 69.59%
=======================================
Files 86 86
Lines 6983 6983
=======================================
Hits 4860 4860
Misses 2123 2123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
b1ink0
requested review from
adamsilverstein,
felixarntz and
westonruter
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @1ucay. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@b1ink0 This looks great! Just one recommendation to simplify the implementation and make it more future-proof.
Another point of feedback: It would be great if you could add a unit test to verify the fix. You could write a test that calls webp_uploads_generate_additional_image_source() with $size_data['crop'] set to an array and verify the output is correct.
plugins/webp-uploads/helper.php
Outdated
| if ( | ||
| isset( $sizes[ $size ]['crop'] ) && | ||
| ( | ||
| ( | ||
| is_array( $sizes[ $size ]['crop'] ) && | ||
| count( $sizes[ $size ]['crop'] ) === 2 && | ||
| isset( $sizes[ $size ]['crop'][0] ) && | ||
| isset( $sizes[ $size ]['crop'][1] ) && | ||
| is_string( $sizes[ $size ]['crop'][0] ) && | ||
| is_string( $sizes[ $size ]['crop'][1] ) | ||
| ) || | ||
| is_bool( $sizes[ $size ]['crop'] ) | ||
| ) | ||
| ) { | ||
| $size_data['crop'] = $sizes[ $size ]['crop']; |
There was a problem hiding this comment.
I don't think we need to do all this granular validation on the crop key. WordPress Core doesn't do it and lets everything through. It only ensures at the very last point when it's actually used (in the image_resize_dimensions() function) that it has the correct format.
I think allowing any value here to pass through makes more sense as it makes our code aligned with Core and more future-proof, in case Core would later add support for other types for the crop key. It's also in line with width and height above, where we don't verify the type.
| if ( | |
| isset( $sizes[ $size ]['crop'] ) && | |
| ( | |
| ( | |
| is_array( $sizes[ $size ]['crop'] ) && | |
| count( $sizes[ $size ]['crop'] ) === 2 && | |
| isset( $sizes[ $size ]['crop'][0] ) && | |
| isset( $sizes[ $size ]['crop'][1] ) && | |
| is_string( $sizes[ $size ]['crop'][0] ) && | |
| is_string( $sizes[ $size ]['crop'][1] ) | |
| ) || | |
| is_bool( $sizes[ $size ]['crop'] ) | |
| ) | |
| ) { | |
| $size_data['crop'] = $sizes[ $size ]['crop']; | |
| if ( isset( $sizes[ $size ]['crop'] ) ) { | |
| $size_data['crop'] = $sizes[ $size ]['crop']; |
There was a problem hiding this comment.
I added granular checks to ensure that the Modern Image Formats code does not produce errors on invalid input. However, to future-proof code, as you suggested, simplifying the condition makes more sense. Fixed in af35deb.
westonruter
left a comment
westonruter
left a comment
There was a problem hiding this comment.
The changes look good to me!
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@b1ink0 This looks excellent, thank you for the iterations and the unit test!
3 participants
Summary
Fixes #1881
Relevant technical choices
This PR fixes the issue of Modern Image Format not cropping image if crop is an array.
Below is the code snippet to reproduce the bug: