BabDev / Pagerfanta

Pagination library for PHP applications with support for several data providers
Other
381 stars 170 forks source link

Is there any way to set the max per page to "all items"? #20

Closed acelaya closed 3 years ago

acelaya commented 3 years ago

Hey!

I'm considering migrating to this package, as it's mostly a direct replacemente from the package I'm currently using, but this one seems a bit more maintained.

The only thing I'm not being able to do with pagerfanta (or I haven't found the way so far) is allowing to "remove" the limit. Let me elaborate.

The package I'm currently using allows you to set -1 as the max items per page. When you do this, it internally fetches the number of results from the adapter, and when calling the equivalent to getSlice, you get that value as $length, so it's basically to get all results.

I don't see a similar way to do this with pagerfanta, as Pagerfanta::setMaxPerPage seems to not allow providing anything but possitive numbers.

I agree this -1 approach is a bit hacky. Maybe supporting a nullable length would be another option, letting adapter implementations to decide how to act, but having support for this would be useful.

Any thoughts?

Of course, if this is not supported but you are open to have it, I'm willing to provide a PR.

acelaya commented 3 years ago

FYI. As a workaround, using this in place of the Pagerfanta object seems to be doing the trick and all my tests are passing again:

class Paginator extends Pagerfanta
{
    private bool $allResults = false;

    /**
     * @param int $maxPerPage
     */
    public function setMaxPerPage($maxPerPage): self
    {
        $this->allResults = $maxPerPage < 1;

        if (! $this->allResults) {
            parent::setMaxPerPage($maxPerPage);
        }

        return $this;
    }

    public function getMaxPerPage(): int
    {
        if (! $this->allResults) {
            return parent::getMaxPerPage();
        }

        $numberOfResults = parent::getNbResults();
        return $numberOfResults === null || $numberOfResults < 1 ? 1 : $numberOfResults;
    }
}
mbabker commented 3 years ago

There isn’t anything explicitly in the API to do it, and the one time I’ve needed it we just pushed a large number into setMaxPerPage() to pull it off.

Off hand I don’t have a “good” API design for an explicit “set the Pagerfanta class to retrieve all results as a single page” type of call, because presumably that flag would also need to be passed through to the adapter as well so it could make decisions or optimizations based on that. And in all honesty, I can’t say I’ve done a benchmark to determine if non-limited queries versus a query with something like “LIMIT 10000” is worth that effort.

acelaya commented 3 years ago

Actually, I think that's not a bad idea at all, and in most of the cases, if you are already running a query which will return a high number of results, having used a LIMIT with a big number, will never really be the bottleneck.

I think it's a very reasonable approach, very explicit and way less hacky than the original proposal.

Thanks for your feedback 🙂