Skip to content
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

[10.x] allow override of the Builder paginate() total #46336

Merged
merged 2 commits into from
Mar 6, 2023

Conversation

browner12
Copy link
Contributor

This allows the user to declare the total number of results a query returns. If the user provides this argument, the paginate() method will skip running its query to determine the total row count. to be clear, this does not change the number of actual results available, just how many the paginator thinks exist.

Why would we want to set this value when we can use a query to determine it?

Performance.

select count(*) as aggregate from `table_name` where `table_name`.`deleted_at` is null

can be terribly slow on large tables, depending on your database and table engine.

For example, on MySQL with an InnoDB engine, a table with 500,000 rows can take 300ms. Eliminating this query can save a considerable chunk on the entire request time.

You might ask, "why not use the simple paginator or cursor paginator to achieve this?". That would eliminate the extra query, but the downside is you lose the ability to quickly navigate directly to pages.

Users are free to calculate or choose this number any way they like. You could assume your users will never care about more than 100k records back from the most current, and use that if you like. Surprisingly, assuming your have an auto-incremented id field and rows are either never deleted or soft-deleted, you can run

select * from `table_name` order by `id` desc limit 1

to retrieve the last ID, and this query is significantly faster.

browner12 and others added 2 commits March 3, 2023 00:05
this allows the user to set the total number of results a query returns.  If the user provides this argument, the `paginate()` method will skip running its query to determine the total row count.
@taylorotwell taylorotwell merged commit df0135b into laravel:10.x Mar 6, 2023
@browner12 browner12 deleted the AB-paginator-total-override branch March 6, 2023 19:39
@decadence
Copy link
Contributor

So if I have 300k records but set Paginator to total 100k it will show list of pages for 100k but actually will load other 200k if I manually change ?page parameter to bigger page number?

@faytekin
Copy link

faytekin commented Mar 9, 2023

It seems like this is a breaking change because the newly added $total property is causing an error as many packages override the paginate method. Therefore, it can be considered as a breaking change. @taylorotwell

@driesvints
Copy link
Member

Sending in a PR to revert this.

@browner12 we can't change method signatures on patch releases as those will result in breaking changes for people overwriting these (see above).

taylorotwell pushed a commit that referenced this pull request Mar 9, 2023
@browner12
Copy link
Contributor Author

browner12 commented Mar 9, 2023

@driesvints Can you help me understand, I'm a little confused. If the packages are overriding the paginate() method, how would changing our signature affect them? Wouldn't their new definitions take effect, basically ignoring the Laravel method?

And then second question, if this truly does have to be reverted, can it go to a minor, or does it need to go to a major?

@dennisprudlo
Copy link
Contributor

@browner12 In PHP you can't overload methods, meaning to have the same method name but different parameters. It's only possible to override the implementation in a subclass.

When there's a breaking change it has to target the next major version. Otherwise apps with custom paginate-methods would break if they update to the next minor version with this change

@driesvints
Copy link
Member

@browner12 see the above. We reverted this on 10.x

@driesvints
Copy link
Member

@browner12 browner12 restored the AB-paginator-total-override branch March 9, 2023 16:33
@browner12
Copy link
Contributor Author

Thanks for the explanation guys! Completely spaced on PHP inheritance rules.

Resubmitting to master.

@X-Coder264
Copy link
Contributor

@browner12 You can use func_num_args and func_get_args to get around the breaking change. That way there's no need for this to wait until Laravel 11. Symfony does the same thing when they add a new parameter to a method signature so that they can make the change on a minor version and make the new parameter required/an actual part of the method signature on the next major version.

One example on Symfony 5 -> https://github.com/symfony/symfony/blob/v5.4.21/src/Symfony/Component/DependencyInjection/Alias.php#L88-L108

which then became this on Symfony 6 https://github.com/symfony/symfony/blob/v6.0.0/src/Symfony/Component/DependencyInjection/Alias.php#L70

@browner12
Copy link
Contributor Author

@driesvints, are you guys open to @X-Coder264 's suggestion? I've already got the PR open to 11.x, but happy to do this as well.

@browner12
Copy link
Contributor Author

Was pretty easy, went ahead and just made it on the chance the core team is okay with this strategy.

@browner12 browner12 deleted the AB-paginator-total-override branch March 26, 2023 00:16
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.

7 participants