[5.1] Fix for the Pagination Query that selected all rows from a table and would fail on large datasets

[mxaddict]

Implemented my 4.2.x Fix for the pagination count query into 5.1.x

Fix for the Pagination Query that selected all rows from a table and would fail on large datasets

[GrahamCampbell]

Please follow PSR2.

[mxaddict]

Updated the commit to have the changes you requested 😄

[GrahamCampbell]

not string, array

[mxaddict]

Added the changes

[GrahamCampbell]

Ping @codelight89, @fcjailybo, @JoostK, @codelight89, and @jarektkaczyk for review. This function was last modified in #8310 which came out of #8113, #8112 and #7372 if we go all the way back. :)

[GrahamCampbell]

@mxaddict could you use a better commit message please. Don't mention versions, just describe what it does. We then need to wait for some reviews. It's important we're sure this is good. :)

[mxaddict]

@GrahamCampbell updated the commit message 😄

[taylorotwell]

Please send again when it has unit tests.

[mxaddict]

@taylorotwell, this was an existing feature and already has tests here:

tests/Database/DatabaseEloquentIntegrationTest.php in the testCountForPaginationWithGrouping Method

[Edit] I don't see why the original test needs changing, as it does the exact same thing that it used to, just FASTER, for larger Databases 😄

[mxaddict]

So what kind of test do I need to add? Cause the functionality is already tested in the original integration test. Let me know so that I can create one 😄

[GrahamCampbell]

Taylor, this already has unit tests. They were put it earlier when my PR was merged.

[GrahamCampbell]

This is just a change in the implementation for performance reasons rather than a bug fix.

[mxaddict]

I Added more fields to the backupMoreFields array, cause they were not needing for the agregate and were causing slowness with union and join queries that used pagination.

@GrahamCampbell what do you think about the new list of backup fields?

[mxaddict]

@GrahamCampbell , I looked more into the current integration test, and I think the coverage is lacking, would it be OK for me to add a couple of tests to better cover the use-cases for the pagination count query?

Cause I might as well add it now while I'm fresh from making the changes.

[mxaddict]

@GrahamCampbell, I implemented a couple of tests to make sure that the counts work as expected when using normal flow, count without grouping, count with grouping, count with grouping + joins, count with grouping + leftJoins, I tried to follow the way other tests were implemented, but let me know if I need to change/update anything.

Also, I'm not sure if the file I added the tests to is correct location, so let me know if I need to move them elsewhere.

[beanmoss]

👍

[zot24]

👍

[overtrue]

👍

[GrahamCampbell]

Have you guys actually tested this out?

[mxaddict]

@GrahamCampbell, I would advise we not merge this just now, I'm not sure that it's 100% A OK, can we ask for more input from other contributors?

[GrahamCampbell]

Ping @codelight89, @fcjailybo, @JoostK, @codelight89, and @jarektkaczyk for review.

[taylorotwell]

Let's re-open this when we are more confident then.

[giacomoburattini]

We had the same issue and we fixed it in a similar way, this is a big deal with large datasets. Watching this thread since we are interested in knowing when this pull request will be merged.

[mxaddict]

I don't think this will be merged anytime soon, We know the issue and know the solution, but I myself am not familiar with the process of making and getting a PR merged into this project, So i have left it for now, been busy with other stuff, sorry @giacomoburattini