Posted by Steve Dondley on November 15, 2008 at 8:25am
17 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | user system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Needs tests |
Issue Summary
1) Add a user
2) Save the user
3) Edit the user
4) Assign the user to a new role
5) Save the user
6) Note that user is not assigned to the role
Also:
1) Create a user and add to a role while you create the user
2) Submit the user
3) Notice user is in the role
4) Try to remove the user from the role
5) Note that it doesn't work
chx reported that he gets an error when trying to add a role he received a "You must specify a valid role name." error. I cannot duplicate this, however.
Comments
#1
The problem code seems to be here in user.module:
261 $success = drupal_write_record('users', $edit, 'uid');
262 if (!$success) {
263 // The query failed - better to abort the save than risk further data loss.
264 return FALSE;
265 }
$success returns false for me.
#2
Within the drupal_write_record, the problem is with this if statement:
3578 if ($last_insert_id = $query->execute()) {3579 if (isset($serial)) {
3580 // Populate the serial field.
3581 $object->$serial = $last_insert_id;
3582 }
3583 }
3584 else {
3585 $return = FALSE;
3586 }
Line 3578 evaluates to 0, causing the function to return FALSE.
#3
OK, looks like the fault lies with the user_save() making the false assumption that it will only be handling the submission of new users. If lines 261 through 265 are commented out, everything works fine.
The comment on line 263 suggests that the if statement is there to protect from "data loss". Fine, but it seems to me that this kind of logic should be done at the database abstraction layer not at the module layer. I think these lines can be safely removed. Patch attached.
#4
The last submitted patch failed testing.
#5
I pinpointed the patch that created the code that I suggest we remove from user.module:
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/user/user.module?r...
Note that this check to prevent data loss was added before user.module began using the new and improved database abstraction layer. So it did not expect that a "0" would indicate a successful UPDATE query.
What's important to note here is that the patch in question added similar code elsewhere in the user module. Someone has to take a close look at this code and make sure they have been completely removed. Also, the rest of Drupal core should be examined, too.
#6
The last submitted patch failed testing.
#7
Rolled from Drupal root this time.
#8
And here is the thread for the patch submitted back in January: http://drupal.org/node/204705
Pay close attention to comment #23 by Gabor: http://drupal.org/node/204705#comment-686242 and to the previous comments about how they are adding this code only as a temp measure until d7 comes out.
So it seems like a safe bet that the patch generated by that thread (written for drupal 6) should be reversed. I'm rolling a new patch to reverse all that code.
#9
I don't think the @return for user_save() needs to be returned. Also this needs a test. Nice find!
#10
Can we write a SimpleTest for this please? Thanks.
#11
The last submitted patch failed testing.
#12
I've tried writing a test but I'm hitting a major roadblock with drupalPost, apparently. See http://drupal.org/node/335035
I have no idea how to get the test to work, though I've tried a great deal. chx, or somebody with greater knowledge of form api and the test modules, are going to step in here.
#13
I think something is wrong with the test bot. I applied the patch just fine to HEAD, albeit with some offset:
patching file modules/user/user.moduleHunk #1 succeeded at 139 (offset -2 lines).
Hunk #2 succeeded at 253 (offset -2 lines).
Hunk #3 succeeded at 301 (offset -2 lines).
Hunk #4 succeeded at 1401 (offset -2 lines).
Hunk #5 succeeded at 2270 (offset -1 lines).
#14
Uploading again....
#15
The last submitted patch failed testing.
#16
@steve, HEAD installation is broken, so don't pay any heed to the test bot.
#17
Trying to resubmit.
#18
Here's a test. It adds and deletes roles and adds and removes users from roles.
#19
New test patch with commented out debug code removed.
#20
A little bit more work on comment clean up.
#21
Patch/test summary:
1) Code was inserted into user.module back in January to d6 as a temporary solution until d7 was released. See http://drupal.org/node/204705. It was intended that the code should be removed upon the introduction of drupal_write_record. Evidence for this is clear in comment #23 in the thread at http://drupal.org/node/204705#comment-686242
2) The code that was inserted in January was written for d6, using the old db api. At some point, someone replaced the old db api for the code written in January. However, they did not consider that a return value of '0' is a valid for an UPDATE query with drupal_write_record. This was the root cause of the problem
3) This patch reverses the code logic added back in January. It keeps the newer drupal_write_record() function call.
4) Another, fresher, patch is attached.
5) A test has been included. See comment #20
#22
Well the tests pass and the patch makes sense.
#23
The test patch fails (comment #20) because the patch in comment #21 has not been applied yet.
#24
Would you mind rolling them into a single patch? Uploading just the test patch is a great way to demonstrate the bug in HEAD, I'll remember that :)
#25
I applied both patches and the tests still failed.
#26
This test is dependant on a fix to Simpletest itself: #335035: drupalPost() incorrectly submits input for disabled elements
Postponed until the fix is in.
#27
Subscribing.
#28
this is a rather annoying HEAD bug that has been lingering for a long time -- can we please break this issue up into a patch that fixes the problem, and a patch for the test ?
i don't see a need to hold up the actual fix just because simpletest is busted...
#29
I agree. The bug fix is ready in #21. The simpletest fix is apparently complicated and need not delay.
#30
The patch doesn't apply anymore.
#31
I consider it drupal_write_record()'s fault for not handling its return value right. Looking at the code, it seems unintentional that it returns FALSE in this situation:
<?php
...
else {
$query = db_update($table)->fields($fields);
foreach ($primary_keys as $key) {
$query->condition($key, $object->$key);
}
$return = SAVED_UPDATED;
}
// Execute the SQL.
if ($last_insert_id = $query->execute()) {
if (isset($serial)) {
// If the database was not told to return the last insert id, it will be
// because we already know it.
if (isset($options) && $options['return'] != Database::RETURN_INSERT_ID) {
$object->$serial = $fields[$serial];
}
else {
$object->$serial = $last_insert_id;
}
}
}
// If we have a single-field primary key but got no insert ID, the
// query failed.
elseif (count($primary_keys) == 1) {
$return = FALSE;
}
?>
After a call to db_update(), it doesn't make sense to check for an insert ID or count the number of primary keys. The "insert ID" is instead the affected row count--and the part that overwrites SAVED_UPDATED to FALSE is trying to detect a failed INSERT.
Changing that last part to:
- // If we have a single-field primary key but got no insert ID, the- // query failed.
- elseif (count($primary_keys) == 1) {
+ // Return FALSE if we expected an insert ID and it was not returned.
+ elseif (count($primary_keys) == 1 && isset($options['return']) && $options['return'] == Database::RETURN_INSERT_ID) {
$return = FALSE;
}
would seem to make more sense. This would make drupal_write_record() reliably return SAVED_UPDATED for updates rather than being hit-or-miss, since detecting if a db_update() failed isn't possible (as far as I can tell?) with the new DB API.
#32
The problem with drupal_write_record() has been fixed, which now fixes this issue as well. See #626790: drupal_write_record returns FALSE for valid update queries
#33
I apologize, I didn't notice that this still needs a test. If comment #26 is correct, the test apparently depends on #335035: drupalPost() incorrectly submits input for disabled elements which has been stalled for months. Is there a way we can make the test not depend on the SimpleTest fixes?
#34
Re-test of from comment #1111438 was requested by @user.
#38
The last submitted patch, user_334671-4.patch, failed testing.
#39
I wrote at test for user roles. I found a small error in
user_role_load(). It was usingis_int()on a number that can also be a string because it came from the url.The error appears when trying to delete a role (that is not possible right now because of that). It is a small one line fix.
The tests still rely on #335035: drupalPost() incorrectly submits input for disabled elements being committed.
#40
The last submitted patch, 334671.patch, failed testing.
#41
#39: 334671.patch queued for re-testing.
#42
The last submitted patch, 334671.patch, failed testing.
#43
Reroll
#44
Another reroll. The other one certainly did not pass tests.
#45
The last submitted patch, 334671_2.patch, failed testing.
#46
Weird. The patch does not fail testing on my machine. Sorry testbot, I am making you try again.
#47
#44: 334671_2.patch queued for re-testing.
#48
The last submitted patch, 334671_2.patch, failed testing.
#49
OK, maybe the xpath likes
a[containsbetter thana[starts-with. Both still works on my machine, but lets see what the test here says.#50
The last submitted patch, 334671_3.patch, failed testing.
#51
OK, test here with no xpath in it. Weird, but let's see if this will pass.
#52
- $field = is_int($role) ? 'rid' : 'name';+ $field = is_numeric($role) ? 'rid' : 'name';
This isn't correct, unfortunately - see #619584: Deleting user role throws PHP notices and prevents delete operation for the full discussion of how to fix that bug. For that reason, I'd suggest removing that line as well as the testAddRole() test from this patch, since it's being fixed over at that other issue.
***
The other tests look pretty good on a quick glance. However, in addition to asserting that the correct text appears on the screen and that the correct role checkboxes are checked, the most important thing that needs to be asserted here is that if we actually load the user account object, $account->roles contains the correct role list - and the current patch does not do that.
+ function testCreateUserAndAssignRole() {Shouldn't this really be named something more like testAssignAndRemoveRole()?
+ $user = $this->drupalCreateUser();Normally we don't user $user here so as not invite confusion with the global $user object. I've seen $web_user used in the tests in this case, or even just $account should be fine.
+ $user2 = user_load_by_name($edit['name']);No need to use a different variable name here; since it's in a different test, you can just use whatever same variable name was chosen above.
#53
Reroll after #619584: Deleting user role throws PHP notices and prevents delete operation was committed. The test now checks that the user object has the role too - that makes a lot of sense - thanks David :)
#54
The last submitted patch, 334671_5.patch, failed testing.
#55
#53: 334671_5.patch queued for re-testing.
#56
#53: 334671_5.patch queued for re-testing.
#57
subscribing
#58
Patch needs reroll against HEAD (can't right now) BUT test is fully functional and code reviewed.
#59
#60
I applied the patch cleanly to HEAD and tests do not fail, so setting back.
#61
#53: 334671_5.patch queued for re-testing.
#62
#53: 334671_5.patch queued for re-testing.
2 months after the roll. Does it still apply?
#63
Committed to CVS HEAD. Thanks.
#64
Automatically closed -- issue fixed for 2 weeks with no activity.