#56442 closed defect (bug) (fixed)
Ensure `wp_editor_set_quality` filter consistently passes output mime type
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | 5.8 |
Component: | Media | Keywords: | has-patch has-unit-tests dev-feedback commit |
Focuses: | Cc: |
Description
In https://core.trac.wordpress.org/ticket/53667 we extended the capability of the wp_editor_set_quality
filter so that the passed mime_type
parameter could be used to set the default image quality per mime type.
Unfortunately, in my testing this filter doesn't quite work correctly: the output mime type (if set with the image_editor_output_formats
filter) is not passed consistently to the filter.
I used this code in an mu plugin to test the adjusting just WebP output quality filter (and log the passed mime type) with a "real world" test: setting output quality either very high (100) or very low (1) for webp only and comparing the output file size. The filter worked for the -scaled.webp
but not the rest of the sub-sizes.
In addition to the filter not passing the correct mimes, we need to expand our testing in test_set_quality_with_image_conversion
which continues to pass even though the filter isn't working correctly, possibly because it is only testing a single image generation call, or only testing the results of calling get_quality
not the actual quality used.
One test we could add would mimic what I am doing to test this manually: generate all subsizes with the filter in place - applying only to image/webp
mime type images - returning first low quality, then high quality, - calling wp_create_image_subsizes
with no filter, then each option, then verifying the file sizes are all smaller with the lower quality applied and bigger (than default) with high quality applied.
Note: I assume this is a bug that affected 5.8+ and have marked the ticket as such. I also verified this is still a bug in trunk.
Attachments (7)
Change History (23)
This ticket was mentioned in PR #3250 on WordPress/wordpress-develop by adamsilverstein.
3 years ago
#1
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
#2
@
3 years ago
- Add a new test
test_quality_with_image_conversion_file_sizes
to verify that thewp_editor_set_quality
correctly passes the mime type and applies correctly:- uses the
image_editor_output_format
filter to generate both JPEG and WebP sub sizes for a test image (including a-scaled
version). - sets quality very low for JPEG and very high for WebP and verifies that the file sizes are all larger for the WebP generated files.
- uses the
- Update
WP_Image_Editor::set_quality
to set the output type by callingget_output_format
.
These tests fail in trunk because the mime type is not set to the output type early enough - the first time it is called for WebP (for each editor initialization), the mime type was set to JPEG even if the eventual output was in WebP. Therefore, certain image sizes were using the JPEG quality setting, not the expected WebP quality setting.
Previous tests for mime type in wp_editor_set_quality weren't catching this because they didn't look at all runs of the filter. The new test looks at all generated sizes.
The fix is a single line change to call get_output_format
in set_quality. This ensures that $this->output_format
is properly set on the next line and the correct mime type is used. After this change, the test passes - the correct mime type is passed to the filter..
adamsilverstein commented on PR #3250:
3 years ago
#3
Hmm, some other tests fail now because they assert that quality will not be set until save (see test_set_quality_with_image_conversion
), I'm going to think and discuss more about the best approach here.
#4
@
3 years ago
- Keywords dev-feedback added
- All tests pass: new tests verify created size is smaller
This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.
3 years ago
#7
@
3 years ago
56442.4.diff refresh against trunk
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
3 years ago
#10
@
2 years ago
56442.5.diff is a refresh against trunk; this should be good to commit, pending review and feedback.
#11
@
2 years ago
56442.5.diff almost looks good for commit to me, I left feedback in https://github.com/WordPress/wordpress-develop/pull/3250.
#12
@
2 years ago
- Keywords commit added
56442.6.diff addresses the latest feedback on the ticket and is ready for commit
adamsilverstein commented on PR #3250:
2 years ago
#13
@peterwilsoncc thanks for the review and feedback. I addressed all your points and assuming tests pass this should be ready for commit.
Still manually testing the fix though.
A good way to test this filter is to output WebPs and try setting the quality very high or low, then checking your output files. A filter based on mime type won't work correctly before this fix.
Here are the filters to use:
{{{php
Output WebP.
add_filter( 'image_editor_output_format', function() {
return array(
'image/jpeg' => 'image/webp',
);
});
}}}
{{{php
Filter WebP quality.
add_filter( 'wp_editor_set_quality', function( $quality, $mime_type ) {
if ( 'image/webp' === $mime_type ) {
return 99; Very high quality, files will be very much larger than usual. Alternately, try a very low value.
}
return $quality;
}, 10, 2 );
}}}
#14
@
2 years ago
Thanks for the feedback @peterwilsoncc - I have addressed and added some steps for manual testing if you still want to try to do that. I have tested manually and am confident in the fix.
Tests fail currently, demonstrating issue
Trac ticket: