"Shifted" $this->page between isLastPage() and applyPagination() in pagination

[boryn]

I'm not sure here, maybe it's on purpose or maybe I cannot debug it properly.

I connect with an API which returns 'total_pages', so the most natural way was to implement this in the isLastPage() method:

return $this->page === (int)($response->json('total_pages'));

Unfortunately, it seems it does not reach the final page. I made tests with simple result set, I have 3 items, I get 1 item per page so in total I have 3 pages. So I should iterate three times, but unfortunately I get two results.

When I put debugging messages in both methods like this:
echo ("page in isLastPage: {$this->page}\n");
echo ("page in applyPagination: {$this->page}\n");

I get:

• page in applyPagination: 1
• now happens the request call with &per_page=1&page=1
• page in isLastPage: 2

So you can see that page number in isLastPage() already jumped to 2. So with second iteration it will have the page number of 3 which equals to 'total_pages' and we never reach the last third iteration.

[Sammyjo20]

Hey @boryn thank you for bringing this to my attention. I also have noticed this when I was last testing and playing around with the paginator in a project of mine. I'll have another look in a fresh Laravel application this week.

If it turns out that the page variable is shifted by the iterator at the wrong time, then I'll probably introduce a new property like "realPage" or maybe a method like $this->page()? I'm not sure yet.

[boryn]

Yes, please check it, as IMHO there is something unpredictable right now. Variable name maybe could be $this->currentPage?

I must admit I miss another variable like $this->currentResults (or $this->currentResultsCount) to get the number of results just for this page. I came across a cursor pagination which even on the last page still returns next cursor and I need to check at isLastPage() if the result set contains 0 items. At this moment I do it this way:

return count($this->getPageItems($response, $this->getOriginalRequest())) === 0;

and this would be for sure more convenient:

return $this->currentResults === 0;

[Sammyjo20]

Hey @boryn I have created a PR first which fixes v1 of the pagination plugin (I'll then PR the fix for v2) to cover both Saloon versions but you were right, the property was wrong - thank you for pointing this out to me!

Would you mind reviewing this PR please: saloonphp/pagination-plugin#10

Thank you

[boryn]

I even was not aware there was the same problem with v1. Probably I was using a different logic then. Now I don't use it any more, so I cannot test it empirically, but the changes in the code look all right!

[Sammyjo20]

When I say v1, I mean Pagination v2 but for Saloon v2 + v3. Too many versions 🤣 Thank you for looking at it. I've just released an update for you!

[boryn]

So if it is for Pagination v2 and Saloon v3 as well, so I will give it some tests the following days.