KnpLabs / knp-components

Various component pack, includes paginator
MIT License
751 stars 141 forks source link

Pass `$options` in `knp_pager.before` event #342

Closed mpdude closed 2 months ago

mpdude commented 2 months ago

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).

mpdude commented 2 months ago

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 commented 2 months ago

Test case added.

What would be appropriate documentation?

garak commented 2 months ago

What would be appropriate documentation?

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

mpdude commented 2 months ago

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 commented 2 months ago

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