A call to user_save to update roles will insert wrong values into the users_roles table. Here is an example:

Before call:

mysql> select * from users_roles where uid = 37;
+-----+-----+
| uid | rid |
+-----+-----+
|  37 |   2 |
|  37 |   5 |
+-----+-----+
2 rows in set (0.01 sec)

The user_save call:

$user = user_save($user, array('roles' => array(DRUPAL_AUTHENTICATED_RID, 4)));

After call:

mysql> select * from users_roles where uid = 37;
+-----+-----+
| uid | rid |
+-----+-----+
|  37 |   0 |
+-----+-----+
1 row in set (0.01 sec)

Attached is a patch that will fix this problem.

CommentFileSizeAuthor
#24 user.module_1.630_ajk011.83 KBAjK
#23 user.patch_7.txt1.82 KBnaudefj
#22 user.patch_6.txt1.6 KBnaudefj
#10 user.patch_5.txt3.77 KBnaudefj
#6 user.patch_4.txt690 bytesnaudefj
user.patch_3.txt552 bytesnaudefj

Comments

chx’s picture

chx commented
Status: Active » Closed (works as designed)

See sesssion.inc for proper structure of roles array.

    $result = db_query("SELECT r.rid, r.name FROM {role} r INNER JOIN {users_roles} ur ON ur.rid = r.rid WHERE ur.uid =
 %d", $user->uid);
    while ($role = db_fetch_object($result)) {
      $user->roles[$role->rid] = $role->name;
    }

or user_save itself foreach (array_keys($array['roles']) as $rid) {

naudefj’s picture

naudefj commented
Priority: Critical » Normal

Thanks - problem solved. The call should be:

$user = user_save($user, array('roles' => array(4=>'Any crappy text that will be ignored')));

The strange thing is that user_save will insert a value (2) for role DRUPAL_AUTHENTICATED_RID when a user is created, but when a users is updated, it will filter it out again. Is it still necessary to specify this?

naudefj’s picture

naudefj commented
Priority: Normal » Critical
Status: Closed (works as designed) » Active

Now I have a problem creating users. The user_save API call is inconsistent:

Existing users:

foreach (array_keys($array['roles']) as $rid) {

so I need to code

$user = user_save($user, array('roles' => array(4=>'Any crappy text that will be ignored')));

New users:

foreach ((array)$array['roles'] as $rid) {

So I need to code

$user = user_save($user, array('roles' => array(4)));

chx’s picture

chx commented
Priority: Critical » Normal

No way a programming problem can be critical. With that said, I will inspect your problem now.

chx’s picture

chx commented
Title: user_save doesn't save roles correctly » unify user_save $user->roles handling
Version: 4.7.2 » x.y.z
Category: bug » feature

Yes, this should be sorted out but API changes are not for 4.7. You want an array_keys in the insert section of user_save. As new users have no roles (authenticated is added automatically on the fly) the change likely won't break core. You want to inspect admin adding users with roles pre-checked.

naudefj’s picture

naudefj commented
Status: Active » Needs review
StatusFileSize
new user.patch_4.txt690 bytes

Looks like admins cannot directly add users with roles. One must first create the user (without roles); then edit the user to set the roles. This explains the oversight and inconsistent API.

Attached is a new patch. Can you please review it?

chx’s picture

chx commented
Title: unify user_save $user->roles handling » Make it possible to add roles on user adding

http://drupal.org/patch/review says

No patch can save the world, not even a Drupal core patch. If it works and does something useful on its own, then it is good to go.

. But for this patch, which actually fixes a minor API glitch, there is no use case so it does not do anything useful on its own. It would be a _very_ cool patch if you could add the roles block to the user add screen for admins.

dries’s picture

dries commented

So with this patch, do we still need to fix the API?

naudefj’s picture

naudefj commented

Hi Dries,

Both the first and second patch will fix the API, or at least make the user_save() function consistent.

Personally I don't care if we use array_values or array_keys to identify RID's. However, Chx wants us to use keys, hence the second patch.

I will prepare a third patch later today to add a roles block to the "user add" screen for admins. That will allow us to test the patched code without a non-core module (that code sections is currently not used by core).

Best regards.

Frank

naudefj’s picture

naudefj commented
StatusFileSize
new user.patch_5.txt3.77 KB

Here is the updated patch. Please review and provide feeback ASAP.

chx’s picture

chx commented

While the patch looks good, we have arrived to the point where we have an almost duplicate of the edit form. So while this is good to go, either in this patch or after commit, we really should unify the two.

dries’s picture

dries commented

Unification is worth investigating. Depends on whether naudefj wants to work on this some more? naudefj?

naudefj’s picture

naudefj commented

Hi Dries,

I feel I've already walked the extra mile by providing a "use case" in core as requested by Chx's. Unification might be worth investigating (though it doesn't look to bad to me). However, I feel that someone else needs to take care of that. I would appreciate if this patch can be committed ASAP.

Best regards.

Frank

naudefj’s picture

naudefj commented
Status: Needs review » Reviewed & tested by the community

Please commit this patch ASAP.

PS: If the one extra field that is duplicated is botherting people I can prepare a patch without it.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Category: feature » bug
Priority: Normal » Critical
Status: Fixed » Active

This patch broke HEAD.

When I create uid1 I get:

# warning: array_filter() [function.array-filter]: The first argument should be an array in user.module on line 1239.
# warning: array_keys() [function.array-keys]: The first argument should be an array in user.module on line 209.
# warning: Invalid argument supplied for foreach() in user.module on line 209.

1239 is:

$roles = array_filter($form_values['roles']); // Remove unset roles

209 is:

foreach (array_keys($array['roles']) as $rid) {

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Make it possible to add roles on user adding » Fatal errors when creation uid 1 in HEAD
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Fatal errors when creation uid 1 in HEAD » Fatal errors when creating uid 1 in HEAD

typo :P

changed title per drumm, to make it more descriptive of the current problem.

naudefj’s picture

naudefj commented

I've applied this patch on my production site and I can create users. However, I haven't tried to create UID 1.

How did you create this user? Via administer->users? Why was it necessary to recreate it?

heine’s picture

heine commented

Creating the first uid is a necessary step of the drupal installation procedure.

naudefj’s picture

naudefj commented

Looks like everyting is working as expected. However, PHP warnings are written out if no roles are set. I will upload a fix later today.

PS: UID 0 is created during installation, UID 1 is the first site user.

naudefj’s picture

naudefj commented
Status: Active » Needs review
StatusFileSize
new user.patch_6.txt1.6 KB

The attached patch should fix the warnings. Please apply it and provide feedback ASAP.

naudefj’s picture

naudefj commented
StatusFileSize
new user.patch_7.txt1.82 KB

Here is the same patch against CVS HEAD.

AjK’s picture

AjK commented
StatusFileSize
new user.module_1.630_ajk011.83 KB

naudefj,

I was able to reproduce this for any new user account, not just UID 1.

I was unable to get your patch against r1.630 to work? I patched it manually and it appears to work fine. I have attached a cvs diff from my manual work just in case.

best regards
--AjK

heine’s picture

heine commented

The issue occurred for every new user when no additional roles had been defined. Patch solved the issue but had to be applied with -p1.

heine’s picture

heine commented

Pardon, patch in #23 solved the issue, #24 the -p1

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Fatal errors when creating uid 1 in HEAD » PHP warnings when creating users

Changing to a more descriptive title.

Is it just me or do the changes that were introduced by this patch being committed conflict with those introduced by http://drupal.org/node/44379? It's late, i might not be reading this correctly.

naudefj’s picture

naudefj commented
Status: Needs review » Reviewed & tested by the community

Thanks for the reviews.

PS: 44379 looks completely unrelated to me.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)