-
Notifications
You must be signed in to change notification settings - Fork 276
Fix #574, split IsGrantedListener event #602
Conversation
Split IsGrantedListener::onKernelControllerArguments() event into
two separate events.
* New IsGrantedListener::onKernelController() is attached to
KernelEvents::CONTROLLER and runs only @IsGranted annotations
WITHOUT subject.
* IsGrantedListener::onKernelControllerArguments() is attached to
KernelEvents::CONTROLLER_ARGUMENTS (no change here) but only
runs @IsGranted annotations WITH subject.
This way simple @IsGranted annotations will be interpreted *before*
preparing arguments for controller action, which fixes #574.
As example — now, there is a way to type-hint UserInterface (from
Security bundle) in controller action without need to set nullable
type-hint.
Unfortunately, my pull request fixes the problem only for one @IsGanted use case — when it's used WITHOUT subject. With subject specified, problem still exists and it's not possible to completely fix it with current Symfony architecture. But, for better developer experience, I added IsGrantedListener::onKernelException which warns Symfony user about unsupported usage of @IsGranted (with subject and UserInterface type hint). https://i.imgur.com/jGmwfz9.png It's a little bit hackish but in my opinion Symfony users are worth it. ;) |
I have the same problem, thanx for opening PR. Would be nice to merge it. // cc @fabpot |
My intention is to show for the user clear error message and instruct him how to fix the problem. Please see my comment: #602 (comment) |
When this pull request will be merge ? Also a work around for using IsGranted with a subject is to have two is IsGranted annotation, one that validate a role (@IsGranted("ROLE_AUTHENTICATED")) and the other one with the subject. That way the first one will fail if the user is not authenticated and will stop everything else. Afterwards injection or annotation will be executed properly. This would be cleaner than have optional argument, will work with all the case. Still need this PR to be merge for it to work... |
@TomaszGasior needs a rebase :) |
@azjezz would you like to take over this PR? I'd then invite you to squash all commits from @TomaszGasior in one, then add yours + send a new PR, when you have some time :) |
@nicolas-grekas sure 👍 :) |
} | ||
} | ||
|
||
public function onKernelControllerArguments(FilterControllerArgumentsEvent $event) |
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.
this type hints a deprecated class, isn't it?
better remove the type hint and replace it with an annotation with the non-deprecated type
that'll preserve compat with 3.4 while preparing for 5.0
please check other event too
Split IsGrantedListener::onKernelControllerArguments() event into
two separate events.
KernelEvents::CONTROLLER and runs only @IsGranted annotations
WITHOUT subject.
KernelEvents::CONTROLLER_ARGUMENTS (no change here) but only
runs @IsGranted annotations WITH subject.
This way simple @IsGranted annotations will be interpreted before
preparing arguments for controller action, which fixes #574.
As example — now, there is a way to type-hint UserInterface (from
Security bundle) in controller action without need to set nullable
type-hint.