REST API: Always register the media creation arguments - #12833
REST API: Always register the media creation arguments#12833adamsilverstein wants to merge 6 commits into
Conversation
Sideloading an external image with the `url` parameter on `POST /wp/v2/media` works around a cross-origin fetch the browser cannot make. That fetch fails regardless of whether client-side media processing is enabled, so the argument was registered too narrowly. Leaving the argument unregistered did not disable it. `create_item()` reads the parameter either way, so on a site without client-side media processing the sideload still ran, but without the `sanitize_url` and `wp_http_validate_url()` callbacks the registered argument carries, and an unsafe URL returned a bare `http_request_failed` rather than a 400. Register `url` unconditionally so its validation always applies. The `generate_sub_sizes` and `convert_format` parameters are different: they hand image processing to the client, and the route the client uploads the results to is only registered when the feature is enabled. Honoring them otherwise would leave an attachment with no sub-sizes and no way to add them, and would relax the unsupported image type check in `create_item_permissions_check()` on a site that never opted in. Both are now ignored unless client-side media processing is enabled. Follow-up to [62659]. See #65517.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
I'm a little on the fence about this one. In terms of the REST API itself, for the behaviours of switching off conversion or generating sub-sizes, do we care if client-side media processing is enabled? Is whether client-side media processing is enabled orthogonal to the behaviour here? I don't mind too much either way, I'm mostly thinking about how to keep things simple. |
|
@adamsilverstein @andrewserong 👋🏻 Sorry for the late flag, I'm still wrapping my head around bits here. Here's my understanding of the paths that we're adding in 7.1. First, there is a new "Upload to Media Library" button in the editor that appears when you have image block markup like this: This submits a payload to the This can also be fired as part of a pre-publish check suggesting that external media be uploaded. Second, when a supported image is uploaded by the user in a supported browser and
And then third, as a side effect of adding those features, the ability for extenders to make calls to these endpoints:
Conclusions It seems like the WordPress core editor side of things is working as expected, but I think there are a handful of other things that can be cleaned up or clarified before folks discover the endpoints and start using them. It will be harder to correct in the future and maintain back-compat.
Items 2, 3, and 4 come from a lack of parity between |
That makes sense, we would want the usual |
| * registered otherwise, so a site that has not opted in should not have | ||
| * this check relaxed by an unrecognized parameter. | ||
| */ | ||
| if ( wp_is_client_side_media_processing_enabled() && false === $request['generate_sub_sizes'] ) { |
There was a problem hiding this comment.
Not sure if my earlier question got lost: but why do we guard generate_sub_sizes and convert_format behind the presence of client side media processing? Should we register these unconditionally, too?
(Not a blocker, you've already explained part of the reasoning in the code comments. My only hesitation was really if it's a strange thing for a REST API to conditionally have some args available only some of the time)
There was a problem hiding this comment.
You're right, and thanks for asking twice - I've registered all three unconditionally now.
The thing that convinced me is that wp_is_client_side_media_processing_enabled() comes from is_ssl() and the host, so gating registration on it means the same site advertises different args over http vs https, or behind a proxy that doesn't set HTTPS. That's request context leaking into the schema, and since an unregistered param is still readable we were duplicating the check at runtime anyway.
I did keep one guard: generate_sub_sizes of false no longer relaxes the unsupported image type check in create_item_permissions_check() unless client side media processing is enabled. That check is there because the server can't process the image, so it seems worth only relaxing when the client can - otherwise we store something nothing can process. Skipping sub-sizes or conversion is fine either way, and they can still be regenerated with wp_update_image_subsizes().
Does that split make sense to you?
There was a problem hiding this comment.
Yep, that split makes sense to me! And thanks for digging in further there, that's exactly the nuance I was wondering about, but unsure of 😄
The comment described why an earlier trunk iteration changed rather than documenting the feature, which read as out of place for new 7.1 code. The registration stands on its own. See WordPress#12846 (comment)
Gating `generate_sub_sizes` and `convert_format` on client side media processing made the schema depend on request context rather than site configuration, since the feature is derived from the scheme and host. The same site could advertise different arguments over http and https, or when reached behind a proxy that does not set HTTPS. Both arguments are instructions the server can carry out on its own - skipping sub sizes or format conversion needs no client - so they are now registered and honored either way, and the runtime checks that duplicated the registration condition are gone. The one condition kept is the unsupported image type check in the permissions callback: that check exists because the server cannot process the image, so it is only relaxed when client side media processing means something else can. Sub sizes skipped this way remain recoverable through wp_update_image_subsizes(), and behavior with client side media processing enabled is unchanged.
url argument
andrewserong
left a comment
There was a problem hiding this comment.
This is looking good to me, and I see you're already following up on the other notes and ideas in a separate PR 👍
Addresses @jeremyfelt's question in https://core.trac.wordpress.org/ticket/65517#comment:14 about
url,generate_sub_sizes, andconvert_formatbeing processed onPOST /wp/v2/mediaeven when they aren't registered in the schema.All three are now registered unconditionally.
Worth being clear that leaving them unregistered was never disabling them.
create_item()reads the parameters either way, so they already worked on sites without client side media processing, just without the validation and sanitization their registration carries. An unsafe sideloadurlcame back as a barehttp_request_failedinstead of a 400. Registering them always is what actually closes that.Registering conditionally also made the schema depend on request context rather than site configuration -
wp_is_client_side_media_processing_enabled()is derived fromis_ssl()and the host, so the same site could advertise different arguments over http and https, or when reached behind a proxy that doesn't setHTTPS. Thanks @andrewserong for pushing on this.urldoesn't depend on the feature: sideloading an external image works around a cross-origin fetch the browser can't make, and that fails whether or not client side media processing is enabled.generate_sub_sizesandconvert_formatdon't either - skipping sub-size generation or format conversion is something the server can do on its own, and sub-sizes stay recoverable throughwp_update_image_subsizes().One condition is kept:
generate_sub_sizesoffalseno longer relaxes the unsupported image type check increate_item_permissions_check()unless client side media processing is enabled. That check exists because the server can't process the image, so it should only be relaxed when something else can. Otherwise the upload is stored unprocessable.Trac ticket: https://core.trac.wordpress.org/ticket/65808
How has this been tested
generate_sub_sizesoffalseskips sub-sizes, and the unsupported image type check still applies. Most fail on trunk without the change - I checked by stashing the source change and re-running.WP_Test_REST_Attachments_Controllerclass: 195 tests, 2 skipped, 0 failures.phpcs --standard=phpcs.xml.distclean on both changed files.wp-api-generated.jsneeds no update. The fixture is generated with client side media processing enabled, and in that configuration the registered arguments are unchanged.Types of changes
url,generate_sub_sizes, andconvert_formatunconditionally inget_endpoint_args_for_item_schema().create_item_permissions_check()from being relaxed unless client side media processing is enabled.disable_client_side_media_processing()test helper.Note on an existing test
test_upload_unsupported_image_type_skipped_when_not_generating_sub_sizes(from #64836) called the permissions check withgenerate_sub_sizesoffalsewithout enabling client side media processing, so it was asserting exactly the behavior this changes. It now enables the feature first, which keeps its original intent - the comment on it already says "when the client handles image processing" - but flagging it since it's an existing test from another ticket. @jeremyfelt @andrewserong does that read right to you?Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Drafting the implementation and tests from the review notes on the ticket. I reviewed and verified the behavior and test results myself.
Commit message