Make WordPress Core

Opened 2 years ago

Closed 14 months ago

Last modified 14 months ago

#54476 closed defect (bug) (fixed)

Improve image engine detection when output format adjusted with filter (example: WebP output)

Reported by: adamsilverstein's profile adamsilverstein Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.1 Priority: normal
Severity: normal Version: 5.8
Component: Media Keywords: has-patch needs-testing commit
Focuses: Cc:

Description

WordPress supports both Imagick and LibGD to generate sub sized images. When users upload a new image, the function wp_get_image_editor is called, which in turn passes the image to _wp_image_editor_choose which uses the file's mime type to determine which engine should be used for that image type.

In some cases, users have both Imagick and LibGD installed and each engine supports different formats. For example, LibGD supports WebP, but Imagick does not. WordPress picks the correct engine to use based on the images mime type. This works fine, except when we attempt to adjust the default output format using the new image_editor_output_format filter introduced in version 5.8.

I saw some reports on this on the modern image plugin, where the user could not output WebP images by default even though their system appears to support it. See https://github.com/adamsilverstein/modern-images-wp/issues/13. In these cases, WordPress picks the wrong engine for the image, and the sub size generation fails.

To fix this, we need to run the mime type through the same transformation we do in get_output_format before calling _wp_image_editor_choose.

Patch incoming.

Attachments (8)

54476.diff (628 bytes) - added by adamsilverstein 2 years ago.
54476.2.diff (1.9 KB) - added by adamsilverstein 19 months ago.
54476.3.diff (2.3 KB) - added by adamsilverstein 16 months ago.
54476.4.diff (3.0 KB) - added by adamsilverstein 16 months ago.
shim-for-testing.diff (1.2 KB) - added by adamsilverstein 14 months ago.
Monosnap Site Health - Info ‹ wpdev build — WordPress 2022-09-30 11-31-48.jpg (490.7 KB) - added by adamsilverstein 14 months ago.
54476.5.diff (3.3 KB) - added by adamsilverstein 14 months ago.
54476.6.diff (2.7 KB) - added by adamsilverstein 14 months ago.

Download all attachments as: .zip

Change History (42)

This ticket was mentioned in PR #1918 on WordPress/wordpress-develop by adamsilverstein.


2 years ago
#1

  • Keywords has-patch added

#2 @mikeschroder
2 years ago

Thanks so much for the patch! Will be glad to see this addressed.

I left a question / comment over on the GitHub side.

#3 @adamsilverstein
21 months ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 6.0

#4 @adamsilverstein
21 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#5 @adamsilverstein
21 months ago

  • Summary changed from Improve image engine detection when output format adjusted with filter. to Improve image engine detection when output format adjusted with filter (example: WebP output)

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


20 months ago

#7 @hellofromTonya
19 months ago

  • Keywords needs-testing needs-testing-info added
  • Milestone changed from 6.0 to 6.1

6.0's last beta is happening in < 20 minutes and tomorrow is RC1. Moving this to 6.1 and adding needs-testing keyword.

@adamsilverstein how can a tester manually test to reproduce the issue and validate the patch?

adamsilverstein commented on PR #1918:


16 months ago
#8

@getsource This is ready for another review, I will fix the failing tests

#9 @adamsilverstein
16 months ago

54476.3.diff includes the latest changes feedback from the PR. Based on feedback from @mikeschroder the check now looks for an engine that supports the input and output types.

#10 @adamsilverstein
16 months ago

  • Keywords needs-refresh needs-testing-info removed

#11 @adamsilverstein
16 months ago

@mikeschroder this is ready for additional review, all tests are passing on the PR.

#12 @adamsilverstein
16 months ago

@adamsilverstein how can a tester manually test to reproduce the issue and validate the patch?

@hellofromTonya to reproduce the bug you need both Imagick and GD set up and active, with WebP support only available in GD. Try testing WebP output - before this patch, the system with prefer Imagick and WebP output will fail. After the patch, wp_get_image_editor will properly consider the output format and return the GD editor - WebP generation will work as expected.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


16 months ago

#14 @mikeschroder
16 months ago

Hey Adam! Sorry for the wait on this.

I'm trying to get an environment set up that can test this. 🤔

#15 @mikeschroder
16 months ago

Some additional details:
I’m trying to test this, but having difficulty creating the conditions at the moment.
It looks like turning off WebP via policy.xml yields existing but empty WebP files. 😅

I believe this due to an existing issue with Imagick not reflecting format availability based on policy.xml, which (slightly off-topic) it’d be great if we had some way of detecting.

In the meantime, it makes it a bit harder to test. If anyone has any recommendations, it'd be appreciated!

#16 @adamsilverstein
16 months ago

I haven't tried changing policy.xml and am not familiar with that. I can one of the contributors that reported this issue on the image plugin to test as well.

#17 @adamsilverstein
16 months ago

I requested review from the contributors on https://github.com/adamsilverstein/modern-images-wp/issues/13 who also reported this issue.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


16 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


15 months ago

#20 @antpb
15 months ago

Hey @adamsilverstein do you think this is still good for 6.1 or should we move to 6.2?

#21 @adamsilverstein
15 months ago

This is a bug so fix can still land in 6.1. Mostly waiting for some confirmation from another tester.

#22 @adamsilverstein
14 months ago

@mikeschroder this is ready for another review if you have a chance; all tests are passing. i'm going to run a manual test to verify this works correctly then mark for commit pending any feedback.

#23 @adamsilverstein
14 months ago

shim-for-testing.diff is a shim that helps testing this bug: it disables WebP support manually for Imagick, with this applied, and a GD engine that supports WebP, WebP output will fail on trunk but work after 54476.4.diff

#24 @adamsilverstein
14 months ago

Monosnap Site Health - Info ‹ wpdev build — WordPress 2022-09-30 11-31-48.jpg shows my site health screen after applying the shim.

Testing in trunk, output files were JPEG, and Imagick was the chosen editor.

Testing after 54476.4.diff WebP image generation works as expected. _wp_image_editor_choose prefers the editor that supports both the input and output mime types - GD in this case, instead of picking the first editor that supports the input type (Imagick in this case).

#25 @adamsilverstein
14 months ago

  • Keywords commit added

#26 follow-up: @adamsilverstein
14 months ago

@mikeschroder after my testing I am highly confident this fixes an (admittedly edge case) bug. Marking as commit; I'd love to have you test/review again as well for confirmation before committing though.

#27 @ironprogrammer
14 months ago

Test Report

This report combines reproduction and patch testing.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/54476/54476.4.diff

Steps to Reproduce

  1. Install Modern Images WP plugin (conveniently sets output format via the image_editor_output_format filter mentioned in original report).
  2. In Settings > Media, configure plugin to set "For jpeg images" to "WebP". Click Save Changes.
  3. Navigate to Tools > Site Health, Info tab, and expand the Media Handling section.
  4. If the "ImageMagick supported file formats" section lists WEBP, apply shim-for-testing.diff provided by @adamsilverstein (this disables WEBP support in ImageMagick). Refresh page after patching to confirm WEBP is no longer in the list of formats.
  5. In Media, upload a JPEG file (either .jpg or .jpeg extension).
  6. On the file system or command line, navigate to the WordPress uploads directory and observe the alternate size files that were generated.

Expected Results

  • ❌ The alternate size images generated are JPEG, not WebP as configured by the plugin (filter).

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 12.6
  • Browser: Safari 16.0
  • Server: nginx/1.23.1
  • PHP: 7.4.30
  • WordPress: 6.1-beta2-54337-src
  • Theme: twentyseventeen v3.0
  • Active Plugins:
    • modern-images-wp v1.0.3

Actual Results

  • ❌ Before patch, issue reproduced: observed that JPEG images were saved as JPEG (original format).
  • ✅ After patch, JPEG images were saved as WebP, as set by the plugin's image_editor_output_format filter.

Supplemental Artifacts

Site Health Media Handling section, showing no WebP support (with shim applied):
https://cldup.com/z3gFV9jXT6.thumb.jpg

Modern Images WP configuration for JPEG, configured for output to WebP (note that WebP support is listed here, though has been disabled, as visible in Site Health):
https://cldup.com/sWJg5uqbXZ.thumb.jpg

❌ Before patch: output to WebP is not honored:
https://cldup.com/qSqQwPPfNE.thumb.jpg

✅ After patch: output to WebP is honored:
https://cldup.com/XZuXHvJnQ2.thumb.jpg

#28 in reply to: ↑ 26 @mikeschroder
14 months ago

@adamsilverstein Thanks!

I still don't have an environment I can test this on, but I can do a code review tomorrow.

Is it 54476.4.diff that should be reviewed, or the most recent revision of the PR?

Last edited 14 months ago by mikeschroder (previous) (diff)

#29 @adamsilverstein
14 months ago

@ironprogrammer - fantastic, thanks for reproducing the testing!

I still don't have an environment I can test this on, but I can do a code review tomorrow.

@mikeschroder I couldn't reproduce either, hence the shim shim-for-testing.diff which takes a platform with imagick and GD both supporting WebP and hacks away support for WebP in Imagick only. You can use this to simulate the failing environment.

Is it 54476.4.diff​ that should be reviewed, or the most recent revision of the PR?

Those should be the same, my preference is for you to review in GitHub - that way you can leave comments inline directly on the code.

#30 @mikeschroder
14 months ago

Left one minor question/suggestion over on the PR.

Otherwise, it looks good!

Since I recognize you're probably trying to land it before the beta:
I haven't had a chance to test this with the shim yet, but I think if it's working for you, along with @ironprogrammer's testing (thank you!!), I'm comfortable with it landing.

Happy to do some more testing as well, but wanted to make sure to leave some timely feedback.
Hopefully this helps, and thanks so much for your work on this!

#31 follow-up: @adamsilverstein
14 months ago

@mikeschroder thanks for the feedback; I updated the PR slightly to address your question. Once tests pass this should be good for commit.

#32 @adamsilverstein
14 months ago

54476.5.diff includes the latest changes from the PR.

#33 @adamsilverstein
14 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 54416:

Media: improve image engine detection when using the output format filter.

When the output format is altered with the image_editor_output_format filter, prefer the image engine that supports both input an output types, falling back to the engine that supports the input type.

Correct an issue where the output format filter wasn't respected because the selected engine didn't support the output format.

Props mikeschroder, ironprogrammer.
Fixes #54476.

#34 in reply to: ↑ 31 @mikeschroder
14 months ago

Replying to adamsilverstein:

@mikeschroder thanks for the feedback; I updated the PR slightly to address your question. Once tests pass this should be good for commit.

Great; thanks so much for handling this!

Note: See TracTickets for help on using tickets.