Problem/Motivation

Prior to Drupal 11.2, when we install multiple modules at once, each of them would go through the regular steps of hook_install() and config install, before the next module is installed.
This means that, if module B depends on module A, then in B_install() it can pick up and alter configuration that module B introduced in B/config/optional.

Since Drupal 11.2., this is no longer the case.
If A and B are installed in bulk, then B_install() will fire _before_ the config from A/config/optional is present.
If A and B are installed one after the other, then B_install() will fire _after_ the config from A/config/optional is present.
This seems to be coming from #3496558: Install entity definitions for all modules in batch install before simple config.

Likewise, services from A won't be available in B_install(), if the container was not rebuilt.

This can be mitigated by setting 'container_rebuild_required' on one of the two modules.
But do we really want this, only to make hook_install() work?

Steps to reproduce

  1. Create module A with config/optional/views.view.a.yml
  2. Create module B with B_install() which loads 'views.view.a' and adds some fields to it.
  3. Let B depend on A (not sure if this is needed to reproduce this).
  4. Install both modules together.
  5. Uninstall both, then install A separately, then install B separately.

Expected: In both cases, the changes from B_install() are applied.
Actual: When both modules are installed together, then changes from B_install() are not applied (or B_install() crashes if it does not have defensive code).

Proposed resolution

We could detect if modules have a hook_install(), and if so, put them in separate groups.
Perhaps we could create "subgroups" where we don't do a full container rebuild in between.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Comments

donquixote created an issue. See original summary.

donquixote’s picture

donquixote commented
Issue summary: View changes
donquixote’s picture

donquixote commented
Related issues: +#3496558: Install entity definitions for all modules in batch install before simple config

Here is a list of commits for ModuleInstaller since 11.1.8.

* 4837c52b Issue #3497105 by quietone, borisson_: Fix LineLength for inline comments
* 101e0606 Issue #3496558 by alexpott, godotislate: Install entity definitions for all modules in batch install before simple config
* 6e7715a8 Issue #3492282 by nicxvan, longwave, catch: Refactor away ModuleInstaller::invokeAll()
* 59ce5ede Issue #3494212 by godotislate, alexpott, larowlan: Modules that provide fields for entities of other modules can't be installed
* 57583222 Issue #3487154 by quietone, daffie, smustgrave: Fix Drupal.Commenting.FunctionComment.MissingReturnComment in core/lib/Drupal/Core/A-E
* 58be5f12 Issue #3492235 by catch, longwave, mikelutz: Default container_rebuild_required to FALSE
* 16967de8 Issue #3492705 by catch, nicxvan, mikelutz, longwave: Install modules with container_rebuild_true set by themselves
* f8090e61 Issue #3416522 by catch, alexpott, longwave, phenaproxima, nicxvan, wim leers, smustgrave, larowlan, berdir, godotislate, dww: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller
* eab46c49 Issue #3486995 by nicxvan: Clean up how ModuleInstaller invokes hooks around installing other modules
* 76323d65 Issue #3490296 by nicxvan: Mark hook_install_tasks and hook_install_tasks_alter as procedural only
* 2c9e1fa9 Issue #3486462 by nicxvan: Support #Hook for several hooks called by ModuleInstaller

nicxvan’s picture

nicxvan commented
Related issues: +#3529670: Config dependency difference between Drupal 11.1 and 11.2
nicxvan’s picture

nicxvan commented

I'm wondering if it's worth considering a new hook: hook_post_install_optional

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

I think this type of config was never really meant to be handled in hook install but worked.

I think adding a new hook might give a place to interact with this config safely.

donquixote’s picture

donquixote commented

I think this type of config was never really meant to be handled in hook install but worked.

I don't think the creators of this hook had any preference whether or not people use it to alter configuration from config/optional from another module.

I'm wondering if it's worth considering a new hook: hook_post_install_optional

To me it seems like a clear regression.
We should fix core, rather than asking people to change their code.

donquixote’s picture

donquixote commented

I noticed that "container_rebuild_required" does not really help with config dependencies, because the following code happens before modules are grouped based on 'container_rebuild_required':

    // Check the validity of the default configuration. This will throw
    // exceptions if the configuration is not valid.
    $config_installer->checkConfigurationToInstall('module', $module_list);
donquixote’s picture

donquixote commented

We could detect if modules have a hook_install(), and if so, put them in separate groups.

I found that:

  • Just checking for hook_install() might not be enough.
    There is also hook_module_preinstall() and hook_modules_installed().
    And then there could be funny side effects if e.g. plugin derivers depend on configuration.
  • With the current implementation, just "put them in separate groups" does not really help if a configuration is created in hook_install().
    Because $config_installer->checkConfigurationToInstall('module', $module); is called for all modules in the initial list, not only for those in the group.

Therefore what I proposed cannot be a sufficient solution.
My original idea was to auto-detect which module needs to be in a separate group. But that might just not be viable, we might have to stick with that "container_rebuild_required", and tweak the order of operations in ModuleInstaller.

nicxvan’s picture

nicxvan commented
Related issues: +#3549397: [regression] Uninstalling and installing modules during config:import can lead to fatal errors
nicxvan’s picture

nicxvan commented

So do we need to call: $config_installer->checkConfigurationToInstall('module', $module_list); per group?

And move it later, and still rely on the module maintainers to tag their modules appropriately?

codebymikey’s picture

codebymikey commented

As per point 2 of @smulvih2's #3529670-16: Config dependency difference between Drupal 11.1 and 11.2 write-up, it seems like this change in behaviour needs to be appropriately documented in a change record (if its an intentional breaking change), or fixed, as it affects existing distributions and features modules.

Within the modules making use of optional configs, a backwards compatible workaround is to move the config-dependant logic from within hook_install() to hook_modules_installed() instead.

An example module affected by this is forum module where the forums vocabulary might not be available before it's saved.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

To me at least it is self evident #3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller broke this by changing the landscape, by changing what installed means. Fixing this without reverting to a slower system is somewhere between hard and impossible. (So much so that I knew it's this issue without checking which version it appeared in and then being not surprised at all to see it's 11.2 which this issue complains about.) I am afraid https://www.drupal.org/node/3473563 applies and an update of the change record to cover this is the only "fix" reasonably attainable.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.