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

per_page default should be the same as user set option in admin #2450

Closed
andrejcremoznik opened this issue Apr 18, 2016 · 17 comments
Closed

Comments

@andrejcremoznik
Copy link

per_page is currently set to 10.

This needlessly complicates things when you want to fetch a paged archive. Instead of simply sending the page you want, you need to either supply { per_page: <?= get_option('posts_per_page') ?>, page: N } or fiddle with the after parameter.

I think per_page should be the same as get_option('posts_per_page') and not hardcoded to 10.

@whyisjake
Copy link

Isn't this easily filterable?

@BE-Webdesign
Copy link
Member

BE-Webdesign commented Apr 21, 2016

Personally, I would say the API is fine the way it is regarding this topic. @andrejcremoznik you can use the rest_{$this->post_type}_query filter so that every request will have the proper posts_per_page.

I do understand that it would be unexpected and is confusing to have the pagination not in sync with your install by default, when using the API. Going to leave this open for discussion until there is a clear consensus. Most likely I imagine the API will remain as is on this issue.

@andrejcremoznik
Copy link
Author

I'm not saying this is an issue, I'm trying to argue that following a global user setting should be preferable.

To add some more context, this topic is irrelevant to off-site apps that are interfacing with some WP API somewhere on the web. It's only relevant when a template uses the WP API to e.g. replace pagination with lazy loading. Right now I have to create a global JS variable with the value of posts per page setting and inline it in template so that I have the setting's value available in my JS app.

@BE-Webdesign what is rest_{$this->post_type}_query?

@websupporter
Copy link
Member

websupporter commented Apr 21, 2016

Just a sidenote: Have a look into wp_localize_script() for your global variable.

@andrejcremoznik
Copy link
Author

andrejcremoznik commented Apr 21, 2016

@websupporter no, I'm using wp_add_inline_script.

@websupporter
Copy link
Member

@andrejcremoznik a right, this exists too :D

@BE-Webdesign
Copy link
Member

@BE-Webdesign what is rest_{$this->post_type}_query?

This is a filter that you can hook into so that if you want to respect get_option( 'posts_per_page' ) on every request, you can. By using the filter you wouldn't have to worry about localizing a variable into your JS.

@andrejcremoznik
Copy link
Author

Is there a reference of all the filters available or do I have to read the source?

Also, thanks.

@websupporter
Copy link
Member

websupporter commented Apr 21, 2016

        /**
         * Filter the query arguments for a request.
         *
         * Enables adding extra arguments or setting defaults for a post
         * collection request.
         *
         * @see https://developer.wordpress.org/reference/classes/wp_user_query/
         *
         * @param array           $args    Key value array of query var to query value.
         * @param WP_REST_Request $request The request used.
         */
        $args = apply_filters( "rest_{$this->post_type}_query", $args, $request );

_
lib/endpoints/class_wp_rest_posts_controller.php_

@BE-Webdesign
Copy link
Member

Is there a reference of all the filters available or do I have to read the source?

Not yet, there should be soon I have one in a PR for the docs site, but I am not sure if people want to use it.

@websupporter
Copy link
Member

websupporter commented Apr 21, 2016

@BE-Webdesign
I am thinking about "best practice". Is it ill advised to do something like this:

<?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 quite sure about the usage of the global $_REQUEST, but it would be good citizenship, since wp/v2 is a commonly shared resource.

Some context
My first thought was parameter suceeds filter. So, if you really want to make sure you get a certain amount of posts, you should use per_page. But its the other way around. So I start to become not the biggest fan of this filter, since instead of specifing the request by parameter and being sure I get for example 10 posts because I said so in my params, some goddamm plugin tells via this filter nope, you get one and not more. So, if people want to use this filter, I think at least they should try to be good citizens to parameter-users :)

The filter-users (imagine a theme and a plugin want to use a specific amount of posts for theire specific requests and do not want to do it via parameter) would still battle each other, but please leave the parameter-users out of this :D

The API is also for applications outside of the current WP system. There are applications, who do not have access to these filters and they want to rely on some things like "I did send this parameter".

@BE-Webdesign
Copy link
Member

I definitely agree with what you are saying, but if you were in a controlled environment you could use the filter, or you might as well not have the filter at all because it will do it for any params. Open up an issue that is separate from this if you believe the filter would lead to adoption of bad practices so there can be discussion about it.

@websupporter
Copy link
Member

Yes, if you are "This is my website, I do what I want" I really have no problem. But If you are like "I write the coolest plugin, put it on wordpress.org and get 100.000 downloads", this is my concern. I will open another issue for this. Thanks :)

@BE-Webdesign
Copy link
Member

Yeah, I definitely think it is worth discussing because it could lead to strange things.

@BE-Webdesign
Copy link
Member

Apologies to @andrejcremoznik for polluting your ticket.

@andrejcremoznik
Copy link
Author

No worries it's an interesting discussion. I agree that this rest_post_query filter shouldn't override explicit values sent in $_REQUEST, and I also think it shouldn't be left up to the developer to check $_REQUEST before changing values with the filter.

Back on topic; I still think the default shouldn't be 10 but the user set option. External devs using the API will know how to get the right results for their app and theme/plugin devs will save some time and won't break the API by using the filter "incorrectly" and overriding $_REQUEST data.

@danielbachhuber
Copy link
Member

Back on topic; I still think the default shouldn't be 10 but the user set option.

We came to the decision a while back that the posts_per_page option shouldn't affect the number of items present in the REST response because it's a presentation-specific option. For instance, the posts_per_page option doesn't affect the number of items in a RSS feed.

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

5 participants