Ensure routes are rebuilt when install modules

Not too worried about a performance regression here. There's several bits in module_enable() where it behaves differently if you install one module vs. ten, we should just fix that.

Note: we should do the same when we uninstall a module.

The update module is not enabled on install, see #2307939: Update notifications are not enabled.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -636,6 +636,9 @@ public function updateModules(array $module_list, array $module_filenames = arra
   +      if ($this->containerNeedsDumping && !$this->dumpDrupalContainer($this->container, static::CONTAINER_BASE_CLASS)) {
   +        $this->container->get('logger.factory')->get('DrupalKernel')->notice('Container cannot be written to disk');
   +      }
   

   What about just calling getContainer() or extract this logic from getContainer()?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -880,6 +880,8 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    
   +    \Drupal::service('router.builder')->setRebuildNeeded();
   +
   

   Ah great this is already in a state, so it will happen in the next request, if needed.

Interdiff in #8 should probably go into #2342807: DisplayPathTest methods enable menu_ui module when it is already enabled.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -636,6 +636,9 @@ public function updateModules(array $module_list, array $module_filenames = arra
     // and container.
     if ($this->booted) {
       $this->initializeContainer(TRUE);
+      if ($this->containerNeedsDumping && !$this->dumpDrupalContainer($this->container, static::CONTAINER_BASE_CLASS)) {
+        $this->container->get('logger.factory')->get('DrupalKernel')->notice('Container cannot be written to disk');
+      }
     }

This looks very similar to the problem encountered over in #2331909-20: Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator, comment in #23:

This patch accidentally removed every invocation of DrupalKernel::getContainer() and therefore the container was not dumped anymore.

Why don't we dump the container in initializeContainer()? Maybe @neclimdul remembers the details?

I am pretty sure this needs a re-roll now ...

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -880,6 +880,8 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+    \Drupal::service('router.builder')->setRebuildNeeded();
+

These are the only two lines of code that need to be left in this patch! Once those two lines are added and we have some test coverage, this is RTBC.

That would be awesome - if we could!

Only 4 fails, go berdir!

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -999,7 +1000,9 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +      \Drupal::service('router.builder')->setRebuildNeeded();
   

   Just in general: You could argue that this maybe belongs in the DrupalKernel but yeah out of scope of this issue.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -61,11 +62,11 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
   +   * The kernel to handle forms returning response objects.
   ...
   +   * @var \Drupal\Core\DrupalKernelInterface
       */
   -  protected $httpKernel;
   +  protected $kernel;
   

   Can we explain somewhere why we use the drupal kernel and not the HTTP handler? Code here is always potentially fragile

3. +++ b/core/tests/Drupal/Tests/Core/Form/FormTestBase.php
   @@ -117,7 +117,7 @@
       */
      protected $httpKernel;
   

   Meh

#26 Patch looks great, but why does FormBuilder needs to use DrupalKernel instead of HttpKernel and under what circumstances would other modules, e.g. contrib, need to use the same?

We definitely have a problem with mid-request container rebuilds. This patch only fixes occurrences where this happens from within forms. We should not take that as granted though, therefore I think we need a different solution here.

Ironically only new style OO code is affected by this problem. In contrast in procedural code (e.g., hook implementations) a fresh version of a service is always fetched directly from the container.

While ugly, I believe something like this would be safer because it will also work when the container rebuild was performed outside of a form.

diff --git a/core/lib/Drupal/Core/EventSubscriber/RouterRebuildSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/RouterRebuildSubscriber.php
index 584b141..a93609d 100644
--- a/core/lib/Drupal/Core/EventSubscriber/RouterRebuildSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/RouterRebuildSubscriber.php
@@ -43,7 +43,7 @@ public function __construct(RouteBuilderInterface $route_builder) {
    *   The event object.
    */
   public function onKernelTerminate(PostResponseEvent $event) {
-    $this->routeBuilder->rebuildIfNeeded();
+    \Drupal::service('router.builder')->rebuildIfNeeded();
   }
 

Probably in a follow-up, but there are lots of $this->rebuild() calls in KernelTestBase tests which can be removed with this.

@Berdir rocks!

This change is fantastic.

Yes this is one of my favourite patches for a very long time. Took us the past three years to get to the point where this was viable.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -1022,8 +1025,6 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   -    drupal_flush_all_caches();
   

   Yep.

2. +++ b/core/modules/config/src/Form/ConfigSync.php
   @@ -396,7 +396,6 @@ public static function finishBatch($success, $results, $operations) {
   -    drupal_flush_all_caches();
   

   Yep.

3. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -511,7 +511,6 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -      drupal_flush_all_caches();
   

   Yep.

Yep yep yep yep.

Committed/pushed to 8.0.x, thanks!

@Berdir: On that reInjectMe: I assume this is done manually right now?

Can't we have an old kernel subscriber that informs all the services with the new injected services.

I now that injection works via __construct, but if we just theoretically made all objects be like:

interface InjectableInterface {
  // Nothing here as injectServices is generic.
}

abstract class InjectableObject implements InjectableInterface {
  public function __construct() {
    call_user_func_array(array($this, 'injectServices'), func_get_args());
  }
}

MyObject extends InjectableObject {

  public function injectServices($container, ) {
    $this->container = $container;
  }
}

and have the container keep track of all objects implementing InjectableInterface would that it not make possible to generically re-inject all services from the container?

Without using synthetic services at all ...

I'd rather prefer to get rid of mid-request container rebuilds in the long run. This should be the responsibility of a (headless?) installer.

Works for me! - If that is not a requirement, it was just an idea how to solve it more generically.

Oh, and there is still #2285621: Remove persist-tag and replace it with a container.state_recorder service where it is pointed out why we should not delegate state recording/restoring to the services themselves.