drupal_write_record() unnecessarily sets primary keys while updating

Bugs are fixed in 7.x first, before being considered for backport.

Shouldn't it be array('pid') then? (No, not required, wasn't clear from the patch...)

Also, did you leave the debugging code there for a reason?

I have implemented this in a cleaner manner.

I also renamed the $primary_keys parameter to $update_keys as it better describes what it (now) does.

I have included your fix to path_save() as it misuses drupal_write_record() .

See also

• #755856: Don't insert default values into db in drupal_write_record()
• #772360: drupal_write_record() does not cast empty fields for 'numeric' columns

// Skip field if it is an update key.

This is not the behavior I would expect. I would expect we skip the field if it is an update key *and* it is not present in $object.

Yes - good point!

Fields are skipped anyway for update if they are not provided. It's not possible for drupal_write_record() to determine if an update key is intended to be updated.

After all, it updates other fields in the row regardless if they have changed or not.

This isn't fixable without changing the drupal_write_record()'s parameters. The values of the update keys would have to be specified separately.

I say we don't fix this, as it doesn't really cause a problem.

However, I do like renaming $primary_keys to $update_keys as it better describes what it is for.

No - sorry I am talking out of my ass.

#14 is incorrect. There is no point in updating a value to the value it is in the WHERE condition.

drupal_write_record() simply does not support modifying the value of update_keys.

I'm still happy with my patch in #13.

@lyricnz The modification I made to your patch means that it earlys out of the foreach. There is no point in setting keys just to unset them immediately after.

I have improved the comment.

It isn't necessary to not update serial fields - although you would probably never need to. I removed this if statement. This exposed a bug in the date format code which I fixed.

Seems a lot of core passes values for serial fields expecting them to be ignored...

$primary_keys is plain wrong. drupal_write_record() supports updating based on key(s) that are not primary.

I think $update_keys makes sense because if you want to perform an update, then you need to specify the key(s) of the record you want to update.

Yes - I think you're right, but I don't think it is feasible to make such an API change this late in the D7 cycle.

If you ask me, the example of using drupal_write_record() with 'created' as a key is just plain misuse, as the 'created' field is not a key at all.

drupal_write_record should only ever insert/update one row (hence being write_record and not write_records), although this isn't enforced with a LIMIT 1 or anything. Use db_update() for the more generic case.

Beyond that, I agree it's meaningless to update a field to the value it already has.

Also, it should probably be a bit more forgiving in its parameters, and cast a FALSE or NULL to be equivalent to an empty array, as I'm sure some contrib would think passing NULL as the update key should prevent the update behaviour.

Just my 2c.

#26

@lyricnz stated that his example was 'silly'.

No one has suggested using drupal_write_record() to insert/update more that one row per invocation. I think drupal_write_record() should only be able to use to the primary key(s) as defined by the schema, but this is a D8 discussion.

This issue is really about making very minor changes to this function.

I implemented your recommendation about handling other empty values for $update_keys .

@lyricnz Are you happy with the patch in #21?

Can you set it RTBC?

That drupal_write_record doesn't ENFORCE the use of actual primary keys doesn't mean the API doc should be disregarded. What's the issue here again?

In system_date_format_save() non-primary keys are passed to $primary_keys . This should either be fixed or $primary_keys should become $update_keys.

I would prefer it if system_date_format_save() was fixed.

I can make this a separate issue if required.

No you didn't - these two patches are the same.

Created another issue: #828990: system_date_format_save() passes non-primary keys to drupal_write_record().

Fixed the comment - we are not testing if the field is primary, but rather if it is in $primary_keys.

Great!

@lyricnz - I only just realized that you are Simon Roberts! We have met several times.

#40: drupal_write_record-update.patch queued for re-testing.

#40: drupal_write_record-update.patch queued for re-testing.

Tagging.

Committed.