saloonphp / saloon

🤠 Build beautiful API integrations and SDKs with Saloon
https://docs.saloon.dev
MIT License
1.99k stars 99 forks source link

Pagination: `currentPage` doesn't align with `page` #432

Open jorqensen opened 1 month ago

jorqensen commented 1 month ago

I stumbled across something rather annoying with the pagination plugin, perhaps I am misunderstanding something. Clarification would be appreciated.

I'm working with an API that uses limit and page for pagination, so I've set my pagination up as such:

protected function applyPagination(Request $request): Request
{
    $request->query()->add('page', $this->currentPage);

    if (isset($this->perPageLimit)) {
        $request->query()->add('limit', $this->perPageLimit);
    }

    return $request;
}

However, this means that my pagination will start at page 0 as currentPage is set to 0. Applying ->setStartPage() on the paginator doesn't do anything, neither does setting the currentPage property on my pagination class to 1.

Moving to the deprecated page property will make everything work as expected:

$request->query()->add('page', $this->page);

What's the idea for currentPage not starting at 1? Or how do you actually properly set the starting page to 1, without making use of the deprecated/internal property?

patrickcarlohickman commented 1 month ago

@jorqensen ,

The currentPage variable is used to track which page the paginator is currently on. When you start and haven't made any requests, the current page is 0 (or, startPage - 1, if you've set the start page). Once you request page 1, then the currentPage will advance to 1.

The applyPagination() method is called before requests to update the request to get the next page. Since you're trying to get the next page, and not the current page, you want to update the request with $this->currentPage + 1.

If you look at the applyPagination() method on the parent PagedPaginator class, you'll see it still uses the deprecated page property (which is equal to $this->currentPage + 1).

Since this property is deprecated, I tend to use $this->currentPage + 1 in my own custom paginators. Also, the $this->currentPage + 1 logic is used multiple times inside the parent Paginator class.

Sammyjo20 commented 1 month ago

Hey @jorqensen

As Patrick kindly mentioned, the page property does start at 1, but that meant that by the time it ran applyPagination, that value was actually 2, so I deprecated it and introduced currentPage which is the "correct" value. Patrick's suggestion above should be suitable to get pagination working.