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

feature: fix #696 - add an maximum configurable number for client pagination #706

Conversation

Simperfit
Copy link
Contributor

@Simperfit Simperfit commented Aug 26, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #696
License MIT
Doc PR O

This PR give the possibility to limit the number of items that the client can see on the same page.

@Simperfit Simperfit changed the title feature: fix #619 - add an maximum configurable number for client pagination feature: fix #696 - add an maximum configurable number for client pagination Aug 26, 2016
@Simperfit Simperfit force-pushed the feature/allow-to-set-maxmium-item-per-page branch 3 times, most recently from 1da6f61 to 3a79d24 Compare August 26, 2016 20:27
@Simperfit
Copy link
Contributor Author

Hi @bwegrzyn, could you please test this PR and tell me if it's what you wanted to achieve ?

@dunglas
Copy link
Member

dunglas commented Aug 26, 2016

Can't this be implemented using a custom event listener validating the page parameter?

@dunglas
Copy link
Member

dunglas commented Aug 26, 2016

With your current implementation, the maximum number of items per page cannot be disabled anymore.

@Simperfit
Copy link
Contributor Author

@dunglas This can be implemented using a custom event listener, but do we have to ?
I find it better to have it in core directly, do you want me to add a parameter to activate or deactivate this configuration ?

@dunglas
Copy link
Member

dunglas commented Aug 27, 2016

Maybe can you allow a null value for this parameter?


public function __construct(ManagerRegistry $managerRegistry, RequestStack $requestStack, ResourceMetadataFactoryInterface $resourceMetadataFactory, bool $enabled = true, bool $clientEnabled = false, bool $clientItemsPerPage = false, int $itemsPerPage = 30, string $pageParameterName = 'page', string $enabledParameterName = 'pagination', string $itemsPerPageParameterName = 'itemsPerPage', int $maximumItemPerPage = 300)
public function __construct(ManagerRegistry $managerRegistry, RequestStack $requestStack, ResourceMetadataFactoryInterface $resourceMetadataFactory, bool $enabled = true, bool $clientEnabled = false, bool $clientItemsPerPage = false, int $itemsPerPage = 30, string $pageParameterName = 'page', string $enabledParameterName = 'pagination', string $itemsPerPageParameterName = 'itemsPerPage', int $maximumItemPerPage = 300, bool $maximumItemPerPageEnabled = false)
Copy link
Member

Choose a reason for hiding this comment

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

Can't you just change the signature to int $maximumItemPerPage = null (and it will be disabled when null is passed).

@Simperfit Simperfit force-pushed the feature/allow-to-set-maxmium-item-per-page branch 2 times, most recently from 0a8eb74 to e6b9323 Compare August 27, 2016 14:38
@Simperfit
Copy link
Contributor Author

@dunglas comments addressed

@dunglas
Copy link
Member

dunglas commented Aug 28, 2016

Great work, thanks! Can you just fix the ne Scrutinizr issue please?

@Simperfit Simperfit force-pushed the feature/allow-to-set-maxmium-item-per-page branch from e6b9323 to bd74767 Compare August 29, 2016 03:38
@Simperfit Simperfit force-pushed the feature/allow-to-set-maxmium-item-per-page branch from a0fa54e to d677cc7 Compare August 29, 2016 13:23
@Simperfit
Copy link
Contributor Author

Pr updated

@dunglas dunglas merged commit b2439d7 into api-platform:master Aug 29, 2016
@dunglas
Copy link
Member

dunglas commented Aug 29, 2016

Thanks Hamza!

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
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