Move hasRole() method from UserInterface to AccountInterface

Great idea! Let's see what breaks. 😅

I guess this is the more accurate title...

Oh hah! I somehow thought #2991232: Add hasRole() method to AccountProxy and UserSession classes had already landed. Tee hee. This issue isn't blocked on 10.0.x branch, it's blocked on 2991232. Updated remaining tasks accordingly.

In this issue we should update \Drupal\Core\Cache\Context\UserRolesCacheContext::getContext to use the method.

This is now postponed on the Drupal 11 version being selectable.

The blocker landed now.

@Berdir I was leaving this postponed until we had a D11 version selector... guess we can use the title for that :(

I'm a bit confused about that part. We can't just add a new method in D11 to an existing interface? We need to prepare for that in 10.x, trigger a deprecation if it's not there and so on?

I don't get the the split between the issue that just got in and this. I did expect the other issue to do that and this one to then enforce it. But the other issues just added two standalone methods that are not backed by an interface, so IMHO their usefulness is quite limited.

We can add methods to interfaces as long as they're not tagged @api and have a 1-1 relationship with a class, or an associated base class/trait:

Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:

https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...

I haven't been following the two issues though, so no opinion on the split as yet.

This is not a 1:1 relationship, that's why that other issue exists (until now, you had to load the user entity to use hasRole() from the user session object), there 3 implementations in core and I know that there are contrib implementations as well, for example simple_oauth.

hmm what am I missing.

core/lib/Drupal/Core/Session/UserSession.php:class UserSession implements AccountInterface {

core/lib/Drupal/Core/Session/AccountProxyInterface.php:interface AccountProxyInterface extends AccountInterface {

core/lib/Drupal/Core/Session/AccountProxy.php:class AccountProxy implements AccountProxyInterface {

So we have one class directly implementing AccountInterface, one interface (AccountProxyInterface) extending AccountInterface, and one class implementing AccountProxyInterface.

That is 2:2 rather than 1:1 but would be covered I think. It doesn't have to be 1:1 that's just one of the examples.

Neither the 5.0.x or 6.0.x branches of simple_oauth appear to implement AccountInterface or AccountProxyInterface afaict.

Well fair enough, there is UserInterface of course, but that already has the method, so nothing changes there. And simple_auth actually implements that, which is IMHO weird but actually helpful here.

IMHO, AccountInterface really does feel like an @api interface to me, it's tied to \Drupal\basic_auth\Authentication\Provider\BasicAuth::authenticate(), so authentication providers return that and you'd expect different kind of implementations from that. UserSession is basically the internal implementation of \Drupal\user\Authentication\Provider\Cookie and if we ever do #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries we would probably deprecate that.

That said, an account is pretty much expected to also exist as a user entity, the separation between the two is vague and IMHO really just exists to be not have a hard dependency in user module, but as soon as you have code doing User::load($account->id()), that dependency is back and very few if any drupal sites would imho be able to handle accounts that aren't user entities.

So... :shrug:

Moving back to 10.1.x but tagging for a second/third opinion.

Just stumbled across a TODO pointing to this issue in the core.

I'm not in the position to judge about the big picture consequences following from this change, but to me it looks logical and brings consistency.

Rebased original code to the 11.x, removed TODO's from where they were referring to this issue, updated method signatures where necessary. Code style checked - no new violations added.

Will see what test results would look like.

Had to create new branch and a merge request as it's impossible (at least for me) to change existing merge request target.

New MR is https://git.drupalcode.org/project/drupal/-/merge_requests/6931

@Berdir re #17 maybe easier to move UserInterface to core and get rid of AccountInterface?

Tests are still passing, @andypost, do you have specific objections to this?

From a developer perspective this is really nice for consistency.

Asked about this in #needs-review-queue-initative and @xjm mentioned can move this to RTBC if feel this is ready and just missing release manager sign off.

Haven't seen a follow up to #26 so looking at just the MR I do not see any issue with the move. Typehints appears good too.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I am setting back the status set by comment #27.

As @berdir has pointed out we can't add the return typehint in Drupal 11. So this is definitely not rtbc - although maybe it is for 12.x - but that's not open.

Working on the remaining issue fix in DrupalConVienna2025 with help from @anmolgoyal74

Removed the return typehint

Pipeline is failing on php 8.3 so moving back to needs work

I checked the pipeline failure and tested it locally — it appears to be a random failure. It has now passed, so I am moving the ticket back to NR.

The feedback was addressed. I don't have anything to add. So I'm going to move this back to RTBC.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Post-bot-rebellion rebase

Let's just do this in 11.4, it's been waiting long enough and nobody has any better ideas. The split between account and user is unclear and awkward, but we don't have a better solution right now. Removing RM review tag.

Committed and pushed 83869c4f1b3 to main and bb5178f2101 to 11.x. Thanks!

@mondrake pointed out in slack this seems to have broken 11.x tests, reverting for now to see if that gets us back to green.

No, that's not it, reverted the revert.