Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Documented the new kernel.controller_arguments event#6434
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
javiereguiluz commentedApr 5, 2016
| Q | A |
|---|---|
| Doc fix? | no |
| New docs? | yes |
| Applies to | master (3.1+) |
| Fixed tickets | symfony/symfony#18440 |
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
| The ``kernel.controller_arguments`` event was introduced in Symfony 3.1. | ||
| Once controller arguments have been resolved by Symfony, the ``kernel.controller_arguments`` | ||
| event is triggered. This allows to modify or replace the arguments to fit your |
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.
it now also allows to replace the controller
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.
I've reworded this. Thanks!
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
| This event can be an entry point used to modify the arguments of the controller | ||
| once they have been resolved by Symfony:: | ||
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.
namespace is missing, otherwise its not very clear where to put this method IMO
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 can put it where you want (as long as you tell Symfony that this method is a listener for this event) .
And the current proposal is consistent with other snippets in the page
javiereguiluz commentedMay 3, 2016
I've made the requested changes. Another PR ready to be merged. Thanks! |
reference/events.rst Outdated
| **Event Class**::class:`Symfony\\Component\\HttpKernel\\Event\\FilterControllerArgumentsEvent` | ||
| This event can be an entry point used to modify the arguments of the controller |
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.
I would remove "an entry point".
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.
Fixed. Thanks.
xabbuh commentedMay 3, 2016
👍 |
javiereguiluz commentedMay 21, 2016
After the last fixes, this is ready to be merged. Thanks! |
wouterj commentedMay 27, 2016
This is probably the most confusing feature of Symfony 3.1 and that's already visible in this PR: Replacing arguments isnot the main reason for this new event. This new event is triggered after arguments are resolved, so some logic can be executed based on these arguments. For instance, the If you want to replace arguments, writing a custom ArgumentValueResolver is the way to go (after that PR is merged, we should definitely reference it in this section). |
stof commentedMay 27, 2016
A use case to replace arguments is when you want to replace both the controller and the argument, after resolution of the original arguments (because your new controller is a closure wrapping the original controller). This was the use case brought by@fabpot (for a Blackfire use case IIRC) to allow replacing the controller in this event as well. |
wouterj commentedJun 12, 2016
Status: Needs Work |
javiereguiluz commentedJun 20, 2016
@wouterj I can't finish this PR. If you or any other volunteer can finish it, please take over this PR. Thanks. |
javiereguiluz commentedAug 5, 2016
Closing it because I can't finish it. I've opened an issue related to this:#6854. |