Add parameter to User::getRoles() to exclude built in rids, (authenticated, anonymous)

I think I opened a similar issue too...

Hello guys

This is a small patch to add the functionality requested in this issue. I modify the EntityUser and the UserSession.

I tested the changes creating two different accounts using the following code

    $account = \Drupal::currentUser();

    $account = user_load(1); 

Please check and let me know any fix required for this patch.

Berdir, if you can remember the other issue you mentioned you created, please let me know.

1. +++ b/core/lib/Drupal/Core/Session/UserSession.php
   @@ -115,8 +115,14 @@ public function id() {
   +  public function getRoles($exclude_rol = NULL) {
   

   Parameter name should be something more readable, E.g. $exclude_locked_roles. It should also be TRUE or FALSE. I.e. It's either excluding the roles or it isn't. True or false makes this clearer.

2. +++ b/core/lib/Drupal/Core/Session/UserSession.php
   @@ -115,8 +115,14 @@ public function id() {
   +    if($exclude_rol) {
   

   Add space between if and (.

Yes, see also the notes in #2057999: Add argument to AccountInterface::getRoles() that allows to exclude the anonymous/authenticated roles

Hello folks

I follow the recommendations from Berdir and pingers and I use constants for locked roles.

Please check my new patch.

+++ b/core/lib/Drupal/Core/Session/UserSession.php
@@ -115,8 +115,14 @@ public function id() {
+      $roles = array_diff($roles, array(DRUPAL_ANONYMOUS_RID,DRUPAL_AUTHENTICATED_RID));

There needs to be a space afer the , same for the other implementation.

There is at least one use case where we can use this, in user_admin_account().

We will possibly also need some specific tests for it.

remove spaces as per @Berdir comments.

We do have unit tests for both User and UserSession so it would be cool to extend that with this functionality.

+++ b/core/modules/user/lib/Drupal/user/Entity/User.php
@@ -156,11 +156,15 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
+      if (!($exclude_locked_roles AND in_array($role->value,array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID)))) {

Let's use && instead of AND in core and use proper smaling after the ',' in in_array()

Thanks @dawehner. Update patch as per comments.

hello folks

I follow the @dawehner recommendations, include the changes in test unit.

IMHO we need to move the session unit test to user section, instead of PHP Unit where is located right now.

Yeah, that's why I just used the string of the constant and not the actual string, phpunit doesn't like code that it can't autoload.

You could define them in the test, but that's just a hack. Personally, I'd just use the string and add a @todo about moving the constants to a class/interface but not sure what others think.

Hello Bendir

I applied the change to use the value instead of the constants and add the @todo message was added.

1. +++ b/core/lib/Drupal/Core/Session/UserSession.php
   @@ -115,8 +115,14 @@ public function id() {
   +  public function getRoles($exclude_locked_roles = FALSE) {
   
   +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
   @@ -156,11 +156,15 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
   +  public function getRoles($exclude_locked_roles = FALSE) {
   

   This change should be added to interface as well

2. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
   @@ -156,11 +156,15 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
   +      if (!($exclude_locked_roles && in_array($role->value,array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID)))) {
   

   needs space after $role->value,

Hello Andy

Can you explain a little better what you mean when you said "This change should be added to interface as well", can you provide an example.

Regards,

There is an interface: core/lib/Drupal/Core/Session/AccountInterface.php which is implemented by both User as well as UserSession, so you have to change the arguments of the signature
of getRoles there as well.

The signature of a method implementing one defined in a parent class OR interface must match. I.e. the parameters must be the same. E.g.

<?php
interface Fruit {
  function setColor($color);
}
class Banana implements Fruit {
  function setColor($color) {
    // Implements the interface method.
  }
}
?>

setColor() has the same number of parameters (one called $color).

Hello folks

I did the changes related with spaces and the changes in interface, please review

+++ b/core/lib/Drupal/Core/Session/UserSession.php
@@ -115,8 +115,14 @@ public function id() {
-  public function getRoles() {
-    return $this->roles;
+  public function getRoles($exclude_locked_roles = FALSE) {
+    $roles = $this->roles;

You have also changed the signature of User::getRoles(), which is inherited from UserInterface and AccountInterface, so you would need to update those interfaces too.

Updated patch with missing interface

Uh, now

+++ b/core/modules/user/lib/Drupal/user/UserInterface.php
@@ -18,10 +18,13 @@
-  public function getRoles();
+  public function getRoles($exclude_locked_roles);

Now UserInterface:getRoles() doesn't conform to AccountInterface::getRoles(). All inherited methods would need to be updated.

1. +++ b/core/lib/Drupal/Core/Session/AccountInterface.php
   @@ -26,10 +26,13 @@ public function id();
   +   * @param bool $exclude_locked_roles
   +   *   Exclude or not locked roles.
   +   *
       * @return array
       *   List of role IDs.
       */
   -  public function getRoles();
   +  public function getRoles($exclude_locked_roles);
   

   Unlike the implementations, this is missing a default value. Also needs to have a = FALSE.

   Then, the documentation standard for those is something lke "(optional) If TRUE, locked roles (anonymous/authenticated) are not returned. Defaults to FALSE."

2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserEntityTest.php
   @@ -39,6 +39,7 @@ public static function getInfo() {
       * @see \Drupal\user\Entity\User::addRole()
       * @see \Drupal\user\Entity\User::removeRole()
   +   * @todo Please move roles constants to a class/interface
       */
   
   @@ -72,6 +73,12 @@ public function testUserMethods() {
   +
   +    $user->addRole('authenticated');
   +    $this->assertTrue($user->hasRole('test_role_two'));
   +    $this->assertTrue($user->hasRole('authenticated'));
   +    $this->assertEqual(array('authenticated', 'test_role_two'), $user->getRoles());
   +    $this->assertEqual(array('test_role_two'), $user->getRoles(TRUE));
   

   I think it's ok for the test to use the DRUPAL_AUTHENTICATED constant. No need for the @todo then as this actually tests the user entity (and not the UserSession class), where we don't have as much of a problem with the constant.

   Also, we don't write "Please" in the UI or code documentation :)

3. +++ b/core/modules/user/lib/Drupal/user/UserInterface.php
   @@ -18,10 +18,13 @@
      /**
       * Returns a list of roles.
       *
   +   * @param bool $exclude_locked_roles
   +   *   Exclude or not locked roles.
   +   *
       * @return array
       *   List of role IDs.
       */
   -  public function getRoles();
   +  public function getRoles($exclude_locked_roles);
   

   There's no reason to have the method declaration duplicated here. Just remove it completely :)

4. +++ b/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php
   @@ -153,4 +154,15 @@ public function testHasPermission($permission, array $sessions_with_access, arra
   +   * Tests getRoles method.
   

   "Tests the getRoles() method." ?

5. +++ b/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php
   @@ -153,4 +154,15 @@ public function testHasPermission($permission, array $sessions_with_access, arra
   +   * @todo Please move roles constants to a class/interface
   

   Same here, no please, please :)

   Personally, I'd leave out the @todo completely and instead create a follow-up issue and reference it here. If you want to work on that, awesome, but it's an API change and needs approval.

Hey @Berdir

I did almost all recommendations you did.

I maintain the Todo with some changes, because in the comment #14 yourself recommend me replace for the values because the patch unit test fail, event if in my local environment works.

About change propose in items #5, please create a new issues and I could work on that.

Please check the new patch and let me know if works for you.

Hey guys any update about my last comment and patch?

Remember to set issues to needs review to have them tested by the testbot and get reviews. Will try to review ASAP.

In PHPUnit tests, you need to use assertEquals(), not assertEqual().

I've rerolled patch #30.

Fixed some whitespaces + replaced the assertEqual() with assertEquals().

There is still a failing test because the constant DRUPAL_ANONYMOUS_RID is undefined.

Drupal\Tests\Core\Session\UserSessionTest::testUserGetRoles Use of undefined constant DRUPAL_ANONYMOUS_RID - assumed 'DRUPAL_ANONYMOUS_RID' /var/www/drupal/core/lib/Drupal/Core/Session/UserSession.php:122 /var/www/drupal/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php:154

Re-rolled patch.

Re-rolled Patch.

I am checking this..

Fixed the failing tests with the following changes:

• DRUPAL_ANONYMOUS_RID and DRUPAL_AUTHENTICATED_RID seems not available in UserSession.php so used "anonymous" and "authenticated" instead
• changed assertequals() to TestBase::assertEqual()

The test is failing because the getRoles() mocked in UserTest.php doesn't have any arguments defined but we are trying to call the getRoles() method with arguments in UserSessionTest.php.
I got this: http://privatepaste.com/57e9d421ee from IRC. But still its not helping much.

After some difficulty, here we have a working patch. Ran the tests locally and passed.
Thanks to Cottser and the author of this privatepaste(http://privatepaste.com/57e9d421ee) :)

1. +++ b/core/lib/Drupal/Core/Session/AccountInterface.php
   @@ -26,10 +26,13 @@ public function id();
   +   * @param bool $exclude_locked_roles (optional).
   +   *  If TRUE, locked roles (anonymous/authenticated) are not returned.
   

   To match 100% our codeing styles, the" (optional)" should be part of the next line, see https://drupal.org/node/1354

2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserEntityTest.php
   @@ -39,6 +39,7 @@ public static function getInfo() {
   @@ -72,6 +73,12 @@ public function testUserMethods() {
   
   @@ -72,6 +73,12 @@ public function testUserMethods() {
        $this->assertFalse($user->hasRole('test_role_one'));
        $this->assertTrue($user->hasRole('test_role_two'));
        $this->assertEqual(array('test_role_two'), $user->getRoles());
   +
   +    $user->addRole('authenticated');
   +    $this->assertTrue($user->hasRole('test_role_two'));
   +    $this->assertTrue($user->hasRole('authenticated'));
   +    $this->assertEqual(array('authenticated', 'test_role_two'), $user->getRoles());
   +    $this->assertEqual(array('test_role_two'), $user->getRoles(TRUE));
      }
   

   It is a bit odd to basically test the same code twice. This is also tested by the UserTest class below but I am fine with that.

Thanks dawehner,
I have made the changes suggested in #51.

Awesome, thank you!

Committed/pushed to 8.x, thanks!

This issue introduced a regression. See follow-up: #2277547: User::getRoles test coverage & regression fix