Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
UserValueResolver and SecurityUserValueResolver improvement#26971
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
…lass implementing UserInterface
…solve class implementing UserInterface
…solve class implementing UserInterface
chalasr left a comment
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 was on my list :)
| { | ||
| // only security user implementations are supported | ||
| if (UserInterface::class !==$argument->getType()) { | ||
| if (!$argument->getType() || !$this->implementsCorrectInterface($argument->getType())) { |
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.
Should be inSymfony\Component\Security\Http\Controller\UserValueResolver instead (see deprecation notice above)
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's in both classes :)
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.
Deprecated classes are frozen until they are suppressed, it should not get new features.
| */ | ||
| privatefunctionimplementsCorrectInterface($type) | ||
| { | ||
| return UserInterface::class ===$type ||array_key_exists(UserInterface::class,class_implements($type)); |
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.
Let's inline this logic in the calling condition
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.
isn't this whatis_a() oris_subclass_of() does?
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.
Didn't know 😳thanks!
linaori commentedApr 18, 2018
Discussion why this feature wasn't introduced when initially adding the resolver:#18510 (comment) |
davidkmenta commentedApr 19, 2018
@iltar thanks for the link. I agree that it may collide with the ParamConverter. On the other hand I still think the UserValueResolver should support classes implementing the UserInterface. Few quick ideas:
|
jvasseur commentedApr 19, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I agree with@davidkmenta here, the param converters are executed before the value resolver so the concern highlighted in#18510 (comment) shouldn't be a problem. Forcing users to type they controller arguments with |
linaori commentedApr 19, 2018
@davidkmenta@jvasseur If you type-hint an entity, it will actually try to inject that entity based on the request, which will fail, because there's no way of fetching it. This isbecause the parameter converters are triggered first.
If people use it as quick way to get their current user object, why not provide a (by the SecurityBundle) configurable AVR that can return a given object based on the current |
jvasseur commentedApr 19, 2018
@iltar didn't think of that since I tend do disable the automatic conversion. We need to think of the future of the future of param converters (sensiolabs/SensioFrameworkExtraBundle#436 ?) maybe it's time to deprecate them and introduce configurable argument resolvers, this would allow us to have a configurable security user resolver. |
linaori commentedApr 19, 2018
Yes,something has to be done about the parameter converters, but I'm not sure what and I don't have much time myself 😢 Regarding the current AVR, what if we add an additional interface to the interface DomainUserProviderInterface{// would return your entity for example, names to be discussedpublicfunctionconvertToDomainUser(UserInterface$user);} # follows the basic configurationsecurityproviders:hostnet:id:App\Security\Authentication\CustomerPortal\UserProvider This would make it possible for the AVR to resolve the security user to a domain user. If you implement the interface on your user provider, it will simply be "enabled" for whatever has that user. We have the firewall information available, so perhaps it's not even hard to find the user provider. |
davidkmenta commentedMay 31, 2018
So I guess this PR can be closed for now :) |
gmponos commentedJul 6, 2019
TBH I do not follow why this was closed because of bundle that it's not inside symfony.. Personally I do not use Sensio bundles... :( I believe that this is a problem of sensio bundle that should either decorate the user AVR or overwrite it. |
linaori commentedJul 6, 2019
Symfony needs a generic controller configuration system that works similar to the idea behind the annotations from that bundle (but configurable without annotations as well). When this is done, AVR can fully replace the parameter converters and it won't be an issue anymore. The issue arises becausea lot of developersdo use the |
UserValueResolver and SecurityUserValueResolver should also resolve class implementing UserInterface.