Problem/Motivation

  • Modules can implement preprocess functions, hook_entity_view_alter(), and other code that affects what is rendered. Therefore, installing a module should invalidate all cached renderings (anything with the 'rendered' cache tag).
  • In #2493091: Installing block module should invalidate the 'rendered' cache tag, it was made the responsibility of block module to do this for itself.
  • Other modules, however, do not have a similar hook_install() implementation. And yet, quickedit and rdf are two examples of modules that need it, and there are likely many others throughout core and contrib.

Steps to reproduce

  • Go to the path alias page, then install pathauto, refresh the page and the new locals tasks don't show up (wondering if local tasks block should have a cache tag that is invalidated when local task plugin cache is invalidated for other use cases).

Proposed resolution

  • Somewhere within the code path of module installation (for any module, not just block module), we should call Cache::invalidateTags(['rendered']);. TBD as to where.
  • Additionally, are there other caches we should invalidate when modules are installed? Seems to me it should be pretty much all caches, or is that overkill?

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Title: Module install/uninstall doesn't invalidate render cache » Module install doesn't invalidate render cache
Issue summary: View changes

Actually ModuleInstaller::uninstall() does flush all caches, so updating title and summary to only deal with installation.

dawehner’s picture

Additionally, are there other caches we should invalidate when modules are installed? Seems to me it should be pretty much all caches, or is that overkill?

One thing we should probably do in \Drupal\Core\EventSubscriber\MenuRouterRebuildSubscriber::onRouterRebuild is to not only invalidate local tasks, but also local actions and contextual links. Given that I bet there is some issue which does that already.

Berdir’s picture

Issue tags: +Novice, +Needs tests

Agreed, wanted to create this issue for a long time already.

@dawehner: those are plugins, we invalidate all plugin caches already, so that should be fine?

But yes, local tasks are a common case where you see this bug.

For example, go to the path alias page, then install pathauto, refresh the page and the new locals tasks don't show up (wondering if local tasks block should have a cache tag that is invalidated when local task plugin cache is invalidated for other use cases).

Tagging as novice, because at least adding the same line in install() as we already have in uninstall() should be easy. Not sure about test coverage, we could just remove block_install() again or remove the code and we should have failing/passing tests?

tim.plunkett’s picture

joshi.rohit100’s picture

Status: Active » Needs review
FileSize
597 bytes

Lets see if this can be used as test-only patch as per #5.

joshi.rohit100’s picture

Shouldn't render cache be cleared on theme registry rebuild as we are rebuilding the theme registry on install ?

Status: Needs review » Needs work

The last submitted patch, 7: 2783791-7-test-only-patch.patch, failed testing.

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
1.18 KB

If this simply fixes or not.

effulgentsia’s picture

Shouldn't render cache be cleared on theme registry rebuild as we are rebuilding the theme registry on install ?

Yes, seems to me that in \Drupal\Core\Theme\Registry::reset(), where we do Cache::invalidateTags(array('theme_registry'));, perhaps we should also add 'rendered' to that argument? Especially, since 'theme' is a required_cache_contexts for the render cache, seems to me that anything that changes the behavior of a theme needs to invalidate anything that was using it as a context. Do folks here think we should do this as part of this issue, or open a separate one to discuss it? E.g., ImageStyle::flush() currently calls drupal_theme_rebuild(), so that's one example where invalidating the render cache in that flow is slightly out of scope for this issue's current title.

Berdir’s picture

No strong opinion on that. One (positive) side-effect of that would be that a drush cr would then also invalidate the rendered cache tag, for example (through \Drupal\Core\Extension\ThemeHandlerInterface::refreshInfo). That's something I wanted to suggest for a while, because it means that a drush cr is then also sent an invalidation to e.g. varnish and other proxies, which is something that doesn't happen at the moment.

It does seem a bit unrelated to this. And in fact we'd actually invalidate that tag twice then in install/uninstall. Compared to all the other things we're doing, that's pretty harmless though :)

So I'm fine either way.

Berdir’s picture

Issue tags: -Needs tests

I think those tests are sufficient? And if we need test coverage for that other change, then that might be an argument for a separate issue?

effulgentsia’s picture

Works for me. Should we move the invalidation to earlier? E.g., prior to hook_modules_installed()? Also, are we ok with it only being invalidated after all modules are installed? Meaning each module's hook_install() itself is still running with a stale render cache, but maybe that's ok if we're assuming hook_install() implementations don't render anything?

dawehner’s picture

That is a good point @effulgentsia. Maybe we should put it next to

      // Clear plugin manager caches.
      \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions();

,and if not, just for consistency reasons.

Berdir’s picture

Status: Needs review » Needs work

Sure, lets move it there.

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

I think this is good to go then? Except maybe creating that follow-up for theme registry cache invalidation.

dawehner’s picture

I'm wondering whether having some test coverage for this could be helpful.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -199,6 +199,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+        // Clear render cache.
+        Cache::invalidateTags(['rendered']);
+

Should we use \Drupal::service('cache_tags.invalidator')->invalidateTags($tags) instead?

catch’s picture

Status: Reviewed & tested by the community » Needs review

Other modules, however, do not have a similar hook_install() implementation. And yet, quickedit and rdf are two examples of modules that need it, and there are likely many others throughout core and contrib.

There are also lots of modules that don't need it because they don't affect rendering. Does enabling say syslog module really need to invalidate cached pages in reverse proxies?

If we had the theme registry cache invalidation, do we need the invalidation here at all? Then the theme registry invalidation could potentially detect whether it actually changed or not. The more explicit cache clears we add to module install, the harder it is to do things like that.

effulgentsia’s picture

Then the theme registry invalidation could potentially detect whether it actually changed or not.

Interesting idea. But what if a module implements hook_ENTITY_TYPE_view_alter(), hook_block_view_alter(), or some other hook outside of the theme system that affects rendering?

catch’s picture

@effulgentsia that's why I said 'potentially' - we're not there yet with hook implementations, but adding/relying on explicit clearing in the module system gets us further away from that.

effulgentsia’s picture

we're not there yet with hook implementations

So potentially, we could:

  • Add a 'cache_tags' key to hook_hook_info().
  • Add ['rendered'] as a value for the hooks that are involved in rendering (per #21).
  • Have ModuleInstaller::install() check if the installed modules implement any hooks that have cache tags, and if so, invalidate them.

But:

  1. Would this actually be more performant than just clearing the render cache blindly?
  2. Until we find out, what should we do? Commit something like #17, or add a hook_install() implementation to do so within rdf module and other modules we find that need them?
catch’s picture

What I was thinking here was clear the render cache in theme registry rebuilds, which will in practice clear it on module install - that fixes the bug in the meantime.

Adding the hook_install() in the modules that need it also works.

dawehner’s picture

The more I think about it, the less I think we should special case the render cache here. If you are honest, given hook_module_implements_alter() basically every cache depends on the list of active modules.
What about making config:core.extension a required cache tag?

tim.plunkett’s picture

Works when running the failing test (\Drupal\block\Tests\BlockInstallTest::testCacheTagInvalidationUponInstallation) from #7
Guessing it will fail many other tests that assert cache tags though.

Status: Needs review » Needs work

The last submitted patch, 26: 2783791-render-26.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
12.57 KB

Status: Needs review » Needs work

The last submitted patch, 28: 2783791-render-28.patch, failed testing.

dawehner’s picture

Somehow I would have expected to patch \Drupal\Core\Render\Renderer::doRender to add the cache tags

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
22.34 KB

Not sure why that would be better or worse than where 'rendered' is set, but open to other ideas.
This is my last patch trying to fix fails, let's discuss!

Status: Needs review » Needs work

The last submitted patch, 31: 2783791-render-30.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
952 bytes

+1 for the idea behind this approach.
-1 for the implementation.

First problem with the implementation: it only is being applied to the total rendered page. Not to individual fragments. That's done in \Drupal\Core\Render\RenderCache. So the code would need to live there, not in HtmlRenderer. (This is what @dawehner refers to in #30.)

More importantly/fundamentally, adding another cache tag to every single rendered thing is not very helpful: it means more cache tags to be stored for every single render cached thing. A much less invasive approach would be to listen to the ConfigEvents::SAVE event and then invalidate the 'rendered' cache tag.

This is exactly what we do in the \Drupal\system\EventSubscriber\ConfigCacheTag::onSave() config save event subscriber.


I think this improves DX by matching expectations. However, I agree with @catch in #22:

but adding/relying on explicit clearing in the module system gets us further away from that.

If we add this, we IMHO should add a @todo comment to it to remove it in D9.

Berdir’s picture

Interesting idea, some questions/inputs/possible problems:

1. The time when exactly we invalidate the cache tag might be relevant. This does it when the config is saved, which is much earlier than my patch and is before entity types, or other caches are invalidated. For example, it could result in a race condition as the module implements and other caches are not yet invalidated. So a parallel request might still get cached without the new hook being executed.

2. Uninstall is still different from install in that it explicitly empties all caches.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alphawebgroup’s picture

Version: 8.4.x-dev » 8.5.x-dev

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kanav_7’s picture

Updated for drupal 8.6.x

alexpott’s picture

Issue tags: +Needs tests

The most recent patch is missing the change to block_install().

I was wondering if potentially there is another possibility. We could add a list of cache tags to the .info.yml file that will be cleared on install and (uninstall too once we remove the general cache clear) but the default value has the rendered cache tag. Discussed with @Berdir and thought this idea would work. @catch, @effulgentsia - any thoughts?

This would mean we could address the syslog issue by setting the cache tags in info.yml to an empty list and other module which have more tags that need clearing can add their own. We default to rendered if not provided because it is very common. For example if a module adds local tasks to a page you've already visited.

catch’s picture

#40 seems reasonable to me.

effulgentsia’s picture

Issue tags: -Novice

+1 to #40. Except I wonder if info.yml is the most logical place for it. Would it make more sense as a container parameter within MODULE.services.yml? Either way, I don't think this issue qualifies as Novice anymore.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

Issue summary: View changes
larowlan’s picture

Status: Needs review » Postponed (maintainer needs more info)
Issue tags: +Bug Smash Initiative

This was triaged as part of Drupal South.

We followed the steps to reproduce from #5

For example, go to the path alias page, then install pathauto, refresh the page and the new locals tasks don't show up (wondering if local tasks block should have a cache tag that is invalidated when local task plugin cache is invalidated for other use cases).

And were unable to reproduce the issue.

We spent some time looking through the code in ModuleInstaller and couldn't immediately see anything that would have resolved this issue.

Can folks report back if this is resolved now?

Thanks

DamienMcKenna’s picture

I used to run into this a few years ago, but honestly I don't remember the last time I did, maybe it is indeed fixed now?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Kristen Pol’s picture

Status: Postponed (maintainer needs more info) » Closed (cannot reproduce)

Thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.

As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" a year ago.

Like @larowlan, I also was unable to reproduce the issue. I tested on 9.5 with the steps noted in #50 as well.

Since neither I nor @larowlan could reproduce this issue and since there weren't other steps to reproduce the issue provided since the issue was postponed a year ago, I'm marking the issue "Closed (cannot reproduce)". If anyone can provide complete steps to reproduce the issue (starting from "Install Drupal core"), document those steps in the issue summary and set the issue status back to "Active" [or "Needs Work" if it has a patch, etc.].

Thanks!