Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security/Http] Make UserValueResolver accept any subtype of UserInterface#30401
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
chalasr commentedFeb 28, 2019
Already thought about this, but was unsure. ping@linaori |
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.
Could you please add a note in the Security CHANGELOG?
| // only security user implementations are supported | ||
| if (UserInterface::class !==$argument->getType()) { | ||
| $type =$argument->getType(); | ||
| if (UserInterface::class !==$type && !is_subclass_of($type, UserInterface::class)) { |
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 don't get new features, should be reverted
| // only security user implementations are supported | ||
| if (UserInterface::class !==$argument->getType()) { | ||
| $type =$argument->getType(); | ||
| if (UserInterface::class !==$type && !is_subclass_of($type, UserInterface::class)) { |
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 wonder if l48 should be changed toreturn $user instanceof $type to prevent a type error?
| // only security user implementations are supported | ||
| if (UserInterface::class !==$argument->getType()) { | ||
| $type =$argument->getType(); | ||
| if (UserInterface::class !==$type && !is_subclass_of($type, UserInterface::class)) { |
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.
Please test this with a parameter converter to see if this will work as expected when the UserInterface is an entity and parameter converters are enabled.
Koc commentedMar 3, 2019
This was proposed multiple times ago (#18510 ,#26971) and was rejected due to reasons described in this comment#18510 (comment) |
chalasr commentedApr 18, 2019
Now that we have#30914 I think we can close here. |
Uh oh!
There was an error while loading.Please reload this page.
With a controller we can inject the
UserInterfaceobject like this:However, this has a drawback: it hides thedomain user for actions that need the right type-hint, else, it means we're "blind", which means that we must add
if ($user instanceof MyUserObject) { /* ... */}to execute our logic.The goal of this PR is to allow this:
This is just a basic suggestion that is quite straightforward (so if it's not suitable, feel free to tell me, if you know the component better than me 👍 )!
If it's not possible, I also thought about creating a
SecurityTokenValueResolver, also probably straightforward 👍 (will probably work on a separate PR for this)