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

Pass $options in knp_pager.before event #342

Merged
merged 3 commits into from
Sep 20, 2024

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Sep 20, 2024

With this PR I'd like to suggest to pass the $options array with the knp_pager.before event in a way that makes it possible to modify the options from event subscribers.

Motivating use case:

I'd like to use values for the sort field parameter that do not expose implementation details in the URL. This is necessary to maintain stable URLs.

For example, instead of using blog.title+blog.published_date+blog.id as a value in the sort query parameter, use title.

The translation from title to the actual sort criteria could take place in a knp_pager.before subscriber; however, it would need to be able to access the options, since I need to pass in information about which pagination use case is being executed (the mapping from query parameter to actual sort expression depends on it).

Copy link
Collaborator

@garak garak left a comment

Choose a reason for hiding this comment

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

phpstan needs to be fixed.
This PR needs to add some documentation and some tests.

@@ -15,7 +15,8 @@ final class BeforeEvent extends Event
public function __construct(
private readonly EventDispatcherInterface $eventDispatcher,
private readonly ArgumentAccessInterface $argumentAccess,
private readonly ?Connection $connection = null
private readonly ?Connection $connection = null,
private array &$options = [],
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we really need to pass this argument by reference?

@@ -29,6 +30,11 @@ public function getArgumentAccess(): ArgumentAccessInterface
return $this->argumentAccess;
}

public function &getOptions(): array
Copy link
Collaborator

Choose a reason for hiding this comment

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

same objection about reference here

@mpdude
Copy link
Contributor Author

mpdude commented Sep 20, 2024

I share your reservations regarding the use of references 🤔.

I am not quite sure, but my feeling is that the knp_pager.before subscriber might need to be able to modify option values like the sortFieldAllowList as well; that is, during event handling we need to modify options in a way that changes flow back to the Paginator class.

We could follow the same pattern that is used for the ItemsEvent, with options being a public property on the event that is bound by reference.

WDYT?

@mpdude
Copy link
Contributor Author

mpdude commented Sep 20, 2024

Test case added.

What would be appropriate documentation?

@garak
Copy link
Collaborator

garak commented Sep 20, 2024

What would be appropriate documentation?

I don't know. It's your proposal, meant to fit your needs.

@mpdude
Copy link
Contributor Author

mpdude commented Sep 20, 2024

I've scanned the current documentation and there is nothing that explains in detail how events work or how they should be used. So, I'd say one has to understand it from looking at the source code, and there is nothing particular I'd suggest to add in the scope of this PR.

@garak
Copy link
Collaborator

garak commented Sep 20, 2024

I've scanned the current documentation and there is nothing that explains in detail how events work or how they should be used. So, I'd say one has to understand it from looking at the source code, and there is nothing particular I'd suggest to add in the scope of this PR.

Do you think there's room for improvement? Maybe you can propose a different PR to enhance documentation, explaining a bit how a generic customization (or even your specific customization) can work.

It would be much appreciated

@garak garak merged commit a1ccc00 into KnpLabs:master Sep 20, 2024
7 checks passed
@mpdude mpdude deleted the options-in-before-event branch September 20, 2024 12:21
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