Add centralized container invalidation method

Here is the patch from that issue.

Uhm, #4 was fail as it should have used $this->storage() for now.

I checked that there are no more direct references to the PhpStorageFactory with the container directly.

Just some quick feedback about the API.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -357,7 +357,7 @@ public function getAppRoot() {
+  public function boot($rebuild = FALSE) {
     if ($this->booted) {
       return $this;

Mh I'm not sure whether we really need a rebuild parameter. This feels optional for me. Can't you also always use $kernel->invalidateContainer() and then boot()? Splitting it in two steps is not a bad idea, IMHO.

#6: No, this is needed for being able to do core/rebuild.php.

Else this will quickly get really complicated, because without bootstrap container, how could you access the database or any other service to delete?

Or rather how can invalidateContainer() ensure that the container is invalidated, when settings might not yet be loaded.

Unless we want to do:

invalidateContainer() {
  if (!$this->booted) {
    $this->rebuildContainer = TRUE;
    return;
  }

  // Do the real invalidation.
}

That could work. Thoughts?

New approach using a flag if the container needs to be rebuild on next initialization or not.

I think that should be more correct in general anyway.

e.g. updateModules() if ever called before boot (no one in core) would have been broken, because if there would have been a cached container it would have been used.

This looks better for me!

For now I think this is a sane step forward.

I'm not sure whether its a good idea to encourage more code to use the kernel directly instead of providing something in the middle which does the rebuilding, and is used by kernel.

Is the invalidate method testable using a unit test or a kernel test?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -781,6 +788,11 @@ protected function initializeContainer($rebuild = FALSE) {
+    // The container was rebuild successfully.
+    if ($this->containerNeedsRebuild) {
+      $this->containerNeedsRebuild = FALSE;
+    }

Why bother with the if?

Forgot to say that this is way better than reaching into PhpStorage from the config system, +1 to the approach.

Can we please have a beta eval as well?

#17 I don't believe an event will work in the core/rebuild.php case or in the drush cr case, because it means you need to have the invalidation service loaded to invalidate the container.

That feels wrong, while isolating to the Kernel we can actually swap out the service.

Edit:

Talked a little more in IRC:

< Fabianx-screen> msonnabaum: The idea is to make that swappable.
< Fabianx-screen> msonnabaum: Once it is internal to the kernel (regardless if we have a bootstrap container or not) we can implement swappability without any overhead.
< Fabianx-screen> msonnabaum: And I think that is better than something listening to an event that is not even loaded yet at core/rebuild.php time.
< Fabianx-screen> e.g.
< Fabianx-screen> msonnabaum: So a module would miss most of the rebuild information.

Thanks for all the reviews!

#13: Fixed.

Added tests and beta evaluation.

Adding correct interdiff for #19.

Removed the sleep(5), which was only necessary because my local opcache is set to 2s revalidate interval, so it saw the old settings.php still.

Test looks fine, thanks!

I also very much like the fact that this issue moved away from events. Container rebuilds are to avoid at all cost on production sites. Exposing a high-level API for rebuilding the container is not a good idea IMHO. I am fine with the new approach though.

+1 for the RTBC

Let's get a CR for this. So that if any custom/contrib code is calling PhpStorageFactory::get('service_container')->deleteAll(); they can discover it needs changing.

Just nits.

1. +++ b/core/includes/common.inc
   @@ -1295,8 +1295,10 @@ function drupal_flush_all_caches() {
   +  // Wipe the PHP Storage cache.
   

   Wipe the Twig Php Storage cache?

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -127,6 +127,13 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +   * Whether the container needs to be rebuild the next time it is initialized.
   

   s/rebuild/rebuilt

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -695,11 +702,13 @@ public function updateModules(array $module_list, array $module_filenames = arra
   +    // containerNeedsRebuild flag so that a potentially cached container is not
   

   Does this need a self::?

4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -781,6 +788,9 @@ protected function initializeContainer($rebuild = FALSE) {
   +    // The container was rebuild successfully.
   

   rebuilt.

5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -998,15 +1008,33 @@ protected function persistServices(ContainerInterface $container, array $persist
   +    // Also wipe the PHP Storage caches, so that the container is rebuild
   

   rebuilt.

Created https://www.drupal.org/node/2528246 as change record draft.

Will work on the nits next.

Fixed all the nits.

Committed 5a528e5 and pushed to 8.0.x. Thanks!