User::addRole() and ::removeRole() should be chainable

patch was broken because using "web" prefix

Here's a test fix for new expectation

You're missed to add a test from my patch

+++ b/web/core/modules/user/src/UserInterface.php
--- a/web/core/modules/user/tests/src/Kernel/UserEntityTest.php
+++ b/web/core/modules/user/tests/src/Kernel/UserEntityTest.php

your patch against "web" again( not applicable to core

It may need change record

Can we make removeRole() chainable in the same way?

I think it would be a good idea to change both at the same time, given that addRole and removeRole complement each other. I found this issue because I tried to chain $user->removeRole('foo')->save() today and it didn't work.

We should also write a short change record to inform developers of this API addition.

I will work on it.

Providing extended patch with interdiff file.

Release notes snippet needs tuning

Added CR https://www.drupal.org/node/3326231 and fixed release snippet

Just a wee thing

+++ b/core/modules/user/src/UserInterface.php
@@ -66,6 +66,9 @@ public function hasRole($rid);
+   *   The called user entity.

@@ -74,6 +77,9 @@ public function addRole($rid);
+   *   The called user entity.

Pretty sure these can be removed. See #3129184: Remove description from @return $this.

Scope creep drupal.org/core/scope

+++ b/core/modules/user/src/UserInterface.php
@@ -106,7 +109,6 @@ public function getPassword();
-   *   The called user entity.

@@ -117,7 +119,6 @@ public function setPassword($password);
-   *   The called user entity.

@@ -136,7 +137,6 @@ public function getCreatedTime();
-   *   The called user entity.

@@ -155,7 +155,6 @@ public function getLastLoginTime();
-   *   The called user entity.

@@ -179,7 +178,6 @@ public function isBlocked();
-   *   The called user entity.

@@ -187,7 +185,6 @@ public function activate();
-   *   The called user entity.

this changes are unrelated, please remove it

@quietone Thank you! good old issue) Re #22 Checked core with git grep -A2 -F ' @return $this' and the most of places has no value, so it needs to fix coding standard before related issue as we just missing sniffer

Thanks, this looks good to me.

As a followup we could start using this across core, e.g. standard.install:

  // Assign user 1 the "administrator" role.
  /** @var \Drupal\user\Entity\User $user */
  $user = User::load(1);
  $user->addRole('administrator');
  $user->save();

+++ b/core/modules/user/src/UserInterface.php
@@ -69,6 +69,8 @@ public function hasRole($rid);
+   *
+   * @return $this

@@ -77,6 +79,8 @@ public function addRole($rid);
+   * @return $this

There is a slight disruption in adding the documentation to the interface, because it means child implementations that return nothing no longer implement the interface as documented and might fail coding standards checks. There's also hypothetically the possibility of disruption if a child implementation is already returning something other than $this (like a boolean flag, or whatever). Both those things are fine as disruptions in a minor release with a CR. I don't think it's disruptive enough that we need to add it to the release notes.

Tagging "Needs followup" for the issue to implement this where feasible.

Thanks for the test coverage and for keeping the out-of-scope changes out of scope. Committed to 10.1.x and published the CR.

Follow up made, removing tag. #3331229: Use chaining for User::addRole() and ::removeRole()