If a test case calls drupalCreateUser() (with empty argument list) then the user will be created with roles 1 and 2 (anonymous and authenticated). The problem is this line:

    $account = user_save(drupal_anonymous_user(), $edit);

First, drupal_anonymous_user() adds the anonymous role; then user_save() adds the authenticated role.

I added a line to the code in the Content Access module:

    $this->test_user = $this->drupalCreateUser();
    debug('test_user has roles ' . print_r($this->test_user->roles, TRUE));

Here are the results:'test_user has roles Array ( [1] => anonymous user [2] => authenticated user ) '

I have not yet tested in D8.

If you agree that this is a bug, then I will supply a patch.

CommentFileSizeAuthor
#6 user-test-1873606-6-do-not-test.patch1.34 KBbenjifisher
#4 create-user-minimal-1873606-3.patch761 bytesbenjifisher
#4 create-user-1873606-3.patch741 bytesbenjifisher
#4 user-save-1873606-3.patch511 bytesbenjifisher
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Version: 7.18 » 7.x-dev

I think you're right.

I looked at Drupal 8 a bit and while I didn't test it out, I don't think it has the same issue since drupalCreateUser() does not start off saving an anonymous user object like it does in Drupal 7.

I'm kind of curious what fixing this would break (whether anything actually relies on the current set of roles)... but it does seem pretty broken.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Assigned: Unassigned » benjifisher

I am working on a few patches. Just to reduce my cognitive load, let me put down some notes.

The commit that changed this for D8 is 93c20fd. The corresponding issue is #1361228: Make the user entity a classed object. (The issue does not refer to the commit. OK, I fixed that. The commit does refer to the issue, but the automatically generated link does not go there.)

I am not sure what the standards are for change records. I did search the change records (first the ones in the issue summary, then all of them) for "drupalCreateUser", and I did not find any mention of this change. (Only one change record matched.) Should someone add a change record for the new behavior of drupalCreateUser() in D8?

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

I'm guessing there wasn't one because it wasn't intended to be a new behavior (presumably no one knew about this bug)...

Probably this falls more into the category of things that aren't worth a change notification anyway, I would think. Especially if we wind up fixing it in Drupal 7 too.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
Assigned: benjifisher » Unassigned
Status: Active » Needs review
FileSize
user-save-1873606-3.patch511 bytes
create-user-1873606-3.patch741 bytes
create-user-minimal-1873606-3.patch761 bytes

I'm kind of curious what fixing this would break

I am setting the status to NR, so the testbot will give us some answers.

The first patch is the minimal required to fix the problem.

The second patch is a little cleaner.

The third patch fixes user_save(). It should never create a user with the anonymous role. This bug is already fixed, I think, in D8: both parts of the big if statement in UserStorageController::postSave() use these lines to make sure that they do not save the anonymous rid to the databse:

        $query = db_insert('users_roles')->fields(array('uid', 'rid'));
        foreach (array_keys($entity->roles) as $rid) {
          if (!in_array($rid, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
            $query->values(array(
              'uid' => $entity->uid,
              'rid' => $rid,
            ));
          }
        }
        $query->execute();
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented

Oh, no: cross post, so my patch file names are off!

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher commented
FileSize
user-test-1873606-6-do-not-test.patch1.34 KB

The attached patch adds a test to the User module to check that users created with drupalCreateUser() do not have the Anonymous rid. It does not include any of the patches from #4, so it fails (the test user has the anonymous role) with an unpatched D7. Running the test through the UI, I got the expected results: the test passes after applying any of the patches from #4 fixes it.

For some reason, I could not get this to work with drush:

$ drush sa @self
$aliases['self'] = array (
  'root' => '/Users/benji/Sites/drupal7',
  'uri' => 'http://d7test.local',
);
$ drush @self test-run UserRolesAssignmentTestCase 
Role assignment 53 passes, 0 fails, 0 exceptions, and 13 debug       [ok]
messages
No leftover tables to remove.                                        [status]
No temporary directories to remove.                                  [status]
Removed 1 test result.                                               [status]
$

It is not even the right number of tests. Probably I am doing something dumb, but this is why I am not pasting in the results from "drush test-run".