Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#47987 closed task (blessed) (fixed)

REST API: Add handling of PHP fatal errors while resizing images after upload

Reported by: azaozz's profile azaozz Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 5.3 Priority: highest omg bbq
Severity: normal Version:
Component: REST API Keywords: dev-feedback
Focuses: Cc:

Description (last modified by azaozz)

Update WP_REST_Attachments_Controller to be able to handle subsequent requests for making missing image sub-sizes similarly to #47872.

Attachments (5)

47987.diff (7.9 KB) - added by TimothyBlynJacobs 5 years ago.
47987.2.diff (6.7 KB) - added by TimothyBlynJacobs 5 years ago.
47987.3.diff (7.5 KB) - added by azaozz 5 years ago.
47987.4.diff (14.1 KB) - added by TimothyBlynJacobs 5 years ago.
47987.5.diff (11.3 KB) - added by TimothyBlynJacobs 5 years ago.

Download all attachments as: .zip

Change History (49)

#1 @azaozz
5 years ago

Generally when uploading an image the client should be able to send an unique "upload reference" which is then stored in a transient for one hour, and deleted if all image sub-sizes were created successfully. See
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/media.php?rev=45934#L379 and
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/media.php?rev=45934#L417.

Then if the response is HTTP 500, the client should be able to do another request including the upload reference from above, and the server should try to make any missing image sub-sizes. The upload reference is used to find the new attachment ID as the client doesn't know it yet. On the server side that would work similarly to:
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/ajax-actions.php?rev=45934#L2416.

Then, if all attempts fail, the client should be able to do a "cleanup": delete the just created attachment. Again, the upload reference is used to find the attachment ID.

And lastly, the error message shown should be... More helpful than the default "This site is experiencing difficulties...". That could be on the client side though.

This is now implemented in #47872 for the media library screen, Add New media (old upload UI), and the media modal. Any suggestions for improvements welcome :)

Last edited 5 years ago by azaozz (previous) (diff)

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


5 years ago

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

#4 @azaozz
5 years ago

  • Description modified (diff)
  • Keywords needs-patch added

#5 @azaozz
5 years ago

Added three "helper functions" that deal with the temp upload ref in [46174].

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

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


5 years ago

#8 follow-up: @noisysocks
5 years ago

@azaozz: What's left to do here? What do we need to happen for 5.3?

#9 in reply to: ↑ 8 @azaozz
5 years ago

  • Priority changed from high to highest omg bbq
  • Type changed from defect (bug) to task (blessed)

Replying to noisysocks:

This still needs a patch for 5.3. It is to bring the same functionality from #47872 to the API. Then the corresponding part has to be added to Gutenberg.

Currently retrying to post-process images works when uploading from the Media Library and from the Media modal, but not when uploading through the API.

Rising priority as ideally this would be done before beta2 :)

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

#11 @TimothyBlynJacobs
5 years ago

  • Owner set to TimothyBlynJacobs
  • Status changed from new to accepted

Instead of a separate endpoint for continuing, I think we need to have all of this done on the POST create_item() route, so that the WP_REST_Request object has all the expected data for the additional fields that are processed, and the *_insert actions.

So I was thinking the request would be the normal POST but with an added header, something like X-WP-UploadRef: randomReference. Then, if the server responds with a 500, you’d make the same POST request, but with the header X-WP-RetryUploadRef: randomReference. The POST fields should be identical to the initial request, but the file data could be omitted.

It feels a bit weird to me that we are controlling the request flow based on a header name, but it seemed like the most straightforward way to implement the feature while still maintaining as full compatibility as possible with existing code.

Additionally, as @kadamwhite mentioned, we'd be introducing more arbitrary header names.

Initially, the description of the feature made me think a lot about Stripe's Idempotent Request API. Not an exact match ( we'd need to handle server errors ), but could be a single header key that we'd use for the entire API. However, I think that'd be too complex for 5.3 and I don't think getting in a solution specific to media would block us from considering a more extensive feature in the future.

We could consolidate into a single header name, and decide whether to continue or create a new attachment based on if the upload reference is valid. If the upload reference was persistent, perhaps post meta on the attachment that would be removed once the attachment is fully processed, I'd be more comfortable with that. But with transients being transient, it seemed like it could result in an unclear error, or a double attachment ( since the request would be otherwise the same ) if the transient was evicted before the client was finished with it.

Thoughts? Looping in @rmccue and @joehoyle.

#12 @azaozz
5 years ago

I was thinking the request would be the normal POST but with an added header, something like X-WP-UploadRef: randomReference.

Sure but... Why in the header? The upload ID/reference is still part of the data sent to the server. It is not that different than the rest of the data that the server needs in order to process the upload?

Instead of a separate endpoint for continuing, I think we need to have all of this done on the POST create_item() route, so that the WP_REST_Request object has all the expected data for the additional fields that are processed, and the *_insert actions.

It's up to the API maintainers but having a separate "action" for post-processing seems to make sense?

  • The upload is complete by that point and the attachment post is already created (or at least is expected to have been created).
  • At this point we are just monitoring or triggering post-processing of the uploaded file. Eventually that can be used for other post-processing, not just image resizing.
  • None of the original fields are needed any more.
  • Re-sending the same original data is redundant or may potentially result in a duplicate?

Actually I'm not entirely sure how uploads are supposed to work through a REST API. Streaming from js to an end-point has certain disadvantages (may require the whole file to be loaded in memory by the browser, then it fails for large files). "Direct" POST from the browser usually works more reliably. Perhaps worth a look in the future :)

#13 @talldanwp
5 years ago

I like the stripe idempotency idea @TimothyBlynJacobs mentioned. It would definitely require some agreement that it's the right approach across the API, but it seems a close fit to the retries model being proposed.

Some more detail here: https://stripe.com/au/blog/idempotency

The difference would be that those keys shouldn't really be shared across endpoints, so a key wouldn't necessarily be considered a reference to the media, but a reference to the specific HTTP request that needs to be retried/resumed.

It's up to the API maintainers but having a separate "action" for post-processing seems to make sense?

I'm a little unfamiliar with the media endpoints, but it'd seem to me that if the create_item() endpoint tries to handle post-processing on the first attempt, but fails, then it should try again on the second attempt in an idempotent model.

It could probably also work as separate endpoints, though. As long as they're initiated separately in the first instance.

A separate question @azaozz, how would the client handle incomplete requests if it's interrupted (i.e. the user reloads the page after an upload)?

Last edited 5 years ago by talldanwp (previous) (diff)

#14 @azaozz
5 years ago

...a key wouldn't necessarily be considered a reference to the media, but a reference to the specific HTTP request that needs to be retried/resumed.

Right. The key/upload_ref is specific for the upload request. It is needed so the server can find the just created attachment_ID and resume post-processing of the uploaded file. If the upload has failed before an attachment post was created, it just returns an error.

...the create_item() endpoint tries to handle post-processing on the first attempt, but fails, then it should try again on the second attempt in an idempotent model.

Sure. Post-processing is started automatically by the server after the uploading of the file is done. If it fails, and the client gets an HTTP 500 response, the client should do another request to continue the post-processing.

how would the client handle incomplete requests if it's interrupted (i.e. the user reloads the page after an upload)?

This is/should be handled by the UI as "uploading is in progress". The upload is not complete until all requests for post-processing are done. If the user reloads the page or navigates away while the upload is in progress, it will fail.

If the post-processing fails after all attempts, the final request is to do a "clenaup" (delete the new attachment). Then the UI should show an appropriate error message. In this case it should advise the user to scale down the image and upload it again, see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/script-loader.php?rev=46289#L1237.

Last edited 5 years ago by azaozz (previous) (diff)

#15 follow-up: @TimothyBlynJacobs
5 years ago

Sure but... Why in the header? The upload ID/reference is still part of the data sent to the server. It is not that different than the rest of the data that the server needs in order to process the upload?

REST semantics wise, the reference isn't describing the actual resource itself. It provides information about how the request should be processed; metadata. Which would typically be in the header. I'm not strongly opposed to it being in the request body, but I think it makes more sense for supplementary information to be in the headers.

The upload is complete by that point and the attachment post is already created (or at least is expected to have been created).

None of the original fields are needed any more.

The attachment is created, but the alt text hasn't been saved, the additional fields haven't been set, and the rest_after_insert_attachment hook hasn't fired yet. So we'd either need to make that happen before wp_update_attachment_metadata is called, in which case the state of the attachment will be different during all of those callbacks.

Or, duplicate those calls in the other endpoint. Duplicating wouldn't really be an option because the WP_REST_Request object will be different than expected for creating a new attachment.

Re-sending the same original data is redundant or may potentially result in a duplicate?

It is redundant, but wouldn't result in a duplicate because of the presence of the header.

At this point we are just monitoring or triggering post-processing of the uploaded file. Eventually that can be used for other post-processing, not just image resizing.

I personally don't think we have enough time to scope out how that would work before the deadline for 5.3. I think it'd also be easier to add a new endpoint at the point it is required, than trying to modify a very specifically designed endpoint just for doing resizing after 500 errors.

For instance, the image ref isn't permanently stored and at that point we'd have the attachment ID. So it'd make sense to be an endpoint off of the single attachment route. ie /wp/v2/media/{id}/process ( or some other better named endpoint ).

I think it'd also give us the opportunity to explore a stepped-processing endpoint that isn't specific to media. I think that'd be the best way to handle this, but if we need something by Monday :)

For just this feature, I personally don't think we get much of a benefit from creating a separate endpoint.

Actually I'm not entirely sure how uploads are supposed to work through a REST API.

The REST API supports $_FILES or passing the data in the request body. So it should be possible to do a direct POST.


I like the stripe idempotency idea @TimothyBlynJacobs mentioned. It would definitely require some agreement that it's the right approach across the API, but it seems a close fit to the retries model being proposed.

I don't think it'd be something that we could implement in time for 5.3, and thus solve this feature, but I agree it'd be interesting.

#16 in reply to: ↑ 15 @azaozz
5 years ago

Replying to TimothyBlynJacobs:

REST semantics wise, the reference isn't describing the actual resource itself. It provides information about how the request should be processed; metadata.

Right. Lets go with the header then. I know at some point having "weird" HTTP headers resulted in "weird" caching/handling of such requests somewhere along the nets, leading to "unexplained", very hard to debug edge cases. Hopefully that's all in the past now :)

Then in practice we will only need a HEAD request after a HTTP 500 response. This is basically "error handling".

The attachment is created, but the alt text hasn't been saved, the additional fields haven't been set, and the rest_after_insert_attachment hook hasn't fired yet.

Right, but this is just metadata about the attachment post, not the actual post (i.e. the resource). All of these steps can be seen as "post processing".

It is redundant, but wouldn't result in a duplicate because of the presence of the header.

Yes, I understand. It just seems wasteful to keep sending all the data again and again when none of it is needed. Also handling on the client will have to be different so it doesn't send/upload the actual file again. Seems a bit... unclear but lets just go with it if it is needed.

I personally don't think we have enough time to scope out how that would work before the deadline for 5.3.

Yeah, I agree. We are running out of time. Lets just add something that'd work. It is a simple exchange between the client and the server:

  • The client: I want to upload this. Handle it.
  • The server: Crash!!!
  • The client: Hey, what's up with that upload? What happened?
  • The server: Oops, lets try to finish it... Crash again.
  • The client: Hey, what's going on there with my upload?
  • The server: Oops, crashed again. Lets try again...

......

  • The client: Well, seems you cannot handle that upload, just do a cleanup. Delete all that you got til now. I'll tell the user to try to fix it.

The REST API supports $_FILES or passing the data in the request body. So it should be possible to do a direct POST.

Yeah, the question is how the binary data gets to the PHP $_FILES array. A multipart/form-data request outside the API? In all other cases it seems the client (js in the browser) will have to "deal" with the uploaded before sending it. I think in some cases it will load the whole file in memory which may be the reason uploads fail more often through the API.

Last edited 5 years ago by azaozz (previous) (diff)

#17 follow-up: @TimothyBlynJacobs
5 years ago

All of these steps can be seen as "post processing".

I agree they are post processing, but then we still need the data from the request object to make that happen. Or am I misunderstanding the distinction.

A multipart/form-data request outside the API?

Yeah. I think you can use FormData and a File object?


Cool, I'm on a plane for the rest of the day. Will hopefully have a patch for review this Saturday.

#18 in reply to: ↑ 17 @azaozz
5 years ago

Replying to TimothyBlynJacobs:

I agree they are post processing, but then we still need the data from the request object to make that happen. Or am I misunderstanding the distinction.

I see what you mean. Yes it needs the data, but this "post meta" should probably be saved as soon as the attachment post has been created, if it exists at all at the time.

Generally outside the API, upload requests do not contain any of that post meta. They only contain a file in the $_FILES array. I'm not sure how this is supposed to be handled through the API or by the client, and why there may be other data together with the file.

As far as I see all uploads are started by the user dropping a file in the browser window or selecting a file from the OS dialog. In these cases no additional data is sent together with the upload. Not sure where that data is supposed to come from..?

Then, after the upload is complete, all the additional post meta is saved with another request, when the user actually adds it. (By that time we already have the attachment_id as the attachment post was created when the file was uploaded).

But that's a question we can look at later...

Cool, I'm on a plane for the rest of the day. Will hopefully have a patch for review this Saturday.

Great! Thanks :)

Last edited 5 years ago by azaozz (previous) (diff)

#19 follow-up: @TimothyBlynJacobs
5 years ago

  • Keywords has-patch needs-unit-tests dev-feedback added; needs-patch removed

Uploaded a patch that seems to work. A bit proof of concepty. It doesn't handle "giving up" and deleting an upload ref. I'm also not really a fan of how the control flow works here. But I figured some patch was better than nothing.

#20 in reply to: ↑ 19 @azaozz
5 years ago

Replying to TimothyBlynJacobs:

Thanks for the patch!

I'm also not really a fan of how the control flow works here.

Yeah, the "weakness" is probably because creation of the attachment post and uploading of the file is mixed up/mashed together. These are two distinct actions, should be fairly separate. I understand that until now "upload post-processing" was brute-forced while creating the attachment meta data, but now it requires additional steps.

It is a three step "process". The upload ref is always present in all upload requests for images (could potentially be present in all upload requests regardless of file type, but is not currently used for non-image files).

Step one
On the initial request the file is uploaded and attachment post created. Then the upload ref is saved in a transient and the request continues as before.

Looking at the code perhaps do_action( 'rest_insert_attachment', $attachment, $request, true ); should run then, as soon as the attachment post is created. This will be consistent with other WP actions and filters that run at the time an attachment post is created. I'm not sure what is the intended purpose of that action but it seems it is related to the attachment post and not to the uploaded file.

Step two
If the step one request fails (with HTTP 500 error), the client does another request still including the same upload ref. This is a "retry" request and needs to be marked as such (another custom header perhaps?). There is no file included in that request, just the upload ref and a "retry" flag (and a nonce, etc.).

When the server sees this request it doesn't try to create another attachment post but instead tries to find the just created post using the upload ref. If found, it makes a call to wp_update_image_subsizes( $attachment_id );. If not, returns error as the upload has failed.

Step two can run several times if the response is still HTTP 500. If it succeeds in creating all image sub-sizes, it should return the standard response as in step one.

Step three
If all step two requests fail, the final request should do a "cleanup" and delete the attachment post. This still includes the upload ref so the server can find the attachment_id. It should also include another flag similarly to step two to tell the server that it is a cleanup.

On success in any step the transient made from the upload ref is deleted.

I'll try to update the patch and include the above.

#21 follow-up: @TimothyBlynJacobs
5 years ago

I think that patch is following that request outline, no? Am I missing something?

To test this, I inserted a die at the top of wp_generate_attachment_metadata. Then did the following requests

POST /wp-json/wp/v2/media
X-WP-ImageRef: myreference
Content-Type: multipart/form-data; boundary=X-INSOMNIA-BOUNDARY
file => file
alt_text => This is my alt

Then...

POST /wp-json/wp/v2/media
X-WP-RetryImageRef: myreference
Content-Type: multipart/form-data; boundary=X-INSOMNIA-BOUNDARY
alt_text => This is my alt

The thing missing from the patch being clearing the upload ref after repeated failures.

#22 follow-up: @TimothyBlynJacobs
5 years ago

I think I see at least part of it, that right now rest_insert_attachment is firing on both the initial upload and the retry.


I wish the flow worked something like...

POST /wp-json/wp/v2/retry with the minimal creation data and get returned a reference.
POST /wp-json/wp/v2/retry/{ref} to process the action that might fail part way through. This would be the only endpoint where an empty 500 response would be expected.
DELETE /wp-json/wp/v2/retry/{ref} to delete the action.

#23 @TimothyBlynJacobs
5 years ago

Added a patch that consolidates those steps into the insert_attachment method to see if I'm understanding correctly.

#24 in reply to: ↑ 21 @azaozz
5 years ago

Replying to TimothyBlynJacobs:

To test this, I inserted a die at the top of wp_generate_attachment_metadata. Then did the following requests

POST /wp-json/wp/v2/media
X-WP-ImageRef: myreference
Content-Type: multipart/form-data; boundary=X-INSOMNIA-BOUNDARY
file => file
alt_text => This is my alt

Yes, this is the initial upload request. The only thing here is that alt_text => This is my alt will never be set as the user has not had a chance to enter any alt text yet, so it is completely pointless. This is a file upload request, no user input is/should be possible other than selecting a file to upload :)
(Not the time to fix that now though).

Then...

POST /wp-json/wp/v2/media
X-WP-RetryImageRef: myreference
Content-Type: multipart/form-data; boundary=X-INSOMNIA-BOUNDARY
alt_text => This is my alt

If this is a retry request it should include a "retry" flag, perhaps another custom header. Then it won't need to be multipart/form-data, can actually be a HEAD request, but GET or POST would work too (the request body should be empty, there's nothing else to send to the server at this time).

Again, alt_text => This is my alt is completely pointless as the user cannot set anything there yet. This is still a "file upload" request, step two. Lets leave that for now, it is a bug that will need fixing in the future.

Also, as the additional data was sent with the first request, there is no point in re-sending it, same as the actual file. The attachment post has been created by now and any additional data already added.

In practice no additional data should be sent together with the file that needs to be uploaded. It is a bad thing to "mix up" uploading and creating the attachment post (see above) and the user has not had a chance to enter any data anyway. So the only additional data present in upload requests would be automatically added and not user input.

Then step three will need another flag in the head. this time that would be a "cleanup" instead of "retry".

Last edited 5 years ago by azaozz (previous) (diff)

#25 in reply to: ↑ 22 @azaozz
5 years ago

Replying to TimothyBlynJacobs:

I wish the flow worked something like...

POST /wp-json/wp/v2/retry/{ref} to process the action that might fail part way through. This would be the only endpoint where an empty 500 response would be expected.
DELETE /wp-json/wp/v2/retry/{ref} to delete the action (the remnants of the failed upload)

Yeah, I was imagining something like that too. Looks a lot better and more consistent with what's actually happening :)

Perhaps not the POST /wp-json/wp/v2/retry. This is the "standard" upload end point, right? Thinking it can stay as-is at the moment. The only difference would be adding support for the client to send the upload ref, and save a transient with it when present.

It would be great to have a separate end point for post-processing after uploading.

Last edited 5 years ago by azaozz (previous) (diff)

@azaozz
5 years ago

#26 @azaozz
5 years ago

In 47987.3.diff:

  • Based on 47987.2.diff.
  • Handle the three "steps" as outlined above when uploading images.
  • Some minor cleanup, also removed modification of _wp_set_upload_ref(). It is a simple wrapper for set_transient, seems best to keep returning a boolean.

Tested with the current code but not the new functionality to retry to create image sub-sizes.

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


5 years ago

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


5 years ago

#29 follow-up: @TimothyBlynJacobs
5 years ago

Some minor cleanup, also removed modification of _wp_set_upload_ref(). It is a simple wrapper for set_transient, seems best to keep returning a boolean.

We aren't handling when the set transient fails though. And since that function performs two tasks, we can't tell the client whether it failed due to their input being invalid or an internal DB error. If the transient wasn't saved, and an upload error occurred, they wouldn't be able to clean up the failed upload, no?

#30 in reply to: ↑ 29 @azaozz
5 years ago

Replying to TimothyBlynJacobs:

We aren't handling when the set transient fails though.

Right. This is not handled well currently. If it fails, it won't be able to retry to create the sub-sizes, i.e. reverts back to the current behaviour.

If the transient wasn't saved, and an upload error occurred, they wouldn't be able to clean up the failed upload, no?

Yes, it will show as a partial/failed upload in the media library, same as before.

The only way we can "act" on that error is to disregard the uploaded file (as the upload has already completed) and return an error telling the user (the client) to try uploading again. For the user/client the error would still be the same "please try again" regardless of the cause. It will also need to delete the just created attachment post as the upload will be considered to have failed.

However that means disregarding the uploaded file and deleting the attachment in all cases, even when it would not fail the post-processing. As retrying is needed quite rarely, that seems like a high price to pay?

Another option that I looked at was to return the new attachment_id instead of the "Your site is experiencing problems" error message. However was unable to confirm it would work in all cases, so opted to store the transient. Perhaps can revisit this again, not too late yet. Will look at this now :)

#31 @azaozz
5 years ago

Related #48200. Thinking using a custom header is a lot better way than storing an unique upload ref in a transient. Thanks for the idea @TimothyBlynJacobs :)

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


5 years ago

#33 @azaozz
5 years ago

  • Keywords commit added

47987.5.diff looks very good! :)

The only thing I'm a bit unsure about is including the header only on the first upload request. This bit:

if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {
   // Set a custom header with the attachment_id.
   // Used by the browser/client to resume creating image sub-sizes after a PHP fatal error.
   header( 'X-WP-Upload-Attachment-ID: ' . $attachment_id );
}

Thinking that the API shouldn't be concerned about this header at all. It is rather a part of "site health PHP fatal error protection" and would allow the client to "do something" when a fatal error happens while uploading a file. The API just provides the methods/end points to make this possible. Ultimately the client should decide what needs to be done when there are PHP fatal errors/HTTP 500 responses :)

Think this is ready for committing, perhaps without that header() call.

#34 follow-up: @TimothyBlynJacobs
5 years ago

The only thing I'm a bit unsure about is including the header only on the first upload request.

I included it pending #48200. If it's decided not to use that patch, then we could remove it.

But it would be here to only send when someone is making an HTTP request to /wp-json not when someone is doing rest_do_request().


To summarize the current flow.

  1. POST /wp/v2/media
  2. If the upload failed, look for a response header with X-WP-Upload-Attachment-ID header that contains the newly created attachment ID.
  3. POST /wp/v2/media/{id}/post-process with { "action": "create-image-subsizes" }`. This request may still fail, but it will save its progress.
  4. On continued failure, DELETE /wp/v2/media/{id} to give up on the upload and instruct the user to resize their image before uploading.

Separately, to try and summarize the changes and conversation between @azaozz and myself.

The latest patch moves wp_generate_attachment_metadata to the very end of the upload process. The saving of additional fields and the rest_after_insert_attachment hook would now happen _before_ that metadata is persisted. Generating a subset of that attachment data, and excluding the subsizes generation was explored, but didn't look possible because there are a number of places where WP expects that the "sizes" array is valid. This would've allowed us to ensure that POST /wp/v2/media wouldn't fail.

Secondly, a separate RPC-like REST route was introduced /wp/v2/media/{id}/post-process. This endpoint will perform post processing actions, starting with create-image-subsizes. This is not very RESTful, but I think a separate RPC endpoint makes sense here.

  1. I think we'd end up with a more unintuitive interface if we tried to represent this as a state transfer, instead of a command to perform an action.
  2. Additionally, these action commands may fail midway with internal server errors. This would make performing other updates on the PUT /wp/v2/media/{id} endpoint unsafe.
  3. For the same reason, we also wouldn't be able to guarantee that rest_after_insert_attachment would fire.

So to me, it makes sense to consolidate these "unsafe" operations into an RPC-like endpoint.

We also removed the ability to resume an upload using the same POST request by setting a custom request header with an attachment ref or id. This means that if a plugin was adding response data only on the initial create item response, that code may no longer work ( if the initial request failed ).

For example:

<?php
add_filter( 'rest_prepare_attachment', function ( WP_REST_Response $response, WP_Post $attachment, WP_REST_Request $request ) {
    if ( $request->get_method() === 'POST' && $request->get_route() === '/wp/v2/media' ) {
        $response->data['custom_field'] = 'only on create';
    }

    return $response;
} );

This code would have an upgrade path by looking for the post-process route in the request.

CC: @kadamwhite, @joemcgill

#35 in reply to: ↑ 34 @azaozz
5 years ago

Replying to TimothyBlynJacobs:

I included it pending #48200.

47987.5.diff looks ready to go imho. Added the patch on #48200, anything else left to do here?

#36 @azaozz
5 years ago

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

In 46422:

REST API: Add support for continuing the post-processing of images after upload. Flow:

  1. POST /wp/v2/media.
  2. If the upload failed (HTTP 500 error), look for a response header with X-WP-Upload-Attachment-ID header that contains the newly created attachment ID.
  3. POST /wp/v2/media/{id}/post-process with { "action": "create-image-subsizes" }. This request may still fail, but it will save its progress.
  4. On continued failure, DELETE /wp/v2/media/{id} to give up on the upload and instruct the user to resize their image before uploading.

Props TimothyBlynJacobs.
Fixes #47987.

#37 follow-up: @kadamwhite
5 years ago

  • Keywords needs-patch added; has-patch needs-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks @azaozz and @TimothyBlynJacobs for driving this. Reopening because @rmccue caught that we provide no discovery mechanism for this endpoint. We should add another commit to include a Link header in the response to the POST request which points to the new endpoint. (This could technically serve as an alternative to the new X-header, but I think exposing the ID directly is also useful)

#38 @kadamwhite
5 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Unfortunately after investigation by myself, Timothy & Ryan over the past 24 hours, implementing this link properly appears to be just complex enough that I do not believe we have the bandwidth to execute by today's beta deadline. Punting the Link question to the next release, see #48257.

#39 in reply to: ↑ 37 @azaozz
5 years ago

Replying to kadamwhite:

We should add another commit to include a Link header in the response to the POST request which points to the new endpoint.

Yeah, would have been nice to add that but it needs to happen before "post processing" starts i.e. before the call to generate attachment meta.

Was also thinking the whole "create attachment + upload a file" flow may need another look. Generally "streaming" from js in the client seems to perform a lot worse than "direct" multipart/form-data request to the web server, both on the client side (large files may need to me fully loaded in memory) and on the server side (PHP may start "execution" as soon as the request starts, counting the time it takes to transfer the file as "runtime" which makes it more likely to time out).

Last edited 5 years ago by azaozz (previous) (diff)

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


5 years ago

#41 @azaozz
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to reevaluate the need for the create-image-subsizes action in post_process_item().

This came up during implementing use of the end-point in https://github.com/WordPress/gutenberg/pull/17858. removing create-image-subsizes would work well when the new end-point is used to allow post-processing of an uploaded file to continue after a fatal error. At this point the server "knows" what needs to be done, there is no point in limiting the action.

The code there can be changed to test if the attachment is an image and if any sub-sizes are missing, then create the sub-sizes if necessary. In the future if post-processing is implemented for other file types, that can be added as well.

That'd be the same logic as in the current switch ( $request['action'] ) but instead of expecting the client to "know" the action, the server will determine it.

Last edited 5 years ago by azaozz (previous) (diff)

#42 follow-up: @TimothyBlynJacobs
5 years ago

I don't really have any further concerns than the one I outlined on the Github issue. That being that I think this will make it harder to introduced new functionality in the future. Especially since we don't want to add many RPC-like endpoints. And I don't think it is necessary to achieve the required functionality.

#43 in reply to: ↑ 42 @azaozz
5 years ago

Replying to TimothyBlynJacobs:

I don't really have any further concerns than the one I outlined on the Github issue. That being that I think this will make it harder to introduced new functionality in the future.

Can at least the create-image-subsizes action be made optional? It doesn't really serve any purpose. It's like adding a param to a function that is never used in the function, and that can be determined from the existing params.

#44 @TimothyBlynJacobs
5 years ago

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

Reclosing based on @kadamwhite's feedback as well. https://github.com/WordPress/gutenberg/pull/17858#issuecomment-542309204

Can at least the create-image-subsizes action be made optional? It doesn't really serve any purpose. It's like adding a param to a function that is never used in the function, and that can be determined from the existing params.

Part of the issue is the compressed timeline, so we haven't been able to thoroughly map out how we'd want this endpoint to work and evolve in the future. Once we've done that ground work, and if we then determine the actions can be automatically determined, then it'd be simple to make that parameter optional.

However, once we omit the action parameter, or can't guarantee that it is provided, it'll be difficult to workaround if we determine that isn't possible in the future.

Note: See TracTickets for help on using tickets.