Skip to content
This repository has been archived by the owner on Sep 24, 2018. It is now read-only.

Filter vs. Parameter & good citizenship #2460

Closed
websupporter opened this issue Apr 21, 2016 · 5 comments
Closed

Filter vs. Parameter & good citizenship #2460

websupporter opened this issue Apr 21, 2016 · 5 comments

Comments

@websupporter
Copy link
Member

tl;dr

  1. which filters should overrule parameter and 2. the documentation should encourage "best practices"/"good citizenship" when explaining those filters

Out of a small discussion, which has its origins here #2450 a thought popped up in my mind concerning the rest_{$this->post_type}_query-filter and powerful filters for the REST API in general. Usually, I love powerful filters and I love to play with pre_get_posts although you can (hence powerful) easily smash the whole site with an improper usage.

Some weeks ago I started to read more and more of the code of the REST API and started to wonder, why isnt here a filter, why not there... Stuck in my WordPress mind, I thought, "well, those filters will come, maybe they will add them in the end or something", but then I watched a talked, I think by Joe Hoyle, who explained why, for example, you can't simply remove title or content from the posts response. Javascript (as an example) would always need to ask if typeof( bla ) != 'undefined'... parsers would crash and I thought: "Yeah right, its an API, output is expected in a certain way".

And all this "title" instead of "post_title" to get a unified data outcome and not just a WP database dump.

Okay, back to topic. The REST API is in core, thousands of plugins will take advantage of it, some themes want to be super fancy, SAAS-service grow around. And wp/v2 will be a commonly shared resource between all those parties and they all expect a certain outcome. So the theme says, I look only pretty with two posts per page. I will use rest_{$this->post_type}_query and say $args['posts_per_page'] = 2, the plugins purpose is to query (for whatever reason) 10 posts. It will also use the same filter and both will fight over priority. This filter is placed in a way, if you have to make sure, you get only a certain amount of posts, you have to use this filter, since you can't be sure the parameter per_page won't be overwritten (which to me is a bit counterintuitive to an API: "I've said to the API give me 10 but it only gives me 2 and now I have to make 5 requests, can anyone explain me this?").

The description of this filter:
Enables adding extra arguments or setting defaults for a post collection request.
So, its certainly not intended by the filter to overwrite incoming parameters, but I am very sure, this will happen every now and then and is annoying, escpecially for parties, which do not have superior access to the API by filters.

Well, as I've mentioned, I am about to dive into the code, I do not know all the filters and especially not all those discussions around them. But in this case, I think, it might be worth a thought to place this filter in a way, parameters like per_page do not get overwritten.

More generally (and I bet there are also good reasons to keep the filter at its place) I was thinking about "best practices", "good citizenship" and stuff like this. If the API does have such strong filters, I think in the documentation there be codesamples and explanations, how to use them in a respectful way (you are using a commonly used resource here!). What I mean:

<?php
    add_filter( 'rest_post_query', 'slug_rest_post_query', 10, 2 );
    function slug_rest_post_query ( $args, $request ) {

        //Catch the $_REQUEST to check if the current
        //request maybe wants explicitly a certain no. of
        //posts. I do not mind, since I do not send 'per_page'
        //but others might
        if ( isset( $_REQUEST['per_page'] ) )
            return $args;

        $args['posts_per_page'] = get_option('posts_per_page');
        return $args;
    }
?>

I am not to sure about the usage of $_REQUEST and "best practice" :D, but what I like about it is if an request explicitly wants a certain amount of posts, this usage would not overrule the parameter.


While I was working on this thought I was wondering: It would be cool if I could seperate my requests from other requests in filters. Okay, now I just play a bit, but imagine every request could somehow be identified wp-json/wp/v2/posts/?client=my-plugin

<?php
    add_filter( 'rest_post_query', 'slug_rest_post_query', 10, 3 );
    function slug_rest_post_query ( $args, $request, $client ) {
    if ( 'my-plugin' != $client )
        return $args;

        $args['posts_per_page'] = get_option('posts_per_page');
        return $args;
    }
?>

Okay, anyways. Lets sum up:

  • WP-Filters have a general character, parameter are specific. Shouldn't parameter succeed?
  • WP-Filters who overrule parameter, some kind of "good citizenship" should be encouraged from the beginning by the documentation

I am curious to hear what others might think about this.

@BE-Webdesign
Copy link
Member

WP-Filters have a general character, parameter are specific. Shouldn't parameter succeed?

There should probably be some sort of warning or something attached to the documentation of the filter, as you suggested. There are many ways to use the filter that are legitimate and won't override the request arguments, so I think the filter would still be very useful to have.

I imagine a lot of filters in WordPress, if improperly used, would create really bad experiences for the ecosystem as a whole. Maybe include an @warning with a link to examples of how the filter could easily and unintentionally be used to break compatibility with other entities interacting with the API. Once a filter reference is complete probably have links to those with examples and add that to the docs for the filters.

If you want to do PRs that would add information to the docs for the rest_{resource}_query filters and work on examples for docs-v2, that would be great! There are four different filters of this kind, one for posts, users, comments, and terms. So do it for all four. Feel free to check out #1167 and audit some of the other filters as well.

@BE-Webdesign
Copy link
Member

On the idea of Filter vs. Param: That decision should be up to the dev and what they want to have happen. The filter, as is, is flexible and powerful, which it should be in my opinion. You would just need to be careful about your implementation and understand what you are doing exactly, so examples would probably go a long way to avoid potential bad code.

@websupporter
Copy link
Member Author

websupporter commented Apr 23, 2016

Yes, I've looked a bit through the code and basically, there is no sane way to have a filter and prioritize parameter, even if you wanted. And I think, there should be a filter, I just thought, maybe it could be moved a bit around, but basically no. So it is up to the devs :)

So, I will have a look into the docs repository. Cool, I didn't see this before.

so examples would probably go a long way to avoid potential bad code.

Not quite sure, if I do understand you correctly. Well, basically the usage will show traps to avoid and out of the usage something like "best practice" will grow. And I didn't intend to run for every potential bad code, this will be endless. But since I saw this problem (out of my mislead assumption GET-parameter would succeed), there can be done some work by me in the docs.

Thanks for your response.

@BE-Webdesign
Copy link
Member

BE-Webdesign commented Apr 23, 2016

Thanks for digging in. Right now it is probably more of a priority to hit the 2.0 milestone and we would love to have you help out with that.

As far as examples go, I think illustrating what could be a bad practice would be good. You wouldn't need to do an exhaustive list of bad code; just an example or two.

@BE-Webdesign
Copy link
Member

Going to close this out as it is more of a documentation issue. We need examples good and bad on docs-v2 eventually.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants