drupal_write_record returns FALSE for valid update queries
Amitaibu - November 8, 2009 - 13:46
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | user system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | justinrandell |
| Status: | closed |
Description
1) Create a new user - demo
2) Edit demo, and add "administrator" role. notice that the following is TRUE
// Save changes to the user table.
$success = drupal_write_record('users', $edit, 'uid');3) Again edit demo, and remove "administrator" role - $success == FALSE.
This happens in drupal_write_record() :
<?php
// Execute the SQL.
if ($last_insert_id = $query->execute()) {
// ...
}
// If we have a single-field primary key but got no insert ID, the
// query failed.
elseif (count($primary_keys) == 1) { // <-- It will fail here.
$return = FALSE;
}
?>
#1
Path attached. Please take note this is my first patch ever. Hope I did it right.
#2
Please note that above patch relates to #623460: SQlite Database Issues... I just posted in the wrong thread.
#3
thanks for the report, i can confirm this bug following the instructions in the issue description.
i'll try to write a failing test to isolate the issue.
#4
woo, this is a nice bug. its actually drupal_write_record that is broken for the case where we run a valid update query, and the values you are going to set are the same as existing values. number of effected rows == 0, and the current code treats that as a failure. renaming to reflect this, and bumping to critical.
a broken drupal_write_record is a release blocker i think.
#5
updated title, patch attached, still need to write a test.
would like feedback on a more invasive patch that makes the code more readable. we're using $last_insert_id as a variable name even when running updates, which is ugly, but i'll leave it be if its too late in the cycle for that sort of change.
#6
updated patch with a test.
#7
+++ modules/simpletest/tests/common.test 6 Dec 2009 02:27:41 -0000@@ -1476,6 +1476,12 @@ class DrupalDataApiTest extends DrupalWe
+ // Run an update query where no field values are changed. The database
+ // layer should return zero for number of affected rows, but
Trailing white-space here. Can we quickly remove that before a core committer hits this issue, please?
I'm on crack. Are you, too?
#8
thanks for the quick review, whitespace removed in updated patch.
#9
Committed to HEAD! Thanks!
bilxxa: Welcome to the core development team! :D
#10
Cheers, this is an issue I've been trying to get fixed for the last month :)
#11
Automatically closed -- issue fixed for 2 weeks with no activity.