Exclude unsupported query parameters in posts controller by rmccue · Pull Request #2770 · WP-API/WP-API

rmccue · 2016-10-06T22:29:56Z

danielbachhuber · 2016-10-06T22:31:56Z

lib/endpoints/class-wp-rest-posts-controller.php

+		$args['posts_per_page'] = $request['per_page'];
+		$args['name']           = $request['slug'];
+		$args['post_status']    = $request['status'];
+		$args['s']              = $request['search'];


We should have conditionals around these too, so a user can deregister the parameter if they don't want to support the query (or if the resource isn't meant to implement a given field like slug)

Addressed in 62a75b9

kadamwhite · 2016-10-07T02:46:59Z

lib/endpoints/class-wp-rest-posts-controller.php

+		}
+		if ( isset( $registered['parent_exclude'] ) ) {
+			$args['post_parent__not_in']  = $request['parent_exclude'];
+		}


What about possibly changing this section to something like

$parameter_mappings = array( 'offset' => 'offset', 'order' => 'order', 'orderby' => 'orderby', 'page' => 'page', 'include' => 'post__in', 'exclude' => 'post__not_in', 'slug' => 'name', 'status' => 'post_status', 'search' => 's', 'author' => 'author__in', 'author_exclude' => 'author__not_in', 'menu_order' => 'menu_order', 'parent' => 'post_parent__in', 'parent_exclude' => 'post_parent__not_in', ) foreach ( $parameter_mappings as $query_param => $wp_query_param ) { if ( isset( $registered[ $query_param ] ) ) { $args[ $wp_query_param ] = $request[ $query_param ]; } }

to make it a little less repetitive?

that would also be so much easier to read.

rachelbaker · 2016-10-08T14:06:27Z

Just a note that after this PR gets merged we will need to implement in the other endpoints as well.

rachelbaker · 2016-10-08T14:17:25Z

lib/endpoints/class-wp-rest-posts-controller.php

-		$args['post_parent__not_in']  = $request['parent_exclude'];
-		$args['post_status']          = $request['status'];
-		$args['s']                    = $request['search'];
+		$registered = $this->get_collection_params();


@rmccue Seems like this check against the registered params should happen in get_allowed_query_vars(), there is some duplicate logic there.

rachelbaker · 2016-10-11T15:55:14Z

This looks to be a pretty high-priority ticket if we want to get this done prior to a merge. The approach set in this PR will need to be duplicated in all of our other controllers.

kadamwhite · 2016-10-11T16:08:07Z

Assigning myself to take a stab at refreshing this, will be coordinating with @rachelbaker to get more feedback

rachelbaker

I agree with @kadamwhite's suggestion regarding using a loop to reduce the long series of isset() conditionals.
Because we have to do this collection parameter processing across all of our Controllers, it would be ideal to think about how we want to define the mapped parameters in the Controllers. With a protected property or method? This would be cleaner than shoving this logic into the get_items() logic across the board.
Would it make more sense for the check against the registered collection params to happen in prepare_items_query() or get_allowed_query_vars()?
(Very related to #3) The query parameter handling logic in the Posts Controller is already difficult to follow, is there anything we can simplify?

This tightens up the patch in #2770 to use an associative array of public api-to-internal-wp query parameters, reducing the repetition of if blocks in the code. The array only describes those properties that to not require special handling; others (like `before` or `filter`) are still checked against `$registered` individually because their values may not be set as-passed.

kadamwhite · 2016-10-11T19:59:29Z

Added a commit to address (1) of @rachelbaker's feedback. Thoughts on the remainder below:

get_allowed_query_vars doesn't feel like the right place for this because the set of allowed query vars is independent of the vars that are passed for any given request, and the goal here is to ensure a given request does not accept or consider parameters that are not allowed.
prepare_items_query feels like the "right" place to do this: except that by that point we've set WP-internal properties on $args, while the registered parameters list is built off of the API-oriented parameters (include instead of post__in, e.g.). It almost feels like what we want is a prepare_items_request or something that pre-processes the request object to strip values that aren't whitelisted. I don't think that solves the need for much of this code, though, just a couple of the statements in conditionals below; our handling of parameters is non-trivial!
Regarding how to broadly define this type of functionality across controllers... I'm still thinking about it.

kadamwhite · 2016-10-11T21:34:03Z

Open question:

Should "context" be treated in this way too, or should we presume it is present? Seems to be more "global" than these others.

The only params not currently being checked against registered are the "context", which I have an open question on above; and taxonomies, for which a fix would be this diff:

diff --git lib/endpoints/class-wp-rest-posts-controller.php lib/endpoints/class-wp-rest-posts-controller.php
index 86ef4da..45acf9b 100755
--- lib/endpoints/class-wp-rest-posts-controller.php
+++ lib/endpoints/class-wp-rest-posts-controller.php
@@ -191,19 +191,19 @@ class WP_REST_Posts_Controller extends WP_REST_Controller {

        $taxonomies = wp_list_filter( get_object_taxonomies( $this->post_type, 'objects' ), array( 'show_in_rest' => true ) );
        foreach ( $taxonomies as $taxonomy ) {
-           $base = ! empty( $taxonomy->rest_base ) ? $taxonomy->rest_base : $taxonomy->name;
-           $tax_exclude = $base . '_exclude';
+           $tax_base = ! empty( $taxonomy->rest_base ) ? $taxonomy->rest_base : $taxonomy->name;
+           $tax_exclude = $tax_base . '_exclude';

-           if ( ! empty( $request[ $base ] ) ) {
+           if ( isset( $registered[ $tax_base ] ) && ! empty( $request[ $tax_base ] ) ) {
                $query_args['tax_query'][] = array(
                    'taxonomy'         => $taxonomy->name,
                    'field'            => 'term_id',
-                   'terms'            => $request[ $base ],
+                   'terms'            => $request[ $tax_base ],
                    'include_children' => false,
                );
            }

-           if ( ! empty( $request[ $tax_exclude ] ) ) {
+           if ( isset( $registered[ $tax_exclude ] ) && ! empty( $request[ $tax_exclude ] ) ) {
                $query_args['tax_query'][] = array(
                    'taxonomy'         => $taxonomy->name,
                    'field'            => 'term_id',

but this depends on #2802.

rachelbaker · 2016-10-12T01:24:30Z

Should "context" be treated in this way too, or should we presume it is present? Seems to be more "global" than these others.

@kadamwhite Great question. Sounds like ideally we want to prevent context from being de-registered (just a "nice to have" suggestion for a future enhancement), but in the meantime we should fallback to view if it is not present.

…broadly Exclude unsupported params in all controllers (extends #2770)

Exclude unsupported query parameters

aab6377

rmccue added the Bug label Oct 6, 2016

rmccue added this to the 2.0 milestone Oct 6, 2016

danielbachhuber suggested changes Oct 6, 2016

View reviewed changes

rmccue added 2 commits October 6, 2016 18:32

Merge branch 'develop' into exclude-unsupported-params

a472d54

Check other query parameters against registered

62a75b9

kadamwhite reviewed Oct 7, 2016

View reviewed changes

rachelbaker reviewed Oct 8, 2016

View reviewed changes

kadamwhite added the Needs Refresh label Oct 10, 2016

rachelbaker mentioned this pull request Oct 10, 2016

Add support for filter param to comments query #2192

Closed

danielbachhuber approved these changes Oct 11, 2016

View reviewed changes

kadamwhite self-assigned this Oct 11, 2016

rachelbaker suggested changes Oct 11, 2016

View reviewed changes

kadamwhite removed the Needs Refresh label Oct 11, 2016

kadamwhite mentioned this pull request Oct 11, 2016

Exclude unsupported params in all controllers (extends #2770) #2804

Merged

8 tasks

rachelbaker approved these changes Oct 14, 2016

View reviewed changes

rachelbaker merged commit a7a9c39 into develop Oct 14, 2016

joehoyle added a commit that referenced this pull request Oct 14, 2016

Merge pull request #2804 from WP-API/exclude-unsupported-params-more-…

3d08e90

…broadly Exclude unsupported params in all controllers (extends #2770)

kadamwhite mentioned this pull request Oct 15, 2016

Exclude unsupported params more broadly (was #2804) #2839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude unsupported query parameters in posts controller#2770

Exclude unsupported query parameters in posts controller#2770
rachelbaker merged 4 commits intodevelopfrom
exclude-unsupported-params

rmccue commented Oct 6, 2016

Uh oh!

danielbachhuber Oct 6, 2016

Uh oh!

rachelbaker Oct 8, 2016

Uh oh!

kadamwhite Oct 7, 2016 •

edited

Loading

Uh oh!

rachelbaker Oct 7, 2016

Uh oh!

rachelbaker commented Oct 8, 2016

Uh oh!

rachelbaker Oct 8, 2016 •

edited

Loading

Uh oh!

rachelbaker commented Oct 11, 2016

Uh oh!

kadamwhite commented Oct 11, 2016

Uh oh!

rachelbaker left a comment

Uh oh!

kadamwhite commented Oct 11, 2016

Uh oh!

kadamwhite commented Oct 11, 2016

Uh oh!

rachelbaker commented Oct 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rmccue commented Oct 6, 2016

Uh oh!

danielbachhuber Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

rachelbaker Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

kadamwhite Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rachelbaker Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

rachelbaker commented Oct 8, 2016

Uh oh!

rachelbaker Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rachelbaker commented Oct 11, 2016

Uh oh!

kadamwhite commented Oct 11, 2016

Uh oh!

rachelbaker left a comment

Choose a reason for hiding this comment

Uh oh!

kadamwhite commented Oct 11, 2016

Uh oh!

kadamwhite commented Oct 11, 2016

Uh oh!

rachelbaker commented Oct 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kadamwhite Oct 7, 2016 •

edited

Loading

rachelbaker Oct 8, 2016 •

edited

Loading