[12.x] feat: Add $total param to the getPerPage() in Model

[ah-rahimi]

No description provided.

[GrahamCampbell]

What does this fix?

[ah-rahimi]

I want to specify inside the BaseModel.php that if per_page was defined in the Request, that per_page will be returned, otherwise it will return the $total value.

It would be much better if the paginate() function could be customized to first check if per_page was not present return get(), but I haven't found a way to do that.

[macropay-solutions]

@ah-rahimi have a look here how you can pass the limit(perPage) to the paginator.
https://github.com/macropay-solutions/laravel-crud-wizard-free/blob/050b4a86e0699d43ae0b0d8ef9f5b16524482e82/src/Http/Controllers/ResourceControllerTrait.php#L431

By the way this should work in the same way for simplePaginate, Paginate and cursor paginate.

If you will return all table you will have memory issues but, anyway you can achieve that without changes in the core of laravel so, why did you open this PR?

[ah-rahimi]

You are right, @macropay-solutions.
But I want this to happen at the same time as paginate.
Not to do any extra work in the controller.
Because in the future, when the number of controllers increases, it will be difficult to manage them.

[macropay-solutions]

In theory, this can be useful for populating drop down in filters with all values. But if you code it, it will for sure lead to out of memory issues so, you would need to put a max limit anyway to avoid that.

We use limit=1000&simplePaginate=1 for this to avoid counting because we can use has_more_values from response to see if there are more results.

[rodrigopedra]

I want to specify [...] that if per_page was defined in the Request, that per_page will be returned, otherwise it will return the $total value.

$records = User::query()->paginate(fn ($total) => request('per_page', $total));

You can define a static method in your BaseModel, and use that instead of copying the closure on every usage:

// on a helpers file

class BaseModel
{
    public static function perPage($total) 
    {
        return request('per_page', $total);
    }
}

// on a controller

$records = User::query()->paginate(BaseModel::perPage(...));

Note that BaseModel::perPage(...) creates a closure from the static method. Reference: https://www.php.net/manual/en/functions.first_class_callable_syntax.php

That also allows you to call the inner model's getPerPage(), if needed.

class BaseModel extends Model
{
    public static function perPage($total)
    {
        if (! request()->has('per_page')) {
            return $total;
        }
        
        return static::query()->newModelInstance()->getPerPage();
    }
}

---

It would be much better if the paginate() function could be customized to first check if per_page was not present return get(), but I haven't found a way to do that.

You can use the when() method for that:

$records = User::query()->when(
    request('per_page'),

    // request has a per_page value
    fn ($builder, $perPage) => $builder->paginate($perPage),

    // request does not have a per_page value
    fn ($builder) => $builder->get(),
);

Or to reuse the logic across controllers, you can create an invokable class, and use the newly introduced query builder's pipe() method:

<?php

namespace App;

use Illuminate\Database\Eloquent\Builder;

class CustomPaginator
{
    public function __construct(
        private readonly ?int $perPage,
    ) {}

    public function __invoke(Builder $builder)
    {
        if (is_int($this->perPage) && $this->perPage > 0) {
            return $builder->paginate($this->perPage);
        }

        return $builder->get();
    }
}

Then in your controllers:

$records = User::query()->pipe(new CustomPaginator(request('per_page')));

---

Lastly, if you still think this PR is needed (I like it, as it makes the $this->model->getPerPage() call symmetric to the value($perPage, $total) call), you should send it to the master branch.

Adding a parameter to a public method is a breaking change.

Any class overriding that method with a custom logic -- which is very likely to happen as that method is meant to be overridden -- will break due to the added parameter on the base class.

[macropay-solutions]

@ah-rahimi If you want to avoid the memory issue when returning all table, you could try to stream the json to FE by using the lazy chunk if you eager load relations or the cursor chunk when no eager loading is involved

Update

Also you can have a look at streamJson doc

Update
Use lazyByIdDesc instead of lazy to avoid offset time issue.

[taylorotwell]

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!