Fix the method used to create image sub-sizes when uploading fails with a PHP fatal error

[azaozz]

While working on the REST API implementation of #47872 it became apparent that using an unique upload id sent by the client and storing it in a transient is needlessly complex and brings some edge cases/possible race conditions.

[azaozz]

In 48200.diff:

• Remove the use of an "upload ref" send by the client and the helper functions to set/get the attachment_id.
• Set a custom header with the attachment_id after an image file is uploaded and attachment post created, but before any sub-sizes are made. This ensures the new attachment_id is always sent back to the client/browser, even when there is a PHP fatal error.
• Change the Plupload handlers to use the header.

Tested this quite a bit and it seems to work even better than before. Thanks @timothyblynjacobs for the idea and @clorith for the help :)

Please test, planning to commit it asap.

[azaozz]

In 46382:

Upload: Fix the method used to create image sub-sizes when uploading fails with a PHP fatal error. Use a custom header to send the new attachment post ID even in HTTP 500 responses instead of an upload reference sent by the client. Also add another cap check and remove the action when deleting an attachment post during a failed upload cleanup.

Props timothyblynjacobs, clorith, azaozz.
Fixes #48200.

[TimothyBlynJacobs]

I don't think this is strictly necessary, but for consideration, I uploaded a patch that only sends the header in the API endpoints instead of the lower level sub-sizing functions. I think it'd be good to only send the headers when the client would expect them.

This would also allow us to make the REST API version a bit more behaved. Where ideally the header would only be sent for the main REST API request, not internal API requests. If we trigger the sending in the endpoint code, we'd be able to achieve this.

[SergeyBiryukov]

Reopening to address comment:3.

[azaozz]

Replying to TimothyBlynJacobs:

I don't think this is strictly necessary, but for consideration, I uploaded a patch that only sends the header in the API endpoints instead of the lower level sub-sizing functions.

This will change the way (prevent) how this is used outside the API. With 48200.2.diff it won't be able to retry to make sub-sizes when using wp_create_image_subsizes() or wp_update_image_subsizes() directly.

I'm actually thinking the opposite :) The API shouldn't be concerned about this header at all, It is part of "site health" fatal error protection. Also thinking it may make sense to always set it for all uploads, so plugins have a chance to "do something" or at least show better error messages when any upload fails.

When there is no fatal error and we don't want to send it, it can be "unset" just before the "success" response. But I don't see why that would be needed.

This would also allow us to make the REST API version a bit more behaved. Where ideally the header would only be sent for the main REST API request, not internal API requests.

Hmm, what would be "internal" requests here and why would they be worse off with this header? The presence of this header is strictly for use by the client in case of a PHP fatal error, while using the API or not.

If it is a concern that it may be set several times, we can probably add a small helper function for it. Calling header() multiple times overrides the value, but may also add another header.

[TimothyBlynJacobs]

This will change the way (prevent) how this is used outside the API. With 48200.2.diff it won't be able to retry to make sub-sizes when using wp_create_image_subsizes() or wp_update_image_subsizes() directly.

How so? The only time that the header is needed is when uploading an attachment and immediately processing the sub-sizes. If someone provides a media upload endpoint, for instance a front-end form of some kind, they don't necessarily also have the equivalent of wp_ajax_media_create_image_subsizes or wp/v2/media/{id}/post-process. So sending the attachment ID header isn't useful information.

Like I said, I don't think it is strictly necessary. But it seemed cleaner to only send the information when it is necessary. If I called wp_create_image_subsizes, I don't think I'd expect that function to send any output.

[azaozz]

Uh, sorry, think I didn't explain well above. I don't mind changing it just don't see why this header should be added in different context/under different conditions when using the API.

If we commit 48200.2.diff and https://core.trac.wordpress.org/attachment/ticket/47987/47987.5.diff, see #47987:

• Upload from the Media modal:
  • The header is added in media_handle_upload() for all uploads, not just for images. Header is not added when calling media_handle_sideload() and wp_handle_upload() directly. These are used by some upload plugins, but perhaps we can leave the header out in these cases.
  • The header is also added while receiving the AJAX "post-process" request for all uploads, not just for images. In the current code the header is not added if the attachment is not an image.

• Uploading using the API:
  • The header is added on the response to the initial upload request regardless if the upload is an image or not.
  • Header is not added on responses to requests to the new post-process end-point.

Looking at adding header in wp_ajax_media_create_image_subsizes(): this is just a pass-through as the attachment_id is already known. It may be nice to include the attachment_id in these cases, makes it clearer to the client when there is a HTTP 500 error that this specific attachment has problems during post-processing. Perhaps the header should be added to the new post-process end-point? Alternatively we should remove it from wp_ajax_media_create_image_subsizes() to make it more inline with the API behaviour.

To only send the information when it is necessary we'll have to look at the uploaded file type and set that header only for images (current behaviour). However thinking that header can be set for all uploads, like in the above patches.

[azaozz]

Replying to TimothyBlynJacobs:

Thinking more about this, 48200.2.diff looks better :) You're right, no point in setting a header every time _wp_make_subsizes() runs.

Lets do it this way, and (if there's enough time) can try to remove the header "pass-through" from wp_ajax_media_create_image_subsizes().

[azaozz]

In 46421:

Upload: Set custom header with the attachment ID for all uploads from media_handle_upload(). Let the REST API endpoint set it separately.

Props timothyblynjacobs.
Fixes #48200.