Repository navigation
Conversation
|
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. |
|
(cc @prettyboymp @kraftbj for feedback too, as an alternative to #10080) |
|
Love the ergonomics of this approach 👍 |
|
On first glance, these two approaches seem complementary rather than competing, as they tackle 63946 at different layers of the stack. #10080 defers at the namespace boundary: register_rest_route() never runs for a namespace that isn't matched, which also sidesteps whatever REST-only setup plugins put alongside registration — controller instantiation, This one defers at the options boundary: The two stack nicely. We could register a lazy namespace via #10080, and in the action that loads it, use resolvable options for any routes with expensive schemas that might still not be the one hit. Skip the namespace entirely, then if the namespace is hit, skip option construction for non-dispatched routes, and then only fully resolve the actually matched route. tl;dr: :why-not-both.gif: |
The big factor here is the complexity, both conceptually and from an implementation standpoint. The benefits from switching registration approach come from all users adopting the approach, so that we can offload as much work to just-in-time as possible. That naturally means that any new approach is going to want to be evangelised as "the" way to do it, so we should carefully consider the ergonomics. Lazy namespaces are conceptually more complex: they require stacking multiple actions on top of each other (including a dynamic action name) which run at different times. It moves the mental model from "when the API starts, register your endpoints" to "when the API starts, register your namespace. when the namespace is being used, register your endpoints". Don't get me wrong, it's not like it's the end of the world, but if we can avoid that friction through careful design we should. From an implementation perspective, it's also more complex - both in core, and in plugins working with it. It introduces that new "tier" above routes as an object we have to deal with, and in some ways it also moves the current The core thesis here really is that adding items into an array (and calling the callback that does that) isn't actually expensive, the expensive part of registering routes is building the options. If we offload that and it "solves" the performance concern, why add the complexity of lazy namespaces? That thesis is as-yet untested; to put it through its paces, I'd want to grab a selection of plugins and adapt them and do a before/after on the timing. I take your point around controller instantiation and similar operations that happen at the registration stage, but are those actually that expensive? (There's of course also nothing here that would block us from doing both in the future if we wanted to at that stage.) |
|
That's all fair. I have no objection to your approach, with the door being open to something more akin to what @prettyboymp proposed later, if we can articulate the case. |
kadamwhite
left a comment
There was a problem hiding this comment.
I like the direction this is going a lot, thanks for raising it at WCEU contributor day. Still working through my understanding of the internals, noting one surface-level thing here
| public function __construct( string $namespace, string $route, callable $closure ) { | ||
| $this->namespace = $namespace; | ||
| $this->route = $route; | ||
| $this->callable = $closure; |
There was a problem hiding this comment.
Feels a little odd to me to name this property after its type rather than by purpose, as we do with the namespace and route. Why not $this->resolver or initializeEndpoint or something similar?
There was a problem hiding this comment.
Either/or; "callable" to me is also the purpose, but happy with either name.
kadamwhite
left a comment
There was a problem hiding this comment.
@rmccue Following up on this to check on the intended API signature when registering multiple endpoints within one route and whether common_args is supposed to get dropped.
The AI second-pass review I noted below also lead me to two apparently-preexisting issues:
- the WP_REST_Server::get_namespace_index method assembles all namespaces globally with
get_routes(), then discards all but the requested one. That is fine in the status quo but will be disproportionately inefficient once this patch lands and we update routes accordingly. schemaarguments on individual endpoints, like in your PR description, are silently ignored: only top-levelschemacallbacks are processed in thehelpcontext. This may be a documentation issue, but does feel like a footgun
Neither of these needs to be handled here, I suppose, but might be worth ticketing out.
| continue; | ||
| } | ||
|
|
||
| $arg_group = normalize_rest_endpoint_options( $clean_namespace, $route, $arg_group, $common_args ); |
There was a problem hiding this comment.
To check how I'm reading this: your approach here seems to be that since $common_args is pulled from a top-level named args property on the passed array, to support a multi-route registration syntax like this,
register_rest_route( 'my/namespace', '/route', [
[ 'methods' => 'GET', ..., 'args' => ... ],
[ 'methods' => 'POST', ..., 'args' => ... ],
'args' => [ 'id' => [ ... ] ], // shared
'allow_batch' => 'etcetera',
then therefore, $args['args'] would never exist in the callable $args variation because the callable API requires that register_rest_route be called only for a single endpoint rather than an array of multiple endpoints for a route, and can accordingly skip handling $common_args when instantiating the WP_REST_Resolvable_Route. Is that an accurate summary?
Since we still iterate through $args as $key => &$arg_group and check each $arg_group for callability, it seems like we are discarding $common_args in the multi-endpoint case like this pattern:
register_rest_route( 'my/namespace', '/route', [
fn() => [ 'methods' => 'GET', ... ],
fn() => [ 'methods' => 'POST', ... ],
'args' => [ 'id' => [ ... ] ], // shared
'schema' => fn() => [],
'allow_batch' => 'etcetera',
My understanding of this patch is that this syntax would be valid, and that in this case we'd be dropping those top-level arguments where they are handled today.
If you passed one callback and one array, like
register_rest_route( 'my/namespace', '/route', [
[ 'array-endpoint' ],
fn() => [ 'closure endpoint' ],
'args' => [],
then $common_args would be used by one but not the other.
If we want one-endpoint-per-registration-call to become our recommended approach, we should explicitly require it and disallow these variations, otherwise I think we're discarding information we want to keep and we need to adjust our handling to preserve the common arg passthrough.
Note
AI disclosure: I got confused by the $common_args asymmetry in my own initial human review of the code, so I used Claude/Opus 4.8 to do a second review scan to validate that dropping that top-level args object would cause problems. That tool also noted the asymmetric registration case, which I'd missed.
There was a problem hiding this comment.
Yeah, I think this is an edge case that isn't fully accounted for. My design here is basically:
There's currently two ways you can pass args. The "normal" case is an array of endpoints:
register_rest_route( 'ns/v1', '/route', [
[ 'methods' => 'GET', 'args' => [ ... ], ... ],
[ 'methods' => 'POST', 'args' => [ ... ], ... ],
] );There's also a "convenience" case where when you register a single endpoint, you can just pass that "inner" endpoint array.
Frequently, you have arguments that are in the URL which need the same validation/etc, and which may be "extended" by the individual endpoints. To facilitate that, in the normal case for convenience we support common args at the top level:
register_rest_route( 'ns/v1', '/route/(?P<id>\d+)', [
[ 'methods' => 'GET', 'args' => [ ... ], ... ],
[ 'methods' => 'POST', 'args' => [ ... ], ... ],
'args' => [ 'id' => ... ],
] );The callback support here replaces the endpoint registration only, and we no longer know about common args when it gets resolved. It's actually also the case that common args aren't truly "common", if you register multiple times (which is a less common use):
register_rest_route( 'ns/v1', '/route/(?P<id>\d+)', [
[ 'methods' => 'GET', 'args' => [ ... ], ... ],
'args' => [ 'id' => ... ],
] );
// This second call won't have the common args applied, as they're never stored:
register_rest_route( 'ns/v1', '/route/(?P<id>\d+)', [
[ 'methods' => 'POST', 'args' => [ ... ], ... ],
] );I did experiment a bit with getting this to work, but it required some strangeness to work, wherein WP_REST_Resolvable_Route basically needed to be tied back to a WP_REST_Server instance in an icky way (related to some of the _doing_it_wrong() calls, iirc).
Note though that this only affects the args which is that convenience merge; schema and etc are properties of the route rather than the endpoint, and remain registered against it. I'm not actually sure how common in reality this usage even is, as I don't think it's documented anywhere? (We use it in core controllers (e.g. the id parameter in Posts Controller), so we can't break compat in any case.)
We can take another swing at implementing it though, maybe fresh eyes will help with getting a less painful implementation :)
|
Aha, the get_routes greediness was also noted in #10080 (comment) and that second piece might fix that |
|
@kadamwhite For clarity, the proposal here is for this pull request as the solution to the performance problem stated as the goal for 63946, with 10080 hopefully being obsolete (but still in our back pocket if we need it). What I'm still a little unclear on is the practical performance implications; I think we need some better data on the current impact so we can measure the improvements and decide if it's enough. (Specifically because 10080 changes the API surface, and in a way I don't love.)
Whoops, I missed that - I'd considered it for
That does seem like an unintended consequence here, tied to what I mentioned in the comment above: schema and similar properties are actually properties of the route rather than the endpoint. Is this actually already the case though? 🤔 If you register a route today in the "convenience" manner, does it silently ignore the schema? We wrap single registrations in an array: wordpress-develop/src/wp-includes/rest-api/class-wp-rest-server.php Lines 981 to 984 in 9569f22 which means it'd never get lifted to the route options: wordpress-develop/src/wp-includes/rest-api/class-wp-rest-server.php Lines 992 to 997 in 9569f22 If so, seems like a bug we should sort out generally and which would still be compatible with this approach (basically: if there's exactly-one handler, "lift" any necessary route options ( |
Adds the ability to register just-in-time resolvable routes for the REST API.
When calling
register_rest_route(), you can now pass a function instead of the options for the route directly:This has the benefit that any more expensive operations (translations, args-to-schema building, etc) are only run for matched routes, rather than all of them. But, routing is still possible without this (including listing all available namespaces).
Trac ticket: https://core.trac.wordpress.org/ticket/63946
See also #10080
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.