-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Remove the new SecurityUserValueResolver #19452
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
While I do not disagree with your points, I would like to additionally add some comments:
This is pretty much a practice because it's a recommended path via the FOSUserBundle, which is more of a pain than a useful bundle lately if you ask me (A lot of questions on I know that @wouterj was trying to phase out the security user completely because of its redundancy in the Token. So maybe phasing out the user completely would definitely be a good option which could indeed revert this feature.
100% of the same behavior as
Yet another issue of having your Entity as Security User.
The
I think this is indeed a fair point (besides of some people not using an IDE and asking this in So with these comments added, I have no hard feelings if it gets removed. I know that a bunch of people would like the feature, but I don't want it to become more complex for the majority. |
IDE use will use the annotation instead of the type hint for concrete class, I don't think this is really a point. |
@HeahDude it will require everyone to use phpdoc to override the type-hint, which is less than practical. |
We also have the EntityUserProvider in the core. Storing your users in the database is a very common use case.
This would be a major change in the way the component work, so this should be discussed somewhere first (this is the first time I hear about this idea) |
The IDE autocompletion would be the least of your problems. Assume your user entity adds methods not in class User implements UserInterface
{
public function extraMethodNotInInterface() {}
// All other methods from UserInterface here
} Calling public function someAction(UserInterface $user)
{
if ($user instanceof User) {
$user->extraMethodNotInInterface();
}
} I think there is too much potential to forget this and run into big issues when changing something in the firewall, so I'd suggest rolling back the original PR. |
@alcaeus you already have to do this with |
I'll propose 2 other issues:
First, there's more typing: // current
public function fooAction()
{
$user = $this->getUser();
}
// new
use Symfony\Component\Security\Core\User\UserInterface;
public function fooAction(UserInterface $user)
{
} Second, a beginner needs to know / look up the Thanks for the conversation :) |
@weaverryan perhaps removing the deprecation from |
I'm only against to deprecate
Personally, i like the base concept of a controller: transform a request into a response. in symfony 4 i would now add a trait with "getUser". |
@weaverryan would un-deprecating getUser fix all your issues or not? |
Same for me. I want to keep Thank you @iltar for your contribution and @weaverryan to improve it ^^ |
03646d8
to
87bd06e
Compare
87bd06e
to
da7daee
Compare
Ok, this is ready! This just un-deprecates I think the argument of only having one way of doing something is quite sound, but I'm not at all convinced that the new argument-resolver is a better way. Let's have both, and see how it all works out - we have plenty of time to deprecate something before 4.0. |
@weaverryan I agree that this is a nice compromise, we still have roughly a year to see if people use that feature over the other. I will create a PR to document the resolver (I was waiting for this PR decision). |
Thank you @weaverryan. |
This PR was merged into the 3.2-dev branch. Discussion ---------- Remove the new SecurityUserValueResolver | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no (the feature hasn't been released yet) | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Hi guys! This is a revert for #18510 (ping @iltar), which is a nice idea, but will have some big practical implications: 1) You are only allowed to type-hint the argument with `UserInterface` exactly. For the 90% of Symfony project's that user a User entity for their User, this will be weird: I'll receive a `UserInterface`, that immediately call methods on it that aren't in the interface (and also, your IDE won't give you auto-completion). And as #18510 mentions, we can't allow people to type-hint their concrete `User` class, because this will conflict with SensioFWExtraBundle ParamConverter if there is a user id in the URL 2) Deprecating and removing `$this->getUser()` in a controller is a step back. Where we can, let's make controllers and services act *more* like each other. You can't call `$this->getUser()` in a service, but at least if you look at this method in `Controller`, you can see that it uses `security.token_storage` - which is how you will get the User object if you need it from within services. Sorry for being late on this original issue! It looked good to me at first :). Cheers! Commits ------- da7daee Removing the Controller::getUser() deprecation
This PR was merged into the master branch. Discussion ---------- Added docs mentioning UserInterface in action args Added notes about the `UserInterface` added in symfony/symfony#18510, was waiting for the in-deprecation of the `getUser()` method: symfony/symfony#19452. Commits ------- 7849fa2 Added docs mentioning UserInterface in action args
Hi guys!
This is a revert for #18510 (ping @iltar), which is a nice idea, but will have some big practical implications:
You are only allowed to type-hint the argument with
UserInterface
exactly. For the 90% of Symfony project's that user a User entity for their User, this will be weird: I'll receive aUserInterface
, that immediately call methods on it that aren't in the interface (and also, your IDE won't give you auto-completion). And as Added a SecurityUserValueResolver for controllers #18510 mentions, we can't allow people to type-hint their concreteUser
class, because this will conflict with SensioFWExtraBundle ParamConverter if there is a user id in the URLDeprecating and removing
$this->getUser()
in a controller is a step back. Where we can, let's make controllers and services act more like each other. You can't call$this->getUser()
in a service, but at least if you look at this method inController
, you can see that it usessecurity.token_storage
- which is how you will get the User object if you need it from within services.Sorry for being late on this original issue! It looked good to me at first :).
Cheers!