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.)
Comment | File | Size | Author |
---|---|---|---|
#11 | drupal_write_record_docs_update-1617006-6199626.patch | 652 bytes | ben.kyriakou |
#8 | drupal_write_record_docs_update-1617006-6192734.patch | 677 bytes | ben.kyriakou |
#1 | 1617006-drupal_write_record_docs_update.patch | 761 bytes | ben.kyriakou |
Comments
Comment #1
ben.kyriakou CreditAttribution: ben.kyriakou commentedAttached a patch with documentation changes - hopefully it sufficiently addresses the issue!
Comment #2
jhodgdonThat 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?
Comment #3
michaellenahan CreditAttribution: michaellenahan commented"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?
Comment #4
kalabroThanks 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...”?
Comment #5
jhodgdonRE #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.
Comment #6
ben.kyriakou CreditAttribution: ben.kyriakou commentedI 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;
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.
Comment #7
jhodgdonRE #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.
Comment #8
ben.kyriakou CreditAttribution: ben.kyriakou commentedThat makes much more sense - I've rolled a new patch as above.
Comment #9
jhodgdonThanks! I'll get this version committed. :)
Comment #10
jhodgdonCommitted 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!
Comment #11
ben.kyriakou CreditAttribution: ben.kyriakou commentedPatch for common.inc in 7.x attached.
Comment #12
jhodgdonLooks good, I'll get it committed. Thanks!
Comment #13
jhodgdonCommitted to 7.x - thanks again!