Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#48542 closed defect (bug) (fixed)

Uploading a large image through the block editor fails

Reported by: azaozz's profile azaozz Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: highest omg bbq
Severity: blocker Version: 5.3
Component: Editor Keywords: has-patch needs-testing commit dev-reviewed
Focuses: Cc:

Description

Seems the new functionality to retry post-processing is triggered correctly but the new REST API end point post-process always returns a 404 error.

Attachments (1)

48542.diff (815 bytes) - added by azaozz 4 years ago.

Download all attachments as: .zip

Change History (11)

#1 @azaozz
4 years ago

  • Keywords needs-patch added
  • Priority changed from normal to highest omg bbq
  • Severity changed from normal to blocker

Testing in both trunk and the 5.3 branch, requests to wp/v2/media/{attachment ID}/post-process?_locale=user and to wp/v2/media/19348?force=true&_locale=user with X-HTTP-Method-Override: DELETE are 404.

Discussion in Slack: https://wordpress.slack.com/archives/C02RQC26G/p1573246877137300.

Version 0, edited 4 years ago by azaozz (next)

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


4 years ago

@azaozz
4 years ago

#3 @azaozz
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

In 48542.diff: move wp.apiFetch.createRootURLMiddleware() before wp.apiFetch.use( wp.apiFetch.mediaUploadMiddleware ); as suggested on Slack: https://wordpress.slack.com/archives/C02RQC26G/p1573247239138300.

The alternative solution of doing apiFetch() instead of next() in /middlewares/media-upload.js seems perhaps riskier change at this point.

Thanks @mikeschroder, @TimothyBlynJacobs, and @epiqueras for looking at this asap :)

#4 @azaozz
4 years ago

I know this is somewhat hard to test/trigger the retry requests. Here's an image that may do it: https://upload.wikimedia.org/wikipedia/commons/f/ff/Pizigani_1367_Chart_10MB.jpg

To test: download that image, then drop it in a new "image" block in the block editor (make sure the Gutenberg plugin is not installed or is disabled), and watch the network tab in the browser tools.

As this is a "high density" 10MB JPEG image it fails to get post-processed on the initial request on my test server, even with the default sub-sizes and PHP settings. Alternatively few more large sub-sizes may need to be added or the max_execution_time=30 setting in php.ini may need to be reduced for this test.

#5 @johnbillion
4 years ago

I reproduced the issue with the Pizigani_1367_Chart_10MB.jpg image locally and saw the Media upload failed error message due to a 500 response from wp-json/wp/v2/media?_locale=user and the successive 404s from wp/v2/media/141/post-process?_locale=user.

After applying 48542.diff, the request to wp-json/wp/v2/media?_locale=user still returns a 500 as expected, but the subsequent request to wp-json/wp/v2/media?_locale=user now uses the correct URL and returns a 200. The image appears as expected and the image sizes are all generated correctly.

I'm unable to assess whether the fix is a technically good one, but I can confirm that it fixes the issue.

#6 @youknowriad
4 years ago

I'd say that the best solution would be to ensure the media middleware is independent of the root URL middleware by using the full URL there too. I think landing the currently proposed core patch is a good change regardless.

LGTM 👍

#7 @SergeyBiryukov
4 years ago

  • Keywords commit added

Marking for commit, per the above comments.

#8 @johnbillion
4 years ago

  • Keywords dev-reviewed added

#9 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 46703:

Script Loader: In wp_default_packages_inline_scripts(), make sure the root URL middleware is registered before using the media middleware.

This allows for requests to retry post-processing large images using the post-process REST API endpoint to work as expected.

Props azaozz, mikeschroder, TimothyBlynJacobs, epiqueras, johnbillion, youknowriad.
Fixes #48542.

#10 @SergeyBiryukov
4 years ago

In 46704:

Script Loader: In wp_default_packages_inline_scripts(), make sure the root URL middleware is registered before using the media middleware.

This allows for requests to retry post-processing large images using the post-process REST API endpoint to work as expected.

Reviewed by johnbillion, youknowriad, SergeyBiryukov.
Props azaozz, mikeschroder, TimothyBlynJacobs, epiqueras, johnbillion, youknowriad.
Merges [46703] to the 5.3 branch.
Fixes #48542.

Note: See TracTickets for help on using tickets.