Render cache is not cleared when module is uninstalled

When I uninstall the module I get a white screen with a not-very-helpful error message:

Because non-verbose error messages are enabled by default. See https://www.drupal.org/node/2259531 how to disable that while developing.

I guess the best solution would be to automatically invalidate the render cache of objects affected by the uninstalled module. However I'm not sure if we actually have enough information to do that. Alternatively, we might be able to check if the post render cache callback still exists before trying to call it.

Correct! And I thought this was already what was happening, but apparently, that's not true. I thought all caches were being cleared, but apparently that's not the case :(

To invalidate just the render cache, you only need to do this: Cache::invalidateTags(['rendered'].

It'd be great if you could work on this patch :) I'll provide reviews!

Correct notes, but more generally: ALL caches must be invalidated when uninstalling a module. Lots of things will break if that's not the case. That's why I'm surprised this doesn't happen already. In fact, I'm fairly certain it *does* happen. Hence I'm in fact pretty sure that this is caused by something else.

This changed a few days ago, where we removed the drupal_flush_all_caches() jackhammer from uninstall() and the UI's.

So no, it doesn't happen anymore. And yes, I was expecting we would run into a few cases that we're missing.

I can imagine two things, one would be to go after the render cache explicitly and do a 'rendered' cache tag deletion, or probably better, copy the relevant parts of looping over all cache bins and call deleteAll() into uninstall(). Possibly check if there is something else that we need to clear.

Not sure if we need to flush assets for example? As they, I think, are always built based on a list of js/css that you want to have displayed, if the installed/uninstalled module affects that, it will just results in new asset caches?

Ohhh! Then this is definitely critical. That explains a lot! Thanks for chiming in, Berdir!

The issue is #2248297: Ensure routes are rebuilt when install modules, but that will not help you much.

The part I was talking about from drupal_flush_all_caches() is this:

// 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.
$module_handler->invokeAll('cache_flush');
foreach (Cache::getBins() as $service_id => $cache_backend) {
$cache_backend->deleteAll();
}

That should fix your problem. After that, we need to check all the other stuff we do in drupal_flush_all_caches() and if there is something else we should call.

I didn't see anything in drupal_flush_all_caches() that isn't covered in the

foreach ($module_list as $module) { .. }

loop, other than the code that @Berdir mentions, and what this patch adds back in.

Do we need additional assertions in UninstallTest and InstallUninstallTest? If so, what would be a cache tag that we can guarantee that would exist in those tests to check to see if it is not there? Maybe set one and check it after the uninstall?

Setting a custom cache entry and ensuring it no longer exists after uninstalling a module sounds good to me.

The second sentence in the comment is specific to drupal_flush_all_caches() and should be removed. Instead, explain why we need to clear caches, something like "Caches might implicitly depend on the uninstalled modules, clear them explicitly.".

I tested this fairly thoroughly by commenting out bits and pieces to make sure the test coverage was actually working. The \Drupal::cache()->set() is done immediately before the confirmation POST to ensure that nothing else in the chain has cleared caches.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -1030,8 +1030,8 @@
   +    // so clear all of thethem explicitly.
   

   Not sure what a "tethem" is :)

2. +++ b/core/modules/system/src/Tests/Module/UninstallTest.php
   @@ -74,8 +75,19 @@ function testUninstallPage() {
   +    // Set a unique cache entry to be able to test whether all caches are
   +    // cleared during the uninstall
   +    \Drupal::cache()->set('UninstallTest', 'testUninstallPage', Cache::PERMANENT);
   

   Missing "." at the end of the comment.

   Cache keys are usually lowercase, something like 'uninstall_test'.

"thethem" is a slang term in English for someone developing presbyopia who needs a new prescription from the optometrist. Or someone who makes a sloppy copy/paste :)

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -1028,6 +1029,14 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
 
+    // Flush all persistent caches.
+    // Any cache entry might implicitly depend on the uninstalled modules,
+    // so clear all of them explicitly.
+    $this->invokeAll('cache_flush');

Good! This is why documentation.

Re-roll, moved the snippet to the new class, should be still RTBC if tests pass.

Much stupid.

Attending to issue with patch 20.

- +    $this->invokeAll('cache_flush');
+ +    $this->moduleHandler->invokeAll('cache_flush');

This is the only change compared to #20, and looks sane.

Yep this is a case where cache tags don't really help much, and uninstalling is sufficiently infrequent that a full cache clear is fine.

Committed/pushed to 8.0.x, thanks!