Add the ability to enable Backdrop-only required modules during an upgrade from Drupal 7.

[bugfolder]

There are modules (e.g., Entity Plus, Entity UI) that didn't exist in D7 and are required by Backdrop versions of D7 modules. Examples of B modules that need Entity Plus (E+, for short), for example, are Rules, Ubercart, and the modules in this list.

A problem arises when upgrading a D7 site that uses those modules. If the user merely adds the E+ module to the B codebase (the logical thing to do), when trying to upgrade the site, B sees that there was no enabled version of the D7 module, so it assumes the B version is not enabled, and therefore disables it and all of the downstream modules (like Rules, Ubercart, and their submodules), and so doesn't run their update hooks, and the user ends up with a site with only partially upgraded and a bunch of disabled modules. (This is particularly problematic with Ubercart, which has about 20 submodules, plus further contrib sub-submodules.)

The current solution, which has been recommended for Rules and Ubercart for a while, has been stub modules, basically fake/empty D7 modules that you enable before performing the upgrade. See here and here.

More recently, it was suggested to enable the B modules in an update hook. We can't actually do this during hook_update_N(), because by the time that would get called, the damage is done: the downstream modules are already disabled, so their update hooks don't get called. However, it can be done during hook_requirements('update') (suggested example here, PR that does this for Rules module here). So that works, but it's not ideal, for a couple of reasons:

• If this is the preferred solution, (nearly) the same chunk of code would need to be implemented by each contrib module that requires E+, E-UI, etc.
• More importantly, if we enable the required module by calling module_enable() as part of hook_requirements() (as in the PR), this results in calling hook_enable() for the module, and hook_modules_enabled() for all other modules, and this happens before the whole series of hook_update_N() calls has happened; so these hooks, which might be expecting fully-configured B modules, will be fired before the B modules are upgraded from D7.
• Also, if multiple modules are doing this, there's no particular control over the order in which the modules are enabled. (If that matters.)

So, it would be nice if Backdrop core was able to provide support for this situation: when an enabled D7 module is being upgraded to B and depends on a B module that didn't exist in D7.

Some possibilities:

• Add a module_enable_lite() function that lets the client module "enable" the dependency B-only module (i.e., create the appropriate entry in the system table) without firing the enable hooks, for use by the contrib modules in their hook_requirements() call.
• Add a new updating hook that lets client modules declare dependency B-only modules that ought to be treated as enabled if they exist in the codebase, and then core handles the "lite" enablement of the B-only modules.

We could also just put E+ (which is currently the most popular contrib module) and E-UI into core, so that they're already enabled in new installs. (But this wouldn't be a total solution; B Ubercart still has a B-only module that isn't in D7.)

This issue arose from discussions and issues with @argiepiano and @laryn; please add anything I've missed, and everyone else, please give your thoughts. (H/T also to @indigoxela, who raised the issue for Rules.)

---

Advocate: @bugfolder
PR: backdrop/backdrop#3950

[docwilmot]

More importantly, if we enable the required module by calling module_enable() as part of hook_requirements() (as in the PR), this results in calling hook_enable() for the module, and hook_modules_enabled() for all other modules, and this happens before the whole series of hook_update_N() calls has happened; so these hooks, which might be expecting fully-configured B modules, will be fired before the B modules are upgraded from D7.

I'm not seeing quite why this is a problem though. Both hook_enable() and hook_modules_enabled() in that case would only be passed the entity_plus and entity_ui modules right, so unless those two actually implement those hooks, then nothing "bad" should happen?

Thinking about update.php though, there should never normally be a time when a module is running updates and finds that it newly depends on another, disabled, module. This should reasonably only happen during a B-7 upgrade. So I think its reasonable to put a little stop in update.php when its checking for update dependencies, so that it pauses and asks "module x depends on module y, which is disabled, do you want to enable it?", or even a checkbox thats asks "Enable dependencies?". (I think it should be a choice rather than auto). We can then have update.php simply as possible switch the disabled module's status to 1 in System's tables, instead of fully calling module_enable() to avoid whatever complications you have found arise from that.

[argiepiano]

@bugfolder, thanks for this very thorough summary.

@docwilmot, the idea of asking the user may be a bit confusing. Basically, there is no option. Either you enable E+, or you can't use Rules or Ubercart in your migrated site (or even worse, you'll get undefined functions all over the place). If a user is migrating a site with Rules or Ubercart, it's because they want the site to be as functional as possible once migrated.

So, another option, which would prevent unintended enabled modules, is to add a key to the Backdrop's Rules and Ubercart info file - something like enable_on_migration thus, Backdrop's Rules would have

enable_on_migration[] = entity_plus
enable_on_migration[] = entity_ui

This way we are VERY specifically telling the update script that we want those (and only those) modules enabled when migrating, and only then.

Regarding your comment:

We can then have update.php simply as possible switch the disabled module's status to 1 in System's tables, instead of fully calling module_enable() to avoid whatever complications you have found arise from that.

This is tricky. It will work with Entity Plus and Entity UI because those two modules do not create tables in the DB. But, I can see a situation when we need to enable a module that DOES need to create a table - in which case switching the entry to 1 in the db will not work.

[docwilmot]

the idea of asking the user may be a bit confusing

I understand. The issue here though is this will apply in any future situation where a user runs an update and a module requires a disabled module, which would otherwise be automatically enabled if we dont ask. I again confess I cant think why this would ever happen, but I would prefer if it did the user be warned. If not a checkbox, at least something like a message "module E will be enabled, since modules X . Y, Z depend on E" with a "continue" button or such might make sense. Then there will be no real option.

This is tricky.

Yes this is the "simply as possible" bit. I mean, just extract the minimal from module_enable(): if tables, install them, if none just whats necessary to switch to enabled without running all hooks.

enable_on_migration[] = entity_ui

Does Backdrop know when an update.php run is a migration? Or isnt it all just update_Ns running in reality? This would still be special-casing a D7-B migration though, whereas looking for dependencies would be a more generic solution I feel.

[argiepiano]

Does Backdrop know when an update.php run is a migration? Or isnt it all just update_Ns running in reality? This would still be special-casing a D7-B migration though, whereas looking for dependencies would be a more generic solution I feel.

Yes. The update.php script invokes update_fix_requirements() early in the process, which has a way to check if this is the first time update is run in a new installation of Backdrop 1.x. In fact this function enables Views with a special ("lite") enabling process through update_module_enable() which avoids firing any "enabled" hook (I think). This function would be a good spot to either inform the user and ask them to allow enabling other dependencies, and/or checking info files of enabled modules in D7's db for enable_on_migration keys.

[docwilmot]

Good catch. I'm still feeling that catching dependencies would be preferable to a new enable_on_migration key (which would have no other purpose apart from first upgrade yet persist forever in the info file, and which would also duplicate the dependencies key in the info file). Furthermore, for users who dont recognize they need E+ for example, and dont place it in the modules folder, they would still end with the same issue of non-enabled Rules etc, so it would be preferable to have a pause in the update process for a message that says required modules are missing.

[bugfolder]

How about instead of putting more info in the .info file (which, as noted, persists visibly forever) if we add a hook, like hook_declare_upgrade_required_modules($modules) in the install file, where a module could declare B modules that should be lite-enabled? Then we wouldn't need to ask the user anything, and while that code would persist forever, like hook_update_N() calls, it can live in mymodule.install with the other functions that are only needed once in the lifetime of the site.

[argiepiano]

Furthermore, for users who dont recognize they need E+ for example, and dont place it in the modules folder, they would still end with the same issue of non-enabled Rules etc, so it would be preferable to have a pause in the update process for a message that says required modules are missing.

Actually, the update script currently stops the process if Entity Plus (or any other dependency declared in the info file) is not present in the modules folder. The issue we are trying to catch is when the E+ is present, but the database doesn't have it as enabled (because it did not exist in D7). So, what we are trying to do here is an automatic way to enable E+ once it's present in the modules folder.

How about instead of putting more info in the .info file (which, as noted, persists visibly forever) if we add a hook, like hook_declare_upgrade_required_modules($modules) in the install file

That seems like a good strategy. I'm going to try putting something together along these lines tonight.

[argiepiano]

I have created a PR (backdrop/backdrop#3950) as a first step. This PR:

1. Only runs during the migration process, on the very first update.php of a D7 database
2. Gets all enabled modules from the database
3. Invokes hook_migrate_required_modules_info() - a new hook that must be placed in the install file
4. Gathers a list of all modules to be enabled. If they are not present in the modules folder, nothing is done. update.php will stop the process if that's the case, later in the process
5. Invokes update_module_enable() with those modules. This function is a "lite enable" which won't fire any "enable" or "install" hooks

I've tested with a module that depends on Entity Plus (Entity Tokens), and it worked well. Entity Plus was enabled at the end of the process (NB: no "stub" modules was used in D7, AND Entity Plus was present in the modules folder).

Please test with Rules and others.

hook_migrate_required_modules_info() must return an array of modules needed to be enabled.

/**
 * Implements hook_migrate_required_modules_info().
 */
function entity_token_migrate_required_modules_info() {
  return array('entity_plus');
}

One option I did not test yet is - what happens if the "stub" Entity Plus was enabled in the D7 db? I'll test that tomorrow.

I have not added @docwilmot 's suggested user dialog - but we could add it here later if there is consensus.

[argiepiano]

I've modified the PR a bit to remove duplicate modules from the array of modules to be enabled, and to remove modules that are already enabled.

This takes care of my comment re: stub modules in the previous comment, and makes the enable process more efficient by passing an array with unique values.

[klonos]

This would still be special-casing a D7-B migration though...

I don't think that this argument should be a show-stopper for any suggested solution in this thread. It is safe to assume that if sites are to upgrade from D7, then that will happen in the 1.x cycle of Backdrop. If we don't want this special-casing, we can remove it in Backdrop 2.0. Right?

...so it would be preferable to have a pause in the update process for a message that says required modules are missing.

I agree that that would be useful 👍🏼

Actually, the update script currently stops the process if Entity Plus (or any other dependency declared in the info file) is not present in the modules folder. ...

That's great then! ...I could test things and find out myself, but since you already mentioned it @argiepiano can you please let me know if this is a "graceful" pause, with a helpful message, or if it is a php error/exception or otherwise unhelpful message? If you have a screenshot of what happens at that point, then that would also be great.

...and a HUGE thanks for putting a PR together already! 🙏🏼 ❤️

[docwilmot]

A hook works better, but still feels unnecessary IMO. Backdrop already knows that module X requires module Y: its in the info file, as dependencies[]. Adding a hook, though less visible I guess than another info file declaration, still duplicates. And I feel that any situation where this occurs would then benefit from a dialog, even if the disabling was accidental or bug or a D7-B upgrade, to let the user knows he's got a missing dependency.

[argiepiano]

If you have a screenshot of what happens at that point, then that would also be great.

@klonos, this is what update.php currently shows (without this PR) when a dependency is not in the modules folder

(BTW some formatting issues here - the portion of the screen is too narrow)

[klonos]

Thanks @argiepiano 🙏🏼 ...so the issue here seems to be that this problem is communicated to the person performing the upgrade during the "Verify requirements" step, which normally (for other requirements listed here) expects you go away and fix any issues, then come back and refresh the page. In our use case here though, there is something that we can do to fix the issues in this screen (but there is no submit button to indicate and allow that) 🤔

[argiepiano]

A hook works better, but still feels unnecessary IMO. Backdrop already knows that module X requires module Y: its in the info file, as dependencies[]. Adding a hook, though less visible I guess than another info file declaration, still duplicates.

I can see your point, @docwilmot. To me this hook feels like a safety measure - the developer is explicitly telling update.php that indeed, we need to enable a module that did not exist with that name in D7.

In a typical migration situation most of the modules listed in dependencies[] are modules that existed in D7 with the same name in D7 and are already enabled in the D7 database. Therefore checking if they are enabled is a bit of a waste of processing time, and is unnecessary for most of them. (the script already checks if those modules are present in the folder).

Any thoughts, @bugfolder and @klonos?

Re: adding a dialog. That will require a very different approach, and I may need help with this. The question is: how do we force-stop the process with a form? And then, once submitted, how do we restart the process where it was stopped? We may need to make further modifications to the update.php. Thoughts?

[argiepiano]

In our use case here though, there is something that we can do to fix the issues in this screen (but there is no submit button to indicate and allow that)

When you get this error message in the current version of update.php you are supposed to go back and put the missing modules in the modules folder, then restart (the bottom of this says "Check the messages and try again"

In the situation we are trying to fix with this PR there is basically nothing you can do to fix the problem. If you put a dialog saying something like "The migration script needs to enable Entity Plus to make your migration functional. Do you wish to proceed?" If you say NO, basically you have a non-functional migration!

To me there is no option. Why ask then? We could inform, but not ask. Stopping the process is also not acceptable. There is no option for the user. Either you enable Entity Plus, or you don't have a site.

[argiepiano]

You can look at it this way:

In a normal Backdrop or D7 site, if you have Rules enabled, you simply can't disable Entity API or Entity Plus. The UI does not give you that option. Its checkbox is grayed out. There is a reason for this. You basically have no option. You MUST have Entity Plus or Entity API enable.

This is a similar situation, except that during the migration process Entity API is not called Entity API in Backdrop, rather, Entity Plus. So by automatically enabling it (and not giving them the option) we are not taking away a freedom that they had before. They never had the "freedom" to say no to Entity Plus. If you want Rules, you are required to have Entity Plus/Entity API

[argiepiano]

After further thought: give me a day or two and I'll try to put something together that:

1. Has a way to inform the user that this enabling is happening
2. Uses dependencies[] instead of the new hook

I do think those options are really worth considering and I appreciate your suggestions, @klonos and @docwilmot

[bugfolder]

Adding a hook, though less visible I guess than another info file declaration, still duplicates.

Not entirely. The idea of the hook was that this was a way of declaring module dependencies that could be automatically enabled if the module code was present, as opposed to the default behavior of not auto-enabling non-enabled upstream modules (and therefore disabling the downstream modules).

But I am inclined to agree with @argiepiano's argument that in proper use, the user would have no choice about having E+ enabled, and that doing a D7->B upgrade might warrant special rules to apply. So I think it would be OK to just let the user know that any modules that are required by dependencies[] of enabled modules and exist in the codebase are going to be enabled automatically (so no extra hook required).

Looking forward to that next/updated PR.

[argiepiano]

While working on this issue I discovered that there are 8 core modules (all of them declared as dependencies of standard.install) that are never enabled during the migration. See #5506. The PR I'm creating would solve that issue too.

[argiepiano]

I have updated the PR

• There is an informational message that tells the user that some modules had to be enabled. This includes the 8 core required modules that were not enabled during migration - see Eight core modules are never enabled during D7 migration #5506
• The disabled required modules are all enabled using module_enable() rather than the "lighter" update_module_enable(). This was necessary because some of the modules need to create tables in the DB
• I have tested with a small module that depends on entity plus, and things seem to work fine. However, testing is needed with more complex modules like Rules and Ubercart
• This PR does not contain any type of checkbox or confirmation process like the ones mentioned above and in Eight core modules are never enabled during D7 migration #5506. I'm still unsure this is necessary. These "core 8" and Entity Plus are ALL declared as dependencies of other currently enabled modules, so it's safe to assume that they are needed by a functional installation, and giving the option of "not enabling" them will lead to problems (otherwise they would not be declared as dependencies!). This PR, however, does inform the user of the enabling.

A couple of important clarifications:

1. This "auto-enabling" only happens during migration from D7, on the first update.php run. update.php will not enable any module from the standard install that you have manually disabled in your Backdrop site
2. The new hook mentioned above is not needed anymore in this PR. All is done based on the dependencies declaration of the enabled modules (which is why I discover the other issue mentioned here)

[klonos]

Thank you for working on this @argiepiano 🙏🏼

Can we please make the modules in the message a proper <ul> list (using theme_item_list()), and have the human-readable names of the modules there? It will be easier to parse that way.

[argiepiano]

Thanks @bugfolder. I just committed your docblock suggestion.

[argiepiano]

One aspect that needs to be documented somewhere is that, if your module depends on a Backdrop-only module that needs to create a table, the new approach in the PR will not create it by default (since it uses update_module_enable(), which doesn't invoke hook_schema()). The Backdrop-only module that needs to create the table module needs to implement hook_schema_0() which is invoked during update_module_enable().

hook_schema_0() is not documented anywhere. I'll add it to this PR. It should probably be documented in system.api.php, right?

[bugfolder]

I'll add [documentation of hook_schema_0()] to this PR.

I think that should be a separate issue; since hook_schema_0 is invoked by a public API function, it too is a public API function and should be documented, whether or not this particular issue calls for it. (And yes, system.api.php, where hook_schema() and hook_schema_alter() are documented, seems to be the right place.)

I would further suggest that a note be added to the documentation of hook_schema() pointing out the existence of hook_schema_0() and its possible need, but leave the primary discussion in the docblock of hook_schema_0(). Others might have further opinions about the appropriate way to document these two functions' relationship, but the appropriate place to discuss how they are documented would be in the new issue.

[quicksketch]

Thanks @argiepiano for the thorough explanation and testing of different situations! I left one more comment on the PR about the point at which we're enabling the new modules: https://github.com/backdrop/backdrop/pull/3950/files#r831343018

The separate issue for hook_schema_0() is a great idea, that way we'll be able to reference that issue in the future.

[argiepiano]

@quicksketch I'm pasting my response to your comment to give others a chance to chime in if they wish.

@quicksketch said:

This [update_upgrade_enable_dependency()] perhaps should be moved before the "Apply pending updates" button is clicked? The update_fix_compatibility() function does the exact opposite of this function: it disables old modules. This function enables new ones. Perhaps the two of them could be either named similar or be merged together some how?

By moving up to above, before the updates are applied, any installation requirements that are not met by the newly installed modules will be displayed by update_check_requirements(). For example if a newly installed module required a PHP extension that we not available, that should be installed before attempting to upgrade the site, as update hooks may depend on the extension.

@quicksketch , I see the logic of your comment. One of the reason I decided to place this call later in the upgrade process was that the user would have a chance to see what modules would be automatically enabled before proceeding with the updates, giving them a chance to cancel. This was suggested in a comment by @docwilmot. If I move update_upgrade_enable_dependency() before update_fix_compatibility() then this option is gone.

If you are OK with this, I'll move it as requested. Let me know. (BTW the call to update_upgrade_enable_dependency() could be moved up one step, to the operation stage 'selection' with the same result as the current placement, but I think you are asking me to move it even earlier, before the update_fix_compatibility() call)

[argiepiano]

@quicksketch - any thoughts about my last question regarding moving this auto-enabling functionality earlier in the upgrade process?

[bugfolder]

Since this issue fell off the end of today's dev meeting and needs @quicksketch input: ping. 😉

[quicksketch]

@argiepiano Okay that makes sense. And the use-case of new dependencies not having requirements met is less likely than the need to inform users of new modules being enabled. I think that's fair.

In that case I think we may be ready to go on this one.

[quicksketch]

With no other concerns raised, I've merged backdrop/backdrop#3950 into 1.x for 1.22.0. Thanks @argiepiano and @bugfolder for really paving the way here! Thanks @docwilmot and @klonos for input and reviews!