wintercms
Run a more robust count query that supports "group by"
@lex0r thanks for this PR.
Can you give a bit more details on exactly what those methods do, I am not familiar with them.
@mjauvin both the original count() method and getCountForPagination() are available in Illuminate\Database\Query\Builder, but the main difference is that count() is much simpler and I suppose was never meant to be used for pagination, while the second one has support for more complex queries that may have grouping.
@lex0r what about toBase()?
Also, unit test for this would be very appreciated.
@lex0r what about
toBase()?
toBase() will retrieve the data from the database but it will not prepare the associated Eloquent models.
If you are not really going to use features like Mutators, Relationships of Eloquent and loading a large amount of data, then the toBase function can be a game changer in performances.
I'm worried that the code here will have a hard time because of this:
https://github.com/wintercms/winter/blob/054fa0552f1362decbff67abd2d462384700622e/modules/backend/widgets/Lists.php#L738
@lex0r what about
toBase()?
toBase()will retrieve the data from the database but it will not prepare the associated Eloquent models. If you are not really going to use features like Mutators, Relationships of Eloquent and loading a large amount of data, then the toBase function can be a game changer in performances.
The good news is that we only need to run a count query, so not interested in any actual record data.
I'm worried that the code here will have a hard time because of this:
https://github.com/wintercms/winter/blob/054fa0552f1362decbff67abd2d462384700622e/modules/backend/widgets/Lists.php#L738
@mjauvin not sure I'm following your concern. The code I suggested is absolutely pagination friendly, it's even in the name of the method. Surely it will work whatever pagination method is used?
@lex0r what about
toBase()?Also, unit test for this would be very appreciated.
I would expect a unit test that checks for what records are returned based on the requested page to already exist. If it doesn't exist then it's probably not very important, but even if it is, I can't promise I will have time to look into creating a test for pagination unless there's some ground work that has been done and it won't take long to implement :)
@lex0r if you aren't able to create a unit test case for this can you create a PR in the test plugin that replicates the original issue and provides instructions for testing this fix?
