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

Files: 
CommentFileSizeAuthor
#11 drupal_write_record_docs_update-1617006-6199626.patch652 bytesben.kyriakou
PASSED: [[SimpleTest]]: [MySQL] 39,214 pass(es).
[ View ]
#8 drupal_write_record_docs_update-1617006-6192734.patch677 bytesben.kyriakou
PASSED: [[SimpleTest]]: [MySQL] 37,063 pass(es).
[ View ]
#1 1617006-drupal_write_record_docs_update.patch761 bytesben.kyriakou
PASSED: [[SimpleTest]]: [MySQL] 37,019 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new761 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,019 pass(es).
[ View ]

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

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?

"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?

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...”?

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.

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.

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.

StatusFileSize
new677 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,063 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

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!

Status:Patch (to be ported)» Needs review
StatusFileSize
new652 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,214 pass(es).
[ View ]

Patch for common.inc in 7.x attached.

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Fixed

Committed to 7.x - thanks again!

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