WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#53475 closed defect (bug) (fixed)

No error shown on uploading a WebP image when the server doesn't support editing it

Reported by: azaozz Owned by: adamsilverstein
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.8
Component: Media Keywords: has-patch
Focuses: Cc:

Description

Follow up to #35725.

When the server doesn't support editing of WebP files (sub-sizes cannot be created), and the user is trying to upload one, seems the uploading should be interrupted/denied and an error message shown.

This seems to fail in some cases. Testing in PHP 7.3.14, no ImageMagick, GD version bundled (2.1.0 compatible) (no WebP support), the image is uploaded successfully and sub-sizes are not created.

The expected behavior would be to show an error and not allow the uploading.

Attachments (4)

53475.diff (1.8 KB) - added by adamsilverstein 6 months ago.
Media Library ‹ test — WordPress 2021-06-22 09-41-52.jpg (294.0 KB) - added by adamsilverstein 6 months ago.
53475.2.diff (1.7 KB) - added by adamsilverstein 6 months ago.
53475.3.diff (3.2 KB) - added by azaozz 6 months ago.

Download all attachments as: .zip

Change History (31)

#1 @azaozz
6 months ago

  • Keywords needs-patch added

Thinking perhaps would be good to have a wp_image_webp_support() or similarly named function to quickly/easily determine if WebP images can be processed by the server.

Seems such function can use WP_Image_Editor_GD::supports_mime_type( 'image/webp' ) or WP_Image_Editor_Imagick::supports_mime_type( 'image/webp' ) although to access these functions the classes may need to be tweaked a bit.

Alternatively can do directly gd_into() or Imagick::queryFormats().

#2 @desrosj
6 months ago

There's currently wp_show_heic_upload_error() from [48288]. What if this was made more generic to support any format that a user can upload but the server cannot actually manipulate?

#3 follow-up: @joyously
6 months ago

Why is the expected behavior an error? Would it be better to fall back to JPG?

#4 in reply to: ↑ 3 @desrosj
6 months ago

Replying to joyously:

Why is the expected behavior an error? Would it be better to fall back to JPG?

Maybe error is the wrong term here.

I think the goal is to let the user know that sub-sizes could not be created. In my opinion, I consider that more of a warning because the upload will succeed, but the user should be made aware that all of the expected steps could not be taken because the server lacked the needed support.

I don't think that there's really any way to fallback to another format because ImageMagick/GD won't be able to read the WebP format. The notice for HEIC format is "This image cannot be displayed in a web browser. For best results convert it to JPEG before uploading." Personally, I see that as a similar scenario. The only difference is that there's no way to add support for HEIC currently. WebP support can be added.

#5 @adamsilverstein
6 months ago

Interesting, I hadn't seen wp_show_heic_upload_error!

I was thinking that if the server doesn't support WebP maybe we should block uploads, it is a soft error in that the upload will work and only the image resizing will fail, but this may mean a broken site for the user when they try to use the image, so perhaps preventing the upload in the first place is better?

I prepared a patch that alters get_allowed_mime_types, excluding WebP when the server lacks support. This causes an error to display when you try to upload a WebP on such a system I also updated the error message slightly. Patch incoming.

#6 follow-up: @joyously
6 months ago

If the original WebP can be displayed, are the subsizes really needed? That way the "error" represents what's really happening.

#7 @adamsilverstein
6 months ago

If the original WebP can be displayed, are the subsizes really needed?

In most cases WordPress outputs a srcset attribute for images that specifies a specific image file for specific "media breakpoints" - using this approach, we serve smaller sized images for smaller screens, and larger images for larger screens. The exact cutoffs and number of image sizes is typically set by the theme.

Without the resized images, the page will display the original image, which is typically much larger than what is needed - wasting bandwidth and slowing down the page load. In short something you really want to avoid!

Unfortunately most users won't realize what has happened if we don't display an error, they will be able to use the image but in a way that makes their site slower. These users are better off using jpeg until they are on server that supports WebP. They may be able to upload a WebP image, but without the server side support for WebP, WordPress doesn't fully support using the images, for example cropping the image will also fail on these systems!

That way the "error" represents what's really happening.

I changed the error string to "Sorry, this file type is not supported." which seems accurate, what do you think?

#8 follow-up: @adamsilverstein
6 months ago

In 53475.diff:

  • Improve get_allowed_mime_types: only enable WebP when the server supports it.

Looking at this now I can move the logic into a more general function as @azaozz suggested, I'll update the patch.

#9 @adamsilverstein
6 months ago

Since we don't have access to the file in get_allowed_mime_types, we still need the conditional based on the active image editor, making a generalized helper call harder. I cleaned the logic up a bit in 53475.2.diff

#10 @adamsilverstein
6 months ago

  • Tested with PHP 5.6, no WebP support: Error on uploading, prevented.
  • Same PHP with Imagick with WebP support: image upload works, resizing works
  • PHP 7.4.13 GD with WebP support: image upload & resizing works

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


6 months ago

  • Keywords has-patch added; needs-patch removed

#12 in reply to: ↑ 8 @azaozz
6 months ago

Replying to adamsilverstein:

Uh, there's a better way to do this, can use wp_image_editor_supports(). Will push an update to the PR.

Yeah, been wondering if it would make sense to have a more generic function. Seems using wp_image_editor_supports( array( 'mime_type' => 'image/webp' ) ) is good enough unless we want to cache the result to speed it up a bit.

But then it is pretty fast already, and get_allowed_mime_types() doesn't seem to be used a lot, so... 50/50 in it :)

Last edited 6 months ago by azaozz (previous) (diff)

#13 in reply to: ↑ 6 @azaozz
6 months ago

Replying to joyously:

If the original WebP can be displayed, are the subsizes really needed?

In some cases it will probably work but the image will not be post-processed and will not be displayed properly on the front-end. There will be no srcset and sizes attributes, the originally uploaded file will be used in all cases even when it is way too large and few MB in size.

However when a specific image sub-size is needed, for example when a (cropped) sub-size is required by the theme or the image is used for a header background, site icon, etc. it will fail as that image will not be available.

Last edited 6 months ago by azaozz (previous) (diff)

#14 @adamsilverstein
6 months ago

  • Keywords needs-testing added
  • Owner set to adamsilverstein
  • Status changed from new to assigned

Uh, there's a better way to do this, can use wp_image_editor_supports(). Will push an update to the PR.

Aha, excellent! I completely missed that.

PR looks good to me, I can give it another test to verify it correctly detects/applies webp server support in my local test environments.

This ticket was mentioned in Slack in #core by azaozz. View the logs.


6 months ago

#16 @adamsilverstein
6 months ago

  • Keywords commit added

Testing results:

  • Tested with PHP 5.6, no WebP support: Error on uploading, prevented.
  • Same PHP with Imagick with WebP support: image upload works, resizing works
  • PHP 7.4.13 GD with WebP support: image upload & resizing works

Commit!

#17 @adamsilverstein
6 months ago

  • Keywords needs-testing removed

#18 @azaozz
6 months ago

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

In 51211:

Media: Prevent uploading and show an error message when the server doesn't support editing of WebP files and image sub-sizes cannot be created.

Props adamsilverstein, desrosj, azaozz
Fixes #53475

This ticket was mentioned in Slack in #core by azaozz. View the logs.


6 months ago

#20 @iandunn
6 months ago

In 51223:

Media: Revert r51211 to restore ms-files.php assets.

r51211 accidentally introduced a fatal error for Multisite instances with ms_files_rewriting enabled. Reverting removes the error, and the original purpose of the commit can be solved in another way.

Props otto42, barry, ryelle, azaozz.
Fixes #53492. See #53475.

#21 @iandunn
6 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@azaozz
6 months ago

#22 @azaozz
6 months ago

  • Keywords needs-testing added; commit removed

In 53475.3.diff: Fix this similarly to how .heic images are handled, see [48288]. Prevents uploading of WebP images when the server cannot edit them by passing a setting to Plupload, then triggering an error.

That also allows to show a better error message (suggestions for improving the text are welcome):

This image cannot be post-processed by the web server. Convert it to JPEG or PNG before uploading.

Please test asap to be able to add in beta-4.

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


6 months ago

#24 follow-up: @antpb
6 months ago

Mentioned in the Media meeting it was agreed that instead of saying "post-processed" by the server we could just say "processed"

This image cannot be processed by the web server. Convert it to JPEG or PNG before uploading.

Last edited 6 months ago by antpb (previous) (diff)

#25 in reply to: ↑ 24 @azaozz
6 months ago

  • Keywords needs-testing removed

Replying to antpb:

Thanks, changed.

Re-testing this again, it seems to work properly (code is very close to how .heic images are handled). Commit time imho.

#26 @azaozz
6 months ago

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

In 51227:

Media: Prevent uploading and show an error message when the server doesn't support editing of WebP images, take II. Add new, better error message for it.

Props antpb, joedolson, iandunn, azaozz.
Fixes #53475.

#27 @SergeyBiryukov
6 months ago

In 51230:

Coding Standards: Fix WPCS issues in [51227].

This fixes a "Tabs must be used to indent lines; spaces are not allowed" error.

See #53475.

Note: See TracTickets for help on using tickets.