WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#48542 closed defect (bug) (fixed)

Uploading a large image through the block editor fails

Reported by: azaozz Owned by: 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 10 months ago.

Download all attachments as: .zip

Change History (11)

#1 @azaozz
10 months 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/12345/post-process?_locale=user and to wp/v2/media/12345?force=true&_locale=user with X-HTTP-Method-Override: DELETE are 404.

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

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

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


10 months ago

@azaozz
10 months ago

#3 @azaozz
10 months 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
10 months 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
10 months 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
10 months 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
10 months ago

  • Keywords commit added

Marking for commit, per the above comments.

#8 @johnbillion
10 months ago

  • Keywords dev-reviewed added

#9 @SergeyBiryukov
10 months 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
10 months 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.