Adding a role to a user throws a php error

As mentioned in IRC, I'd be +1 to a addRole() and removeRole() method. Doesn't need to happen here but might not be much more work to put that code in methods. Would simplify quite a few places (mostly in tests) where we remove and add roles.

+++ b/core/modules/user/lib/Drupal/user/UserBCDecorator.phpundefined
@@ -29,4 +29,32 @@ public function &__get($name) {
+  /**
+   * Returns a list of roles.
+   *
+   * @return array
+   *   List of role IDs.
+   */
+  public function getRoles() {
+    return $this->getBCEntity()->getRoles();
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function addRole($rid) {
+    $this->getBCEntity()->addRole($rid);
+  }
+
+  /**
+   * Remove a role from a user.
+   *
+   * @param string $rid
+   *   The role ID to remove.
+   */
+  public function removeRole($rid) {
+    $this->getBCEntity()->removeRole($rid);

Why @inheritdoc on one of them and not the others?

+++ b/core/modules/user/tests/Drupal/user/Tests/Plugin/Action/AddRoleUserTest.phpundefined
@@ -0,0 +1,71 @@
+    $account = $this->getMockBuilder('Drupal\user\Plugin\Core\Entity\User')
+      ->disableOriginalConstructor()
+      ->getMock();

As in the other issue, why not use the interface, then you don't need to mess with the constructor?

And nice that the methods now allow to test the actions without messing with entity internals, I'm starting to like this stuff ;)

However, there's an untested blackbox now, and that's those methods.

So I think we also need actual test coverage for them, which can't be a unit test yet, as sad as this is ;)

Looks good!

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -236,4 +236,31 @@ public function getBCEntity() {
+  public function getRoles() {
+    $roles = array();
+    foreach ($this->get('roles') as $role) {
+        $roles[] = $role->value;
+      }
+    return $roles;
+  }

Weird indentation.

Could just be return $this->get('roles') ?: array();

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -236,4 +236,31 @@ public function getBCEntity() {
+    $this->roles = array_unique($roles);
...
+    $this->roles = array_diff($this->getRoles(), array($rid));

Should we use ->set() now? ->getRoles() uses $this->get().

+++ b/core/modules/user/lib/Drupal/user/UserBCDecorator.phpundefined
@@ -29,4 +29,32 @@ public function &__get($name) {
+  /**
+   * Returns a list of roles.
+   *
+   * @return array
+   *   List of role IDs.
...
+   * Remove a role from a user.
+   *
+   * @param string $rid
+   *   The role ID to remove.

Not that it matters (as it's the BC decorator) but should be {@inheritdoc}.

Double post.

I see, "PHP Fatal error: Class Mock_UserInterface_f482d180 must implement interface Traversable as part of either Iterator or IteratorAggregate in Unknown on line 0", that's what you get when you try to mock that interface. Almost sounds like a PHPUnit bug to me?

Anyway, let's go with User then :)

+++ b/core/modules/user/lib/Drupal/user/Tests/UserEntityTest.phpundefined
@@ -0,0 +1,64 @@
+   * @todo Convert this to a phpunit test.

I know you really want to to that but we could add that to every single test we write :)

+++ b/core/modules/user/lib/Drupal/user/Tests/UserEntityTest.phpundefined
@@ -0,0 +1,64 @@
+    $values = array('roles' => array(Language::LANGCODE_DEFAULT => array('test_role_one')));

The language stuff shouldn't be necessary, array('roles' => array('test_role_one')) should work too.

Not sure about using random roles here, we might want to use real ones (that we create first), to avoid issues later on with validation or something like that.

Opened #2015937: Use addRole() and removeRole() whenever a user role is added or removed as a nice follow-up.

#17: user-2015721-17.patch queued for re-testing.

#17: user-2015721-17.patch queued for re-testing.

+++ b/core/modules/user/tests/Drupal/user/Tests/Plugin/Action/AddRoleUserTest.phpundefined
@@ -0,0 +1,80 @@
+    $account = $this->getMockBuilder('Drupal\user\Plugin\Core\Entity\User')
+      ->disableOriginalConstructor()
+      ->getMock();

You could do this in setUp and store account in a property, so you dont have to repeat it on every test.

+++ b/core/modules/user/tests/Drupal/user/Tests/Plugin/Action/AddRoleUserTest.phpundefined
@@ -0,0 +1,80 @@
+    $remove_role_plugin->execute($account);
...
+    $remove_role_plugin->execute($account);

+++ b/core/modules/user/tests/Drupal/user/Tests/Plugin/Action/RemoveRoleUserTest.phpundefined
@@ -0,0 +1,80 @@
+    $remove_role_plugin->execute($account);
...
+    $remove_role_plugin->execute($account);

i guess we are just executing it to watch for fatal errors, but would be cool if we could assert something..is anything available that we could use?

The assertions happens through the mock classes that verify that the correct methods were called on the user entity, the execute() method doesn't return anything that could be verified, so I think that's fine.

+++ b/core/modules/user/tests/Drupal/user/Tests/Plugin/Action/RemoveRoleUserTest.phpundefined
@@ -0,0 +1,91 @@
+    $this->account = $this
...
+    $this->account = $this->getMockBuilder('Drupal\user\Plugin\Core\Entity\User')
...
+    $this->account = $this->getMockBuilder('Drupal\user\Plugin\Core\Entity\User')

I guess you meant to remove this in the test methods and just leave it in setUp?

+++ b/core/modules/user/tests/Drupal/user/Tests/Plugin/Action/AddRoleUserTest.phpundefined
@@ -0,0 +1,83 @@
+    $remove_role_plugin = new AddRoleUser($config, 'user_remove_role_action', array('type' => 'user'));
...
+    $remove_role_plugin = new AddRoleUser($config, 'user_remove_role_action', array('type' => 'user'));

Shouldn't this be a user_add_role_action or something like that?

I got VERY VERY angry last night while working on #1851086: Replace admin/people with a View for this, so thank you.
RTBC if green.

Committed bf5c0ae and pushed to 8.x. Thanks!

I think we need to have a change notice for the new methods...

Nice, change notice is clear to me.

I suggest we move on with #2015937: Use addRole() and removeRole() whenever a user role is added or removed

Yes, looks good.

@xatoo: When a change notice is done, the issue can be set to fixed and restored to the old values of the title, priority, category and the tag can be removed.