Make WordPress Core

Opened 7 years ago

Last modified 14 months ago

#39473 new enhancement

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

Reported by: ruudjoyo's profile ruud@… Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: needs-unit-tests good-first-bug dev-feedback
Focuses: Cc:

Description

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

Attachments (4)

39473.patch (5.1 KB) - added by ruud@… 7 years ago.
39473-2.patch (2.0 KB) - added by ruud@… 7 years ago.
Patch against version 4.7.1, extra level of indentation removed, made getter/setter functions protected
39473-3.patch (2.1 KB) - added by ruud@… 7 years ago.
Methods now public, added clarification about use of $route_map vs $endpoints
39473-2023-1.patch (3.1 KB) - added by mreishus 14 months ago.
refreshed for 2023; handling new $route_namespace parameter

Download all attachments as: .zip

Change History (14)

@ruud@…
7 years ago

#1 @ruud@…
7 years ago

  • Component changed from General to REST API
  • Keywords has-patch dev-feedback added

#2 @rmccue
7 years ago

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

#3 @jnylen0
7 years ago

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;
}

#4 @ruud@…
7 years ago

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@…
7 years ago

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

#5 @ruud@…
7 years ago

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.

#6 @jnylen0
7 years ago

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@…
7 years ago

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

#7 @ruud@…
7 years ago

@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() ?

#8 @TimothyBlynJacobs
3 years ago

  • Keywords needs-refresh needs-unit-tests good-first-bug added; has-patch dev-feedback removed

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
14 months ago

refreshed for 2023; handling new $route_namespace parameter

#9 @mreishus
14 months ago

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.

Last edited 14 months ago by mreishus (previous) (diff)

#10 @obenland
14 months ago

  • Keywords dev-feedback added; needs-refresh removed
Note: See TracTickets for help on using tickets.