Update documentation for hook_update_N() for Drupal 8

Moving to the documentation component, for better visibility with docs folks.

I'll be working on this the next few days or early next week.

I discussed this with webchick just now in IRC and here are some additional notes about what we need this documentation to cover:

1) When (in D8) you need to provide an update hook for your module. https://www.drupal.org/node/2507899

2) Reference examples of update hooks for each. (schema - docs should more or less work as-is I think, configuration change, etc.) [might be able to crib that from head2head, not sure]

3) How testing works

Examples of issues with hook_update_N() that are at or near completion:
https://www.drupal.org/node/2528178
https://www.drupal.org/node/2455125

See also the https://www.drupal.org/project/head2head project, specifically http://cgit.drupalcode.org/head2head/tree/head2head.module, for some examples from beta7 on.

Before we start the documentation we should actually know how to actually write them.

I think the discussion in #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager) is quite important for this issue to be honest.

Calling this a meta, because there are a few issues floating around that need an umbrella.

Thanks! They're definitely all related. I may end up combining a few together (marking them duplicates of this one) so that I can make one patch that would really fix the docs and can be reviewed as a chunk. Or I might end up making a new sub-issue for the part that is here, or adopting one of those sub-issues for this stuff.... Anyway, they needed to be collected so thanks a bunch!

One other thing I thought of and am adding here so I don't forget it:

I think we either have a topic or an issue for creating a topic about the Migrate API. We should make sure that this topic and the hook_update_N() stuff are connected by @see links or whatever as appropriate.

I decided the best way to get started on this was to create a new page/section about updates in Drupal 8. So I've made a start here:
https://www.drupal.org/node/2535316

There are obviously a few "to be written" sections there, but if anyone has comments about the general structure or outline, please comment here.

My plan is to finish that first so that we have all the details documented, and then figure out what to distill into *.api.php topics and/or the hook_update_N() documentation itself.

Over on #2528178-49: Provide an upgrade path for blocks context IDs #2354889 (context manager), @dawehner pointed out that the docs didn't talk about what to do if your module was not able to update all the data, and also that there were various discussions in that issue about other things that would need to be covered to make it an all-encompassing example.

So...

a) I updated https://www.drupal.org/node/2535454 to talk about what to do if your module is not able to update all the data (see Example 2 on that page, which is taken from this issue).

b) I looked through the rest of the issue there to see if there were other things that should be mentioned...

c) One thing that was mentioned was what to do if you wanted to rename config keys. I don't see that this is all that different from updating other config data though, so I haven't highlighted that in the docs.

d) I'm not seeing much else beyond what I just added to the docs... Any thoughts?

e) I still need to write the docs on how to make a test.

If anyone sees anything else missing or wrong... maybe you can clue me in on what needs to be said, highlighted, emphasized, or changed?

OK, I've written the page on testing too. As far as I can tell, the documentation has at least the basics covered. Please review!

I guess we need to also make a Core patch to get some of this on api.drupal.org... will work on that shortly but for now please review
https://www.drupal.org/node/2535316
and its child pages.

I took a look at the issues that were marked Related to this one. They're not really the same issue as here, so I took the [Meta] out of this issue title. I think we should return to them when this issue is finished (this one is Major if not Critical).

Meanwhile, here is a patch for the API docs. See what you think? It:
- Creates an Update API topic for api.drupal.org
- Updates the hook_update_N() docs
- Updates the class docs for the update test base class

Overall this looks pretty good to me. A few minor nits:

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -8,6 +8,51 @@
   + * Drupal 8), updates will not run and you'll need to use the
   

   I don't think we use contractions. "you'll" -> "you will"

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -8,6 +8,51 @@
   + * Update code should be tested both manually and by writing an automatic test.
   

   automatic or automated?

3. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -419,75 +464,71 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + * @section sec_bulk Multi-pass or bulk updates
   

   Could we just call these "batch updates" or "batched updates"?

4. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -500,40 +541,58 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   +  // For non-multipass updates, the signature can simply be:
   

   "multipass" vs "multi-pass" above - sidestepped if we rename to "batch(ed) updates" :)

Thanks for the review! Good points. Not sure about "we don't use contractions" but I have no objection to removing them... the rest is all on target for sure.

Here's a new patch.

This looks great now. There is one place where the line wrapping looks a little odd, but this could be fixed on commit - otherwise this is RTBC.

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -500,40 +541,58 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
+  // If you are performing database updates, as in the
+  // examples below, you may need to put use statements at the top of
+  // your mymodule.install file for the referenced classes, such as:
+  // use Drupal\Core\Database\Database;

I just updated https://www.drupal.org/node/2535316 (the page on how to make update functions for config changes) to agree with what was just committed on #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager).

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -419,75 +464,71 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
     * - mymodule_update_8100(): This is the first update to get the database ready
   

   This is pre-existing, but it's no longer true.

   When modules are first installed, their schema version is set to 8000 if they have no updates (see key_value system.schema) - so the first update for 8.x has to be 8001.

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -419,75 +464,71 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + * Writing hook_update_N() functions can be a bit tricky. Here are a few
   

   This is an understatement.

3. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -419,75 +464,71 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + * First, not all module functions are available from within a hook_update_N()
   

   This is no longer the case - everything is available.

4. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -419,75 +464,71 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + * this reason, caution is needed when using any API function or class within an
   

   Should probably say 'service' rather than class?

5. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -419,75 +464,71 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + * run.
   

   Could be more clear why this happens and what to do.

   There's two reasons the schema can be out of date:

   1. Other modules or core updates are getting run at the same time as this one.

   2. This update is running as part of a series of updates for the same module.

   Or both at the same time.

   Could also mention hook_update_dependencies() here.

6. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -500,40 +541,58 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   -  db_add_field('mytable1', 'newcol', array('type' => 'int', 'not null' => TRUE, 'description' => 'My new integer column.'));
   +  // If you are performing database updates, as in the
   +  // examples below, you may need to put use statements at the top of
   +  // your mymodule.install file for the referenced classes, such as:
   +  // use Drupal\Core\Database\Database;
   +
   

   Does this need to be mentioned explicitly? By the time you're writing an update, you've probably got used to use statements.

7. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -500,40 +541,58 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   +  // Example function body for a batch update. In this example, all user
   +  // names except user 1 are updated so that they end in !, for illustration
   +  // purposes.
      if (!isset($sandbox['progress'])) {
   +    // This must be the first run. Initialize the sandbox.
        $sandbox['progress'] = 0;
        $sandbox['current_uid'] = 0;
   -    // We'll -1 to disregard the uid 0...
        $sandbox['max'] = db_query('SELECT COUNT(DISTINCT uid) FROM {users}')->fetchField() - 1;
      }
    
   -  $users = db_select('users', 'u')
   +  // Update in chunks of 20.
   +  $users = Database::getConnection()->select('users', 'u')
        ->fields('u', array('uid', 'name'))
        ->condition('uid', $sandbox['current_uid'], '>')
   -    ->range(0, 3)
   +    ->range(0, 20)
        ->orderBy('uid', 'ASC')
        ->execute();
   -
      foreach ($users as $user) {
        $user->setUsername($user->getUsername() . '!');
   -    db_update('users')
   +    Database::getConnection()->update('users')
          ->fields(array('name' => $user->getUsername()))
          ->condition('uid', $user->id())
          ->execute();
   

   The {users} tables no longer has a name column.

   The COUNT query doesn't need a distinct.

   Why db_query() for the COUNT and Database:: for everything else?

   Might want to revisit the example overall.

Thanks for the reviews! So I fixed some wrapping problems in comments in the hook_update_N() mentioned in #14.

Also addressed #16, very good points. Note that catch, longwave, and I discussed the numbering in IRC and I've reworked this to stress that (a) you have to use the major version number and (b) x000 is not allowed.

I've added an interdiff file, but it is nearly as long as the patch so you might just want to look at the patch file.

This fixes all the points raised in #16 and the text is good. Two minor nits, otherwise this is RTBC.

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -419,75 +465,90 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + * Writing hook_update_N() functions is tricky. There are several reasons
   + * why this is the case:
   

   Wrapping is not quite right here.

2. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -15,7 +15,24 @@
   + * - Write the hook_update_N() implementations that you are testing.
   + * - Create one or more database dump files, which will set the database
   + *   to the "before updates" state. Normally, these will add some
   + *   configuration data to the database, set up some tables/fields, etc.
   + * - Create a class that extends this class.
   + * - In your setUp() method, point the $this->databaseDumpFiles variable
   + *   to the database dump files, and then call parent::setUp() to run the
   + *   base setUp() method in this class.
   + * - In your test method, call $this->runUpdates() to run the necessary
   + *   updates, and then use test assertions to verify that the result is what
   + *   you expect.
   + *
   

   Looks like this could all be wrapped a bit tighter as well.

Fixed wrapping. Thanks for taking a look!

Wow. There is apparently some other issue where someone made major changes to the hook_update_N() docs. I don't have time to sort this out now. WHY ARE THERE TWO ISSUES???????

It was #2521776: Update documentation for hook_update_N() for Drupal 8. Sigh.

OK this is probably a good reroll. Please check that I got everything from the other patch in here...

Link above should be #2538228: Document that Config save/delete/rename events may be dispatched during hook_update_N(), what that means for subscribers, and fix core subscribers accordingly I think. Taking a quick look at this now.

The reroll is good, and I like the explicitly separate list of "things that are safe". I think though that the other issue was stronger in mentioning "Loading, saving, or performing any other operation on an entity" is unsafe. Here we just say "be careful about CRUD operations" which is not quite the same thing.

Two other minor problems:

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -421,94 +467,99 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + *   when saving configuration, use the $trusted_data = TRUE parameter so that
   

   The parameter appears to be called $has_trusted_data.

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -421,94 +467,99 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + *   use the $trusted_data argument in the save operatoin.
   

   Same as above, plus typo in "operation".

Yes, correct link, sorry, I was in a rush (obviously, as I didn't notice my typo). :)

And yes, that parameter may have changed recently, or else everyone else was referring to it wrong on the recent "make some hook_update_N() patches that change config" issues and I never went and checked it. Good catch! You are right about that name.

So... here's one more patch!

Updating issue summary.

This looks great. This patch vastly improves this section of the documentation, includes all the suggestions made in this issue, and incorporates the changes from the other issue as well. RTBC!

Thanks for all the reviews longwave! Checking box to make sure you get commit credit, and unassigning to prevent confusion since patches assigned to me sometimes don't get committed as committers think they're waiting on something. ;)

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -5,8 +5,54 @@
   + * - Configuration schema changes: adding/removing/renaming a config key,
   + *   changing the expected data type or value structure, changing dependencies,
   + *   etc.
   

   Configuration changes - not schema changes.

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -421,94 +467,101 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   + * - Never assume that the database or configuration schema is the same when
   + *   the update will run as it is when you wrote the update function. So,
   + *   when saving configuration, use the $has_trusted_data = TRUE parameter so
   + *   that schema is ignored, and when updating a database table or field, put
   + *   the schema information you want to update to directly into your function
   + *   instead of calling your hook_schema() function to retrieve it (this is
   + *   one case where the right thing to do is copy and paste the code).
   

   Let's not mix config schema and database schema. I think this is confusing. This also does not mention that updates have to be sure that they are setting data with the correct type at the time the update was written. That bit of the previous documentation seems lost.

Um. In #30 item 1, we're enumerating about data model changes, so I think it is actually when you change the schema of configuration that it's a data model change, which would "make stored data incompatible with the codebase". Right? Here's the context of that bit:

 * You need to provide code that performs an update to stored data whenever your
 * module makes a change to its data model. A data model change is any change
 * that makes stored data on an existing site incompatible with that site's
 * updated codebase. Examples:
 * - Configuration schema changes: adding/removing/renaming a config key,
 *   changing the expected data type or value structure, changing dependencies,
 *   etc.

But you're right, we should mention config changes too, which are separate from config schema changes. So I added an item for that.

In #30 item 2, good idea. Fixed that too hopefully.

New patch... maybe the last one this time? Anyway the docs are improving I think!

Interdiff looks like it covers #30 to me, probably needs a final signoff by @alexpott though.

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -5,8 +5,57 @@
+ * - Configuration schema changes: adding/removing/renaming a config key,
+ *   changing the expected data type or value structure, changing dependencies,
+ *   etc.
+ * - Configuration changes: sometimes even though the configuration schema
+ *   does not change, you may still need to update configuration to be
+ *   compatible with new code.

I think having this as separate points is not necessary. Unlike databases, configuration schema are not mandatory, just highly recommended. And making someone ponder about the potential difference of "Configuration schema change" vs "Configuration change" feels off topic.

How about:

 * - Configuration changes: adding/removing/renaming a config key,
 *   changing the expected data type or value structure, changing dependencies,
 *   schema changes, etc.

Also I think the etc. brigade will demand the correct ... used.

etc. is preferred against ... by the ... brigade. I personally don't think ... is so bad but some people do. Whatever.

Anyway, I'm fine with that proposed text. It's pretty much what I had in #26 that you wanted changed before, except you took out the word "schema" there. Whatever. :) Anyway, here's one more patch.

Back once again to RTBC.

I think this is a massive improvement over what we have. Once the entity upgrade mess get's sorted out (#2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state) I think we need to improve the examples to cope this use-case. And perhaps we should also include examples of config hook_update_N()'s in the config schema upgrade patch (#2543150: Document consequences of contrib changing config schema without core's API supporting config version tracking).

Committed 486038f and pushed to 8.0.x. Thanks!