Pagerfanta::setCurrentPage() must be called AFTER Pagerfanta::setMaxPerPage()

[weaverryan]

Hi!

I'm been testing out the library & bundle to include in a Symfonycasts screencast - it's very nice!

I ran into one minor issue:

// works correctly - yay!
$pagerfanta = new Pagerfanta(new QueryAdapter($queryBuilder));
$pagerfanta->setMaxPerPage(5);
$pagerfanta->setCurrentPage($request->query->get('page', 1));

// does not work - boo :(
$pagerfanta = new Pagerfanta(new QueryAdapter($queryBuilder));
$pagerfanta->setCurrentPage($request->query->get('page', 1));
$pagerfanta->setMaxPerPage(5);

When I say "does not work" - here is specifically what happens. Suppose I have 20 "items". This would be 2 pages using the default 10 max per page. With the 2nd code, it correctly renders 5 items on each page. And when rendering the pagination links, it correctly renders 4 pages. However, when you click to page 3, you get an error:

I believe that Pagerfanta::resetForCurrentPageChange() also needs to have $this->nbResults = null;.

Thanks!

[mbabker]

Makes sense, care to do a PR on the 2.x branch?

[mbabker]

Ya know, now that I'm looking at this more, I don't know if there's a way that the second snippet can work without disabling the out-of-range validation, and that the private resetForMaxPerPageChange() method shouldn't be resetting $this->nbResults.

Easy one first, Pagerfanta::getNbResults() should return a consistent number regardless of pages (in database terms, it's basically the result of a COUNT(*) FROM foo query minus the limit and offset portions of the query), so changing the maximum number of items per page shouldn't affect that result and resetting that property just opens the door to needless duplicate queries if someone is changing the maximum multiple times on one Pagerfanta instance for some silly reason.

Now the hard one. Pagerfanta::setCurrentPage() by default goes through validation to make sure that the page you give isn't outside the range of pages available, and that's based on the current max items per page (default 10) and total number of items (Pagerfanta::getNbResults()). So if you're setting page 3 on a 20-item list with 5 items per page, then yes, it would error out if the max isn't set first. You can bypass this by calling Pagerfanta::setAllowOutOfRangePages(true) (which I don't have documented for some strange reason), but that will also have side effects later on as you'll get an empty list when calling Pagerfanta::getCurrentPageResults().

There might be ways to improve the DX around this (make a named constructor that takes the adapter and desired current and max pages and have it handle calling the setters in the right order), but I don't think there's a good code fix without either removing validation or changing the point the validation takes effect (instead of validating when it's set, validate it when Pagerfanta::getCurrentPageResults() is called).

[flohw]

Hi @mbabker,

We just ran into this issue too and fixed by calling setMaxPerPage before setCurrentPage too.

I agree with the fact that nbResults should never be reinitialized.

Having the constructor (or named one) with $adapter, $currentPage, $maxPerPage looks good to me (and make setMaxPerPage private). Please note that, as the library is built, changing the constructor looks like a better/more logical choice to me.

Another alternative I have in mind would be to revalidate currentPage in setMaxPerPage. (a call to filterOutOfRangeCurrentPage in setMaxPerPage should be enough?)

I can make a PR for the second option if you want. The first one requires more changes, deprecations and maybe discussions?

[mbabker]

I can make a PR for the second option if you want.

Feel free to do so.

[mbabker]

309aee6267efecff80943dd3892f06703e07d6c0 adds a static constructor and tries to expand on why the order of operations has to be the way it is.

[flohw]

Hi @mbabker

Sorry for the late answer but holidays have a higher priority for me :-)

I think your commit solves this issue properly so we can close it.

Thanks for this nice addition.