WIP QueryBuilder pagination

[joelambert]

This adds a paginate() method to the QueryBuilder that returns an instance of Timber's PostQuery instead of a Collection.

This needs to be tested in a WordPress install.

Possible enhancements could be an abstraction of the PostQuery class that gives access to the Pagination object as well as a Collection of posts rather than an Array.

[coveralls]

Coverage decreased (-0.7%) to 88.747% when pulling 7c78e56 on querybuilder-pagination into 5b71360 on master.

[coveralls]

Coverage decreased (-0.7%) to 88.747% when pulling 7c78e56 on querybuilder-pagination into 5b71360 on master.

[coveralls]

Coverage decreased (-0.7%) to 88.747% when pulling 7c78e56 on querybuilder-pagination into 5b71360 on master.

[coveralls]

Coverage decreased (-0.7%) to 88.747% when pulling 7c78e56 on querybuilder-pagination into 5b71360 on master.

[coveralls]

Coverage decreased (-0.7%) to 88.747% when pulling 7c78e56 on querybuilder-pagination into 5b71360 on master.

[coveralls]

Coverage decreased (-0.7%) to 88.747% when pulling 7c78e56 on querybuilder-pagination into 5b71360 on master.

[coveralls]

Coverage decreased (-0.7%) to 88.747% when pulling 7c78e56 on querybuilder-pagination into 5b71360 on master.

[idflood]

I was missing this feature and stumbled upon this merge request. I quickly checked how it was implemented by curiosity and I might have found an issue here : )

Since the $page attribute has a default value in the function params it will always be set. This means that if the user doesn't pass the $page attribute this will always reset the $paged value to 1.

One solution is to set the $page = NULL in the params and check if $page !== NULL here instead of the isset. I still didn't test it in a real wordpress setup so I might be wrong.

[joelambert]

Thanks for the feedback, I guess the if is redundant here. I can't see that it would cause any issue but you're right that it will always be set.

I think it's important to provide a default as I can't think of a scenario where when providing no $page param you wouldn't want the first page back.

[nealoke]

Lumberjack is awesome but really needs this functionality. @joelambert is there a way we can help to get this merged? Pagination is quite helpful 😅.

[martinpl]

IMO paginate() should only set paged parameter and if empty set it from global $paged.
So there would be needed two additional methods getTotalCount() for return found_posts and
run() for running query. Advantage is that we still return collection and don't duplicate limit functionality and get functionality.

Code example:

public function paginate($page = null)
{
    if ($page) {
        $this->params['paged'] = $page;	
    } else {
        global $paged;
        $this->params['paged'] = $paged;
    }

    return $this;
}

public function getTotalCount() : int
{
    if (!$this->query) {
        $this->run();
    }

    return $this->query->found_posts;
}

public function run()
{
    $this->query = new PostQuery($this->getParameters(), $this->postClass);

    return $this;
}

public function get() : Collection
{
    if (!$this->query) {
        $this->run();
    }

    return collect($this->query);
}

[joelambert]

@martinpl I'm not too keen on adding more internal state to the query builder. At the moment get() isn't a chainable method call and triggers the query, the PR as proposed makes paginate() behave the same. By adding internal state we get into temporal coupling issues and would need to work out when to refresh the internal state (e.g. subsequent calls to query builder methods that invalidate the query).

@adamtomat do you have any thoughts on this? To my mind, this PR is pretty close, the only question is whether we should write an abstraction around the PostQuery object that returns a collection? Maybe this is something we can add in a future release though?