#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: |
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 totrue
if thes
parameter is used in a REST request? Similarly, shouldis_date
and others be set based on query arguments, as they are now? - Do we need an
is_rest
attribute forWP_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)
Change History (14)
This ticket was mentioned in Slack in #core by danielbachhuber. View the logs.
9 years ago
#4
@
9 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
@
9 years ago
Just so it's clearly stated, as a developer:
- I'd expect
is_home=>false
andis_rest=>true
when I hook intopre_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 ontopre_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
@
9 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.
9 years ago
#8
@
9 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
@
9 years ago
- Keywords has-patch commit added; dev-feedback needs-patch removed
- Owner set to boonebgorges
- Status changed from new to assigned
Is this true universally? Is there a legitimate use case for a "home" endpoint? If
WP_Query
hardcodes anis_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 ofparse_query()
.is_feed
(which I assume is what you're basingis_rest
on) is one such offender. Anis_rest
flag, and anis_home
exception whenis_rest
, has the selling point of internal consistency. This is, in fact, what the new embed features in 4.4 do (see [34903] andis_embed
). So, short of a rewrite of howWP_Query::parse_query()
determines context, I guess it's what we should do here too.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, awp/v2/post/
request containing date params is, in fact, a date request.is_search
is a bit different becauseWP_Query
uses it internally to determine fallback values forposts_per_page
andposts_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.