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

[adamsilverstein]

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.

Trac ticket: https://core.trac.wordpress.org/ticket/54476

[kirasong]

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

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

[hellofromTonya]

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?

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

[adamsilverstein]

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.

[adamsilverstein]

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

[adamsilverstein]

@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.

[kirasong]

Hey Adam! Sorry for the wait on this.

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

[kirasong]

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!

[adamsilverstein]

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.

[adamsilverstein]

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

[antpb]

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

[adamsilverstein]

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

[adamsilverstein]

@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.

[adamsilverstein]

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​

[adamsilverstein]

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).

[adamsilverstein]

@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.

[ironprogrammer]

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):

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):

❌ Before patch: output to WebP is not honored:

✅ After patch: output to WebP is honored:

[kirasong]

@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?

[adamsilverstein]

@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.

[kirasong]

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!

[adamsilverstein]

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

[adamsilverstein]

54476.5.diff​ includes the latest changes from the PR.

[adamsilverstein]

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.

[kirasong]

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!