Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Add the kernel.controller_arguments event#18440
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| /** | ||
| * Allows filtering of controller arguments. | ||
| * | ||
| * You can call getController() to retrieve the controller and getArgument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor inconsistency:getArgument ->getArgument()
dc19f30 todb73ebdCompare| { | ||
| switch ($eventName) { | ||
| case KernelEvents::CONTROLLER: | ||
| case KernelEvents::CONTROLLER_ARGUMENTS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you're removing the stopwatch start oncontroller events then, to apply it only oncontroller_arguments events ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Taluu this is about starting the stopwatch for the controller execution itself. As there is now an event after the resolution of arguments, I starting it there now, as it makes the timing more accurate (in existing versions, the argument resolution is included in the controller time, even though it is also timed on its own)
stof commentedApr 6, 2016
@javiereguiluz thanks for writing the doc PR |
stof commentedApr 6, 2016
@fabpot@nicolas-grekas can you give your mind on whether this PR can still be considered for 3.1 ? |
fabpot commentedApr 6, 2016
That's going to be for 3.2 as we are in feature freeze now (except for old PRs that are about to be finished). |
stof commentedApr 7, 2016
@fabpot this means that SensioFrameworkExtraBundle |
HeahDude commentedApr 7, 2016
This event was on the table the day before the feature freeze, it's a nice following of#18308, it deserves a nice exception :) |
linaori commentedApr 7, 2016
I agree that this might be a nice addition to speed up the development by ~6 months |
db73ebd toaf02e2aComparestof commentedApr 7, 2016
As requested by@fabpot in person, I made the controller callable mutable as well during this event, making it possible to replace the controller by a different callable, after arguments of the original controller are resolved. This is great when wanting to wrap the controller into a callable having a different signature than the original one for instance (@fabpot has such a use case for blackfire btw). And there is a test covering this use case. |
fabpot commentedApr 7, 2016
Thank you@stof. |
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#6434I'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
javiereguiluz commentedApr 7, 2016
@stof now that this is merged, please share your thoughts about its docs here:symfony/symfony-docs#6434 Thanks! |
HeahDude commentedApr 7, 2016
🎉 |
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
@Securityannotation 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).