Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
andrdf
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
Comment | File | Size | Author |
---|---|---|---|
#39 | render-cache-2783791-39.patch | 851 bytes | kanav_7 |
#33 | 2783791-33.patch | 952 bytes | Wim Leers |
#31 | 2783791-render-30.patch | 22.34 KB | tim.plunkett |
#28 | 2783791-render-28.patch | 12.57 KB | tim.plunkett |
#26 | 2783791-render-26.patch | 1.31 KB | tim.plunkett |
Comments
Comment #2
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #3
effulgentsia CreditAttribution: effulgentsia at Acquia commentedActually
ModuleInstaller::uninstall()
does flush all caches, so updating title and summary to only deal with installation.Comment #4
dawehnerOne 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.Comment #5
BerdirAgreed, 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?
Comment #6
tim.plunkett#4: #2497457: Router builder does not rebuild local actions
Comment #7
joshi.rohit100Lets see if this can be used as test-only patch as per #5.
Comment #8
joshi.rohit100Shouldn't render cache be cleared on theme registry rebuild as we are rebuilding the theme registry on install ?
Comment #10
joshi.rohit100If this simply fixes or not.
Comment #11
effulgentsia CreditAttribution: effulgentsia at Acquia commentedYes, seems to me that in
\Drupal\Core\Theme\Registry::reset()
, where we doCache::invalidateTags(array('theme_registry'));
, perhaps we should also add'rendered'
to that argument? Especially, since 'theme' is arequired_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.Comment #12
BerdirNo 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.
Comment #13
BerdirI 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?
Comment #14
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWorks 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?
Comment #15
dawehnerThat is a good point @effulgentsia. Maybe we should put it next to
,and if not, just for consistency reasons.
Comment #16
BerdirSure, lets move it there.
Comment #17
joshi.rohit100Comment #18
BerdirI think this is good to go then? Except maybe creating that follow-up for theme registry cache invalidation.
Comment #19
dawehnerI'm wondering whether having some test coverage for this could be helpful.
Should we use
\Drupal::service('cache_tags.invalidator')->invalidateTags($tags)
instead?Comment #20
catchThere 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.
Comment #21
effulgentsia CreditAttribution: effulgentsia at Acquia commentedInteresting 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?Comment #22
catch@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.
Comment #23
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSo potentially, we could:
'cache_tags'
key tohook_hook_info()
.['rendered']
as a value for the hooks that are involved in rendering (per #21).ModuleInstaller::install()
check if the installed modules implement any hooks that have cache tags, and if so, invalidate them.But:
rdf
module and other modules we find that need them?Comment #24
catchWhat 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.
Comment #25
dawehnerThe 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?
Comment #26
tim.plunkettWorks when running the failing test (\Drupal\block\Tests\BlockInstallTest::testCacheTagInvalidationUponInstallation) from #7
Guessing it will fail many other tests that assert cache tags though.
Comment #28
tim.plunkettComment #30
dawehnerSomehow I would have expected to patch
\Drupal\Core\Render\Renderer::doRender
to add the cache tagsComment #31
tim.plunkettNot 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!
Comment #33
Wim Leers+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 inHtmlRenderer
. (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:
If we add this, we IMHO should add a
@todo
comment to it to remove it in D9.Comment #34
BerdirInteresting 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.
Comment #37
alphawebgroupComment #39
kanav_7 CreditAttribution: kanav_7 at Google Summer of Code commentedUpdated for drupal 8.6.x
Comment #40
alexpottThe 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.Comment #41
catch#40 seems reasonable to me.
Comment #42
effulgentsia CreditAttribution: effulgentsia at Acquia commented+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.Comment #49
larowlanComment #50
larowlanThis was triaged as part of Drupal South.
We followed the steps to reproduce from #5
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
Comment #51
DamienMcKennaI 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?
Comment #55
Kristen PolThanks 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!