[5.5] Prevent negative offsets when paginating collection

[tautvydasr]

Recently I faced a problem with collection pagination because it calculates negative offset and returns entries from the end of collection items.

For example:

$c = new Collection(['one', 'two', 'three', 'four']);
$c->forPage(0, 2)->all() // returns ['three', 'four']

I believe it would be nice if offset could not be negative and only positive numbers could be accepted as page.

[GrahamCampbell]

Instead of wriapping, perhapse we should throw an invalid argument exception?

[vlakoff]

Agree with Graham. Silently "fixing" the argument doesn't sound right.

[vlakoff]

Similar issue in Query Builder's forPage(), it uses offset() which also has a "0 capping", see #3017.

[vlakoff]

@tautvydasr Out of curiosity, could you quickly explain your use case, where the application calculates negative offsets (and capping to 0 is an acceptable solution)?

[tautvydasr]

@vlakoff I detected the problem when page number comes from request and if there is no page in request query it becomes 0 and offset becomes -10 (because for example we are showing 10 entries per page)

$page = (int)$request->get('page'); // here page parameter becomes 0
$entries = $collection->forPage($page, 10); // here we pass 0 as page and offset becomes -10 and we get last 10 entries from collection

[devcircus]

You shouldn't be using a cast non-existent value as a valid parameter. Either get the page number from the request or default to 1. Now that we've hacked a solution into the core, I guess it doesn't matter, but you shouldn't be trusting that value.

[vlakoff]

That's what I was fearing. It should be your application's responsibility to filter the user input:

// specify default value for clarity,
// even if it would work without it (thanks to the sanitization below)
$page = (int) $request->input('page', '1');

// if value is invalid (out of range), fall back to default value
if ($page < 1) {
    $page = 1;
}

$entries = $collection->forPage($page, 10);

@devcircus, no release has been tagged since, so it's still time to revert this, if desired so. I still think it should be reverted, and an exception thrown instead, but that's only my opinion.

edit: The change has been released, with laravel 5.5.15. I guess we'll have to live with it.