8000 [RFC] Add a new kernel event after resolving controller arguments · Issue #18362 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC] Add a new kernel event after resolving controller arguments #18362

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.

Sign up for GitHub

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

Jump to bottom
Closed
stof opened this issue Mar 30, 2016 · 10 comments
Closed

[RFC] Add a new kernel event after resolving controller arguments #18362

stof opened this issue Mar 30, 2016 · 10 comments
Labels
Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@stof
Copy link
Member
stof commented Mar 30, 2016

Currently, kernel.controller is dispatched between the resolution of the controller and the resolution of arguments. this is great as it means that listeners can replace the controller being found.

Currently, SensioFrameworkExtraBundle also uses this event to alter the Request object for ParamConverters so that the argument resolution will see these objects, and then run the Security annotation, relying on these converted objects being in the Request.
If the argument resolution refactoring (#18308) is merged, we would need to make security check run after the resolution of arguments (to be able to use objects converted by the ArgumentResolver in expressions, and then deprecate ParamConverter in favor of custom value resolver).
For that, I suggest adding a new kernel event triggered between the resolution of arguments and the call to the controller in HttpKernel (name of the event to be discussed). this event would not allow to change the controller (it could maybe allow to change arguments, but I'm not sure it is necessary).
@fabpot
Copy link
Member
fabpot commented Mar 30, 2016

Big 👍 as I could have used this one in the past.

@stof stof mentioned this issue Mar 30, 2016
Merged
@stof
Copy link
Member Author
stof commented Mar 30, 2016

I suggest kernel.controller_arguments as event name, with a FilterControllerArgumentsEvent class (if it allows changing arguments).
What do you think ?

@fabpot
Copy link
Member
fabpot commented Mar 30, 2016

LGTM, can you submit a PR?

@stof
Copy link
Member Author
stof commented Mar 30, 2016

can it wait until Friday evening, or is it too late for the 3.1 feature freeze ? If it is too late, I can try to work on it late this evening

@xabbuh
Copy link
Member
xabbuh commented Mar 30, 2016

Sounds like a good idea to me.

@HeahDude
Copy link
Contributor

Looks very powerful indeed. Thanks @iltar @stof for working on this!

@javiereguiluz javiereguiluz added Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Mar 30, 2016
@linaori
Copy link
Contributor
linaori commented Apr 4, 2016

@stof is it the idea to modify the arguments after they've been resolved? I don't think this is a good idea due to performance. The parameter converters can each have their own ArgumentValueResolverInterface implementation and I would recommend to register them after the default, in case they are provided already (think about subrequests for example).

I'm 100% for changing parameter converters to ArgumentValueResolvers, but right now the PC configuration would have to be fetched directly from the Request unless a small layer in-between fixes this, but would mean they need 1 value resolver which handles all current PC.

Regarding the Security annotation (or controller/action annotations in general)

The annotation reading is currently done on kernel.controller. For 3.2 I'm planning the next part of my original PoC issue (#17933) which will probably include a cache-warmer that reads annotations already and caches them away. This means that the ControllerListener in the FWEB won't have to read them anymore and can simply put them in the request for BC reasons.

At that point, the Template, Security etc listeners simply have to request the controller information (specific annotation most likely) and can work with that.

anyway, this is future talk of what I'm trying to work out
https://github.com/sensiolabs/SensioFrameworkExtraBundle/blob/master/EventListener/SecurityListener.php#L48-L54

  • A lot of checks can already be moved to a compiler pass/extension
  • The part retrieving the annotation could be as simple as
list($security) = $event->getConfiguration(Security::class);

First I was thinking of making this annotation specific, but it would be far more flexible if it's simply configuration, so people with yaml or xml structures can also use this. I'm not 100% sure how to do this yet though.

@stof
Copy link
Member Author
stof commented Apr 4, 2016

@iltar the idea is to be able to run some logic after the arguments are resolved (for instance the security annotation).
Using a kernel.controller event for that won't work anymore if the arguments are converted in the resolver instead of using a kernel.controller listener to do it, as the security annotation needs to have converted arguments when evaluating the expression (otherwise, it is a big BC break and useless for most devs)

@linaori
Copy link
Contributor
linaori commented Apr 4, 2016

Ah I see what you mean, you're referring to this line: https://github.com/sensiolabs/SensioFrameworkExtraBundle/blob/master/EventListener/SecurityListener.php#L95

@stof
Copy link
Member Author
stof commented Apr 4, 2016

@iltar yes

@stof stof mentioned this issue Apr 5, 2016
Merged
fabpot added a commit that referenced this issue Apr 7, 2016
@fabpot
feature #18440 Add the kernel.controller_arguments event (stof)
eb40f16 
This PR was merged into the 3.1-dev branch.

Discussion
----------

Add the kernel.controller_arguments event

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #18362
| License       | MIT
| Doc PR        | symfony/symfony-docs#6434

I'm not sure this can be integrated in 3.1 due to the feature freeze, but it would be great if it is, as it is a must-have to be able to make the ``@Security`` annotation compatible with the new argument resolver system (as we need to be able to run the security assertion after the resolving).

I made the arguments mutable here for consistency with ``kernel.controller`` (and @fabpot replied LGTM in the RFC when I suggested it).

Commits
-------

af02e2a Add the kernel.controller_arguments event
@fabpot fabpot closed this as completed Apr 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @javiereguiluz @stof @linaori @xabbuh @HeahDude
0