WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#34373 closed defect (bug) (fixed)

Don't set is_home=>true in WP_Query during REST request

Reported by: danielbachhuber Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

WP_Query sets is_home=>true when a variety of other conditions aren't set: https://core.trac.wordpress.org/browser/tags/4.3/src/wp-includes/query.php#L1751

In a REST request, is_home should remain false as home isn't a defined context in a REST request.

We could extend the conditional to respect the REST_REQUEST constant. However, it raises a couple more questions:

  • Should is_search be set to true if the s parameter is used in a REST request? Similarly, should is_date and others be set based on query arguments, as they are now?
  • Do we need an is_rest attribute for WP_Query?

Related conversation: https://github.com/WP-API/WP-API/issues/926

Core ticket stems from: https://github.com/WP-API/WP-API/issues/1014

Attachments (2)

34373.1.diff (754 bytes) - added by danielbachhuber 4 years ago.
Define WP_Query::$is_rest and WP_Query::is_rest()
34373.2.diff (882 bytes) - added by danielbachhuber 4 years ago.
Don't set is_home when REST_REQUEST

Download all attachments as: .zip

Change History (13)

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


4 years ago

#2 @boonebgorges
4 years ago

In a REST request, is_home should remain false as home isn't a defined context in a REST request.

Is this true universally? Is there a legitimate use case for a "home" endpoint? If WP_Query hardcodes an is_rest, it's going to mean some awkward workarounds in the case where one actually wants to make a 'home' query in a REST request.

Then again, WP_Query is terrible about mixing concerns in exactly this way. It's the whole purpose of parse_query(). is_feed (which I assume is what you're basing is_rest on) is one such offender. An is_rest flag, and an is_home exception when is_rest, has the selling point of internal consistency. This is, in fact, what the new embed features in 4.4 do (see [34903] and is_embed). So, short of a rewrite of how WP_Query::parse_query() determines context, I guess it's what we should do here too.

Should is_search be set to true if the s parameter is used in a REST request? Similarly, should is_date and others be set based on query arguments, as they are now?

The is_* date flags are generally used in templates, which makes them irrelevant for REST requests. My inclination would be to let them continue to be set, since it's semantically true that, eg, a wp/v2/post/ request containing date params is, in fact, a date request. is_search is a bit different because WP_Query uses it internally to determine fallback values for posts_per_page and posts_per_page. Historically, these fallbacks are intended for regular browser requests, so it makes sense that you wouldn't want them set in a REST request. Those are my thoughts, at least - I'm happy to defer to what your team thinks, since you've thought a lot more about use cases.

@danielbachhuber
4 years ago

Define WP_Query::$is_rest and WP_Query::is_rest()

#4 @danielbachhuber
4 years ago

I started working on a patch to set WP_Query::$is_rest, and got stuck on how the the parameter should actually be set.

In the main query, we can check for rest_route. If it's not empty, it would sensibly define is_rest. However, the main query isn't very useful a request to /wp-json/.

As a developer, if I need to make use of WP_Query within my endpoint callback, I'll instantiate a new WP_Query, which won't have a rest_route argument (typically).

If I want to only hook into pre_get_posts or similar, I could check defined( 'REST_REQUEST' ). But, this constant is defined in rest_api_loaded, which may not have executed fully if I'm calling the REST API from some alternative context.

#5 @danielbachhuber
4 years ago

Just so it's clearly stated, as a developer:

  • I'd expect is_home=>false and is_rest=>true when I hook into pre_get_posts to filter a query.
  • I'd expect is_rest=>true when I use WP_Query inside of a custom endpoint callback, such that any callbacks hooked onto pre_get_posts know the appropriate context.

For the greatest precision, WP_Query should set is_rest=>true any time it's called within WP_REST_SERVER::dispatch().

#6 @joehoyle
4 years ago

I think is_home should be false, as there's no such thing as a "home" request in the API. I don't really see the architectural argument for having is_rest in WP_Query, but given it's convenient for 3rd parties hooking into to WP_Query to modify things, having it there will no doubt be useful.

It's worth mentioning that the REST API doesn't make use of the "main query", there is no global $wp_query on rest requests, where these nasty is_ things are typically used.

In terms of how we achieve this, I think the implementation of is_rest could just check for defined( 'REST_REQUEST' )?

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


4 years ago

@danielbachhuber
4 years ago

Don't set is_home when REST_REQUEST

#8 @danielbachhuber
4 years ago

34373.2.diff bails on setting is_home=>true when defined( 'REST_REQUEST' ) && REST_REQUEST

While this approach adds more ugliness to an already ugly conditional, it's also the most future proof. We solve most of the immediate problem while keeping the possibility of introducing WP_Query::$is_rest in the future, or changing how variables are set.

#9 @danielbachhuber
4 years ago

  • Keywords has-patch commit added; dev-feedback needs-patch removed
  • Owner set to boonebgorges
  • Status changed from new to assigned

#10 @boonebgorges
4 years ago

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

In 35690:

In WP_Query, set is_home to false during REST requests.

Props danielbachhuber.
Fixes #34373.

This ticket was mentioned in Slack in #core-restapi by user-267. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.