#38629 closed defect (bug) (fixed)
REST API: Allowed public or private WP_Query vars are lost in `prepare_items_query()`
Reported by: | rachelbaker | Owned by: | joehoyle |
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | has-patch |
Focuses: | Cc: |
Description
Expected behavior #1: using a public WP query var like ?cat=5
should filter the collection response from wp/v2/posts
Expected behavior #2: Using the rest_query_vars
filter, I should be able to include a private query var, like tag_slug__in
and requests to wp/v2/posts?tag_slug__in=myslug
should filter the collection response.
The issue appears to be in prepare_items_query()
where we are only checking if $prepared_args
are set and included in the $valid_vars
array. We are not including additional vars from the $request
object.
Attachments (4)
Change History (30)
#2
@
8 years ago
- Owner set to rmccue
- Status changed from new to assigned
@rmccue There seems to be some disagreement among @joehoyle, @kadamwhite, and myself regarding if public WP Query vars were expected to be supported.
Right now get_allowed_query_vars()
is broken. It does nothing for any parameter that isn't defined in the collection schema AND in the $parameter_mappings
array in get_items()
.
@joehoyle and I tossed around some possible alternate solutions such filtering get collection_params()
for each endpoint's schema to be filterable. I know this is a "feature" that the API has had from the beginning, and that losing this functionality with the removal of the filter
param may not have been something you were aware of.
This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.
8 years ago
#4
@
8 years ago
I don’t think they need to be supported, in the rest api. the most we should do is at least make it possible for devs to add their own query args. That would mean adding filters to the collection args, and then leaving it up to developers to implement their modification to the WP_Query via the rest_$type_query
.
#5
@
8 years ago
38629.1.diff transforms request arguments to query arguments based on an optional query_var
parameter, which permits plugins and themes to filter collection params to more easily include their own.
#6
follow-up:
↓ 7
@
8 years ago
This looks nice and clean overall. A comment and a question:
- I'm not sure about "Enables adding or removing collection parameters from the endpoint." Maybe it shouldn't be strictly impossible, but we decided over at #38339 that we should discourage disabling the built-in features.
- Do all of the affected arguments have working test cases, including anything that touches
WP_REST_Attachments_Controller
? I know'slug' => 'post_name__in'
is fine and'status' => 'post_status'
will be fine once #38420 is resolved.
#7
in reply to:
↑ 6
@
8 years ago
Replying to jnylen0:
- Do all of the affected arguments have working test cases, including anything that touches
WP_REST_Attachments_Controller
? I know'slug' => 'post_name__in'
is fine and'status' => 'post_status'
will be fine once #38420 is resolved.
Our test coverage is pretty good but not perfect right now. We'd probably want to add a test for registering a custom public query arg and a custom private query arg
$ ack 'test_get_items' rest-posts-controller.php rest-pages-controller.php rest-attachments-controller.php rest-posts-controller.php 111: public function test_get_items() { 123: public function test_get_items_empty_query() { 134: public function test_get_items_author_query() { 160: public function test_get_items_author_exclude_query() { 188: public function test_get_items_include_query() { 207: public function test_get_items_exclude_query() { 229: public function test_get_items_search_query() { 245: public function test_get_items_slug_query() { 257: public function test_get_items_multiple_slugs_array_query() { 275: public function test_get_items_multiple_slugs_string_query() { 293: public function test_get_items_status_query() { 313: public function test_get_items_invalid_status_query() { 321: public function test_get_items_status_without_permissions() { 338: public function test_get_items_order_and_orderby() { 357: public function test_get_items_with_orderby_relevance() { 367: public function test_get_items_ignore_sticky_posts_by_default() { 388: public function test_get_items_offset_query() { 407: public function test_get_items_tags_query() { 424: public function test_get_items_tags_exclude_query() { 443: public function test_get_items_tags_and_categories_query() { 463: public function test_get_items_tags_and_categories_exclude_query() { 485: public function test_get_items_sticky_query() { 502: public function test_get_items_sticky_with_post__in_query() { 531: public function test_get_items_not_sticky_query() { 548: public function test_get_items_sticky_with_post__not_in_query() { 570: public function test_get_items_pagination_headers() { 649: public function test_get_items_private_status_query_var() { 666: public function test_get_items_invalid_per_page() { 673: public function test_get_items_invalid_context() { 680: public function test_get_items_invalid_date() { 688: public function test_get_items_valid_date() { rest-pages-controller.php 80: public function test_get_items() { 84: public function test_get_items_parent_query() { 100: public function test_get_items_parents_query() { 118: public function test_get_items_parent_exclude_query() { 134: public function test_get_items_menu_order_query() { 161: public function test_get_items_min_max_pages_query() { 178: public function test_get_items_private_filter_query_var() { 196: public function test_get_items_invalid_date() { 204: public function test_get_items_valid_date() { rest-attachments-controller.php 166: public function test_get_items() { 194: public function test_get_items_logged_in_editor() { 221: public function test_get_items_media_type() { 240: public function test_get_items_mime_type() { 259: public function test_get_items_parent() { 295: public function test_get_items_invalid_status_param_is_error_response() { 310: public function test_get_items_private_status() { 330: public function test_get_items_invalid_date() { 338: public function test_get_items_valid_date() {
#8
@
8 years ago
WP_REST_Comments_Controller
should get similar treatment if we are doing it this way: https://github.com/WP-API/WP-API/blob/57e027e/lib/endpoints/class-wp-rest-comments-controller.php#L132
#11
@
8 years ago
I think the filter is good, which we should add to all controllers. I'm still -1 on moving the query_var
to the registration. Mainly because I think it's a level of abstraction that isn't needed, and carry this pattern to cross function boundaries (get_collection_params to get_items) makes this unpleasant peace of code even more embedded across the controller. I think it was simpler beforehand, and I don't think it's worth the up side flexibility to add this.
I'm deferring to @rmccue for the direction there though - above is my opinion but I have no issue if I'm outvoted on this!
#12
@
8 years ago
@joehoyle How do you propose handling the request arg to query var transformation for any collection arguments handled by the filter?
#13
@
8 years ago
@danielbachhuber I don't think we should do it in an automatic way, if I wanted to add support for a query var, I'd do this:
<?php add_action( 'rest_post_collection_params', function( $params ) { $params['date_month'] => array( 'type' => 'integer', 'description' => 'Restrict posts to ones published in a specific month.' ); return $params; } ); add_filter( 'rest_post_query', function( $query_vars, $request ) ) { if ( $request['date_month'] ) { $query_vars['month'] = $request['date_month']; } return $query_vars; }, 10, 2 );
#14
@
8 years ago
@joehoyle I'm amenable to the approach you described. If this is the case, then we only need to add the filters to fix this ticket?
#16
@
8 years ago
Added a patch that removes get_allowed_query_vars()
. Per discussion, this is really only needed when we were accepting anything. A plugin that re-implements filter
(removed in #38378) would need to add these filters back for filter
sub-parameters.
This patch adds a new filter: rest_{type}_collection_params
, allowing implementation per Joe's comment above. I've also retained the rest_query_var-{var}
filter, although I'm not sure whether we need that, as rest_{type}_query
can be used on the array instead.
#17
@
8 years ago
Added a second patch that also removes the rest_query_var-{var}
filter, as it's somewhat useless.
@rachelbaker Was this the solution you were thinking of here?
This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.
8 years ago
#21
@
8 years ago
Created https://core.trac.wordpress.org/ticket/38710 to implement collection params in other controllers, per @jnylen0's comment.
#22
follow-up:
↓ 23
@
8 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
@joehoyle it looks like the rest_query_var-{$key}
filter is still in trunk after [39162], I think we should remove it here since there's a cleaner way of doing the same thing.
#23
in reply to:
↑ 22
@
8 years ago
Replying to jnylen0:
@joehoyle it looks like the
rest_query_var-{$key}
filter is still in trunk after [39162], I think we should remove it here since there's a cleaner way of doing the same thing.
FWIW, it was a conscious decision from @joehoyle to leave this to avoid the breaking change on that filter.
@joehoyle Up to you on whether you want to remove it in a follow-up commit here, but if you do, please do so before beta 3.
#24
@
8 years ago
- Resolution set to fixed
- Status changed from reopened to closed
Yes, I think we should leave the filter in, it does no harm and won't break any compat. It's also useful in some situations as there's a filter per-var. @jnylen0 I'll setting this back to closed due to the explanation as I'm presuming this was re-opened as it was thought there it was overlooked. However, of course if you disagree I'm happy to discuss!
#25
@
8 years ago
I'm presuming this was re-opened as it was thought there it was overlooked
Yep, my mistake.
I figured it was better to remove this than have to support it. I also don't think it's very well-named, since there's nothing in the filter name that suggests post
, but this filter isn't present anywhere else.
Not a big deal either way though.
So this is what you were talking about in #38579. Would be good to get a test case for the
name
query var because I suspect it may be broken in trunk.