#34373 closed defect (bug) (fixed)
Don't set is_home=>true in WP_Query during REST request
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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_searchbe set totrueif thesparameter is used in a REST request? Similarly, shouldis_dateand others be set based on query arguments, as they are now? - Do we need an
is_restattribute 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.
10 years ago
#4
@
10 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
@
10 years ago
Just so it's clearly stated, as a developer:
- I'd expect
is_home=>falseandis_rest=>truewhen I hook intopre_get_poststo filter a query. - I'd expect
is_rest=>truewhen I use WP_Query inside of a custom endpoint callback, such that any callbacks hooked ontopre_get_postsknow 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
@
10 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.
10 years ago
#8
@
10 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
@
10 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_Queryhardcodes 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_Queryis 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_reston) is one such offender. Anis_restflag, and anis_homeexception 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_searchis a bit different becauseWP_Queryuses it internally to determine fallback values forposts_per_pageandposts_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.