API page: http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_w...

The drupal_write_record() function will not work correctly during a hook_update_N() function (especially if that update function is altering the table schema). This should be noted in its documentation. (The reason is that the schema may already have been cached, and anyway it's in flux during update functions, so it can't really be relied upon.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ben.kyriakou’s picture

Status: Active » Needs review
FileSize
761 bytes

Attached a patch with documentation changes - hopefully it sufficiently addresses the issue!

jhodgdon’s picture

That looks kind of reasonable... But I'm not sure that the part about "errors will be thrown" is the right thing to say. I think I would just say "may not work as intended". I'm also not sure if I would get into so much detail about the $rebuild flag -- maybe just say that since this function depends on the database table schema, which is cached, it may not work as you expect during an update that is altering the database?

Thoughts?

michaellenahan’s picture

"may not work as intended" - yes this is better than "errors will be thrown".

The comment about the $rebuild flag - this seems useful because it explains *why* the schema may be in an outdated state.

Otherwise, we're just saying that it might be out of date but not giving an explanation ... this seems a little unfair to the person reading the documentation.

What do you think?

kalabro’s picture

Thanks for this issue! I just tried to use drupal_write_record() in update function and there were no errors but update didn't work. db_update() solved the problem. Maybe documentation can be more strict like “It's not recommended to use drupal_write_record() in hook_update_N(), since the schema...”?

jhodgdon’s picture

RE #3 - Within an update function, you probably shouldn't try to rebuild the schema -- you should just not use drupal_write_record, in my opinion, because you don't know if other update functions are in process of running (other tables may not have the fields you think they have). Also, for instance if someone was running a whole set of update functions for your module via the update.php script (like, they haven't run the updates in a while), if they did a schema rebuild, it would get the latest schema from hook_schema, but the database table itself might not yet have all the fields that are added by higher-numbered update functions.

So I think talking about the $rebuild flag is not a great idea -- we shouldn't encourage people to think that there is any fool-proof way to use drupal_write_record in an update function. We should instead encourage them to use update/insert queries directly.

ben.kyriakou’s picture

I agree that it isn't advantageous to encourage flushing of the cache given the case of multiple updates - I've re-worded the paragraph as follows;

 * Avoid using drupal_write_record() within HOOK_update_N() functions, since
 * the schema retrieved by drupal_get_schema() may not match the current table
 * structure if multiple schema changes have occurred between queued updates.
 *
 * Instead, alterations should be made directly using database queries.
 * This will ensure that they are compatible with the expected table schema
 * when the update runs.

I've tried to avoid the specification of database interface functions so that it's equally compatible across Drupal versions. Thoughts? I'm undecided as to whether the final sentence is gratuitous.

jhodgdon’s picture

RE #6 - I think that wording is pretty good... I'd put it all in a single paragraph, though, and I think I'd just leave out the "if multiple schema changes..." clause, as well as the last sentence. Neither one seemed to be giving me useful information that wasn't already covered by the rest. Also note that hook_update_N() should not be written HOOK_update_N() [it needs to match the function name exactly to make links on drupal.org].

Hmmm.

Since the first line of this function doc already says "saves a record based on the database schema", maybe we can just say something like this:

Do not use drupal_write_record() within hook_update_N() functions, since the database schema cannot be relied upon when a user is running a series of updates. Instead, use db_insert() or db_update() to save the record.

ben.kyriakou’s picture

That makes much more sense - I've rolled a new patch as above.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I'll get this version committed. :)

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x. drupal_write_record appears to be in a different file in 7.x (there is no schema.inc), so this needs a port. Thanks!

ben.kyriakou’s picture

Status: Patch (to be ported) » Needs review
FileSize
652 bytes

Patch for common.inc in 7.x attached.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, I'll get it committed. Thanks!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks again!

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