add_image_size: add a new “disable_output_mapping” parameter#34
Conversation
eg. cat.jpeg, cat.jpg and cat.jpe (all valid jpegs) generate the same file names before this change, eg. cat-150x150.webp. adding the original file’s extension as a suffix on the generated filename neatly avoids this.
adamsilverstein
changed the title
add_image_size: add a new “disable_output_mapping” parameter
adamsilverstein
force-pushed
the
add/new-webp-approach-br
branch
from
5da6509 to
1446b81
Compare
adamsilverstein
changed the base branch from
add/new-webp-approach-br
to
add/new-webp-approach-br-additional-changes
There was a problem hiding this comment.
@adamsilverstein Left two early comments here. I think this PR and its approach are a bit preliminary and can only really be started once the two dependencies have been committed.
| // Check if this image size supports output mapping. | ||
| if ( ! isset( $this->size_name ) || empty( $_wp_additional_image_sizes[ $this->size_name ]['disable_output_mapping'] ) ) { | ||
| /** | ||
| * Filters the image editor output format mapping. | ||
| * | ||
| * Enables filtering the mime type used to save images. By default, | ||
| * the mapping array is empty, so the mime type matches the source image. | ||
| * | ||
| * @see WP_Image_Editor::get_output_format() | ||
| * | ||
| * @since 5.8.0 | ||
| * | ||
| * @param string[] $output_format { | ||
| * An array of mime type mappings. Maps a source mime type to a new | ||
| * destination mime type. Default is array( 'image/jpeg' => 'image/webp' ). | ||
| * | ||
| * @type string ...$0 The new mime type. | ||
| * } | ||
| * @param string $filename Path to the image. | ||
| * @param string $mime_type The source image mime type. | ||
| */ | ||
| $output_format = apply_filters( 'image_editor_output_format', array( 'image/jpeg' => 'image/webp' ), $filename, $mime_type ); |
There was a problem hiding this comment.
I would suggest we avoid changing any logic here. It would make more sense to incorporate that logic in the filter callback which was introduced in #35 and then is being further refined in WordPress#3166.
There was a problem hiding this comment.
makes sense, I'll adjust this once those other changes land.
| 'width' => absint( $width ), | ||
| 'height' => absint( $height ), | ||
| 'crop' => $crop, | ||
| 'disable_output_mapping' => $disable_output_mapping, |
There was a problem hiding this comment.
Why not keep with the original name and intended behavior? $additional_mime_types boolean still makes sense even with the new approach, the only difference is that now the additional MIME types are generated as a replacement for the default one.
In any case, at a minimum we need to keep the boolean value here compatible with the old idea that we developed previously: Default null, initially set to false and later that's gonna be changed to true. We must not diverge from that as then it'll be incompatible with the long-term goal here and the potential of reintroducing multi MIME types at some point.
There was a problem hiding this comment.
👍🏼 except I was assuming we would just start with this defaulting to enabled, then let developers opt out
There was a problem hiding this comment.
after discussing this further i'm open to going with the same null initial default approach we were using previously, then changing the default in a future release, giving developers more time to opt out for image sizes they know won't be used in a front end context.
|
Closing in favor of WordPress#3217 |
2 participants
Follow on to #35