Make WordPress Core

Opened 21 months ago

Closed 3 days ago

#57455 closed enhancement (duplicate)

respond_to_request: store matched handlers across other methods, saving a call to get_routes().

Reported by: mreishus's profile mreishus Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Current Behavior

Out of the box, API requests (example: GET http://localhost:8889/wp-json/wp/v2/posts) will make two calls to WP_REST_Server::get_routes().

Call 1: WP_REST_Server::dispatch() -> WP_REST_Server::match_request_to_handler() -> WP_REST_Server::get_routes().

This is the main path used when serving the request. It gets the routes, matches to the request, then determines which handler will be used for the request. Later, WP_REST_Server::respond_to_request() saves the matched route and matched handler.

Call 2: rest_send_allow_header() -> WP_REST_Server::get_routes().

This is used to set the allow header of the response (example: "Allow: GET, POST, HEAD").
This header shows all HTTP methods that are allowed to be sent on the same route. This information was already found in the "Call 1" pathway above, but it was discarded.

To find the allowed headers, rest_send_allow_header() calls WP_REST_Server::get_routes() which rebuilds information for all existing routes (but we only need information for one route).

Objective

To reduce the number of calls to WP_REST_Server::get_routes() per single request from 2 to 1.
While this only saves 0.1ms on a fresh site, on sites with large custom APIs, this could save 10ms or more.

Patch

When WP_REST_Server::match_request_to_handler() -> WP_REST_Server::get_routes() finds the matching route,
it will now not only remembers the exact matching handler, but all other handlers matching the route with different http methods.

For example, imagine an "/a/b/c" route exists with GET and POST handlers.
Previous to the patch, match_request_to_handler() would find the GET /a/b/c handler and remember it, setting it on $response->matched_handler.
After the patch, a new variable $response->all_methods_matched_handlers is set containing the array: [ (GET /a/b/c handler), (POST /a/b/c handler) ],

rest_send_allow_header() now looks for $response->all_methods_matched_handlers and uses it to set the Allow header if possible, saving a call to WP_REST_Server::get_routes().

This was my way to save the information found in call 1 and to pass it along to call 2, but I definitely open to other ideas of accomplishing the same thing.

Basic Testing

Add a log message when get_routes is called:

  • src/wp-includes/rest-api/class-wp-rest-server.php

    a b class WP_REST_Server { 
    862862        *               `'/path/regex' => array( array( $callback, $bitmask ), ...)`.
    863863        */
    864864       public function get_routes( $route_namespace = '' ) {
     865
    865866               $endpoints = $this->endpoints;
    866867
    867868               if ( $route_namespace ) {

Query an endpoint:
curl http://localhost:8889/wp-json/wp/v2/posts

Before the patch: See two calls to get_routes
After the patch: See one call to get_routes

Alternative Approaches

#39473 adds a per-request cache to get_routes. I'd be happy to rework this if this is a better method.

Attachments (1)

patchfile57455.patch (5.2 KB) - added by mreishus 21 months ago.

Download all attachments as: .zip

Change History (4)

#1 @mreishus
21 months ago

For the alternative approach: #39473 adds a per-request cache to get_routes: I don't know how to resolve the caching behavior with the new $namespace parameter added to get_routes().

In my example request of GET http://localhost:8889/wp-json/wp/v2/posts, the two calls to get_routes() have different parameters, since only one of the two applies a namespace optimization:

  • WP_REST_Server::match_request_to_handler() -> WP_REST_Server::get_routes( 'wp_v2' )
  • rest_send_allow_header() -> WP_REST_Server::get_routes( '' )

A straight forward cache of $namespace -> built $endpoints wouldn't work without other changes.

#2 @obenland
20 months ago

  • Keywords has-patch dev-feedback added

#3 @TimothyBlynJacobs
3 days ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Thanks for the ticket @mreishus! Yeah I think it makes more sense to approach this as a fix to get_routes() instead of passing around the data. I'm going to close this as a dupe of #39473 so we can keep the work consolidated in one place.

Note: See TracTickets for help on using tickets.