drupal_write_record() is doing something like this:

if( something) {
   $query = db_insert($table, $options)->fields($fields);
}
else {
   $query = db_update($table)->fields($fields);
}
// ...
  if ($last_insert_id = $query->execute()) {

The return value of db_update()->execute() is NOT the last ID, it's the number of rows affected. It's possible that this code works, but it sure looks dangerous/wrong. Needs fixing IMO.

CommentFileSizeAuthor
#8 719468.patch3.4 KBjhodgdon

Comments

Crell’s picture

Crell commented

Subscribe. I don't know what this code is doing either, but it doesn't look good. (But then, I've always hated drupal_write_record(), especially now that we have DBTNG.)

mcarbone’s picture

mcarbone commented
Category: bug » task

I think this is just a case of poor variable naming and/or a confusing control structure. Below the bit you quote above, there seems to be awareness that that variable is over-loaded with the result of a possible db_insert or db_update:

 // If we have a single-field primary key but got no insert ID, the
  // query failed. Note that we explicitly check for FALSE, because
  // a valid update query which doesn't change any values will return
  // zero (0) affected rows.
  elseif ($last_insert_id === FALSE && count($primary_keys) == 1) {
    $return = FALSE;
  }

So I don't think this is a bug, but it's definitely worth fixing. Since drupal_write_record is called 26 times at the moment, I'd be suprised if something was actually broken and not being found in other tests.

I wonder, however: should this function be rewritten to use db_merge?

mcarbone’s picture

mcarbone commented

Just found the thread where this bit of code was added, and it also happens to show that moshe deliberately decided not to use db_merge for this, though not with any explanation: #299088: [DBTNG]: simplify drupal_write_record() with db_insert() and db_update()

Crell’s picture

Crell commented

Could it perhaps be related to #715108: Make Merge queries more consistent and robust?

carlos8f’s picture

carlos8f commented

Yeah, I think drupal_write_record() was sloppily ported to DBTNG, and that caused #626790: drupal_write_record returns FALSE for valid update queries due to the return value confusion. The code currently works, but could be cleaned up a bit. Namely, don't call the variable "last insert id" if it's an update :)

Crell’s picture

Crell commented

We should go a step farther and just remove that abomination of a function in Drupal 8. :-) For now, though, cleaning it up to at least read clearer and be better documented internally would be good.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: drupal_write_record() is doing some weird stuff with db_update->execute() return value » drupal_write_record() code is written in a confusing way
Category: task » bug
Priority: Normal » Minor
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 719468.patch3.4 KB

Here's a patch. I couldn't resist clarifying the function header doc a bit too.

Crell’s picture

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

Looks good to me. Really it's just a doc change, even though one of those docs is a variable name. :-)

dries’s picture

dries commented

#8: 719468.patch queued for re-testing.

dries’s picture

dries commented

Asked for a re-test because of another change that was committed to drupal_write_record() in #353918: drupal_write_record writes empty string instead of empty serialized array. Haven't reviewed this patch yet.

Crell’s picture

Crell commented

Looks like it's still passing.

dries’s picture

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

Great. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

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