REST API: Allowed public or private WP_Query vars are lost in `prepare_items_query()`

[rachelbaker]

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.

[jnylen0]

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.

[rachelbaker]

@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.

[joehoyle]

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.

[danielbachhuber]

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.

[jnylen0]

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.

[danielbachhuber]

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() {

[jnylen0]

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

[danielbachhuber]

@rmccue @rachelbaker @joehoyle How do we get to a decision on this?

[joehoyle]

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!

[danielbachhuber]

@joehoyle How do you propose handling the request arg to query var transformation for any collection arguments handled by the filter?

[joehoyle]

@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 );

[danielbachhuber]

@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?

[joehoyle]

@danielbachhuber yes, I think we just need the filters.

[rmccue]

Remove WP_REST_Posts_Controller::get_allowed_query_vars()

[rmccue]

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.

[rmccue]

Patch without rest_query_var-{var} filter

[rmccue]

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?

[joehoyle]

In 39162:

REST API: Remove get_allowed_query_vars() now filter is gone.

Now all public query vars are not supoprted via ?filter in the REST API, we can remove the get_allowed_query_vars() method and filter. To provide developers with a good altnerative to filter, the "rest_{$this->post_type}_collection_params" filter has been added.

Props rmccue, rachelbacker, danielbachhuber.
Fixes #38629.

[joehoyle]

Created https://core.trac.wordpress.org/ticket/38710 to implement collection params in other controllers, per @jnylen0's comment.

[jnylen0]

Remove rest_query_var-{$key} filter

[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.

[rmccue]

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.

[joehoyle]

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!

[jnylen0]

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.