Skip to content

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

Merged
acrobat merged 1 commit into
KnpLabs:masterfrom
Nyholm:per-page
May 20, 2021
Merged

acrobat merged 1 commit into
KnpLabs:masterfrom
Nyholm:per-page

Conversation

@Nyholm

@Nyholm Nyholm commented May 13, 2021

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Contributor

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

@Nyholm

Nyholm commented May 13, 2021

Copy link
Copy Markdown
Collaborator Author

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

Nyholm commented May 13, 2021

Copy link
Copy Markdown
Collaborator Author

oops, Im wrong again.

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

@acrobat

acrobat commented May 14, 2021

Copy link
Copy Markdown
Collaborator

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

acrobat commented May 20, 2021

Copy link
Copy Markdown
Collaborator

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.

3 participants