Problem/Motivation

Drupal core and contrib modules often spin out functionality to different or sub-modules for various reasons.

When this is done, often an upgrade path will be included to enable the new module. This is true for the field, text, file (and etc.) modules in Drupal 7.

Especially during major core upgrades, updates need to run in strict order with a specific schema version. However core has traditionally used module_enable(), which installs the contrib module with whichever schema is currently specified in hook_schema(), this means that subsequent updates (such as a data migration) may end up targeting a different schema to the one they were originally written for, depending on which version of Drupal core is actually used.

Additionally, module_enable() itself is tied to a specific core schema version of the {system} table which can be subject to change during a major release, so can not be reliably used across multiple updates. It also fires hooks such as hook_module_enable() and hook_modules_disabled(), and hooks cannot safely be executed during major version core upgrades since they may depend on schemas which are outdated.

Proposed resolution

Add a dedicated function for enabling modules during updates, which looks for hook_schema_0() (i.e. the version of the module's schema at the time it was introduced and the update written). This means the module can be installed with a predictable schema, after which subsequent updates can be run safely.

Remaining tasks

Patch for Drupal 8 passed and was committed, but there still may be additional work needed. Patch for Drupal 7 has not passed testing.

User interface changes

(new or changed features/functionality in the user interface, modules added or removed, changes to URL paths, changes to user interface text)

API changes

(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate)

Original report by [sun]

// Text of original report here.
(for legacy issues whose initial post was not the issue summary)

The problem

Drupal core and contrib modules often spin out functionality to different or sub-modules for various reasons.

When this is done, often an upgrade path will be included to enable the new module. This is true for the field, text, file (and etc.) modules in Drupal 7.

Especially during major core upgrades, updates need to run in strict order with a specific schema version. However core has traditionally used module_enable(), which installs the contrib module with whichever schema is currently specified in hook_schema(), this means that subsequent updates (such as a data migration) may end up targeting a different schema to the one they were originally written for, depending on which version of Drupal core is actually used.

Additionally, module_enable() itself is tied to a specific core schema version of the {system} table which can be subject to change during a major release, so can not be reliably used across multiple updates. It also fires hooks such as hook_module_enable() and hook_modules_disabled(), and hooks cannot safely be executed during major version core upgrades since they may depend on schemas which are outdated.

Proposed solution

Add a dedicated function for enabling modules during updates, which looks for hook_schema_0() (i.e. the version of the module's schema at the time it was introduced and the update written). This means the module can be installed with a predictable schema, after which subsequent updates can be run safely.

Original report

 /**
+ * Prepares system records of new core modules to include their updates.
+ *
+ * In order to have module updates from newly added core modules picked up
+ * during a major version upgrade, their schema_version needs to be changed from
+ * SCHEMA_UNINSTALLED to 0 (unless they existed in the previous version
+ * already), since only updates of installed modules are ran.
+ *
+ * This cannot be done during execution of updates, since we need to calculate
+ * update dependencies. Without this adjustment, only module updates being
+ * required by other modules would be executed.
+ *
+ * Lastly, each of these modules should have an accompanying
+ * MODULE_schema_7000() implementation of hook_schema(), which specifies the
+ * original schema at the time of upgrading, in case the module has later
+ * updates that are changing the schema. The module update that enables the
+ * module MUST NOT use module_enable() to install the module.
+ */
+function update_fix_new_modules() {
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

We need to resolve the following too:

image_schema()

shortcut_schema()

Image module existed in contrib, and my D6-7 update for a site that used to have image module installed a long time ago failed, so that may be an additional issue on top of this one. But the inability to run updates and moving target schema is definitely the same issue.

sun’s picture

Right, we also need to account for the separate and different case of new core modules that take over contrib namespaces, which are:

shortcut, overlay, toolbar, image, rdf

Out of those, new schema definitions:
http://api.drupal.org/api/function/shortcut_schema/7
http://api.drupal.org/api/function/image_schema/7
http://api.drupal.org/api/function/rdf_schema/7

sun’s picture

For the ex contrib module names, I'd suggest to do the following:

$module_list = array('image', 'overlay', 'rdf', 'shortcut', 'toolbar');
$schema = array();
// Collect module tables, if any.
foreach ($module_list as $module) {
  $function = $module . '_schema';
  if (function_exists($function)) {
    $schema += $function();
  }
}
// Rename tables in NEW schema definitions that already exist.
foreach ($schema as $table => $spec) {
  if (db_table_exists($table)) {
    db_rename_table($table, '__d6' . $table);
  }
}
// Mark the modules as not installed.
db_update('system')
  ->condition('type', 'module')
  ->condition('name', $module_list, 'IN')
  ->fields(array('schema_version' => SCHEMA_UNINSTALLED))
  ->execute();
Dave Reid’s picture

How will this handle a module which has to change namespaces due to core having its schema_version record wiped out before it can copy it to the new record?

Status: Needs review » Needs work

The last submitted patch, drupal.update-module-enable.0.patch, failed testing.

catch’s picture

@Dave Reid - do you mean we should back up the schema versions somewhere? We could possibly save them in a variable or similar? It's ugly but there don't look like a lot of choices here.

sun’s picture

Status: Needs work » Needs review
FileSize
11.33 KB

All of the mentioned modules are possibly affected, and potentially have a new namespace in contrib (prime example might be Image module).

I'm not sure what backing up of the schema version would buy us for those. Some additional considerations:

  • We need to treat all new core modules the same, as if they would have existed in contrib. That is, because we don't know whether anyone anywhere wrote a custom module that was just simply named, say, "field". Perfectly legit to do in D6.

    However, the upgrade to D7 still has to work as we expect; i.e., the new core module is installed with its 7000 schema, and all updates since its inception must be run.

  • We cannot install new core modules like we normally install modules, because many core module updates have interdependencies with other updates - e.g., we can't install the latest version of Field module and run the Node and Comment module updates to migrate their data into that. Additionally, future D7-D7 updates need to be taken into account as well.

umph. This is whack. Comments in code.

Status: Needs review » Needs work

The last submitted patch, drupal.update-module-enable.7.patch, failed testing.

fago’s picture

sub

catch’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
Issue tags: +Needs backport to D7

We need the helper function in Drupal 8 for #1018602: Move entity system to a module.

Using hook_schema_7000/8000() is ambiguous since it could refer to the schema before $module_update_7000/8000() runs or the schema after it. So I changed this to use hook_schema_0() - which matches the 0 value for module schema in {system}.

Since we have failing tests in #1018602: Move entity system to a module I'd suggest we roll a stacked patch over there including this one and see if it fixes those, that should be enough to get it in. Then we'll need to move back to D7 to deal with the actual modules that were added.

catch’s picture

FileSize
1.45 KB
catch’s picture

Version: 8.x-dev » 7.x-dev

8.x patch just got committed with #1018602: Move entity system to a module. Moving this back to 7.x again.

catch’s picture

Status: Needs review » Needs work

Back to CNW too..

catch’s picture

Status: Needs work » Needs review
FileSize
11.32 KB

Re-rolling #7, except with hook_schema_7000() renamed to hook_schema_0() (since hook_schema_7000() would be equivalent to after hook_update_7000() has run).

Likely tests will explode as well here.

Status: Needs review » Needs work

The last submitted patch, drupal.update-module-enable.7.patch, failed testing.

sun’s picture

Note: update_module_enable() of this patch got partially

- added via #1018602-87: Move entity system to a module

- slightly fixed via #1331350: [Upgrade path broken] Entity module not enabled during D8 upgrade

catch’s picture

Version: 7.x-dev » 8.x-dev

This is a Drupal 8 issue again now.

With entity module we escaped hitting the schema versioning issue because entity module has no schema, but language module both has a schema, and also has an update now after #1357918: Missing update for language_default in language langcode update.

We'll need to apply the suggested hook_schema_0() pattern to language module in Drupal 8, currently it looks to be working around this by running language_update_8000() directly from update.inc.

tim.plunkett’s picture

Tagging.

catch’s picture

Issue tags: +D8 upgrade path

Made a start on the summary.

catch’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

I likely have a similar issue as we gear up for renaming locale module (to interface_translation.module I believe :). That would mean we have a "new" module with a "new schema" that needs migration from the old schema and might have old update functions from before the rename. We should either move those functions to bootstrap prepare or to system module. Or we can hack schema version updates, but it would be very specific to the module since we need to set the right previous schema version, to get all other updates run, etc.

catch’s picture

One way to handle the locale/interface_translation rename would be to handle it as literally a rename rather than a new module.

So in the system table, copy the existing schema version of locale module over (i.e. whatever the 7.x one is at time of upgrade), and ensure it's marked as enabled or disabled if locale module is already enabled or disabled. Then all the updates which were in locale.install will run in interface_translation.install as if it had been called that all along.

Gábor Hojtsy’s picture

Hm, so would we do that in a system module update and make all interface_translation updates dependent on it?

catch’s picture

I think so yeah, I'm not sure we've renamed a module in core like this before (and this issue has been unfixed for coming up to two years) so not guaranteeing it'd actually work, but seems like it should.

andypost’s picture

Is this really critical issue?

catch’s picture

It is. If we add an update to file module, it simply won't get run on sites that upgraded from Drupal 6. Also if the schema changes, then modules upgrading from Drupal 6 will always have the newest schema version installed when the hook_update_N() runs, which if another module has an update touching those tables running afterwards could cause very nasty issues.

sun’s picture

That said, I'm unsure what is left for this issue, since we made some good progress on update_module_enable() already.

catch’s picture

We'd still need to ensure that modules are installed with a fixed schema, not the one defined in hook_schema() I think?

sun’s picture

For that, we have this special support for hook_schema_0() in there:

    // Check for initial schema and install it. The schema version of a newly
    // installed module is always 0. Using 8000 here would be inconsistent
    // since $module_update_8000() may involve a schema change, and we want
    // to install the schema as it was before any updates were added.
    $function = $module . '_schema_0';
    if (function_exists($function)) {
      $schema = $function();
      foreach ($schema as $table => $spec) {
        db_create_table($table, $spec);
      }
    }
catch’s picture

That's not implemented by Drupal 7's field or image modules though, for example.

andypost’s picture

got it working on #1029708-16: History table for any entity
The history table in D7 lives in node but for D8 we enable the module

/**
 * Enable History module.
 */
function node_update_8012() {
  // Enable the history module without re-installing the schema.
  update_module_enable(array('history'));
}

But the all broken tests points that updates still not run in upgrade

webchick’s picture

We need docs for that crazy hook_schema_N() business: #1929816: Remove all references to/implementations of hook_schema_0()

effulgentsia’s picture

That's not implemented by Drupal 7's field or image modules though, for example.

This issue is currently for D8 though. What's still needed to finish the D8 piece? Then we can set the version to 7.x, and work on that.

Field.install and image.install no longer have any hook_schema(). Those modules have config though. Do we need a new issue to solve the same problem for config files, or is that covered by #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage?

We have plenty of modules though that do implement hook_schema(). Do all of them need a hook_schema_0() or only certain ones? What do we want to do about ones like history.install, where currently, we rely on it not having a hook_schema_0(), because the original schema was part of node.install?

webchick’s picture

Issue tags: +beta blocker

Tagging all critical D8 upgrade path issues as "beta blocker."

effulgentsia’s picture

catch’s picture

We discussed this at Drupalcon in the light of using migrate for the update path.

- core modules can now be enabled then migrated into during major version updates, won't run into this issue.

- if a contrib module resides in the same project as a module being enabled, it can take care of installing it with the correct schema and run any code it needs after doing that. Not sure if update dependencies are enough for that, or probably we still need to allow a schema to be passed in manually when installing.

- contrib modules enabling a contrib module from a project outside of their control are still going to run into this - I'm not sure there's really a valid case for that though?

Leaving this as is for now but it'd be good to revisit this a bit.

catch’s picture

Issue summary: View changes

Issue summary template applied by minneapolisdan

catch’s picture

Issue summary: View changes
Issue tags: -beta blocker

No longer blocks beta. This could still happen during a minor update to core (if we ever wanted to enable a new module in a minor release), and in contrib.

catch’s picture

Version: 8.x-dev » 7.x-dev

In #1929816: Remove all references to/implementations of hook_schema_0() we more or less decided that if you're enabling a module during a minor update, if it's within the same project as the update hook enabling it, then that project can manage any dependencies. If it's cross-project that's an edge-case and you need to be extra careful.

Due to that, I'm tempted to mark this duplicate, but there's still issues with the 6.x-7.x to upgrade path, so moving there first.

mgifford’s picture

Status: Needs work » Needs review
FileSize
11.32 KB

reroll or drupal.update-module-enable.7.patch

Status: Needs review » Needs work

The last submitted patch, 40: drupal.update-module-enable.1239370-40.patch, failed testing.

webchick’s picture

Issue tags: -D8 upgrade path

Removing the D8 upgrade path tag, since catch pointed out in #14 this was fixed in D8, and we no longer support D7 -> D8 upgrades.

webchick’s picture

Priority: Critical » Major

Based on #37, I'm not sure this any longer qualifies as critical. Downgrading to see what happens. ;)

  • fago committed 6204743 on 8.3.x
    #1239370 patch by catch: add update_module_enable()
    
    

  • fago committed 6204743 on 8.3.x
    #1239370 patch by catch: add update_module_enable()
    
    

  • fago committed 6204743 on 8.4.x
    #1239370 patch by catch: add update_module_enable()
    
    

  • fago committed 6204743 on 8.4.x
    #1239370 patch by catch: add update_module_enable()
    
    

  • fago committed 6204743 on 9.1.x
    #1239370 patch by catch: add update_module_enable()