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

Add comment on AbstractApi::$perPage() #1007

Merged
merged 1 commit into from May 20, 2021
Merged

Conversation

Nyholm
Copy link
Collaborator

@Nyholm Nyholm commented May 13, 2021

The AbstractApi::setPerPage() was removed in the 3.0. I cannot find what PR that removed it.

However, we kept AbstractApi::$perPage and we do use it on a few places.. Im not sure if we should remove the property or add back the method.

Also note, some integration tests are using setPerPage().

@GrahamCampbell
Copy link
Contributor

👎 here. The new design has this immutable doesn't it?

@Nyholm
Copy link
Collaborator Author

Nyholm commented May 13, 2021

Okey, Then I'll remove the AbstractApi::$perPage.

Thank you for the feedback

@Nyholm Nyholm changed the title Add back AbstractApi::setPerPage() Add comment on AbstractApi::$perPage() May 13, 2021
@Nyholm
Copy link
Collaborator Author

Nyholm commented May 13, 2021

oops, Im wrong again.

The ResultPager is using AbstractApi::$perPage. I've updated the integration tests and added a small comment.

@acrobat
Copy link
Collaborator

acrobat commented May 14, 2021

Yes, the resultpager is now the "main" point of entry for paginating results so that's the why the perPage property is not accessable anymore for the api classes. But the resultpager still use this property to do it's job.

See #907 for more context on this.

Thanks for the extra comment and fixed tests!

@acrobat
Copy link
Collaborator

acrobat commented May 20, 2021

Thank you @Nyholm

@acrobat acrobat merged commit 48f1df2 into KnpLabs:master May 20, 2021
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.

None yet

3 participants