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() :

  // 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;
  }
CommentFileSizeAuthor
#18 626790_fix.patch498 bytesvenutip
#13 drupal_write_record_return_values_fix.patch809 bytesvenutip
#8 drupal_write_record.patch2.03 KBAnonymous (not verified)
#6 drupal_write_record.patch2.04 KBAnonymous (not verified)
#5 drupal_write_record.patch785 bytesAnonymous (not verified)
#1 drupal-626790.patch1.57 KBPeter Törnstrand
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Peter Törnstrand’s picture

Peter Törnstrand CreditAttribution: Peter Törnstrand commented
Status: Active » Needs review
FileSize
drupal-626790.patch1.57 KB

Path attached. Please take note this is my first patch ever. Hope I did it right.

Peter Törnstrand’s picture

Peter Törnstrand CreditAttribution: Peter Törnstrand commented
Status: Needs review » Active

Please note that above patch relates to #623460: SQlite Database Issues... I just posted in the wrong thread.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: Unassigned »
Priority: Normal » Critical

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Title: Can't update user account twice » drupal_write_record returns FALSE for valid update queries
Status: Active » Needs review
FileSize
drupal_write_record.patch785 bytes

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
drupal_write_record.patch2.04 KB

updated patch with a test.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community 
+++ 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?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
drupal_write_record.patch2.03 KB

thanks for the quick review, whitespace removed in updated patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD! Thanks!

bilxxa: Welcome to the core development team! :D

carlos8f’s picture

carlos8f CreditAttribution: carlos8f commented

Cheers, this is an issue I've been trying to get fixed for the last month :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)

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.

venutip’s picture

venutip CreditAttribution: venutip commented
Status: Patch (to be ported) » Needs review
FileSize
drupal_write_record_return_values_fix.patch809 bytes

Cool, here's a patch for D6.

Status: Needs review » Needs work

The last submitted patch, drupal_write_record_return_values_fix.patch, failed testing.

IntoTheWoods’s picture

IntoTheWoods CreditAttribution: IntoTheWoods commented
Version: 6.x-dev » 7.x-dev

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,

$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.

IntoTheWoods’s picture

IntoTheWoods CreditAttribution: IntoTheWoods commented
Status: Needs work » Active
marcingy’s picture

marcingy CreditAttribution: marcingy commented
Version: 7.x-dev » 6.x-dev

This is fixed in d7

venutip’s picture

venutip CreditAttribution: venutip commented
Status: Active » Needs review
FileSize
626790_fix.patch498 bytes

@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.

Status: Needs review » Needs work

The last submitted patch, 626790_fix.patch, failed testing.

ethanw’s picture

ethanw CreditAttribution: ethanw commented
Status: Needs work » Needs review

#18: 626790_fix.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 626790_fix.patch, failed testing.

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented

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.

venutip’s picture

venutip CreditAttribution: venutip commented

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.git

Do you know if that is correct? I got that line from this page: http://drupal.org/node/3060/git-instructions/6.x

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented

Yes, that is the correct command line. Could you please try again to submit your patch?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: » Unassigned
brad.bulger’s picture

brad.bulger CreditAttribution: brad.bulger commented
Version: 6.x-dev » 7.x-dev

can someone confirm that in fact this will not be implemented, in 6.x or 7.x or ever? i'm pretty sure that there was a decision that drupal_write_record() will always return SAVED_UPDATED if there was no actual error, regardless of how many rows were or were not affected. that's the documented behavior and the way the current code works.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Version: 7.x-dev » 6.x-dev

#26: This is not a bug in D7, but apparently there is still a bug in D6, so we should leave the issue filed against D6.

Regarding your question, I am confused. It seems you are asking if an existing bug will "never be implemented" as if it were a feature request?

brad.bulger’s picture

brad.bulger CreditAttribution: brad.bulger commented

i am asking if it is true that the patch in #18 will never be applied.

there's some confusion because about two-thirds of the way through this thread it inverts.

in #15 IntoTheWoods points out that the 7.x code returns SAVED_UPDATED even when no rows are actually changed. in #17 marcingy apparently replies to that comment by saying, "This is fixed in d7".

i don't know exactly what was meant by that, but as far as i can tell, it is not true that the behavior pointed out in #15 is considered to be a bug. it is by intent that updates that affect no rows still return SAVED_UPDATED, not zero or FALSE.

but in #18, the implication is that the behavior described in #15 will be corrected or changed in some way. and in fact, the patch in that comment does return FALSE if no rows were affected.

the point is, to get confirmation that the current behavior of drupal_write_record() in both 6.x and 7.x is correct and will not be changed; that updates that do not cause an error will return SAVED_UPDATED even if no rows are changed.

if that's true, then i think that closes this issue.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Critical » Major

This issue is a bug report that indicates that currently, in Drupal 6 only, drupal_write_record() does not always return SAVED_UPDATED, and this issue is about fixing that problem. The patch in #18 is not correct, but the issue is marked needs work, which means that no patch in the issue is considered to be a potential solution.

Downgrading to major as per our issue priority guidelines.

brad.bulger’s picture

brad.bulger CreditAttribution: brad.bulger commented

the bug report was filed against 7.x not 6.x. at one time that was how 7.x behaved.

it is not true that 6.x drupal_write_record() returns FALSE or zero on a query that updates no rows. there is no bug. the comments about backporting this to 6.x, #12 and #13, actually cause the behavior you're saying this issue is reporting as a bug.

either the current behavior of drupal_write_record() in both 6.x and 7.x - SAVED_RETURNED is returned unless there is an error, regardless of how many rows were changed - is correct or it isn't. every indication is that it is correct and will not change. i was hoping to get a definite statement about that from someone who knew.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

the bug report was filed against 7.x not 6.x. at one time that was how 7.x behaved.

And has since been fixed, which is why this issue is now filed against 6.x.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Postponed (maintainer needs more info)

Postponing until someone confirms whether or not the bug in the original post is currently reproducible based on #30.

Status: Postponed (maintainer needs more info) » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.