Consider defaulting requires_container_rebuild to FALSE

Wondering if we can do this in a safe way. If a module calls \Drupal::service() in hook_install() for a service provided by a just-installed dependency then we need to rebuild the container, but that's the problem of the second module - the container only needs to be rebuilt if both are installed at the same time.

Do we know what other scenarios require a container rebuild?

The example @alexpott gave was one of the config trickery modules in contrib which affects how config is installed, I don't think it's config_split but that general area. We could maybe look for the magic service provider class since that is consistently named, if that's a good indication of the problem.

Drupal::service() calls in hook_install() is a different problem I had not even really thought of, but there is no way for modules to know that another module is going to do that, so container_rebuild_required won't help. However that also seems pretty fragile anyway. I guess we could try to detect when we've just installed a module that another module we're about to install depends on, and rebuild the container then?

Found the reference #3416522-22: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller →

Thunder uses the config_selector module. That module decorates the config installer and changes the behaviour of installDefaultConfig().

For #4 I'm trying to think how we could automatically detect that but it's tricky because the decoration is done in a service provider class.

https://git.drupalcode.org/project/config_selector/-/blob/8.x-2.x/src/Co... does exist, so

This from DrupalKernel:

    // Load each module's serviceProvider class.
    foreach ($module_filenames as $module => $filename) {
      $camelized = ContainerBuilder::camelize($module);
      $name = "{$camelized}ServiceProvider";
      $class = "Drupal\\{$module}\\{$name}";
      if (class_exists($class)) {
        $this->serviceProviderClasses['app'][$module] = $class;
      }

So a combination of checking dependencies and whether this class exists might reduce any potential disruption by a lot.

Building on #3, maybe we can just rely on dependencies? If we build a graph of dependencies then we can guarantee container rebuilds between two modules that depend on each other, but we can still combine the branches of the graph in a way that minimises the total number of rebuilds that is required.

If this works then all that modules have to do is ensure their dependencies are correctly declared.

#7 I think will be fine for the 'module calls a service of another service during installation'. If we put all the depended-upon modules at the start, that would be enough to minimise the number of container rebuilds.

However it won't solve the problem of #6 where modules are impacting the installation process itself - so maybe we can do the dependencies here, and then split installer-modification out to its own issue, or just not fix that and open issues against modules like config_selector?

I haven't looked but I don't understand in the config_selector case how it ensures it is installed first when multiple modules are installed simultaneously, if it affects the install process itself.

If we build a graph of dependencies then we can guarantee a container rebuild between a module and any dependency, but we can still combine the branches of the graph in a way that minimises the total number of rebuilds that is required.

I started thinking about this but I think we might be fine without any special logic.

The thing we need to guarantee that is after a module that is a dependency of a module we're also installing is installed, we do a container rebuild.

We could potentially optimize things by grouping together modules that are dependencies of other modules, and then only rebuild the container after those are installed.

However if we do that, we need to account for modules that depend on one module and are also depended upon by another module.

We could handle that by just in time rebuilding the container when a module that specifies dependencies is installed, but we only need to do that if another module with the same dependency wasn't just installed - e.g. if two modules depend on views, we only need to container rebuild once.

If we just find all the modules that are dependencies of other modules, rebuild the container after each one, then we still get a good distribution without anything extra on top.

Let's say three modules that are dependencies and three that specify those as dependencies. xxx and yyy.

If we do xxx yyy - that's four container rebuilds (3 + 1 at the end).

If we do xy xy xy that's also four container rebuilds.

ModuleHandler::buildModuleDependencies() already builds a graph of dependencies so I was hoping we could use that.

I think that we need install modules in the order of the longest possible route through the graph, which will ensure that all dependencies are met by the time each subsequent module gets to be installed.

Drupal::service() calls in hook_install() is a different problem I had not even really thought of, but there is no way for modules to know that another module is going to do that, so container_rebuild_required won't help. However that also seems pretty fragile anyway. I guess we could try to detect when we've just installed a module that another module we're about to install depends on, and rebuild the container then?

Is this an issue though? From a brief reading of the issues and code it seems like we rebuild the container for the batch prior to running any of the install hooks for the batch anyway. We also rebuild the container before the config installer is used, so I'm not even sure that config_selector would be an issue, as we would be using the updated config installer for the batch containing config_selector too. Seems like what would be different would be on hook install you would have items in the container that wouldn't have been there previously, i.e. some service might be decorated from a module dependent on this module that wouldn't have otherwise been decorated when this module is being installed.

So if module a depends on module b, and module a decorates some service, currently it knows that when module b is installed it is using the original service and not the decorated service. With this change it can't be sure of that, and the flag won't help. But I can't think of a scenario or example that would make that an issue? Possible though.

I've put a simpler MR up at https://git.drupalcode.org/project/drupal/-/merge_requests/10495 which only changes the default and doesn't try to handle dependencies, because as @mikelutz points out, hook_install() looks to be already covered.

Postponing on 📌 Some of the package manager kernel tests are much slower than other kernel tests - mark them #slow Active though since that will ensure that when a module specifies this manually, it really does get its own container rebuild.

I think you meant to postpone this on 📌 Install modules with container_rebuild_true set by themselves Active , no?

Oops yes I did, edited the comment.

📌 Install modules with container_rebuild_true set by themselves Active is in. Rebased https://git.drupalcode.org/project/drupal/-/merge_requests/10495

After all the discussions here, and in slack I think it really is this simple. I've updated the change record here → . I really think the best thing is to get this into 11.2 now, as early as possible, just in case there are any unexpected patterns in contrib that this might negatively affect. This will give us as much time as possible to identify and adjust.

If the default is now false we can remove container_rebuild_required: false from all .info.yml files again.

Removed.

+1 to back to RTBC

longwave → changed the visibility of the branch 3492235-rebuild-required to hidden.

Committed 58be5f1 and pushed to 11.x. Thanks!

longwave → closed merge request !10495

Automatically closed - issue fixed for 2 weeks with no activity.