Anonymous user should be a user object, not a string

[SgtPooki]

In AnonymousAuthenticationListener.php:

$this->context->setToken(new AnonymousToken($this->key, 'anon.', array()));

Why are you setting the anonymous user to a string value?

Wouldn't it be better to create an empty user object that all methods would still work on?
I noticed this when resolving a bug where

$user   = $this->get("security.context")->getToken()->getUser();
$user->getId(); //error here.. because $user == 'anon.'

was throwing a "Call to a member function getId() on a non-object" error when the user was an anonymous user. I'm on symfony 2.0.16, but I see that this is still in use, and afaik is extremely bad practice.

Shouldn't that method, and all methods, actually exist for any user returned by getUser()? Sure, the user is anonymous, but wouldn't it be much more intuitive and effective to encapsulate all user logic within the user object, than to require anyone to check for

if ($user === 'anon.') { //this should be unnecessary...

It is also pretty ugly in AbstractToken.php's setUser($user) function.

I understand the need as it applies to simple tokens to allow setting a string or object value.. but a user, which is always a user everywhere on every website, should be an object with the same methods, no matter what type of user it is.

Am I crazy or missing something here...?

[stloyd]

Just call:

if ($this->get("security.context")->isGranted('IS_AUTHENTICATED_REMEMBERED')) {
    $user   = $this->get("security.context")->getToken()->getUser();
    $user->getId();
}

[SgtPooki]

I may not understand the development mentality of the symfony community, but isn't that harder to read and more code-heavy than the following

$user = $this->get("security.context")->getToken()->getUser();
if ($user) {
    //Users that aren't actually users could return null... 
    //the alternative being a string with no methods, isn't this much more readable and intuitive?
}

OR

$user = $this->get("security.context")->getToken()->getUser()->getId();
if ($user->exists()) {
    //this is also more readable.
}

The less obvious consts such as IS_AUTHENTICATED_REMEMBERED seems like it should be done for symfony users behind the getUser() call somewhere to provide functionality such as the above.

I think this is similar: http://refactoring.com/catalog/introduceNullObject.html

[jakzal]

re #3697 (comment)

[stof]

@SgtPooki $token->getUser() is not forced to use a User. And even if we were returning an object for anonymous users, what would it be ? We cannot make it use your User entity class (there is no way to know about it there), so you would still not be able to call the getId method on it either)

[SgtPooki]

@stof valid question, but I think a better question is what is so important about an anonymous user that it is required to be a string instead of a null value. I'm going to go a little overboard with my explanation here, but due to the feedback received so far, I feel it's necessary.

If you needed to separate an anonymous user from a non-existent user (someone not on your site at all...) then sure, I can see creating an anonymous class that has different permissions or abilities from non-users. However, because this scenario should never be considered with seriousness; it's completely pointless and ridiculous to compare a user that is not registered to a user that does not really exist as far as any website implementing symfony is concerned.

They're technically the same, and therefore an anonymous, or unregistered user with absolutely no abilities or permissions, should be a null user. The user has not been set to any value yet.

Therefore, and in answer to your question, it is much more logical and sensible to do either of the following regardless of whether it is possible with your current code or not:

1: return null instead.
2: return some base user object that all other user objects should inherit from.

1 should always be possible, and would take the least effort to implement, and is usually the norm across all languages and frameworks I have used.

I notice that the only arguments against what I view as a better solution are

1. "$token->getUser() is not forced to use a user"
2. "there is no way to return your empty user entity object from this method"
3. "There is nothing in particular which would have prevented us from returning an object from the AnonymousToken, but we would have also gained nothing." - from Anonymous users #3697 (comment) referenced above

In response,

1. getUser(), by it's very name, implies that it returns a user, or by programming convention: null. Change the name of the function if you're not going to implement it properly.
2. Create a base user object which other user objects extend and make it work, because it makes more sense and is better. How is it not?
3. I have already explained what you would gain: more intuitiveness, and your getUser method would finally be implemented properly.

A more productive way of proceeding in the forum here, where productive means the improvement of symfony and it's usability, would be proving why this odd way of doing things is necessary, or why the anonymous string value is not considered odd.

If your argument is only "people are used to using it that way" then say so, and close this bug as a wontfix, recognizing that there is actually a flaw, instead of defending it.

[mvrhov]

Both A and B would mean a BC break so they are a no.

[Lumbendil]

@SgtPooki forcing the User to extend a class locks the extension tree of that class, it can't inherit anything else. I do agree with the option of returning null, but at this point it'd be a BC break.

Atm, the way I go is defining a role which all my DB users have, and check isGranted on that role.

[SgtPooki]

So then, save for 3.0 Symfony? Probably not..

[zyphlar]

The idea that getUser() would return anything except an object or null, indeed may return a non-null non-object, is an extremely poor programming choice. @mvrhov suggests that backwards-compatibility is important: okay, so do like PHP always does, and create a new, sane function while deprecating the old function.

[andho]

Not implemented in Symfony 3? XD

[aharbavets]

I've spent hour on debugging why doctrine complains that it can't use string as entity

[stof]

Well, even if it was an object, it would not be the same object than your User entity. So it would just create other issues

[zyphlar]

The lack of a user should either be null or a blank user object, not a non-null non-object. I know PHP's type system is weak but we don't need to encourage it.

…

On Tue, Nov 29, 2016, 9:49 AM Christophe Coevoet ***@***.***> wrote: Well, even if it was an object, it would not be the same object than your User entity. So it would just create other issues — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10168 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC9MtkfXhoUgeoSSd9FLRcHj1u-YTdEks5rDGWLgaJpZM4Bd7zM> .

[xrow]

Please fix this with the next major version, reopen and have a BC break with this new feature.