Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Modern Image Format not cropping image if crop is an array #1887

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

b1ink0
Copy link
Contributor

@b1ink0 b1ink0 commented Feb 26, 2025

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:

add_filter( 'pre_option_medium_crop', function() { return array( 'right', 'right' ); } );

@b1ink0 b1ink0 added [Type] Bug An existing feature is broken [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Feb 26, 2025
@b1ink0 b1ink0 added this to the webp-uploads n.e.x.t milestone Feb 26, 2025
Copy link

codecov bot commented Feb 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.59%. Comparing base (8050a1b) to head (af35deb).
Report is 35 commits behind head on trunk.

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk    #1887      +/-   ##
==========================================
+ Coverage   67.90%   69.59%   +1.69%     
==========================================
  Files          86       86              
  Lines        6997     6983      -14     
==========================================
+ Hits         4751     4860     +109     
+ Misses       2246     2123     -123     
Flag Coverage Δ
multisite 69.59% <100.00%> (+1.69%) ⬆️
single 39.62% <100.00%> (+1.88%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@b1ink0 b1ink0 marked this pull request as ready for review February 26, 2025 18:30
Copy link

github-actions bot commented Feb 26, 2025

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 props-bot label.

Unlinked Accounts

The 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.

Unlinked contributors: 1ucay.

Co-authored-by: b1ink0 <b1ink0@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment on lines 243 to 257
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'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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'];

Copy link
Contributor Author

@b1ink0 b1ink0 Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Bug An existing feature is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modern Image Format no crop, if crop is array
2 participants