October: Bug with changed query builder paginate signature

[Malezha]

• OctoberCMS Build: 461
• PHP Version: 7.3.13
• Database Engine: mysql

Description:

When used Laravel pagination signature for parameters current page will be reseted.

Steps To Reproduce:

$model = new class () extends \October\Rain\Database\Model {
    protected $table = 'some_table';
};

$perPage = 10;
$currentPage = 2;
$result = $model->query()->paginate($perPage, ['*'], 'page', $currentPage);

dd($result->currentPage(), $currentPage);

[bennothommo]

@Malezha Unless I'm mistaken, $model->query() in the example above should return an instance of Illuminate\Database\Eloquent\Builder, which uses the same signature as above.

Would you be able to double-check if this is the case?

[Malezha]

@bennothommo nope, it's returned October\Rain\Database\Builder
You can reproduce it:

$model = new class () extends \October\Rain\Database\Model {
    protected $table = 'some_table';
};
get_class($model->query()); // "October\Rain\Database\Builder"

[Malezha]

OctoberCMS version was updated to 462.

[bennothommo]

@Malezha You should be able to statically call paginate on the model which will use the same signature as Laravel.

Example:

use Backend\Models\User;

$rows = User::paginate($perPage, ['*'], 'page', $currentPage);

[Malezha]

@bennothommo are you kidding me? Any query builder method returned October builder.

use Backend\Models\User;
get_class(User::where('email', '[email protected]')); // October\Rain\Database\Builder
get_class(User::orderByDesc('created_at')); // October\Rain\Database\Builder

If October\Rain\Database\Builder inherit Illuminate\Database\Eloquent\Builder so method paginate must work exactly same.
@daftspunk when you decide to simplify api no need violation LSP.

[bennothommo]

@Malezha I apologise - you are right. However, it looks like it was an intentional (and justified) change, and given the docs already specify that the second argument in the method is for defining the current page, it's likely not going to change any time soon.

[LukeTowers]

Hmm, this seems like a problem to me.

Our signature: $perPage = 15, $currentPage = null, $columns = ['*'], $pageName = 'page')
https://github.com/octobercms/library/blob/dfd85ebb48922c2abe49b5834c30d9d3e6ef15b5/src/Database/Builder.php#L115

Laravel's signature: $perPage = null, $columns = ['*'], $pageName = 'page', $page = null)
https://github.com/laravel/framework/blob/6.x/src/Illuminate/Database/Eloquent/Builder.php#L698

Note that there is already an initial attempt at supporting Laravel's signature here: https://github.com/octobercms/library/blob/dfd85ebb48922c2abe49b5834c30d9d3e6ef15b5/src/Database/Builder.php#L117-L120

        if (is_array($currentPage)) {
            $columns = $currentPage;
            $currentPage = null;
        }

so I think the appropriate thing to do here would be to support both fully.

Thoughts @bennothommo @daftspunk @Malezha?

[daftspunk]

@daftspunk when you decide to simplify api no need violation LSP.

@Malezha, to give some history on this, this is an artefact from a time when Laravel only supported passing the page parameter via $_POST['page'], you couldn't even customise the variable name. It was unacceptable so we added it ourselves. Eventually, they caught up to our thinking and added it themselves.

[Malezha]

@daftspunk thank you and I apologize for rudeness in statements.

[mjauvin]

Isn't that a breaking change?

[LukeTowers]

@mjauvin no, the commit adds support for both October's signature & Laravel's signature. There is a breaking change to the simplePaginate() method, but given that the current signature didn't even line up with our own signature present in paginate() let alone Laravel's and that it's a pretty obscure and rarely used aspect of the API we're thinking it's going to be fine. It will be mentioned as a change to the API in the changelog however.