-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 converting images from heic to jpeg #7667
base: trunk
Are you sure you want to change the base?
Conversation
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
PR may need to be adjusted a bit after https://core.trac.wordpress.org/ticket/62272. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions for strict comparison, a possibly WPCS fix pointed out and a grammar fix found.
Other than that I think this is fine.
// The image may need to be converted regardless of its dimensions. | ||
$output_format = wp_get_image_editor_output_format( $file, $imagesize['mime'] ); | ||
|
||
if ( is_array( $output_format ) && array_key_exists( $imagesize['mime'], $output_format ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( is_array( $output_format ) && array_key_exists( $imagesize['mime'], $output_format ) ) { | |
if ( isset( $output_format[ $imagesize['mime'] ] ) { |
In my eyes this reduces complexity of the condition and following isset vs array_key_exists, this makes no negative difference and would actually be slightly stricter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Yes, isset
would work here too however it would reduce readability a tiny bit. Having the word array
in the function name makes this bit of code self-documenting. (Also thinking that array_key_exists
may be using isset
under the hood, perhaps.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my eyes this improves the readability, that’s why I suggested it. Since it reduces the complexity.
} | ||
} | ||
|
||
if ( $scale_down || $convert ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( $scale_down || $convert ) { | |
if ( true === $scale_down || true === $convert ) { |
We know for sure that these are bool, so why not do a strict comparison here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both $scale_down
and $convert
are designed to be hard-coded boolean variables. They cannot be anything else. Thinking that strict comparison is redundant here, and may perhaps give the impression that these simple vars can be more complex/different types which would be undesirable.
if ( $scale_down ) { | ||
// Resize the image. This will also convet it if needed. | ||
$resized = $editor->resize( $threshold, $threshold ); | ||
} elseif ( $convert ) { | ||
// The image will be converted (if possible) when saved. | ||
$resized = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( $scale_down ) { | |
// Resize the image. This will also convet it if needed. | |
$resized = $editor->resize( $threshold, $threshold ); | |
} elseif ( $convert ) { | |
// The image will be converted (if possible) when saved. | |
$resized = true; | |
} | |
if ( true === $scale_down ) { | |
// Resize the image. This will also convet it if needed. | |
$resized = $editor->resize( $threshold, $threshold ); | |
} elseif ( true === $convert ) { | |
// The image will be converted (if possible) when saved. | |
$resized = true; | |
} |
Strict comparison as mentioned above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
$rotated = null; | ||
|
||
// If there is EXIF data, rotate according to EXIF Orientation. | ||
if ( ! is_wp_error( $resized ) && is_array( $exif_meta ) ) { | ||
$resized = $editor->maybe_exif_rotate(); | ||
$rotated = $resized; | ||
$rotated = $resized; // bool true or WP_Error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment allowed in WPCS? I always thought the rule of Capital letter at the beginning and a .
at the end is strict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure why this type of comments should be banned? Seems WPCS (the actual coding standards, and the PHPCS sniffs) don't have any restrictions for such inline comments.
It may make sense to enforce capital letters and full stops when the comments are whole sentences (although that would be incompatible with the "Code is Poetry" motto, perhaps). This one is not a sentence :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve checked the documentation and I can neither find anything that says it’s follows the coding standards nor that it does not.
So I guess we can leave it like it is.
} | ||
|
||
if ( ! is_wp_error( $resized ) ) { | ||
/* | ||
* Append "-scaled" to the image file name. It will look like "my_image-scaled.jpg". | ||
* This doesn't affect the sub-sizes names as they are generated from the original image (for best quality). | ||
*/ | ||
$saved = $editor->save( $editor->generate_filename( 'scaled' ) ); | ||
if ( $scale_down ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( $scale_down ) { | |
if ( true === $scale_down ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
* | ||
* @see WP_Image_Editor::get_output_format() | ||
* | ||
* @since 5.8.0 | ||
* @since 6.7.0 The default was changed from array() to array( 'image/heic' => 'image/jpeg' ). | ||
* @since 6.7.0 The default was changed from empty array to array containing the HEIC mime types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @since 6.7.0 The default was changed from empty array to array containing the HEIC mime types. | |
* @since 6.7.0 The default was changed from empty array to an array containing the HEIC mime types. |
Grammar feels wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea the grammar feels a bit wrong. If this is supposed to be a whole sentence wouldn't it be better as:
@since 6.7.0 The default was changed from an empty array to an array containing the HEIC mime types.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for me.
Fix converting all heic images to jpeg
Trac ticket: https://core.trac.wordpress.org/ticket/62305
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.