drupal_rebuild() rebuilds container twice, since drupal_flush_all_caches() also rebuilds it

We need clarity on the appropriate uses of these cache rebuild functions.

Reviewing this as part of #2474049: [meta] Major issue triage. The issue still exists, drupal_rebuild rebuilds the container and then calls drupal_flush_all_caches which also rebuilds the container.

However, I'm recommending that release managers downgrade this to "Normal". While it touches a nerve with respect to various functions (including hook_cache_flush and hook_rebuild), it doesn't trigger a PHP error or cause a failure.

There is a need for clarity on how cache flushes and container rebuilds should interact. However, the current situation where both drupal_rebuild and drupal_flush_all_caches do both, and the latter does it a little more cleanly, fits better with current use cases where drupal_flush_all_caches is more prevalent:

https://api.drupal.org/api/drupal/core!includes!common.inc/function/call...
https://api.drupal.org/api/drupal/core!includes!utility.inc/function/cal...

It looks as though a lot of the code in this patch has already bade it's way into 8.0.x so I'm not sure how relevant some of it is. I think it needs a good review by someone closer to the issue.

Rerolling

Hmm yeah. This rerolling needs bit of insight on caching. It would be helpful to provide some steps how to test the #2 patch against HEAD?

This is definitely still an issue. Unfortunately, with the current code in HEAD. This is harder to fix that it was in sun's patch in #2.

New patch, the only way we can really do this at this point, I think, is to just move more of this code into dfac(). Otherwise we have no real way to avoid the container rebuild happening twice. It just means dfac() does slightly more work, but is also a safer method. Really this happens via drupal_rebuild() anyway.

The way things currently stand we also iterate over and call deleteAll() on all cache bins twice too.

Alternative is to move the container invalidation stuff into drupal_rebuild() and not have any of that in dfac(). And if people want the full/heavier option - they call drupal_rebuild() instead.

This improves rebuild performance, which is really great! It has a good impact in developer experience, just fixing that is already nice!

+++ b/core/includes/common.inc
@@ -1162,6 +1163,23 @@ function element_info_property($type, $property_name, $default = NULL) {
+  // Force kernel to rebuild php cache.
+  PhpStorageFactory::get('twig')->deleteAll();

@@ -1179,9 +1197,6 @@ function drupal_flush_all_caches() {
   // Wipe the Twig PHP Storage cache.
   PhpStorageFactory::get('twig')->deleteAll();

dup call needed?

Rerolled, and no, I don't think there is reason for two. Just an artefact of moving code...I hope.

Thanks @charginghawk for help triaging this issue! Adding issue credit for the triage. Thanks also @damiankloip for the updated patch.

Given the potential performance implications, I think it makes sense to keep this issue marked as major.

removed two lines which made one test at least pass locally (EditorUpdateTest), not sure about the rest, go bot!

Thanks swentel. I forgot about this one. Yes, seems we probably don't need to be doing that. I think I remember trying to boot() etc... after invalidate for the \Drupal::service('page_cache_kill_switch') call afterwards.

I am not 100% sure we can do that.

if hook_cache_flush fails due to a problem that needs caches cleared first, then you are locked in.

Maybe we need an aggressive_rebuild() function instead?

---

+++ b/core/includes/common.inc
@@ -1080,11 +1082,25 @@ function element_info_property($type, $property_name, $default = NULL) {
+  // Invalidate the container.
+  // Bootstrap up to where caches exist and clear them.
...
+  // Invalidate the container.

This comment is doubled here.

I think this is just a normal priority because drupal_rebuild() is only called from rebuild.php and it not at all part of the normal running of the site. Double building of the container is unfortunate but I'm not sure even qualifies this as a bug.

Regarding the rest of the issue summary:

• At this point I don;'t think we can stop drupal_flush_all_caches() from rebuilding the container now - this would be a very difficult behaviour change for contrib to respond too
• Re usages of drupal_flush_all_caches() in core - it is either update.php, the performance page or tests. Yes it would be great to refactor the tests to not do this but then we'd need a greater separator between runner and system-under-test environments - that I've argued for for a long time but people don't want to do because of performance. The runtime usages look acceptable to me.
• Re hook_rebuild - yes we should open a separate issue to deprecate that and refactor block_rebuild() to something sensible - I think there might be issues about that already.

We should definitely add test coverage of rebuild.php too. I don't think this is a bug it's just a task. Nothing is actually broken and the drupal_rebuild() is not on the critical path.

+++ b/core/includes/common.inc
@@ -1080,11 +1082,25 @@ function element_info_property($type, $property_name, $default = NULL) {
+  // Disable recording of cached pages.
+  \Drupal::service('page_cache_kill_switch')->trigger();

I think this should only really be part of the drupal_rebuild() there's no need for it to go here. It can/should be after the dfac().

This is causing https://github.com/drush-ops/drush/issues/4661 so switching this back to be a bug. I also wonder if this is causing #2985199: Extensions in Multisite Directories Not Registered When Rebuilding Cache

I've tested this with drush and it works perfectly when moving a module.

rebuild.php has a test - see \Drupal\Tests\system\Functional\UpdateSystem\RebuildScriptTest - it got added in #2575267: Add test coverage for rebuild.php

Thanks. I think we need to add an optional $request param to drupal_flush_all_caches() because Drush no longer sets the globals needed for createFromGlobals() to work.

I was looking at the failures, while reading your comment @moshe weitzman.

LogicException: Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead.
Drupal\Core\Render\Renderer->doRender()() (Line: 244)

As part of update.php we call drupal_flush_all_caches() right after running normal updates, and before all the post updates are triggered.
This is called, as part of the update controller. In there the previous patch initializes a new request.

The render system itself uses static::$contextCollection keyed by request to track context throughout the rendering process.
As version #36 of drupal_flush_all_caches() creates a completly new request is let's $contextCollection to not longer find the right render context and stops.

The first possible solution to this problem is what @mosh mentioned in #38: Don't create a new request by passing in the request. This works fine for the update system, but I'm wondering, whether it might break drupal_flush_all_caches() in other places.

Another possible solution could be to properly undertand what's happening. drupal_flush_all_caches() initializes the render system as part of \Drupal::service('twig')->invalidate(), which means it has a reference to the request stack of the second created request.
Later in update.php we then render the page using $this->bareHtmlPageRenderer which has a still a reference to the old renderer and old request stack, while the twig subsystem, which itself calls to the renderer, has the new reference.
Lazily initialising $this->bareHtmlPageRenderer solves the problem:

@@ -192,7 +193,7 @@ public function handle($op, Request $request) {
     }
     $title = isset($output['#title']) ? $output['#title'] : $this->t('Drupal database update');
 
-    return $this->bareHtmlPageRenderer->renderBarePage($output, $title, 'maintenance_page', $regions);
+    return \Drupal::service('bare_html_page_renderer')->renderBarePage($output, $title, 'maintenance_page', $regions);
   }
 

Again this solution has the same problem as before, it just fixes update.php, but not potential other usages of the same method.

A third proposal is to use the $request from \Drupal::request(), if available. This way we just create a new request, if we have not initialized the container before. Maybe this could be combined with proposal one as well.

Thanks @dawehner I think preserving the current request - if available makes a lot of sense. Nice sleuthing.

Here's a test to confirm that we are limiting the amount of rebuilds. This is might even make test runs quicker!

Seems some tests are still failing. I think we need to use the current kernel if it is available.

The new `drupal_flush_all_caches()` rebuilds the container, so any reference to a service, like the theme manager, is now not longer the same one as before.

$this>service in functional tests is always a dangerous this to do - we should remove it from this test and call the test rebuild so everything is correctly on the test too.

For containers and tests see #2066993: Use \Drupal consistently in tests

Let's test hacking in a module via editing core.extension. drupal_flush_all_caches is meant to deal with this.

Posting a small documentation improvement.

Here's a fix for the test and a change that means we don't need to "fix" any tests. So less behaviour change.

I've realised that the entire call to \Drupal\Core\DrupalKernel::updateModules() in drupal_flush_all_caches() is obsolete. So we can completely remove it rather than check if it is going to change anything.

Also we can have less complexity if we pass in a DrupalKernel to drupal_flush_all_caches() - if the caller chooses to do this then it is on them to rebuild the container. This allows drupal_rebuild() to minimise container rebuilds.

Here's a drush cache rebuild on minimal showing quite a large performance boost due to one less container rebuild....

https://blackfire.io/profiles/compare/fae29143-3152-4ce1-8b32-40b677af57...

27% quicker - 50% less queries - 94,000 less calls to preg_match() :D

Ah of course drupal_flush_all_caches() is used as a batch operation. So sometime the first argument will be an array.

+++ b/core/includes/common.inc
@@ -531,43 +544,26 @@ function drupal_flush_all_caches() {
 
-  // Ensure that all modules that are currently supposed to be enabled are
-  // actually loaded.
-  $module_handler->loadAll();
+  // Rebuild module data that is stored in state.
+  \Drupal::service('extension.list.module')->reset();
 
   // Rebuild all information based on new module data.
-  $module_handler->invokeAll('rebuild');
+  \Drupal::moduleHandler()->invokeAll('rebuild');

I've added some test coverage that proves removing $module_handler->loadAll(); is okay. Essentially it is irrelevant and going to be done twice because when we call \Drupal::moduleHandler()->invokeAll('rebuild'); it'll be done again because we've cleared the caches - see \Drupal\Core\Extension\ModuleHandler::buildHookInfo().

Also I added empty_module_test when I was trying to test this with a kernel test and it could reload code twice in the same request because in PHP you can't unload code. Given we're now using a functional test we can use a regular module and test that the code is loaded as expected.

Code looks good, and new tests are most welcome. As an aside, this will speed up drush cache:rebuild as soon as people upgrade their Drupal - no Drush upgrade required.

OK this looks really good to me.

We have a patch release scheduled next week, and I'd prefer to give this issue slightly longer than that in the release branch before it goes into a tagged release, so for now just committing to 9.2.x and marking 'to be ported' to 9.1.x, which we can do once the next patch release is out. Since this is likely to fix multiple major bugs in the queue, as well as the performance improvement, I think it's fine to add the optional param in a patch release.

Committed 6e84b57 and pushed to 9.2.x. Thanks!

🤩

I forgot this issue even existed! Epic work 👏

Code looks good, and new tests are most welcome. As an aside, this will speed up drush cache:rebuild as soon as people upgrade their Drupal - no Drush upgrade required.

And perhaps stating the obvious but worth stressing: faster Drupal deployments in general, for both custom/manual deployment scripts and drush deploy.

I think this is worth a release note? 🤓

Yep let's add a release note.

More of a highlight.

Oh wow! Exciting!

Miss you to sun! Finding more issues created by you getting closed recently and consider that a sign Drupal 8/9/10 is getting more mature and hard problems are getting solved. :-D

Would have been nice but not sure if this is worth porting at this point since 9.2 is right around the corner. Patch seems to apply cleanly though if maintainers want to commit it.

Yep, moving this to fixed against 9.2.x

Hi
This change breaks core and contribs in different places
https://www.drupal.org/project/drupal/issues/3222577#comment-14163851

Appreciate any help