Skip to content

[11.x] use value helper for $perPage as used for $total #54650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 17, 2025
Merged

[11.x] use value helper for $perPage as used for $total #54650

merged 1 commit into from
Feb 17, 2025

Conversation

rodrigopedra
Copy link
Contributor

When creating a Paginator from both Illuminate\Database\Query\Builder and Illuminate\Database\Eloquent\Builder, the $total property uses the value() helper, as it can be a \Closure.

But the $perPage property replicates the same logic as the value() helper, as below:

function value($value, ...$args)
{
return $value instanceof Closure ? $value(...$args) : $value;
}

This PR:

  • Changes the $perPage calculation to use the value() helper

Note:

On Illuminate\Database\Eloquent\Builder, the previous code used the ternary shorthand operator (?:) instead of the null coalescing operator (??) to defer checking the model for the $perPage value, in case it wasn't defined.

It would allow a call like this:

Model::paginate(0);

To make the Model@getPerPage() method to be called, which would technically be a breaking change.

I opted to use the null coalescing operator (??) as $total uses it, and as, IMO, it is very unlikely someone to be calling Model::paginate(...) with a zero $perPage value.

But I can revert to the ternary shorthand operator (?:) if we want to avoid any breaking change.

@taylorotwell taylorotwell merged commit 2155a63 into laravel:11.x Feb 17, 2025
46 checks passed
crynobone added a commit that referenced this pull request Feb 18, 2025
taylorotwell pushed a commit that referenced this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants