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() {
Comment | File | Size | Author |
---|---|---|---|
#40 | drupal.update-module-enable.1239370-40.patch | 11.32 KB | mgifford |
#16 | drupal.update-module-enable.7.patch | 11.32 KB | catch |
#13 | grrr.patch | 1.45 KB | catch |
#7 | drupal.update-module-enable.7.patch | 11.33 KB | sun |
drupal.update-module-enable.0.patch | 7.98 KB | sun | |
Comments
Comment #1
catchWe 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.
Comment #2
sunRight, 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
Comment #3
sunFor the ex contrib module names, I'd suggest to do the following:
Comment #4
Dave ReidHow 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?
Comment #6
catch@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.
Comment #7
sunAll 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:
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.
umph. This is whack. Comments in code.
Comment #11
fagosub
Comment #12
catchWe 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.
Comment #13
catchComment #14
catch8.x patch just got committed with #1018602: Move entity system to a module. Moving this back to 7.x again.
Comment #15
catchBack to CNW too..
Comment #16
catchRe-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.
Comment #18
sunNote: 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
Comment #19
catchThis 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.
Comment #20
tim.plunkettTagging.
Comment #21
catchMade a start on the summary.
Comment #21.0
catchUpdated issue summary.
Comment #22
Gábor HojtsyI 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.
Comment #23
catchOne 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.
Comment #24
Gábor HojtsyHm, so would we do that in a system module update and make all interface_translation updates dependent on it?
Comment #25
catchI 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.
Comment #26
andypostIs this really critical issue?
Comment #27
catchIt 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.
Comment #28
sunThat said, I'm unsure what is left for this issue, since we made some good progress on update_module_enable() already.
Comment #29
catchWe'd still need to ensure that modules are installed with a fixed schema, not the one defined in hook_schema() I think?
Comment #30
sunFor that, we have this special support for
hook_schema_0()
in there:Comment #31
catchThat's not implemented by Drupal 7's field or image modules though, for example.
Comment #32
andypostgot 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
But the all broken tests points that updates still not run in upgrade
Comment #33
webchickWe need docs for that crazy hook_schema_N() business: #1929816: Remove all references to/implementations of hook_schema_0()
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedThis 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?
Comment #35
webchickTagging all critical D8 upgrade path issues as "beta blocker."
Comment #36
effulgentsia CreditAttribution: effulgentsia commentedFeedback on #2077427: Every module with a hook_schema() should have a hook_schema_0() would be much appreciated.
Comment #37
catchWe 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.
Comment #37.0
catchIssue summary template applied by minneapolisdan
Comment #38
catchNo 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.
Comment #39
catchIn #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.
Comment #40
mgiffordreroll or drupal.update-module-enable.7.patch
Comment #42
webchickRemoving the D8 upgrade path tag, since catch pointed out in #14 this was fixed in D8, and we no longer support D7 -> D8 upgrades.
Comment #43
webchickBased on #37, I'm not sure this any longer qualifies as critical. Downgrading to see what happens. ;)