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

[DX] Added CurrentUserProvider service #14407

Closed
wants to merge 1 commit into
base: 2.8
from

Conversation

Projects
None yet
5 participants
@Nyholm
Member

Nyholm commented Apr 19, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR symfony/symfony-docs#5192

I believe it would be nice to make it little easier to retrieve the current user. This provider will hide the complexity with the token and check if the $token->getUser() is an object or a string.

return;
}
if (!is_object($user = $token->getUser())) {

This comment has been minimized.

@wouterj

wouterj Apr 19, 2015

Member

-1 on this logic, even if user is not a string, it should return it. anon. is a user too.

This comment has been minimized.

@ProPheT777

ProPheT777 Apr 19, 2015

Contributor

yep a user is not necessarily represented by an object, it depend of implementation of token. (for example UsernamePasswordToken accept string representation of user to handle InMemoryUserProvider).

This comment has been minimized.

@Nyholm

Nyholm Apr 19, 2015

Member

I understand that some user representation might be a string. This PR is a refactor of the FrameworkBundle's Controller::getUser. I've just put it to a service.
The CurrentUserProvider::getUser function do have the same logic as in the FramworkBundle

This comment has been minimized.

@ProPheT777

ProPheT777 Apr 19, 2015

Contributor

It's a mistake from FrameworkBundle so, this is why when we retrieve the current user from different way we obtains 'anon.' or null.

*
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
class CurrentUserProvider

This comment has been minimized.

@iltar

iltar Apr 19, 2015

Contributor

I'm not sure if it's a good idea to add a class just to wrap the TokenStorage.

In my opinion, the token/user shouldn't be called in your application controller, they should only be used to identify the current user and can be used to fetch the information of the current user.

Next to all this, there is already a PR open in the sensio framework-extra-bundle repo regarding a ParamConverter to inject the current user. This is a cleaner solution in my opinion: sensiolabs/SensioFrameworkExtraBundle/pull/327

This comment has been minimized.

@Nyholm

Nyholm Apr 19, 2015

Member

I was not aware of that PR on SensiolabsFrameworkExtraBundle. But that is just for Controllers. This CurrentUserProvider service is meant to make it easier to get the logged in user in your other services.

This comment has been minimized.

@iltar

iltar Apr 22, 2015

Contributor

In my opinion you should not bring that state to your services. You'll still need to check if the user is there and act on it. I think it's a far better solution to just inject the user if you need it in the method that you call. If you really need the user in a deep level of your services, then maybe you shouldn't be using the UserInterface for that.

public function someAction(UserInterface $user)
{
    $this->someService($user);
}

Right now you're adding a class that does nothing more than removing some boiler plating code. It also makes services harder to use on the commandline. The current name CurrentUserProvider also implies that the service has a state, which it shouldn't have.

I have a very big 👎 for this class as there are already other (better) solutions available.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Apr 19, 2015

I know that the anonymous user is also considered as a user. But IMHO it's very rare that you bother about the anonymous user. If you do $this->get('security.current_user_provider')->getUser() you want to take some action on your user model. You want to log something or maybe create something attached to that user.

The intention is to make it easier in the 90% of the cases when you just want to fetch your user model for the current user without any extra check for tokens and if user is the string ´anon.`.

@cvuorinen

This comment has been minimized.

cvuorinen commented Apr 21, 2015

Maybe you could make that intention a little more explicit by renaming the getUser method to getAuthenticatedUser? Or rename CurrentUserProvider to AuthenticatedUserProvider? That way it would be clearer that it will not return 'anon', only an authenticated user.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Apr 21, 2015

You are absolutely correct @cvuorinen. If it is renamed to example AuthenticatedUserProvider::getUser or AuthenticatedUserProvider::getUserObject it will be more obvious what it will return and why.

@Nyholm

This comment has been minimized.

Member

Nyholm commented May 5, 2015

Listening to your arguments and other friends in the community, I'm ready to drop this PR. It is bad design to use the user like this. One should inject the user in the method call as @iltar suggests.

Thank you all for taking time to review this PR.

@Nyholm Nyholm closed this May 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment