Skip to content

[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

Closed
wants to merge 2 commits into from
Closed

[3.x] fix cast paginate per page to int #1997

wants to merge 2 commits into from

Conversation

Yahatix
Copy link
Contributor

@Yahatix Yahatix commented Mar 11, 2020

Cast per_page parameter Collection::paginate to an integer
Added additional test for this case.
Resolves #1750

@codecov-io
Copy link

codecov-io commented Mar 11, 2020

Codecov Report

Merging #1997 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1997   +/-   ##
=========================================
  Coverage     87.04%   87.04%           
  Complexity      667      667           
=========================================
  Files            31       31           
  Lines          1521     1521           
=========================================
  Hits           1324     1324           
  Misses          197      197           
Impacted Files Coverage Δ Complexity Δ
src/Jenssegers/Mongodb/Query/Builder.php 88.88% <100.00%> (ø) 165.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98bc2f5...8577ca8. Read the comment docs.

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');
Copy link
Contributor

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See issue: #1750

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think that we resolve not our problem, framework don't use declare(strict_types=1) and type hints
  2. If you know that first argument of method paginate should int type, why doesn't if you try cast to int before call paginate

@divine divine changed the title Cast paginate per page to int [3.x] Cast paginate per page to int Mar 11, 2020
@divine divine changed the title [3.x] Cast paginate per page to int [3.x] fix cast paginate per page to int Mar 11, 2020
@Smolevich Smolevich added the Needs investigation Need investigation about bugs described in issue label Mar 11, 2020
@divine divine closed this Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs investigation Need investigation about bugs described in issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination does not accept string number
4 participants