Add hasRole() method to AccountProxy and UserSession classes

This is convenient and logical - if AccountProxy gives you hasPermission(), it's reasonable to assume hasRole() will also work, more so given getRoles() exists.

(Is there a good reason we don't have a getPermissions() method for users, only for roles?)

Partial review (not RTBC-ing mostly because I assume we need tests - tagged):

- I too noticed the problem, patch applies cleanly to 8.6.x, works and actually using it
- Diff makes sense (to me) on my read through
- In scope (should we explain the new UserSession method in issue summary?)
- Code style/comments look ok
- No UI / CSS / JS / RTL changes
- No browser testing needed
- No translatable text changes

OK, noted :) Removing tag and RTBCing.

We should have a test for the implementation.

+++ b/core/lib/Drupal/Core/Session/UserSession.php
@@ -99,6 +99,13 @@ public function getRoles($exclude_locked_roles = FALSE) {
+  /**
+   * {@inheritdoc}
+   */
+  public function hasRole($rid) {
+    return in_array($rid, $this->getRoles());
+  }

This should be tested and I think we should always do strict type checking on in_array() calls. Mind you that's not what we do in \Drupal\user\Entity\User::hasRole() so let's open a follow-up to add this to both.

We also need a change record.

Tests and change record look great to me.

Isn't this an API change? Unlike UserInterface, AccountInterface does not have a 1:1 class that others can be expected to implement. So if someone has a custom implementation of this, it would break in 8.7.

So what can we do here? Add a new interface that includes hasRole(), and deprecate AccountInterface?

But the parent interface is an existing interface as well.

And that means it's also "Add method" for AccountInterface and that is not allowed according to exactly those rules you posted :) That said, we have our own rules I think, for example the interface+default class where adding methods is allowed in most cases. But AccountInterface doesn't have default class, which is exactly the problem.

I also think that "Move to parent" means Move to a *new* parent interface :)

Not really, the same rules apply to 9.0, if anything it's stricter as we try to keep it same as 8.9 except deprecation removals and dependency changes. And with 9.1, the same rules apply again.

If this won't be accepted for 9.1 either, so when will this issue have a chance to be fixed? In 10.0 only??

Can we make it in Drupal 10? 😇

We do allow adding methods to interfaces that aren't explicitly tagged with @api, see https://www.drupal.org/about/core/policies/core-change-policies/drupal-8...

However in practice we'd want to avoid breakage in a minor for this change, so I think we should grep contrib.

The question is:

1. Are any contrib modules extending AccountInterface and not implementing ::hasRole() ?
2. Are any contrib modules extending AccountProxy or UserSession (in which case they'd get the method, but that might highlight whether there are custom/theoretical use cases for #1.

If we can't change the interface in a Drupal 9 minor, we could still add the implementations of the method, and document that the additional method will be on the interface in Drupal 10 etc. just without changing the interface itself.

Contrib search for "implements AccountInterface" looks fairly promising:

http://grep.xnddx.ru/search?text=implements+AccountInterface&filename=

Most of these are false positives; they are custom Account entities with a corresponding interface. Only testtools module looks problematic, and that is a one line fix: https://git.drupalcode.org/project/testtools/-/blob/8.x-2.x/tests/src/Un...

Two modules extend AccountProxy: http://grep.xnddx.ru/search?text=extends+AccountProxy&filename=

Three modules extend UserSession: http://grep.xnddx.ru/search?text=extends+UserSession&filename=

None of these look like adding hasRole() would be an issue.

+++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -41,6 +41,17 @@ public function id();
+   * @param string $rid
+   *   The role ID to check.
...
+  public function hasRole($rid);

new method should use type-hints for argument and result

Updating the issue summary.

Since there is not an abstract base class with a 1:1 relationship with AccountInterface, I think according to the API and BC policy:

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:

Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.

Where the interface is implemented directly for other reasons, be prepared to add support for new methods in minor releases.

Also, according to the deprecation policy, we do provide a best-effort BC for internal APIs and we have a path to do so here.

So, I think we should do what @catch says in the second part of his comment in #28:

If we can't change the interface in a Drupal 9 minor, we could still add the implementations of the method, and document that the additional method will be on the interface in Drupal 10 etc. just without changing the interface itself.

Leaving the RM review tag on because we will want to review any RTBC proposal before it is committed.

Removing credit for @keboca's accidental MR.

So to move this forward, I think this needs work to remove AccountInterface::hasRole() from the patch and move the docs to the implementations for the time being, plus a @todo for Drupal 10, and a followup issue to add AccountInterface::hasRole() in D10.

I agree it makes more sense for hasRole() to be in AccountInterface, since the same interface includes the getRoles() method, which implies objects implementing AccountInterface have access to the roles assigned to an account.

On the other side, code could also only rely on getRoles(). Checking the $user account has the $role role just requires code similar to the following one.

if (in_array($role, $user->getRoles())) {
  // $user as the $role role.
}

That is essentially the code used from User::hasRole().

In code used from contributed modules, checking a user account has a role is less common than checking which permission that user account has.

Forgot to mention in my comment, another thing that we should do is raise a deprecation in core callers if an implementation of the interface is lacking an implementation of the method.

Thanks everyone for the input! 🙇‍♂️

I turned the latest patch into a merge request and applied the following changes:

1. Moved hasRole() back from AccountInterface to UserInterface;
2. Adjusted test due to some changes in constructor parameters;
3. Added links to follow-up task: #3228209: Move hasRole() method from UserInterface to AccountInterface;
4. Added type-hints as suggested in #30

I also updated the issue summary and change record.

The remaining item is a deprecation notice mentioned in #42, but I would need some guidance here.

MR #1058 did not trigger tests for some reason.
I created a new branch and new MR: https://git.drupalcode.org/project/drupal/-/merge_requests/1059

Also, hiding a patch to make it less confusing.

I noticed a related task, which is also has a potential change to AccountInterface - #2123883: Add API for "upcasting" from AccountInterface to a full User entity?.
It looks like a good DX improvement but has less activity in the task comments.

I'll be happy to post a patch there as well, but it would probably need a sign-off from one of the release managers.

+1 to adding hasRole() in a BC-friendly way. Thanks for already opening #3228209: Move hasRole() method from UserInterface to AccountInterface to handle the @todo-ness. Seems like this is ready for re-review, although I don't have time to actually review it right now.

Thanks everyone,
-Derek

This is probably already late to be added to 9.3.x... but if not, I would be happy to apply any additional changes during the weekend.
The merge request was rebased against 9.3.x and looking forward to review 🙏

OTOH it fits into 10.x which soon opening

From @xjm in today's #d10readiness meeting:

The question should be "9.5.x or 10.1.x". It has an @todo to move a method to an intertface in D10; that can't happen currently.

We haven't come up with great practices for adding new methods to interfaces; technically the whole interface should be deprecated and replaced.... not sure if we've a better compromise

It could be okay in 9.5.x if we figure out the future interface API addition part in a clean way

Symfony DebugClassLoader supports an @method annotation on interfaces which implies that a new method will be added in the next major version, maybe we should just adopt this? When the debug class loader is used this raises a notice to users if the method is not implemented, without it explicitly being required on the interface.

See how the withOptions method was added to HttpClientInterface: https://github.com/symfony/http-client-contracts/compare/2.5...3.0#diff-...

well now that we are in RC, seems like it's not going to happen for 9.5 so only relevant for 10.1.x

In any case, MR needs updating

Rerolled the patch #58 in Drupal 10.1.x.

Patch applies.

What needs to be sorted out is how we can add a method to an interface, since the code is working setting RTBC. There is already a follow-up to handle that in #3228209: Move hasRole() method from UserInterface to AccountInterface. And that other issue is blocked on this one.

Committed 419b492 and pushed to 10.1.x. Thanks!

The re-roll in #60 was posted less then 24 hours after the patch in #58 and doesn't change anything from that patch. I'm removing issue credit.