Use AccountInterface instead of UserInterface in authentication

Here we go.

This does a little more than strictly necessary:

• Removes the fallback to the anonymous user. Not sure why that was there, it was (arguably?) not necessary even before this change, but it has even less justification now.
• Removes the fallback in the authentication provider. Previously it was already unnecessary because there was always a fallback user that would be returned (see previous point), but it's still unnecessary now
• Renamed methods to getAccount/setAccount vs. getUser/setUser. Not sure what the BC-policy is, that could be reverted of course, or we could leave in the old ones for BC.

Tests are green locally with this.

Changing the method names seems overkill and likely to break other people's integrations.

If you really want to do that, just add the new methods and call them from the old methods and mark the old ones as deprecated. Maybe we need to do that in any case if you are changing the type hinting.

This should probably have some test coverage added?

Thanks for taking a look.

Fine with deprecating the methods instead, sure. Just one question: If someone actually sets an AccountInterface that is not a UserInterface then getUser() would not return a UserInterface with the current code. Not sure how would like that handled, I would see the following options:

1. That's fine, just document that that can happen, since getUser() is deprecated then, all is good (my preference)
2. Actually track a separate $user and $account object (which would be very confusing IMO)
3. In that case return the account from getAccount() but return NULL from getUser() (also confusing IMO, albeit less so)
4. Try to load a user from the account and return NULL otherwise (not much better than the previous one in my book)

Since there are currently no typehints we are pretty much free to do whatever without risking any PHP breakage, but, yeah, would be good to know what the API should be in your opinion with this change.

In terms of test coverage: An actually meaningful test would only be possible with a custom AccountInterface implementation, which this patch enables, but does not exist in core itself. Is that what you had in mind, or would you be happy with unit(-y) test coverage, as well?

Sounds like we should add typehints in the 3.x branch if we do that any time soon.

Option #1 is maybe fine... you can change the @return param comment even and document that it just returns what was added in setUser() / setAccount() ?

As far as tests... well maybe you are right that there is not much to test. You could have a Kernel test or something that sets a \Drupal\Core\Session\UserSession as the user in a test module event subscriber? Why do you need a non-core implementation?

Thanks for the follow-up. Meant to reply with an updated patch "in hand", but that never materialized, sorry about that...

Sounds good regarding the getAccount()//setAccount() methods. 👍

And yes, makes sense regarding the test, as well, I was a bit in a mental rabbit hole when I wrote that thinking about the use-case in our project of having a custom AccountInterface implementation. Just setting a UserSession in a test is absolutely the sensible thing to do. 👍

Not sure when I will get around to updating the patch, but wanted to reply at least, for now.

Created an MR based on @tstoeckler's code.

I've added back the old methods and deprecated them.

I'm not convinced that we need test coverage beyond the existing code coverage - this is more a underlying refactor where we're broadening the interface to allow what Drupal's user authentication allows.

Hiding the patch now we have an MR.

merged to 2.x and will cherry-pick to 3.x

Please make a 3.x issue to remove the old methods.

Wow, that's awesome, thanks a lot!

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