#1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) shows that no-one really gets the point of the _update_7000_foo_bar() functions apart from about three people.

So we need proper documentation explaining why they're there, what they do, how to use them, what to do when a new one needs to be added.

This is the current documentation, which is a bit lacking - http://api.drupal.org/api/drupal/includes--update.inc/group/update-api-6...

We need to get the two critical issues in first before changing the docs (since for a start, those should not be named update-api-6.x-7.x), but I'm opening this as a marker.

Here's a rough example:

function comment_update_8000() {
 foreach (_update_node_get_types_0() as $type) {
    // Do something.
  }
 // Get a list of node types and change the name of a comment_node_type_$type() variable.
}

function node_update_8000() {
  // Change the name of the node_type table to something else.
  db_rename_table('node_type', 'node_types');
}

function comment_update_8010() {
  // Change the name of another comment_node_type_$type() variable.
  // Now comment_update_8000() needs to query the old table name, and
  // comment_update_8010() needs to query the new table name. This means
  // we'd need two _update_7/8/xxxx_node_get_types() functions to handle the
  // two different schema versions.

  foreach (_update_node_get_types_8000() as $type) {
     // Do something else.
   }
}

function node_update_8020() {
  // Rename {node_types}.type to {node_types}.bundle
}

function comment_update_8056() {
  foreach (_update_node_get_types_8020() as $types) {
     // Do something else again.
  }
}


Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Easy example:

function comment_update_8000() {
 // Get a list of node types and change the name of a comment_node_type_$type() variable.
}

function node_update_8000() {
  // Change the name of the node_type table to something else.
}

function comment_update_8010() {
  // Change the name of another comment_node_type_$type() variable.
  // Now comment_update_8000() needs to query the old table name, and
  // comment_update_8010() needs to query the new table name. This means
  // we'd need two _update_7/8/xxxx_node_get_types() functions two handle the
  // two different schema versions.
plach’s picture

I even tried to smell #1 but I'm still not sure I fully grok it :(

(subscribe)

quicksketch’s picture

Component: base system » documentation
Assigned: Unassigned » quicksketch
Priority: Major » Normal

I was one of the ones that misunderstood the system, and after several discussions with catch finally understand why the utility update functions are named the way they are. Since now I'm one of four people that now understand the system, I'll take on this issue.

jhodgdon’s picture

As a note, there are currently no functions listed at http://api.drupal.org/api/drupal/includes--update.inc/group/update-api-6... -- so it looks like there are no functions that are members of that group? That also needs to be fixed -- use @ingroup... there's information on how to do grouping on http://drupal.org/node/1354.

It would probably also be good to make a see also link between that topic and hook_update_N, which seems like it might be related?

And as I'm also one of the people who doesn't have a clue what this is about, I'm glad quicksketch has taken on this issue. Thanks!

catch’s picture

Priority: Normal » Major

Bumping this back to major, see the long and painful discussion at #1164852: Inconsistencies in field language handling.

xjm’s picture

Is quicksketch still working on this? :)

In a sentence (this is catch on IRC):

so if you are thinking of using any CRUD function (or anything that invokes hooks) instead of that, you should create a new function _update_my_function_0()

Also:

I even tried to smell #1 but I'm still not sure I fully grok it :(

+1

catch’s picture

Issue summary: View changes

Updating summary

catch’s picture

jhodgdon’s picture

OK... So what are we trying to do here on this issue?

catch’s picture

In a hurry this morning, but here's new pseudo-code for #1 since this has come up yet again and apparently #1 is impenetrable.

function foo_update_8000() {
  // No updates have been run on the {bar_types} table yet, so
  // this needs to work with the 7.x schema.
  foreach (update_7000_bar_get_types() as $type) {
    // Rename a  variable.
  }
}

function bar_update_8000() {
  // Type and bundle are confusing, so we renamed the table.
  db_rename_table('bar_types', 'bar_bundles');
}

function foo_update_8010() {
   // Since foo_update_8010() is going to run after bar_update_8000()
   // (for whatever reason), it needs to operate on the new schema, not the old
   // one.
   foreach (update_8000_bar_get_types() as $type) {
      // Rename a different variable.
   }
}

function bar_update_8001() {
  // Whoops, db tables are singular when possible.
  db_rename_table('bar_bundles', 'bar_bundle');
}

function foo_update_8036() {
  // This update will run after bar_update_8001().
  foreach (update_bar_get_types_8001() as $type) {
  }
}

function update_7000_bar_get_types() {
  db_query('SELECT * FROM {bar_types}')->fetchAll();
}

function update_8000_bar_get_types() {
  db_query('SELECT * FROM {bar_bundles'})->fetchAll();
} 
function update_8001_bar_get_types() {
  db_query('SELECT * FROM {bar_bundle}')->fetchAll();
jhodgdon’s picture

OK, that's a nice example, but where are we proposing to put this, and what are we trying to document here on this issue? I'm still confused.

catch’s picture

I think we could put something like that in an @code inside the update-api group documentation, which is at http://api.drupal.org/api/drupal/includes%21update.inc/group/update-api-... - this would also need to explain why schema dependent and/or hook-invoking API functions can't be used in updates - i.e why this confusing situation exists at all.

Another example of how vague this is currently is #1439692: Rename field language properties to langcode which just renamed an update_7000_*() function to update_8000_*() despite there not being a schema change.

clemens.tolboom’s picture

I'm missing hook_update_dependencies as that should dictate the order for update_N invocations.

I think I grok that each update_8000 must contain the full monty from upgrading a D7 site into D8 site?

xjm’s picture

Alright, after reading #9 a couple times I think I finally am starting to understand the problem space.

  • Caution is needed when using API functions in update hooks.
  • API functions that use the database schema or run queries are not reliable in update hooks because (choose the correct answer!):
    1. The new database schema has not been installed yet? or is it just
    2. Any query, regardless of whether it uses the schema as defined in hook_schema() or simply writes a plain select query, doesn't necessarily match the current database structure at that point during the upgrade?
  • API functions that invoke hooks are not reliable for similar reasons, because we have no idea whether or not any hook implementations try to run queries.

Is this correct?

catch’s picture

Yes #13 is dead on. the update_xxx_*() functions deal with #1, and hook_update_dependencies() deal with #2 - we should probably add hook_update_dependencies() to the example here too.

xjm’s picture

Assigned: quicksketch » Unassigned
star-szr’s picture

Assigned: Unassigned » star-szr
chx’s picture

#13, choose the correct answer is 2. You can't fire a hook plain and simple because it might trigger a query in a module expecting a certain schema and it's not there yet. Nasty. (Why do you think I took off the upgrade maintainer badge for D8?)

xjm’s picture

Example of this problem space in action: #1292470-115: Convert user pictures to Image Field

star-szr’s picture

Status: Active » Needs review
FileSize
3.67 KB

Here's a first draft.

  • Added a couple paragraphs to start to explain "why they're there, what they do, how to use them, what to do when a new one needs to be added".
  • Added a slightly modified version of the example code from #9. foo_update_8036 now calls update_8001_bar_get_types(), not update_bar_get_types_8001().
  • I also added a @see from hook_update_N() to the defgroup and vice versa.

This does not yet cover hook_update_dependencies().

star-szr’s picture

FileSize
3.68 KB

Oops! I just realized the example code should probably use function names like _update_7000_bar_get_types(), not update_7000_bar_get_types(). Revised.

jhodgdon’s picture

Status: Needs review » Needs work

This looks really good, and I think it makes the idea much clearer, thanks!

A few thoughts:

a) I'm wondering if we could/should use the standard term "invoking" rather than "firing" a hook -- or is "firing" something different?

b) This documentation refers to the "schema version" twice, and while I personally know what that means, I'm not sure everyone would, especially since hook_update_N() does not use that term. So how about the first time the "schema version" is referred to, we say something like:
...create a new utility function named _update_N_my_function() where N is the schema version the function acts on (the schema version is the number N from the hook_update_N() where this schema was introduced, or a number following the same numbering scheme) , and my_function is...

c) I am also a bit confused about how the foo() update hooks would know which bar() schema version is in effect, and hence which bar update function should be called. That wasn't made clear by the example or the text.

d) I think the example code needs to tell which file each function should be placed in (foo.install, bar.install, ???). They should probably be grouped by file instead of mixing them up.

star-szr’s picture

Status: Needs work » Needs review
FileSize
4.05 KB
4.25 KB

All good points, I've made changes accordingly. I think C is where hook_update_dependencies() comes in, so I've added an implementation of hook_update_dependencies() to foo.install. Not sure yet how to incorporate an explanation of hook_update_dependencies() into the text, suggestions are welcome.

Should we have a @see to hook_update_dependencies() as well?

star-szr’s picture

FileSize
4.25 KB

Whitespace fix.

jhodgdon’s picture

Looking better!

I have a couple of suggestions:

a) Yes, it seems like we should have an @see to hook_update_dependencies().

b)

+ * During database updates, it is impossible to judge the consequences of
+ * invoking a hook, because the schema of any given module could be out of date.

- second line - let's lose the word "given" (doesn't add any meaning).
- Also, this applies to not only invoking hooks, but to any other operation involving the schema, such as drupal_write_record(). And I think just saying "impossible to judge the consequences" is too limited -- really, you don't even know if it will work. So this whole sentence just needs to be rewritten (and maybe the following sentence too, which actually says more about the problems than this first sentence). And the final sentence of this paragraph about "not invoke any hooks" also probably needs to be broadened.

c) Given that you have put in hook_update_dependencies, I think in this:

+ *   function foo_update_8010() {
+ *      // Since foo_update_8010() is going to run after bar_update_8000()
+ *      // (for whatever reason), it needs to operate on the new schema, not the
+ *      // old one.

the (for whatever reason) remark should be removed.

jhodgdon’s picture

Status: Needs review » Needs work
star-szr’s picture

Status: Needs work » Needs review
FileSize
3.49 KB
4.4 KB

Here's another update.

  1. Added a @see to hook_update_dependencies().
  2. Revised text and example code as per B in #24.

For this round I compared the update versions of a few of the functions side by side. The following are probably the clearest examples to look at, and at least the function signatures remain more or less the same.

_update_7000_field_create_field() vs. field_create_field()
and
_update_7000_field_delete_field() vs. field_delete_field()

field_create_field() also shows a specific example where drupal_write_record() is converted to db_insert() in the update version, so I used that to get more specific than "not invoke any hooks", and provided a link to the Database abstraction layer group as well.

One more idea in parting: what about linking the update versions of API functions to their "normal" counterparts, or just being more consistent with the naming? I had trouble finding field_sql_storage_field_storage_write() and node_type_get_types() because the update versions were not named consistently.

star-szr’s picture

FileSize
4.4 KB

Sorry, testbot! Corrected the punctuation after the @link as per http://drupal.org/node/1354.

jhodgdon’s picture

Status: Needs review » Needs work

This is looking pretty good, thanks! I have a few suggestions:

a)

+ * These simplified versions of core API functions are provided for use by
+ * update hooks (hook_update_N()).

I think we shouldn't say "update hooks" here. Maybe "update functions (hook_update_N() implementations)" would be better? This terminology should also be used throughout the patch (later references could just say "update functions").

b) [EDIT: Fixed code tag here]

+ * db_insert(), db_update(), db_delete(), db_query() and so on.

The Drupal project officially uses the serial comma (so we need a comma before "and" here).

c)

+ * in the code example.
+ *
+ * foo.install:
+ * @code
...
+ *
+ * bar.install:
+ * @code
...

I think these code examples could be introduced better. I would add "that follows" to the first line here, and then make a better introduction to the code samples, like:

For example, file foo.install could contain:

And bar.install could contain:

d) See #4 above... What functions do we have that belong in this group? They need to get @ingroup added to them.

jhodgdon’s picture

One other suggestion: I think hook_update_N() needs a paragraph added that says something about this and points to this documentation for more details. This patch adds an @see, but it doesn't say anything about why. Maybe a slightly adapted version of this paragraph from the current patch:

+ * During database updates the schema of any module could be out of date. For
+ * this reason, caution is needed when using any API function within an update
+ * hook - particularly CRUD functions, functions that depend on the schema (for
+ * example by using drupal_write_record()), and any functions that invoke hooks.

with a line that says "See .... for details."

star-szr’s picture

Thanks @jhodgdon! I'll work on these revisions.

Regarding point D in #28: It looks like the update-api functions are already grouped with @ingroup in D7 and D8, or are there other functions that need to be added to the group?

jhodgdon’s picture

You are right -- some functions have been added... probably someone should look through the .install files and see if there are any more lurking.

The 8.x list looks suspicious to me though. Although it's supposed to be listing 7.x to 8.x functions, quite a number of them have 7000 in the list. I think probably there need to be two groups defined in the 8.x branch: one for 6-7 updates (left over from 7.x) and one for 7-8 updates (new functions). And the group header for the 6-7 updates should probably be short -- referencing the 7-8 updates page that will have all this nice documentation we've been working on for this issue.

Either that or we should change the group's ID so that it doesn't have 6.x-7.x or 7.x-8.x in the URL/group ID.

catch’s picture

The 7000 updates in 8.x are actually fine - as long as the schema hasn't changed since they were added, those functions should be used to update data in the tables they deal with.

jhodgdon’s picture

Good point (#32/catch).

So let's change the @defgroup/@ingroup machine name to take the 6.x-7.x version parts out entirely. This will let the topics be linked on api.drupal.org (because they will have the same name), and better reflect that the update functions can be useful past a specific version's lifetime.

star-szr’s picture

Status: Needs work » Needs review
FileSize
8.17 KB
8.73 KB

Notes and a summary of changes:

  1. Made the adjustments in #28.
  2. Per #29 I added a paragraph to the hook_update_N doc with an @link to the Update API group. No wording changes other than the "update functions" change from #28.
  3. I updated the group's machine name to "update_api". We'd have to backport that change to D6 and D7 so the topics can be linked on api.drupal.org, right?
  4. Added "mymodule" to the example function name and expanded the naming instructions with a couple examples to show the naming conventions - similar to the hook_update_N docs.
  5. Minor revision to one of the code comments in the sample code.
  6. I searched through the .install files for functions starting with _update and all were in the appropriate group.

Would the naming conventions for these utility functions be slightly different in some cases for contrib? For example, if I'm creating a contrib module that uses a function from the core field module that doesn't have a utility version, I can't just add a utility function to the field module's .install. If I needed a utility version of field_update_field(), would I name my utility function _update_N_mymodule_field_update_field() to prevent clashes?

Also… interdiff_1000.txt :)

jhodgdon’s picture

Status: Needs review » Needs work

Thanks, this is getting close!

One thing: we don't split up @link ... @endlink into separate lines, so this section needs a change in wrapping:

+ * The utility function should not invoke any hooks, and should perform database
+ * operations using functions from the @link database Database abstraction
+ * layer, @endlink like db_insert(), db_update(), db_delete(), db_query(), and
+ * so on.

(move the whole @link/@endlink to a new line)

Also, can we remove the two-space indentation in the @code section?

+ * @code
+ *   function foo_update_dependencies() {
...

I realize I may have suggested it sometime back but we're removing those now (sorry!) -- too much empty space.

Also, just a minor suggestion: I think these two paragraphs could/should be combined in system.api.php:

+ * During database updates the schema of any module could be out of date. For
+ * this reason, caution is needed when using any API function within an update
+ * function - particularly CRUD functions, functions that depend on the schema
+ * (for example by using drupal_write_record()), and any functions that invoke
+ * hooks.
+ *
+ * See @link update_api Update versions of API functions @endlink for details.
+ *

And I'm also wondering why the update-api group is in update.inc. Maybe it should be moved to system.api.php instead, so it's closer to the docs for hook_update_N(), hook_schema(), etc.?

star-szr’s picture

Status: Needs work » Needs review
FileSize
5.35 KB
8.59 KB

Here's a new patch with those changes. I haven't moved the group to system.api.php just yet, I think it makes more sense where it is but I'm ready to be convinced otherwise :)

jhodgdon’s picture

My thinking on this being in system.api.php is that the hook_update_N() documentation is there, plus hook_schema(), etc. (the other hooks that belong in the module.install file). update.inc doesn't seem to contain functions related to this at all. But I'm also willing to be convinced -- why do you think it belongs in update.inc?

Anyway, other than that, this looks good. Thoughts?

jhodgdon’s picture

Status: Needs review » Needs work

Changing status until I'm convinced it belongs where it is in the patch. :)

star-szr’s picture

After looking at this again I'm with you, especially once you brought up the .install file. I'll roll a new patch in the next day or two.

star-szr’s picture

Status: Needs work » Needs review
FileSize
8.87 KB

Moved the update_api @defgroup to the bottom of system.api.php. No other changes.

jhodgdon’s picture

Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Great work! I think this is ready to go. I'll commit it tomorrow unless someone else gets to it first (and I guess a documentation issue shouldn't be "major"?).

jhodgdon’s picture

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

Thanks again! Committed to 8.x. Let's make a version of this for 7.x too -- which will replace a different group there.

star-szr’s picture

I'll roll a patch for D7, likely over the weekend. I'm guessing the main change for the D7 version would be to update the schema numbers in the examples from 8xxx to 7xxx and 7xxx to 6xxx.

jhodgdon’s picture

That would probably be good. Also, the current group ID that the API functions are in for D7 is different, and presumably the actual update functions that are in the group are a different set too.

star-szr’s picture

Status: Patch (to be ported) » Needs review
FileSize
9.82 KB

Here's a patch for D7. Added the missing @ingroup to _update_7000_field_read_fields(), everything else was straightforward.

One convention I wasn't sure about - should there be a blank line before @ingroup, i.e. after comments or a @return? I added a blank line before the @ingroup I added.

jhodgdon’s picture

Assigned: star-szr » jhodgdon
Status: Needs review » Reviewed & tested by the community

Yes, there should usually be a blank line between @return and @ingroup. The @ingroup can be part of a group of @see though without a blank line.

Anyway, this patch looks good, thanks! I'll get it committed shortly.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks all! This is now committed to 7.x.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.