Fix converting images from heic to jpeg #7667
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. |
| // 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.
| 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.
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.
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.
| 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.
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.
| 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.
| $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.
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.
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.
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.
| if ( $scale_down ) { | |
| if ( true === $scale_down ) { |
| * | ||
| * @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.
| * @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.
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.?
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.