Compiled data in PHP storage is cleared too late in drupal_flush_all_caches()

Sending patches to the bot.

Not the block module.

+ // @todo Once file system settings are converted to configuration, this means
+ // a massive circular dependency, since the configuration system requires a
+ // working kernel.

BootstrapConfigStorageFactory::get

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -262,9 +269,16 @@ protected function getClassName() {
+      // Collect all instances of synthetic services.
+      foreach ($this->container->getServiceIds() as $id) {
+        if (!isset($persist[$id]) && $this->container->initialized($id)) {
+          $persist[$id] = $this->container->get($id);
+        }
+      }

1. The comment says "synthetic services", but what's actually returned also includes non-synthetic services for which get() has already been called.

2. 'service_container' is a synthetic service that we do not want to persist.

This fixes the second issue only (and not very nicely). CNR for bot.

Here's a more robust fix to both parts of #11.

The test failure in #12 appears to be a latent bug with how/when LocaleConfigSubscriber gets added to the event dispatcher. The bug appears to get surfaced by the changes to dfac(), but masked by HEAD's overly aggressive service persistence across container rebuilds. To untangle it, I suggest committing #1187726-135: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) first.

Updated issue summary.

That line of code is only called within buildContainer() where we know we have a ContainerBuilder. Looks like #1187726-138: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) is RTBC, so once that's committed, I'll reroll #12 and try to work through the Locale test failure.

Another spin-off for addressing the test failure: #1878512: Clean up Simpletest's kernel/container preparation/rebuild logic now that the kernel is responsive to module enabling

This combines #21 with #1878512-12: Clean up Simpletest's kernel/container preparation/rebuild logic now that the kernel is responsive to module enabling. While the issues have differing scopes, they also have some overlap, and the individual patches are small enough, that even combining them into this one patch is still reviewable, I think.

And a small comment tweak.

#26: unfreeze.26.patch queued for re-testing.

#26: unfreeze.26.patch queued for re-testing.

Reroll.

1) Not sur if the reroll is 100% correct as some stuff changed
2) I don't need any credits for this one.

#31: unfreeze.31.patch queued for re-testing.

testbot fail reference: FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh.

New try

wrong patch...

+++ b/core/includes/bootstrap.incundefined
@@ -3615,3 +3616,105 @@ function lock() {
+  // @todo Once file system settings are converted to configuration, this means
+  //   a massive circular dependency, since the configuration system requires a
+  //   working kernel.

As it looks now, this is not going to happen but the public path will move to $settings. So not sure i that needs to be documented.

Basic re-roll, renamed some rebuild/prepareContainer() method calls. Most of the test fails seem to be about theme() thinking that the module system is not yet fully bootstrapped?

Here's a reroll of #21. I think after the module handler went in, the test related changes became more complicated, what about getting this in separately?

This seems stalled, taking it for sprint work.

This is about half-rewrite. I have added a new rebuild.php that can be requested via PHP and a rebuild.sh that calculates the token necessary. It's a small one but I think it'll do the job. It might be the answer to some drush woes as well. I am not terribly sure how helpful the interdiff is...

This is solvable but the solution makes me cringe. It also makes me question wtf are we doing in tests re container.We have $this->container in tests. If this is not the same object as Drupal::getContainer() then whatever reset, set, etc etc we do via the container on one is not going to affect the services on the other. That's just bad. So in order to keep these together, WebTestBase already does two full container rebuild mostly because of this. After this patch, every time you do a drupal_flush_all_caches somewhere, your test needs to redo the $this->container = \Drupal::getContainer() part. I added that to resetAll hoping that will be enough but I can't be sure.

I see two exits:

1. Don't do this updateModules call in drupal_flush_all_caches. It's almost superfluous, the only way the list of modules can change is if you delete one from disk. Move it perhaps to rebuild.php?
2. Just throw out the 146 $this->container calls from the tests and use Drupal::getContainer. Who are we trying to fool by using $this->container, it's not like we inject anything into tests or we even could inject anything into tests...

Oh and both is an option too :D

It's almost superfluous, the only way the list of modules can change is if you delete one from disk.

The list can change when :

• adding a new module,
• deleting a module files,
• adding a new version of a modules with sub-modules (although in D8 it would not be the case),
• moving the database to a prod/staging/dev server that do not share the exact list of modules.

The "list of modules" in question is the list of enabled modules and that can only change by deletion but, given mostly the patch passes, this is moot now. I will look into the failing test.

I filed separate issues for the interdiff #1948650: Update is broken and #1948702: module_hook is broken for disabled modules and included hooks . Once those two are in, just press re-test on #50. This patch here just shows that once those two are in, we are in business.

This should be postponed as per #54

Nope, nor those patches nor #54 merits a postpone yet -- we need reviews first and then we agree it's ready, we can postpone it then waiting for those.

#50: 1872522_50.patch queued for re-testing.

Here is a reroll of #50 run into conflict

--- a/core/includes/common.inc
+++ b/core/includes/common.inc
@@@ -6367,12 -6365,8 +6368,17 @@@ function drupal_flush_all_caches()
    // Flush all persistent caches.
    // This is executed based on old/previously known information, which is
    // sufficient, since new extensions cannot have any primed caches yet.
++<<<<<<< HEAD
 +  module_invoke_all('cache_flush');
 +  foreach (Cache::getBins() as $service_id => $cache_backend) {
 +    // @todo remove form after http://drupal.org/node/512026 is in.
 +    if ($service_id != 'cache.form' && $service_id != 'cache.menu') {
 +      $cache_backend->deleteAll();
 +    }
++=======
+   foreach (Drupal::service('module_handler')->invokeAll('cache_flush') as $bin) {
+     cache($bin)->deleteAll();
++>>>>>>> patch from http://drupal.org/node/1872522#comment-7196632
    }

    // Flush asset file caches.

and kept the head code as per @chx recommendation on IRC

The approach and the code look good and useful to me.

#58: 1872522_58.patch queued for re-testing.

+++ b/core/includes/common.incundefined
@@ -6385,15 +6386,30 @@ function drupal_flush_all_caches() {
   // @todo Rebuild the kernel/container.

Lets update this comment if we are keeping updateModules()

+++ b/core/includes/common.incundefined
@@ -6385,15 +6386,30 @@ function drupal_flush_all_caches() {
+  Drupal::service('kernel')->updateModules(Drupal::service('module_handler')->getModuleList(), $files);
...
+  Drupal::service('module_handler')->loadAll();

@@ -6409,18 +6425,14 @@ function drupal_flush_all_caches() {
+  Drupal::service('module_handler')->invokeAll('rebuild');

Lets store somewhere module handler instead of calling Drupal::service('module_handler') all the time

+++ b/core/includes/common.incundefined
@@ -9,6 +9,7 @@
+use Drupal\Core\DrupalKernel;

where is this needed?

Lets store somewhere module handler instead of calling Drupal::service('module_handler') all the time

Also we can now use the new and shiny \Drupal::moduleHandler() directly.

changing status to needs work to address #61 and #62

#64: 1872522_64.patch queued for re-testing.

*waves hand* this is not the module handler you are looking for.

+++ b/core/rebuild.phpundefined
@@ -0,0 +1,25 @@
+<?php

+++ b/core/scripts/rebuild_token_calculator.shundefined
@@ -0,0 +1,8 @@
+<?php

Can we have a @file to describe what this is all about?

+++ b/index.phpundefined
@@ -9,4 +9,10 @@
+  print 'If you have just changed code (for example deployed a new module or moved an existing one) read http://drupal.org/documentation/rebuild';

This is an intersting and helpful idea. Can we please make it translatable.

Added some documentation. Translateable is hard because you are practically guaranteed that Drupal is not working and both reading the current language and the translation in that apgnauge is a significant challenge. I think we might be able to do something better in a followup.

He, you broke index.php :)

+++ b/core/rebuild.phpundefined
@@ -1,5 +1,12 @@
+ * Rebuilds all Drupal caches even when Drupal itself does not work.

+++ b/core/scripts/rebuild_token_calculator.shundefined
@@ -1,5 +1,13 @@
+/**
+ * Command line token calculator for rebuild.php.

Thanks for the documentation. please add a @file

meh.

Who is going to create the "rebuild" page?

If you think of http://drupal.org/documentation/rebuild I will write it.

#75: 1872522_75.patch queued for re-testing.

@chx looks like that patch fails with the latest core build. here is my attempt at the updated patch.

Last patch did not include file additions. Lets try this again

+++ b/core/includes/common.incundefined
@@ -6089,16 +6088,32 @@ function drupal_flush_all_caches() {
-  Drupal::entityManager()->clearCachedDefinitions();
+  Drupal::service('plugin.manager.entity')->clearCachedDefinitions();

This change is not required see #1982984: Create Drupal::entityManager for improved DX.

Thanks @jibran. Updated per #83 - #1982984: Create Drupal::entityManager for improved DX

I was reading and noticed a mis-spelling.
Fixed comment line wrap and moved a comment while I was at it.

#83: 1872522_83.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -431,7 +431,19 @@ protected function buildContainer() {
+    // Identify all services whose instances should be persisted when rebuilding
+    // the container during the lifetime of the kernel (e.g., during a kernel
+    // reboot). Include synthetic services, because by definition, they cannot
+    // be automatically reinstantiated. Also include services tagged to persist.
+    $persist_ids = array();
+    foreach ($container->getDefinitions() as $id => $definition) {
+      if ($definition->isSynthetic() || $definition->getTag('persist')) {
+        $persist_ids[] = $id;
+      }
+    }
+    $container->setParameter('persistIds', $persist_ids);

I'm really glad this docblock is here. Very helpful.

+++ b/index.phpundefined
@@ -9,4 +9,10 @@
+try {
+  drupal_handle_request();
+}
+catch (Exception $e) {
+  print 'If you have just changed code (for example deployed a new module or moved an existing one) read http://drupal.org/documentation/rebuild';
+  throw $e;

The only possible change I'd say we would want to make here is moving the try/catch inside drupal_handle_request(), but there are pros and cons to both.

---

Overall, the changes to existing files make 100% sense to me.

I'm less certain about core/rebuild.php and core/scripts/rebuild_token_calculator.sh.

Not enough not to commit, just that I'd like catch's signoff here.

I'm also not sure about core/rebuild.php and core/scripts/rebuild_token_calculator.sh. We've got existing issues to add something like that (although those were originally due to the D7 registry) and I'm sure there's discussion on those that isn't reflected here. For example why a cli token generator and not a flag in $settings similar to update_free_access? The latter would allow people with no cli access to rebuild for example, whereas this makes things command line only - but if it's command line only then why not directly rebuild in the shell script anyway?

I'd really like to get the bug fix in though (and that bit looks great), so could we maybe do a new issue?

Same patch sans rebuild files.

Committed 750b527 and pushed to 8.x. Thanks!

Might I suggest 1) we write the node and 2) make the url a link?

@neclimdul: Looks like you need to open a new issue. I do not think your suggestion applies here.

I've moved neclimdul's suggestion to create the node to #2003204: Create http://drupal.org/documentation/rebuild.

neclimdul's suggestion to make the URL a link has been moved to #2003224: Make http://drupal.org/documentation/rebuild a link in index.php.

@chx: The page http://drupal.org/documentation/rebuild is ready for you to add content.

The error message that was added to index.php here is a bit vague. I filed #2056915: Improve error message when drupal_handle_request() fails so we can try and improve it.

Change author to sun to match author of previous issue & summary. --xjm

Here is the issue for rebuild.php.

#2097189: Add a rebuild script