Make WordPress Core

Opened 6 years ago

Closed 2 months ago

Last modified 12 days ago

#42061 closed enhancement (fixed)

Add new utility function for checking if the current request is a REST API request.

Reported by: lots0logs's profile lots.0.logs Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 6.5 Priority: normal
Severity: normal Version: 4.9
Component: Bootstrap/Load Keywords: has-patch dev-feedback commit has-unit-tests add-to-field-guide
Focuses: rest-api, performance Cc:

Description (last modified by joehoyle)

This patch adds a new function: wp_doing_rest() and filter by the same name that work exactly like the current wp_doing_ajax/cron() functions but for Rest API requests.

Attachments (2)

42061-add-wp-doing-rest-filterable-function.patch (1.7 KB) - added by lots.0.logs 6 years ago.
42061.diff (3.1 KB) - added by flixos90 6 years ago.

Download all attachments as: .zip

Change History (44)

#1 @lots.0.logs
6 years ago

  • Keywords has-patch added

#2 @joehoyle
6 years ago

  • Description modified (diff)
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

See https://github.com/WP-API/WP-API/issues/926 for the previous story on this.

Though we want to discourage use of global state functions like these, there _is_ a REST_REQUEST constant for this purpose. As mentioned in that ticket, the idea of "the current request is a REST one" breaks down for things like _embed requests, and internal rest_do_request calls, therefore we don't want to encourage the use of "is the current request a REST one" as it's not always in that context.

@lots.0.logs perhaps you could define why you need such a function? wp_doing_ajax/cron() are inherently process-level functions, but the REST API callback methods are not intended to scoped to the whole "page load" request.

#3 @flixos90
6 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

I think this needs to be revisited, for the same purpose we have functions like wp_doing_ajax() etc. Every function in WordPress can be misused, but a wp_doing_rest() or wp_is_rest_request() would allow fixing weird hacks necessary when working with unit tests. See #43751 for an example.

#4 @SergeyBiryukov
6 years ago

  • Milestone set to Awaiting Review

#5 @swissspidy
6 years ago

  • Milestone changed from Awaiting Review to 5.0

+1 for consistency.

See #25669, #39591.

@flixos90
6 years ago

#6 @lots.0.logs
6 years ago

I see you changed the name to wp_is_rest_request(). That is inconsistent with existing similar utility functions. IMO, wp_doing_rest() is the most appropriate name for this function.

#7 follow-up: @flixos90
6 years ago

  • Keywords dev-feedback added
  • Owner set to flixos90
  • Status changed from reopened to assigned

As usual, this is more complicated than it may seem at first glance. Other than AJAX and Cron, which are controlled by constants, REST is controlled by a query variable, meaning it relies on the current request to be parsed. The REST_REQUEST constant is only set for an easy check, but is not available from the beginning of the request's lifecycle.

It is therefore impossible to detect a REST request before the parse_request action, at least with the means currently in place. That action is triggered rather late, after init, wp_loaded etc.

There is actually a bug in core which doesn't take the above into account: in wp_debug_mode(), it is checked whether the current request is a REST request in order to not display errors in such a case. However, this check will always result in false, since the function is executed even before most of WordPress files are even loaded. Therefore, currently REST API responses will still be messed up by debugging messages if enabled. This could be fixed in a separate issue though, depending on what we decide here.

I see two possibilities:

  • We could embrace the current functionality, relying on the query variable and introduce the function in a way that is triggers _doing_it_wrong() if called too early. In that case, it should be named differently from the existing wp_doing_ajax() and wp_doing_cron(), to indicate its different behavior (I suggest wp_is_rest_request() because in WordPress terms that connects it to the request which is associated with parsing the current request).
  • We could find a way to detect a REST request through another way and aim for making the process similar to AJAX and Cron so that it can be detected really early. In that case, the function could also be named accordingly, wp_doing_rest().

I'm in favor of the first alternative, and this is what I implemented in 42061.diff:

  • It introduces wp_is_rest_request() in wp-includes/functions.php. No point to put it in wp-includes/load.php under these circumstances.
  • If parse_request has not been executed yet and REST_REQUEST is not set (this check would allow custom setups to still set it early), but the function is called already, a _doing_it_wrong() is triggered.
  • The check for a REST request in wp_debug_mode() has been removed, since it doesn't do anything. Since I adjusted that line anyway, I also fixed a direct check of WP_INSTALLING, which should instead rely on wp_installing().
  • Other checks of REST_REQUEST has been adjusted as well, and the additional check in wp-admin/includes/ms.php is something I detected during #43751 (it could currently simply kill REST requests).

#8 @flixos90
5 years ago

  • Milestone changed from 5.0 to 5.1

#9 @swissspidy
5 years ago

Worth noting that #44534 introduced wp_is_json_request()

#10 @pento
5 years ago

wp_is_json_request() replaced some of the use cases for this function.

@flixos90: Do you think wp_doing_rest() is still needed?

#11 @ocean90
5 years ago

#45875 was marked as a duplicate.

#12 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release

Punting this, as it requires further consideration.

#14 @mikejolley
5 years ago

@pento We could use this in WooCommerce. We do things such as load the cart after wp_loaded action, but we're unable to disable this based on REST_REQUEST due to that constant not being set until parse_request.

A function, available earlier, would be very useful.

ref https://github.com/woocommerce/woocommerce/issues/20937

#15 in reply to: ↑ 7 @iandunn
4 years ago

Replying to flixos90:

REST is controlled by a query variable...
It is therefore impossible to detect a REST request before the parse_request action, at least with the means currently in place...
We could find a way to detect a REST request through another way and aim for making the process similar to AJAX and Cron so that it can be detected really early.

I think that'd be ideal. One benefit would be that WP_USE_THEMES could be set to false for REST API requests, and wp_using_themes() could be used to detect the front end before template_redirect executes.

One way to do that might be something like the solution that Woo experiment with. I haven't looked deeply to see if there are cases where that wouldn't work, though.

#16 @TimothyBlynJacobs
3 years ago

  • Focuses performance added
  • Milestone changed from Future Release to 5.7

I'm going to attempt to take a look at this for 5.7 I agree with @iandunn that we'd need to do something like that to allow for checking if this is a REST API request early enough.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#18 @hellofromTonya
3 years ago

  • Keywords needs-refresh added

Though this ticket has a patch, it's still in the exploration phase. Marking it for refresh. As Timothy noted in today's scrub:

We have some code that does this that I’m going to see if will work in a Core context. So it’ll need testing and review soon.

This ticket was mentioned in PR #916 on WordPress/wordpress-develop by TimothyBJacobs.


3 years ago
#19

  • Keywords needs-refresh removed

#20 @TimothyBlynJacobs
3 years ago

  • Owner changed from flixos90 to TimothyBlynJacobs

As @flixos90 mentioned, checking if this is a REST API request "properly" requires parsing rewrite rules. As such, if we want to support checking this early, we essentially need to duplicate some of the logic in WP::parse_request. I've attached a PR that does this and works in my testing so far with the built-in dev environment for.

  • http://localhost:8889/wp-json
  • http://localhost:8889/index.php/wp-json
  • http://localhost:8889/index.php?rest_route=/
  • http://localhost:8889/?rest_route=/

Just because we are replicating the logic doesn't mean this guarantees correctness since the query could be modified with a number of hooks that would change behavior and the rewrite rules could be setup to be something completely different from rest_api_register_rewrites. I think there may also be issues if WP_Rewrite::$index is overwritten and the function is called before WP_Rewrite is setup.

There is also the handling for when the REST API is accessed without pretty permalink. An issue here is that essentially we are trying to detect if wp will be called, but there is no way to guarantee wp will or will not be called before it is called. I implemented this by checking for the WP_USE_THEMES constant which works for vanilla core AFAICT, but again customized setups might have issues.,

However, I think we should be able to make it work for all "out of the box" server setups, ie where those advanced customizations haven't been made. If a site is making those adjustments, they could use the filter to provide more accurate detection.

If we want to continue with this, it'd need to be manually tested in a number of different server environments. And we'd have to be careful about usage. I'd think Core should probably only use the function post parse_request. I think it is still useful to allow for it to be called prior to that for usage in plugins.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#22 @hellofromTonya
3 years ago

  • Milestone changed from 5.7 to 5.8

Per Timothy during today's scrub:

Definitely :football: . It could use some opinions on my latest comment too if anyone is interested.

Moving to 5.8.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 years ago

#24 @desrosj
3 years ago

  • Milestone changed from 5.8 to Future Release

I'm going to move this to Future Release. It's still waiting for feedback and will likely need refinement. The 5.8 feature freeze date is also today.

#25 @Cybr
10 months ago

Please consider at least implementing 42061-add-wp-doing-rest-filterable-function.patch. There's always time to overhaul the codebase to use this new API function.

#26 @flixos90
5 months ago

Reviving this ticket, as the lack of this function also prevents proper testing to discover bugs like that fixed in https://github.com/WordPress/wordpress-develop/pull/5514.

@TimothyBlynJacobs I wonder whether we should take a step back here. I'm not sure we really need to go down the rewrite rules rabbit hole. What about simply having a function that applies a filter instead of being forced to check defined( 'REST_REQUEST' ) && REST_REQUEST? It would make code paths testable that were previously not testable. The filter could be used to set the value temporarily to true for preloaded REST requests (i.e. those where REST routes are called without actually triggering a REST request).

To some degree, that would achieve a different goal, and we could consider reflecting that in the function's and filter's name accordingly. But I think it's worth considering that approach, as it would be much simpler and would bring immediate benefits to testability of core.

Last edited 5 months ago by flixos90 (previous) (diff)

#27 @TimothyBlynJacobs
5 months ago

Big +1 to making things more testable. But yeah I do think it is solving a slightly different problem, and we do want to make that very clear in the function name.

#28 @flixos90
5 months ago

@TimothyBlynJacobs How do you feel about using this ticket for that function? FWIW, I don't think the original ticket was clear about the intended purpose of the function, as in:

  • Is this an actual REST request?
  • Or is this a WP REST route being hit (whether through a request, or through preloading within a regular WordPress request?

This ticket was mentioned in PR #5658 on WordPress/wordpress-develop by @flixos90.


4 months ago
#29

This PR introduces two new functions:

  • wp_is_rest_request() checks whether the current request is an actual "standalone" REST request. This function does not provide any filter, similar to how e.g. it is not possible to filter whether a request is a WP Admin request. There's probably no good reason to change that now.
  • wp_is_rest_endpoint() checks whether a REST request to an endpoint is currently being handled. This is the case for any standalone REST request, as well as any internal REST requests made via another regular WordPress request (e.g. via rest_preload_api_request()). This function provides a filter as it can allow custom implementations for handling a REST request to remain compatible with this check.

The rationale for two separate functions is that those two things are distinct aspects. Most current core usage checking the REST_REQUEST constant in core is actually about whether a REST endpoint is being handled. Only a few general infrastructure checks depend on whether the current request is actually a "standalone" REST request.

All relevant usages of REST_REQUEST have been updated, or a comment has been added why they should not be updated.

Trac ticket: https://core.trac.wordpress.org/ticket/42061

#30 @flixos90
4 months ago

  • Milestone changed from Future Release to 6.5

@TimothyBlynJacobs I've taken another stab at this via https://github.com/WordPress/wordpress-develop/pull/5658. Please refer to the PR description for more context on what approach I am proposing.

TL;DR: Most core use that checks REST_REQUEST is not actually about whether a "standalone" REST API request is made than it is about whether a REST endpoint request is currently being handled (e.g. could also be a preload request made within a regular WordPress request).

Let me know your thoughts on the PR. If it's valuable, we could also consider splitting this apart into another ticket if you feel like it doesn't address the original request here as much.

I'm putting this into 6.5 for consideration.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


2 months ago

@flixos90 commented on PR #5658:


2 months ago
#33

@TimothyBJacobs Is there any chance you can get to reviewing this? :)

@TimothyBlynJacobs commented on PR #5658:


2 months ago
#34

I know we document expect usage timing in the function docs, but it feels so similar to things like wp_doing_ajax or wp_doing_cron that I think developers will expect they can use it early.

Maybe wp_is_serving_rest_request would make that a bit clearer that it won't return true until the REST API Server is actually handling it?

@flixos90 commented on PR #5658:


2 months ago
#35

Thanks @TimothyBJacobs, I've updated the PR based on your feedback.

#36 @flixos90
2 months ago

  • Keywords commit added

@flixos90 commented on PR #916:


2 months ago
#37

While the purpose of this PR is different, in terms of https://core.trac.wordpress.org/ticket/42061 it is now superseded by #5658.

We may want to open another ticket for the specific point of checking for a REST API request _prior_ to parsing the query, as that's a more specific use-case than what was mostly discussed on the ticket.

#38 @flixos90
2 months ago

  • Keywords has-unit-tests added

#39 @flixos90
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57312:

Bootstrap/Load: Introduce functions to check whether WordPress is serving a REST API request.

This changeset introduces two functions:

  • wp_is_serving_rest_request() returns a boolean for whether WordPress is serving an actual REST API request.
  • wp_is_rest_endpoint() returns a boolean for whether a WordPress REST API endpoint is currently being used. While this is always the case if wp_is_serving_rest_request() returns true, the function additionally covers the scenario of internal REST API requests, i.e. where WordPress calls a REST API endpoint within the same request.

Both functions should only be used after the parse_request action.

All relevant manual checks have been adjusted to use one of the new functions, depending on the use-case. They were all using the same constant check so far, while in fact some of them were intending to check for an actual REST API request while others were intending to check for REST endpoint usage.

A new filter wp_is_rest_endpoint can be used to alter the return value of the wp_is_rest_endpoint() function.

Props lots.0.logs, TimothyBlynJacobs, flixos90, joehoyle, peterwilsoncc, swissspidy, SergeyBiryukov, pento, mikejolley, iandunn, hellofromTonya, Cybr, petitphp.
Fixes #42061.

#41 @stevenlinx
6 weeks ago

  • Keywords add-to-field-guide added

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


12 days ago

Note: See TracTickets for help on using tickets.