Posted by Amitaibu on November 8, 2009 at 1:46pm
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | user system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
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;
}
?>
Comments
#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.
#12
Needs to be backported to D6. Marked #602190: drupal_write_record() unable to determine if changes have been made. as a duplicate since this thread contains the patch that went into D7.
#13
Cool, here's a patch for D6.
#14
The last submitted patch, drupal_write_record_return_values_fix.patch, failed testing.
#15
I believe this patch causes well-formed update queries that don't update the database because the row doesn't exist to return SAVED_UPDATED. For example,
<?php$table = 'node';
$record->nid='99999999';
$record->promote=1;
$key = 'nid';
$updated = drupal_write_record($table, $record, $key);
drupal_set_message($updated);
?>
returns SAVED_UPDATED even when node 99999999 does not exist.
#16
#17
This is fixed in d7
#18
@IntoTheWoods, not sure which patch you're referring to, but patch #13 does fix the issue you brought up. It failed testing for some reason so I'm rerolling here.
#19
The last submitted patch, 626790_fix.patch, failed testing.
#20
#18: 626790_fix.patch queued for re-testing.
#21
The last submitted patch, 626790_fix.patch, failed testing.
#22
If you click in "view details" you will seee the patch failed for this reason: "Ensure the patch applies to the tip of the chosen the code-base."
Meaning your patch wasn't against the latest 6.x-dev codebase.
You should also fix the patch filename
http://drupal.org/node/1054616
[description]-[issue-number]-[comment-number].patch.
#23
Hi dpearceMN,
As far as I know, I did check out the most current version of the codebase. It was a while back, but I'm pretty sure I used the following command:
git clone --branch 6.x http://git.drupal.org/project/drupal.gitDo you know if that is correct? I got that line from this page: http://drupal.org/node/3060/git-instructions/6.x
#24
Yes, that is the correct command line. Could you please try again to submit your patch?
#25