Default container_rebuild_required to FALSE

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?

    if ($enable_dependencies) {
      $module_list = $module_list ? array_combine($module_list, $module_list) : [];
      if ($missing_modules = array_diff_key($module_list, $module_data)) {
        // One or more of the given modules doesn't exist.
        throw new MissingDependencyException(sprintf('Unable to install modules %s due to missing modules %s.', implode(', ', $module_list), implode(', ', $missing_modules)));
      }

      // Add dependencies to the list. The new modules will be processed as
      // the foreach loop continues.
      foreach ($module_list as $module => $value) {
        foreach (array_keys($module_data[$module]->requires) as $dependency) {
          if (!isset($module_data[$dependency])) {
            // The dependency does not exist.
            throw new MissingDependencyException("Unable to install modules: module '$module' is  missing its dependency module $dependency.");
          }

          // Skip already installed modules.
          if (!isset($module_list[$dependency]) && !isset($installed_modules[$dependency])) {
            if ($module_data[$dependency]->info['core_incompatible']) {
              throw new MissingDependencyException("Unable to install modules: module '$module'.  Its dependency module '$dependency' is incompatible with this version of Drupal core.");
            }
            $module_list[$dependency] = $dependency;
          }
        }
      }

So we could make a second array of modules specified as dependencies that are either in the module list passed in, or added to the list of modules to install, and then treat all of those as needing a container rebuild. That bit shouldn't be too bad, we've still got magic service trickery to worry about though.

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().

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.~

edit: crossposted with #5 - I think we could rebuild the container when we find a service provider class at all, unless the module has opted into container_rebuild_required: false. This won't catch decorated services in services.yml but it feels like the sort of edge cases we're worried about are more likely to be in modules with ServiceProvider classes?

We can still use container_rebuild_required: true for any modules that aren't caught by this.

#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?

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.

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 #3492705: Install modules with container_rebuild_true set by themselves though since that will ensure that when a module specifies this manually, it really does get its own container rebuild.

edited to fix the issue reference.

Oops yes I did, edited the comment.

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

Removed.