When using the "update" mode of drupal_write_record(), and your primary keys are not serials, the query that is generated and executed updates the columns that are being used to select the row! Silly Example that updates node.title based on "created":

$object = array('created' => 123, 'title' => 'new title');
drupal_write_record('node', $object, 'created');

This actually executed:

UPDATE {node} SET title = '%s', created = %d WHERE created = %d

with query arguments of: 'new title', 123, 123.

It should really be executing

UPDATE {node} SET title = '%s' WHERE created = %d

with query arguments of: 'new title', 123.

Files: 
CommentFileSizeAuthor
#40 drupal_write_record-update.patch1.38 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 22,753 pass(es). View
#36 drupal_write_record.update.patch1.38 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 20,937 pass(es). View
#35 drupal_write_record.update.patch1.38 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 20,929 pass(es). View
#27 update_keys.patch3.96 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 20,853 pass(es). View
#19 update_keys.patch4.75 KBjbrown
FAILED: [[SimpleTest]]: [MySQL] 20,628 pass(es), 44 fail(s), and 35 exception(es). View
#21 update_keys.patch3.89 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 20,825 pass(es). View
#15 update_keys.patch3.67 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 20,711 pass(es). View
#13 update_keys.patch3.81 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 20,721 pass(es). View
#11 drupal_write_record.noupdate-primarykey.patch1.36 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 20,719 pass(es). View
#9 drupal_write_record.noupdate-primarykey.patch1.45 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 20,716 pass(es). View
#7 drupal_write_record.noupdate-primarykey.patch717 byteslyricnz
FAILED: [[SimpleTest]]: [MySQL] 20,680 pass(es), 0 fail(s), and 13 exception(es). View
#2 drupal_write_record.update.patch686 byteslyricnz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_write_record.update.patch. View

Comments

lyricnz’s picture

PS: there's no need for the third argument to drupal_write_record to actually be primary keys: it's just used to build the WHERE part of the insert/update.

lyricnz’s picture

Version: 6.16 » 6.x-dev
Component: file system » database system
FileSize
686 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_write_record.update.patch. View

Here's a patch that prevents fields in the "primary key" being included in the update fields.

lyricnz’s picture

Status: Active » Needs review

CNR

lyricnz’s picture

Please review.

Damien Tournoud’s picture

Version: 6.x-dev » 7.x-dev

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

lyricnz’s picture

Bug is still present in D7:

UPDATE {node} SET title=:db_update_placeholder_0, created=:db_update_placeholder_1
WHERE  (created = :db_condition_placeholder_0) 
lyricnz’s picture

FileSize
717 bytes
FAILED: [[SimpleTest]]: [MySQL] 20,680 pass(es), 0 fail(s), and 13 exception(es). View

Patch for D7 attached.

Status: Needs review » Needs work

The last submitted patch, drupal_write_record.noupdate-primarykey.patch, failed testing.

lyricnz’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
PASSED: [[SimpleTest]]: [MySQL] 20,716 pass(es). View

Update patch to fix bug in includes/path.inc where it passes NULL to drupal_write_record()

Berdir’s picture

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?

lyricnz’s picture

FileSize
1.36 KB
PASSED: [[SimpleTest]]: [MySQL] 20,719 pass(es). View

The argument I replaced in path.inc was "NULL", which should not be passed as third argument to drupal_write_record(). The third argument to that function lists the "primary key" (used for updates) - when it is missing/array(), it's actually an insert.

Yes, I had just noticed the debug code - removed. Thanks.

lyricnz’s picture

Wahoo. Would you consider that RTBC now, Berdir?

jbrown’s picture

Title: drupal_write_record modifies the primary keys while updating » drupal_write_record() modifies non-serial update keys while updating
FileSize
3.81 KB
PASSED: [[SimpleTest]]: [MySQL] 20,721 pass(es). View

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

Damien Tournoud’s picture

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

jbrown’s picture

Title: drupal_write_record() modifies non-serial update keys while updating » Rename $primary_keys to $update_keys in drupal_write_record()
Category: bug » task
FileSize
3.67 KB
PASSED: [[SimpleTest]]: [MySQL] 20,711 pass(es). View

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.

jbrown’s picture

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.

jbrown’s picture

Title: Rename $primary_keys to $update_keys in drupal_write_record() » drupal_write_record() modifies update keys while updating
Category: task » bug
lyricnz’s picture

How is this "in a cleaner manner"? All you did was rename the argument, and remove the comment that explained *why* items are removed from $fields. Actually, I disagree with the parameter name change - "update keys" has no meaning. (Admittedly, "primary key" wasn't quite accurate either - but that's what we're all used to, and it's roughly right).

jbrown’s picture

FileSize
4.75 KB
FAILED: [[SimpleTest]]: [MySQL] 20,628 pass(es), 44 fail(s), and 35 exception(es). View

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

Status: Needs review » Needs work

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

jbrown’s picture

Status: Needs work » Needs review
FileSize
3.89 KB
PASSED: [[SimpleTest]]: [MySQL] 20,825 pass(es). View

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

lyricnz’s picture

Okay, last patch looks fine - though I don't really agree with the parameter name change.

jbrown’s picture

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

lyricnz’s picture

The whole method is kindof ugly, eh? Missing/extra parameter that totally changes what it does? It should be

drupal_create_record()
drupal_update_record()

Or some other semantic equivalent (thinking CRUD above).

jbrown’s picture

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.

neilnz’s picture

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.

jbrown’s picture

Title: drupal_write_record() modifies update keys while updating » drupal_write_record() unnecessarily sets update keys while updating
Priority: Normal » Minor
FileSize
3.96 KB
PASSED: [[SimpleTest]]: [MySQL] 20,853 pass(es). View

#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’s picture

@jbrown #25: agree, too late for D7. I know this function is on Crell's hit-list also :)

@neilnz #26: yes, my example was contrived - to use a table that everyone would understand

@jbrown #27: I think that silently "handling" bad input is a bad idea - this function takes either a string (field name) or an array (of field names). Nothing else. ... [Edit: or maybe NULL, but document it. Definitely not other bad input]

jbrown’s picture

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

lyricnz’s picture

@jbrown Yes. This is a very long issue, for a very minor fix :)

jbrown’s picture

Can you set it RTBC?

Heine’s picture

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?

lyricnz’s picture

Heine: the original issue was simply that drupal_write_record(), when operating in it's "update" mode, also updated the values of the columns name in the select part.

i.e.: UPDATE {node} SET title = '%s', created = %d WHERE created = %d

Somehow, during this thread, it was decided that the name of the third parameter to drupal_write_record() should be changed too. Personally, I'd rather see that change raised as a separate issue (so we can get the original bugfix in, which is not an API change, and the parameter name change can be debated on it's own merits).

jbrown’s picture

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.

lyricnz’s picture

FileSize
1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 20,929 pass(es). View

Edit: cross-post.

Here is the patch from #21 *without* the parameter name change. @jbrown, can you raise another issue please? Thanks for your patience.

lyricnz’s picture

FileSize
1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 20,937 pass(es). View

Repatch - missed one "update" -> "primary" (make sure you catch it in your ticket :)

jbrown’s picture

Status: Needs review » Reviewed & tested by the community

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

jbrown’s picture

lyricnz’s picture

The patches are not the same.

-+ // Skip field if it is an update key as it is unnecessary to update a
++ // Skip field if it is a primary key as it is unnecessary to update a

Thanks for the issue-split. Will you RTBC this one, once it passes automated testing?

Edit: ha, you already did :)

jbrown’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 22,753 pass(es). View

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

jbrown’s picture

Title: drupal_write_record() unnecessarily sets update keys while updating » drupal_write_record() unnecessarily sets primary keys while updating
lyricnz’s picture

Status: Needs review » Reviewed & tested by the community

RTBC on #40 from me.

jbrown’s picture

Great!

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

jbrown’s picture

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

jbrown’s picture

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

lyricnz’s picture

The patch still looks good (and obvious) to me - I hope a D7 maintainer applies it sometime.... :)

jbrown’s picture

Issue tags: +Quick fix

Tagging.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed.

lyricnz’s picture

Yay. Thanks, Dries.

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix

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