-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[3.x] fix cast paginate per page to int #1997
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=========================================
Coverage 87.04% 87.04%
Complexity 667 667
=========================================
Files 31 31
Lines 1521 1521
=========================================
Hits 1324 1324
Misses 197 197
Continue to review full report at Codecov.
|
Cast per_page parameter Collection::paginate to an integer Added additional test for this case. Resolves #1750
@@ -327,6 +327,20 @@ public function testPaginate(): void | |||
$this->assertEquals(1, $results->currentPage()); | |||
} | |||
|
|||
public function testThatPaginateCastsPerPageParameter() | |||
{ | |||
$results = User::paginate('2'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason pass $perPage as string, if original method expect int or null ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See issue: #1750
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests in laravel don't have case with pass string $perPage
https://github.com/laravel/framework/blob/0b12ef19623c40e22eff91a4b48cb13b3b415b25/tests/Integration/Database/QueryBuilderTest.php#L181
https://github.com/laravel/framework/blob/0b12ef19623c40e22eff91a4b48cb13b3b415b25/tests/Database/DatabaseQueryBuilderTest.php#L3205
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... Whats the problem with adding additional tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we pass not int value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's possible when using the default laravel model without mongodb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i saw in docs and tests and didn't find examples with passing string $perPage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make you happy, i just tested it in a stock laravel installation and confirmed that it actually does work in a stock installation.
And to no surprise it does.
Granted, when i created the PR i didn't fact check the original issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think that we resolve not our problem, framework don't use
declare(strict_types=1)
and type hints - If you know that first argument of method paginate should int type, why doesn't if you try cast to int before call
paginate
Cast per_page parameter Collection::paginate to an integer
Added additional test for this case.
Resolves #1750