get_routes() called multiple times within single REST request causing the rest_endpoints() filter to also fire more than once

[ruud@…]

Hi all,

Many thanks for creating the REST API, and also for getting it into core! :)

When I had a closer look at how to integrate this in our projects I noticed something peculiar with the rest_endpoints() filter: it is called multiple times over; in some cases twice, in others three times.

So I did a little digging around and found that the root cause seemed to be the use of get_routes() at multiple locations:

• in the rest_pre_dispatch filter (rest_handle_options_request)
• in the rest_post_dispatch filter (rest_send_allow_header)
• in the dispatch() itself
• in the get_index() method
• in the get_namespace_index() method

After looking how these locations interact with each other, I couldn't detect any code which altered the generated route map between consecutive calls to get_routes().

I will add a patch in which I propose to store the generated route map in the class, and re-use that one instead of generating yet again the same array (and also re-filtering the same array).

Since the name 'endpoints' is already taken, and being used in the initialization as well, I thought it would be prune to use another variable name: $route_map, which is also being used in the current doc-block.

I did not profile this patch (not really sure how to do that), so I'm not sure if storing this rather large associative array is a good thing to do. However generating it multiple times (and re-filtering it also) may also be quite 'expensive'.

Thanks,
Ruud

[rmccue]

This seems OK, but could potentially break when switching between sites with multisite. I'd be curious to hear from @jnylen0 on that point.

[jnylen0]

I'm not worried about this. Code really shouldn't rely on get_routes being recomputed, and I'm not aware of any that does. But if there is a need to do this in the future, said "interesting" code can call set_route_map( null ).

We should also remove the extra level of indentation and most of the whitespace changes in this patch by switching the conditional around:

if ( ! empty( $endpoints ) ) {
    return $endpoints;
}

[ruud@…]

Hi Ryan and James,

Thanks for having a look at this patch.

What is your opinion about introducing a new variable $route_map, instead of re-using $endpoints?
I this patch I did not use $endpoints with a test for it being empty because it is not, it gets initialized with the default root endpoint.

I have time to fix any code for this patch today, so let me know.

Thanks,
Ruud

[ruud@…]

Patch against version 4.7.1, extra level of indentation removed, made getter/setter functions protected

[ruud@…]

Since the $route_map is a protected variable inside this class, I thought it may be appropriate to also use the getter and setter as protected functions for this class.

[jnylen0]

The getter/setter functions should be public in case other code needs to manipulate the cached route map.

I think we can resolve the remaining naming issues here by calling the variable inside the method $route_map instead of $endpoints. Think of it this way: $endpoints is the raw endpoints and $route_map is the parsed and sanitized map from a regex to an endpoint.

[ruud@…]

Methods now public, added clarification about use of $route_map vs $endpoints

[ruud@…]

@jnylen0. I agree with your assessment about the naming issues. Do you also mean to change all references to $endpoints (except the $this->endpoints) to $route_map in get_routes() ?

[TimothyBlynJacobs]

I don't think we should expose the ability to overwrite the internal request map entirely, I think we should change this to something more specific that only allows for clearing it out. Perhaps ::clear_route_map.

[mreishus]

refreshed for 2023; handling new $route_namespace parameter

[mreishus]

I'm happy to revive this; get_routes() in the meantime now has a parameter: get_routes( $route_namespace ). I came up with an idea in the attached patch above, but it doesn't feel quite right. Any ideas?

This patch also doesn't solve the problem of get_routes() being called more than once, because the $route_namespace optimization was applied to the main callsite of get_routes(), but not the others. But that could be tackled as a second step.