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

[5.6] Integration test to avoid repetitive pagination PR with columns #23259

Merged
merged 2 commits into from
Feb 23, 2018

Conversation

deleugpn
Copy link
Contributor

@deleugpn deleugpn commented Feb 22, 2018

As Taylor recently said, in the last week 3 people already tried PRing a change to the Builder that passes the $columns into the getCountForPagination(). Unfortunately, that doesn't work when you use more than one column and it breaks the SQL being generated.

This is an attempt to avoid people from trying that again in the future. They'll be able to see for themselves that their change breaks on TravisCI. If somebody can apply a change that doesn't break this test, maybe that would be a PR worth looking into.

References:
#23255, #23214, #23209

Edit: StyleCI is failing but I don't know what is the issue as it's failing to analyze. ping @GrahamCampbell

@deleugpn deleugpn changed the title Integration test to avoid repetitive pagination PR with columns [5.6] Integration test to avoid repetitive pagination PR with columns Feb 22, 2018
@taylorotwell
Copy link
Member

Does this test break with the change people have tried to make?

@deleugpn
Copy link
Contributor Author

deleugpn commented Feb 23, 2018

Yes.

root@0ad09383aa5d:/var/www/laravel-framework# ./vendor/bin/phpunit --group=f
PHPUnit 6.5.5 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.12
Configuration: /var/www/laravel-framework/phpunit.xml.dist

E                                                                   1 / 1 (100%)

Time: 1.72 seconds, Memory: 46.00MB

There was 1 error:

1) Illuminate\Tests\Integration\Database\EloquentPaginateTest\EloquentPaginateTest::test_pagination_on_top_of_columns
Illuminate\Database\QueryException: SQLSTATE[HY000]: General error: 1 wrong number of arguments to function count() (SQL: select count("id", "title") as aggregate from "posts")

/var/www/laravel-framework/src/Illuminate/Database/Connection.php:664
/var/www/laravel-framework/src/Illuminate/Database/Connection.php:624
/var/www/laravel-framework/src/Illuminate/Database/Connection.php:333
/var/www/laravel-framework/src/Illuminate/Database/Query/Builder.php:1748
/var/www/laravel-framework/src/Illuminate/Database/Query/Builder.php:1733
/var/www/laravel-framework/src/Illuminate/Database/Query/Builder.php:1833
/var/www/laravel-framework/src/Illuminate/Database/Query/Builder.php:1806
/var/www/laravel-framework/src/Illuminate/Database/Eloquent/Builder.php:708
/var/www/laravel-framework/src/Illuminate/Database/Eloquent/Model.php:1504
/var/www/laravel-framework/src/Illuminate/Database/Eloquent/Model.php:1516
/var/www/laravel-framework/tests/Integration/Database/EloquentPaginateTest.php:36

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.

2 participants