Skip to content

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

Merged
merged 1 commit into from
Oct 15, 2016

Conversation

weaverryan
Copy link
Member

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 Added a SecurityUserValueResolver for controllers #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!

@linaori
Copy link
Contributor

linaori commented Jul 27, 2016

While I do not disagree with your points, I would like to additionally add some comments:

For the 90% of Symfony project's that user a User entity for their User

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 #symfony are related to this). It makes the security look more complex than it actually is.

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.

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).

100% of the same behavior as $this->getUser() so that won't really make a difference imo.

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

Yet another issue of having your Entity as Security User.

Where we can, let's make controllers and services act more like each other.

The UserInterface would give you the ability for a consistent method to retrieve the user regardless of Controller as a Service or simply extending the base 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.

I think this is indeed a fair point (besides of some people not using an IDE and asking this in #symfony anyway). While I agree that looking in the code is a good thing to understand what's going on, the Request has the same challenge. As long as the documentation explains everything, it should be fine. Symfony has (imo) top quality documentation so it shouldn't be hard for people to find the solution. Even if people manage to find out it's the security.token_storage, there's a ton of other challenges they face which may lead to not even getting a token when you expect to have one.


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.

@HeahDude
Copy link
Contributor

HeahDude commented Jul 27, 2016

You are only allowed to type-hint the argument with UserInterface

IDE use will use the annotation instead of the type hint for concrete class, I don't think this is really a point.

@mnapoli
Copy link
Contributor

mnapoli commented Jul 28, 2016

@HeahDude it will require everyone to use phpdoc to override the type-hint, which is less than practical.

@stof
Copy link
Member

stof commented Jul 28, 2016

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 #symfony are related to this). It makes the security look more complex than it actually is.

We also have the EntityUserProvider in the core. Storing your users in the database is a very common use case.

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.

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)

@alcaeus
Copy link
Contributor

alcaeus commented Jul 28, 2016

The IDE autocompletion would be the least of your problems. Assume your user entity adds methods not in UserInterface:

class User implements UserInterface
{
    public function extraMethodNotInInterface() {}
    // All other methods from UserInterface here
}

Calling extraMethodNotInInterface in your controller actions is a fatal error waiting to happen, so you'd need an extra instanceof check before calling any method not in UserInterface:

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.

@linaori
Copy link
Contributor

linaori commented Jul 28, 2016

@alcaeus you already have to do this with getUser as well

@weaverryan
Copy link
Member Author

I'll propose 2 other issues:

  1. Discoverability With the type-hint method, the code isn't self-discovering: there is no way that you could know or discover that you need to type-hint the argument to get the User object. You're reliant on documentation. That's a bad experience - we should make coding as "instinctual" as possible when we can. This is definitely true for the Request object, but that doesn't mean we should repeat this for other things :).

  2. Ease for beginners

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 use statement they need. That's a bummer, and it's why we have shortcuts like createNotFoundException - so that the user can do common things without needing to look up special classes.

Thanks for the conversation :)

@linaori
Copy link
Contributor

linaori commented Jul 28, 2016

@weaverryan perhaps removing the deprecation from getUser() could be sufficient already, this would still make it possible for controllers as service to utilize it without making a sacrifice for the points you've just mentioned.

@DavidBadura
Copy link
Contributor

I'm only against to deprecate getUser(). SecurityUserValueResolver is a nice feature like ParamConverter, but i don't want to be forced to used it (more or less).

A controller is a PHP callable you create that takes information from the HTTP request and creates and returns an HTTP response (as a Symfony Response object).

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".

@nicolas-grekas
Copy link
Member

@weaverryan would un-deprecating getUser fix all your issues or not?

@mickaelandrieu
Copy link
Contributor

mickaelandrieu commented Aug 2, 2016

SecurityUserValueResolver is a nice feature like ParamConverter, but i don't want to be forced to used it

Same for me. I want to keep $this->getUser() and this new resolver, even if it's not perfect (and this can't be perfect :) ).

Thank you @iltar for your contribution and @weaverryan to improve it ^^

@wouterj
Copy link
Member

wouterj commented Aug 3, 2016

I don't agree much with (2) of this issue. We decided to deprecate and remove $this->getRequest() for the same reasons as $this->getUser() is now deprecated: Having 2 ways to achieve the same goal is not great (people have to choose which way to use).

See also the discussion in #12121 and #9553

@weaverryan weaverryan force-pushed the remove-user-value-resolver branch from 87bd06e to da7daee Compare October 14, 2016 23:46
@weaverryan
Copy link
Member Author

Ok, this is ready! This just un-deprecates Controller::getUser().

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.

@linaori
Copy link
Contributor

linaori commented Oct 15, 2016

@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).

@fabpot
Copy link
Member

fabpot commented Oct 15, 2016

Thank you @weaverryan.

@fabpot fabpot merged commit da7daee into symfony:master Oct 15, 2016
fabpot added a commit that referenced this pull request Oct 15, 2016
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
@weaverryan weaverryan deleted the remove-user-value-resolver branch October 15, 2016 16:41
@fabpot fabpot mentioned this pull request Oct 27, 2016
wouterj added a commit to symfony/symfony-docs that referenced this pull request Nov 19, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.