Error: Call to undefined method Drupal\user\Entity\User::getAccount() in event_log_track_auth_user_logout()

Added the change to the issue's fork.

Will this error occur if the Masquerade module not installed?

I suspect this has something to do with what is passed to the the hook user_logout.

The API spec defines it as receiving an AccountInterface
https://api.drupal.org/api/drupal/core%21modules%21user%21user.api.php/f...
Which does not specify a method getAccount
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Session%2...

However my guess is that without masquerade in play the object being passed in has that method. So this is perhaps how this has slipped through.

Hi all,

Sorry for the delay! I just had a chance to check and found that if I uninstall the masquerade module this issue doesn't occur when I log out (and the logout event is logged as expected).

Does that mean this is actually an issue with the masquerade module?

Does that mean this is actually an issue with the masquerade module?

I haven't had a chance to look closely. I can't complain other modules without knowing what exactly happened.

But it is difficult for us to reproduce this issue with our module.

Ideally, we would like to have a PHPUnit test to expose an issue when fixing it. That saying, we can't fix an issue which requires another module to trigger.

Just made a slight change... I added a new $user variable whose value is set based on whether or not the getAccountName method is available. If it's available, it's set to $account->getAccountName(), and if not it's set using the current method, $account->getAccount()->name.

The change still addresses an issue that's only there because of the masquerade module, but could be a good compromise.

Thanks @Joe Sutta for working on this issue.

This issue actually brings in a very interesting question. What user name should be logged? The admin user's name or the name imitated?

In my opinion, the real (admin) user name should be logged.

Change to 'Need work' due to the outstanding question above.

I agree with that, since they're the one taking the action.

Since the error message complains about the getAccount() function, I suggest checking if that function exists instead. For example,

$user_name = (method_exists($account, 'getAccount')) ? $account->getAccount()->name : $account->getAccountName();

The method_exists checks aren't really necessary. When you log out normally, the object that gets passed to hook_user_logout() is an AccountProxy, which is a wrapper around a User entity. The AccountProxyInterface is what defines the getAccount() method used by the current code, which returns the wrapped User. However, AccountProxyInterface also extends AccountInterface—which, as noted above, is all that's guaranteed by hook_user_logout()—and as such, an AccountProxy object also has the getAccountName method, which is implemented like so:

public function getAccountName() {
    return $this->getAccount()
        ->getAccountName();
}

And that getAccountName() method on the User is simply this:

public function getAccountName() {
    return $this->get('name')->value ?: '';
}

Thus, calling $account->getAccountName() on an AccountProxy does the same thing as calling $account->getAccount()->name, the only difference being that getAccountName is guaranteed and getAccount is not.

As for the error thrown when using Masquerade, that's because masquerading first triggers a logout event for the admin, followed by a login event for the user they are masquerading as. When it does so, the object that comes into hook_user_logout() isn't an AccountProxy, it's just a normal User entity for the admin, and thus it will fail if you try to call getAccount, but succeeds with getAccountName. (This also answers the question about which user will be logged: it's the admin.)

So the cleanest solution would be to revert the MR to the original commit f0f48bdab0cac3b9cbc54e07640637a54f8995a0 that just changed $account->getAccount()->name to $account->getAccountName().

smustgrave → closed merge request !72

Change makes sense to me

Automatically closed - issue fixed for 2 weeks with no activity.