Caches dependent on simple config are only invalidated on form submissions

AFAIK the only solution is config event subscribers. Updated the issue summary with the proposed solution.

I think this at least major.

#4: That would be turning things around from how they're typically done right now, but that could also work.

Right now e.g. the UserLoginBlock depends on the user.settings.register configuration. A block has its own cache tag. So the event subscriber listens for the config change event and invalidates that block's own cache tag.

What you propose implies that each things should be tagged with the configuration entity they depend on. Which implies that there would be significantly more cache tags being used, that propagate to all cache entries (including the page cache), which potentially leads to an exponential explosion in the number of cache tags.

Both are possible, we just have to determine what we prefer. I could see either an events.yml file (#2023613-59: Move event registration from services.yml to events.yml) or a follow-up to #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall be super useful to declaratively indicate which config entities something depends on.

Renaming for clarity.

For config entities, I've opened #2342651: Cache tags for *all* config entities (with an initial patch). That makes this solely about config non-entities.

Without proper cache tag bubbling and test coverage, it's extremely likely that many bug reports will be filed about things "not working", due to necessary cache tags not being associated.

Worse, yet: without proper cache tag invalidation, it will seem like CMI deployments are broken! When changing config ("things in CMI") through the UI, the invalidation is performed by the UI, not by the config system!

Discussed with @alexpott, @effulgentsia, and @dawehner. Retitling to focus on the bug we need to fix, instead of a potential implementation.

Discussed this with @WimLeers a bit.

We have multiple options and directions to fix this.

The mentioned example in this issue says to create a specific event listener for a specific config. I'm worried that we end up with a lot of event listeners that way that have to run on every config save and it is also a pretty complex solution that contrib/custom probably won't do.

So we said that instead, we'd prefer an automated cache tag clearing when simple config is saved, either:

a) A global "simple_config" or something cache tag
b) A cache tag per configuration name, like config:system.performance
c) Find a way to attach "cache tag X must be cleared when config object N is saved

a) and b) have the problem that we don't know where the code that does that would live (Config? does have to know about simple/entity config...) and how it identifies simple config, do we have a reliable way for that?

c) was my idea that we didn't actually discuss yet, there I'm not sure how that cache tag-tagging would work.

On performance of those approaches:
a) has the advantage that we only have to check one cache tag additionally, and our cache invalidation optimization would also keep the cache tag deletions as low as possible on multiple config saves. Downside is that we might be clearing a lot every time simple config chances, but that's not something that happens often.
b) would require more cache checks and deletions but we could do more specific cache invalidations
c) depends on how the invalidation would work, the advantage could be that we can use cache tags that are already in use, at least in some cases. c) is still very theoretical :)

So for a) the more items reference the simple_config cache tag, the more will be invalidated whenever any simple config is invalidated.

This performs best if we end up with lots of different cache items referencing lots of different files.

For b) the more items reference the cache tags of different simple config files, the more invalidations we have to check. This will be best if several items check the cache tag of the same configuration file (then they don't get unnecessarily cleared and don't require lots of invalidation checking). It gets worse if lots of different simple config files get referenced.

b) seems like it'd be OK for most sites to me.

c) would be useful for something like the system.performance page so it can just clear the already-existing render cache tag.

For the system performance page though, I think we should remove the option from there, and add it to Settings instead though - then it could become part of the cache key (since we'd have the setting available on cache get too). This is such an obscure setting to provide a UI for.

Now working on this.

I'm making good progress on this. Calling it a day now. Should have a patch ready tomorrow around noon. I'm using the approach effulgentsia proposed in #16, which makes it surprisingly elegant :)

And here's the first iteration. Very unlikely to be green. But pretty complete already.

+++ b/core/includes/theme.inc
@@ -320,6 +320,39 @@ function drupal_find_theme_templates($cache, $extension, $path) {
+ * Theme settings are an abomination, especially if they're used for things that
+ * fundamentally aren't theme settings, but module or template settings. What's
+ * worse is that this means that a preprocess function must itself bubble cache
+ * tags, rather than setting it on a render array and letting that bubble it;
+ * hence we end up with this monstrosity.
+ *

Lol, nice comment, but I guess we should make this a little less harsh :)

Besides that, isn't there a way for contrib or custom themes to not have to care about this function, because it seems likely that this can be missed easily.

In #2373017: No delete link when editing a menu, only reset links I found a @todo that links here and I'm not sure what to do with it. The @todo is not removed in #19.

#21: Yes, that's just there temporarily until e.g. catch and Fabianx have had a chance to look at it and either +1 the direction (in which case I'll clean it up) or propose an alternative solution (in which case it'll go away).

#22: Indeed, that's one thing I missed in this initial patch, I'll fix that todo in the next reroll :)

And green. Tests were already added in #19.

Oops, still forgot to do #22 in #24. Done now.

Noticed a flaw in #25 after uploading.

An abomination indeed! I bet that many preprocess functions are going to forget to call add theme_add_setting_cache_tag() after referencing a setting. FYI, Dries recently opened the door toward removing theme settings at #1256994-37: Allow breadcrumbs and feed icons to be toggled in admin UI.

In any case, this solves the problem at hand and we can discuss removing theme settings in a follow-up.

Thanks :) Cleaning up that comment though, it was a bit too harsh to be committed like that.

Solely docs changes, so leaving at RTBC.

While we might be able to get rid of theme settings, if we don't we're stuck with the strange function in preprocess in contrib themes.

Could we not special case that instead to be a listener and clear the render cache tag when the settings are saved? Then the weirdness is self-contained at least.

I like that idea.

Noticed a small mistake.

Looks great, really like the unification.

With the cache tag invalidation on rendered when a theme settings changes, do we really still need the per-theme and global theme cache tags? What are they used for then?

1. +++ b/core/includes/theme.inc
   @@ -319,6 +319,14 @@ function drupal_find_theme_templates($cache, $extension, $path) {
   +
   +function theme_setting_get_cache_tags() {
   +  $theme = \Drupal::theme()->getActiveTheme()->getName();
   

   documentation? :)

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -372,6 +372,13 @@ public function link($text = NULL, $rel = 'edit-form', array $options = []) {
   +  public function getCacheTags() {
   +    return ['config:' . $this->getConfigDependencyName()];
   +  }
   

   Maybe document that we are doing this to match the cache tag of the underlying config object, with a @see?

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   diff --git a/core/lib/Drupal/Core/Entity/EntityInterface.php b/core/lib/Drupal/Core/Entity/EntityInterface.php
   index d8e8e3e..199282d 100644
   
   +++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php
   @@ -95,8 +95,7 @@ public function getDescription() {
   -    // @todo This will be cacheable after https://www.drupal.org/node/2040135.
   -    return AccessResult::allowedIf($this->staticOverride->loadOverride($this->getPluginId()))->setCacheable(FALSE);
   +    return AccessResult::allowedIf($this->staticOverride->loadOverride($this->getPluginId()))->addCacheTags($this->staticOverride->getCacheTags());
   

   So what we actually care about here is when the config file is *deleted*. We should also make sure that $config->delete() clears the cache tag.

4. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -202,8 +202,7 @@ public function initDisplay(ViewExecutable $view, array &$display, array &$optio
   -          $id = $this->view->storage->id();
   -          \Drupal::cache('data')->set($cid, $this->options, Cache::PERMANENT, array('extension', 'extension:views', 'view:' . $id));
   +          \Drupal::cache('data')->set($cid, $this->options, Cache::PERMANENT, Cache::mergeTags(array('config:core.extension', 'extension:views'), $this->view->storage->getCacheTags()));
   
   +++ b/core/modules/views/src/ViewsData.php
   @@ -200,7 +200,7 @@ protected function cacheGet($cid) {
   -    return $this->cacheBackend->set($this->prepareCid($cid), $data, Cache::PERMANENT, array('views_data', 'extension', 'extension:views'));
   +    return $this->cacheBackend->set($this->prepareCid($cid), $data, Cache::PERMANENT, array('views_data', 'config:core.extension', 'extension:views'));
   

   Discussion about which of those cache tags we really use here doesn't belong here, we already started discussing them in the profiling issue..

Also, we have optimization in place to ignore duplicate cache tag invalidations but it might still be worth to avoid the call there and override the relevant methods in ConfigEntityBase like invalidateTagsOnSave() (we still need the list cache tags there...)

Speaking of list (and _view, if we use that for them..) cache tags, do we keep the separate pattern there or would we want to unify them somehow? I'm perfectly OK with keeping them, just asking.

1. Oops, that was supposed to be removed.
2. Done.
3. Excellent point. That was indeed still missing. Added test coverage as well.
4. Yes, let's keep that out of this issue. I'm just keeping them as-is. Not touching that here.

Also, we have optimization in place to ignore duplicate cache tag invalidations but it might still be worth to avoid the call there and override the relevant methods in ConfigEntityBase like invalidateTagsOnSave() (we still need the list cache tags there...)

Fixed.

Speaking of list (and _view, if we use that for them..) cache tags, do we keep the separate pattern there or would we want to unify them somehow? I'm perfectly OK with keeping them, just asking.

Unified the list cache tags. The view cache tags are unchanged, but they are less closely related to the entities themselves anyway; they're associated with the view builder.

Looking at the fails, having implemented catch's suggestion (Could we not special case that instead to be a listener and clear the render cache tag when the settings are saved? Then the weirdness is self-contained at least.) has a consequence I didn't consider, and I think catch also didn't consider.

The problematic bit is this code in \Drupal\block\Entity\Block :

  /**
   * {@inheritdoc}
   *
   * Block configuration entities are a special case: one block entity stores
   * the placement of one block in one theme. Changing these entities may affect
   * any page that is rendered in a certain theme, even if the block doesn't
   * appear there currently. Hence a block configuration entity must also return
   * the associated theme's cache tag.
   */
  public function getCacheTags() {
    return Cache::mergeTags(parent::getCacheTags(), ['theme:' . $this->theme]);
  }

We had this to ensure that if the block configuration changes, all cached pages that use that theme are invalidated. But this is from a time where we didn't have "list" cache tags yet… and thus:

1. block config entities have list cache tags, just like any other entity, and they're already being invalidated
2. so if we'd tag pages using blocks with the block config entity list cache tag (config:block_list) instead of a theme:<theme name> cache tag, then we're golden.
3. using the block config entity list cache tag actually makes a ton of sense, since we now have the "block" page display variant, which … lists all blocks that match the conditions to be listed on the current page!

So I implemented that approach, and it works beautifully.

EDIT: Only after I did all this, I saw that Berdir wrote With the cache tag invalidation on rendered when a theme settings changes, do we really still need the per-theme and global theme cache tags? What are they used for then? — this is absolutely correct, and would've been necessary in any case to get the tests to pass. The patches in #32/#33 failed because I didn't do that yet :)

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -375,6 +375,8 @@ public function link($text = NULL, $rel = 'edit-form', array $options = []) {
   +    // Use cache tags that match the underlying config object's name.
   +    // @see getConfigDependencyName()
   

   I meant a @see to ConfigBase::getCacheTags(),getConfigDependencyName() is already there.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
   @@ -70,6 +70,9 @@ public function __construct($definition) {
   +    // Prefix the sole default list cache tag with 'config:', to make it
   +    // consistent with the other Configuration System cache tags.
   +    $this->list_cache_tags[0] = 'config:' . $this->list_cache_tags[0];
   

   Shouldn't this be conditional, for those entity types that explicityl want to define a different tag? We could set it conditionally before calling parent::__construct() I think?

1. Ah, that makes much more sense :)
2. Indeed, great catch, fixed!

Looks much better with the event listener, and that's a self-contained workaround we can just remove without an API change later if we end up not needing it.

Easy fix.

I consider this RTBC'able.

Added beta evaluation

Forgot to include the sole new file in the patch. #44 interdiff is still valid; but this is the right patch.

(Just learned about git add -u, so I used that instead of of the git add --all core that I usually do after applying a patch. Looks like I should go back to what I used before.)

Trying to review this with the same level of nitpick... details as you do. Initially wanted to fix them and RTBC it, but there is at least one real problem I think.

1. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -81,14 +81,16 @@ public static function validateTags(array $tags) {
   +   * @param $glue
   +   *   A string to be used as glue for concatenation. Defaults to a colon.
   

   Missing type.

2. +++ b/core/lib/Drupal/Core/Config/ConfigBase.php
   @@ -262,4 +262,15 @@ public function merge(array $data_to_merge) {
   +   * The unique cache tag associated with this configuration object.
   

   Should be cache tags I think but other examples are the same, so not sure about changing it here.

3. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -180,7 +180,7 @@ public function rebuild(array $definitions) {
   -    $cache_tags = Cache::buildTags('menu', $affected_menus);
   +    $cache_tags = Cache::buildTags('config:system.menu.', $affected_menus, '');
   

   It would make more sense for me to make . the glue instead of an empty string.

4. +++ b/core/modules/contact/src/Access/ContactPageAccess.php
   @@ -91,7 +91,9 @@ public function access(UserInterface $user, AccountInterface $account) {
        // If the requested user did not save a preference yet, deny access if the
        // configured default is disabled.
   -    else if (!$this->configFactory->get('contact.settings')->get('user_default_enabled')) {
   +    $contact_settings = $this->configFactory->get('contact.settings');
   +    $access->cacheUntilConfigurationChanges($contact_settings);
   +    if (!$contact_settings->get('user_default_enabled')) {
          return $access;
        }
   

   This was an else if before, now it is just an if, I don't think that is correct?

   #2393577: Access issue with default settings set to disabled should provide the test coverage for this.

5. +++ b/core/modules/system/src/EventSubscriber/ThemeSettingsCacheTag.php
   @@ -0,0 +1,67 @@
   +   * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler
   +   *   The theme handler
   

   Missing .?

6. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
   @@ -400,6 +400,7 @@ public function testCacheContexts() {
       * @covers ::cacheUntilEntityChanges
   +   * @covers ::cacheUntilConfigurationChanges
       */
   
   @@ -447,6 +448,18 @@ public function testCacheTags() {
   +
   +    // ::cacheUntilEntityChanges() convenience method.
   +    $configuration = $this->getMock('\Drupal\Core\Config\ConfigBase');
   +    $configuration->expects($this->any())
   

   Should this say "untilConfigurationChanges()" ?

1. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -81,14 +81,16 @@ public static function validateTags(array $tags) {
   +  public static function buildTags($prefix, array $suffixes, $glue = ':') {
        $tags = [];
        foreach ($suffixes as $suffix) {
   -      $tags[] = $prefix . ':' . $suffix;
   +      $tags[] = $prefix . $glue . $suffix;
        }
        return $tags;
      }
   

   We have a CacheTest, let's expand the test coverage for buildTags to include $glue in there.

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -125,7 +125,7 @@ public function getCurrentRouteMenuTreeParameters($menu_name) {
   -        $this->cache->set($cid, $parameters, CacheBackendInterface::CACHE_PERMANENT, array('menu:' . $menu_name));
   +        $this->cache->set($cid, $parameters, CacheBackendInterface::CACHE_PERMANENT, array('config:system.menu.' . $menu_name));
   
   @@ -255,7 +255,7 @@ public function build(array $tree, $level = 0) {
   -      $build['#cache']['tags'][] = 'menu:' . $menu_name;
   +      $build['#cache']['tags'][] = 'config:system.menu.' . $menu_name;
   

   Its a little bit sad to couple the existance of menu entities now with the menu tree here, it used to be independent, and we had a fight about this earlier.

   The underlying question is whether its okay to add an implicit dependency of \Drupal\Core to the system module.

3. +++ b/core/modules/system/src/EventSubscriber/ThemeSettingsCacheTag.php
   @@ -0,0 +1,67 @@
   +    if (preg_match('/(.*)\.settings$/', $event->getConfig()->getName(), $matches)) {
   +      if (in_array($matches[1], array_keys($this->themeHandler->listInfo()))) {
   

   I wonder whether it is a more sane idea to simply foreach over the existing themes and just not care about the regex? This would at least make the code a little bit more readable.

Happy christmas!

Re-roll and fixed #48 1, 4 (confirmed that the new test failed on this patch), 5 and 6.

Added a test for #49.1 and changed 3 as suggested.

Messed up the merge and a weird typo.

Addressing the parts of #48 and #49 that haven't been fixed yet (Berdir's changes look great):

• #48.2: indeed, I went for consistency. There is a reasoning behind this though: it is a unique cache tag, but we always return an array of cache tags, everywhere, to make it easier to merge them together. IOW: even if we return a single cache tag, we return it as an array, so that it's consistent everywhere.
• #48.3: agreed, done.
• #49.2:
  

  Its a little bit sad to couple the existance of menu entities now with the menu tree here, it used to be independent, and we had a fight about this earlier.

  I agree it's sad. But when was it independent? It's been like this for a long, long time? The dependency is already there before this patch at least; this patch has only made it explicit.
  If you have a counterproposal, I'm all ears :)

I think there were ~3 lines that called buildTags() with empty $glue, you only updated one. I guess the cache tags refactoring patch conflicted with this.

Rebased, and updated those two other instances.

+++ b/core/modules/system/src/EventSubscriber/ThemeSettingsCacheTag.php
@@ -0,0 +1,68 @@
+    // Global theme settings.
+    if ($event->getConfig()->getName() === 'system.theme.global') {
+      Cache::invalidateTags(['rendered']);
+    }
+
+    // Theme-specific settings, check if this matches a theme settings
+    // configuration object, in that case, clear the rendered cache tag.
+    foreach (array_keys($this->themeHandler->listInfo()) as $theme_name) {
+      if ($theme_name == $event->getConfig()->getName()) {
+        Cache::invalidateTags(['rendered']);
+        break;
+      }
+    }
+  }

See #2398085: Inject cache_tags.invalidator instead of using Cache::invalidateTags(), what about updating this to already inject the invalidator? Then we can also drop the global container from the unit tests...

I think the event stuff is now optimized enough to only create this service when we actually need it, so we wouldn't have a performance issue...

Done! For every class were >=1 Cache::invalidateTags() is touched by this patch.

berdir: WimLeers: I'd suggest to revert at least the config injection, I think most fails are because of that
WimLeers: berdir: agreed

Done.

And this will be green again, at last!

Forgot the interdiff for #66 in my enthusiasm.

I don't think my point from #37 was addressed, we are still clearing config cache tags twice, once by saving the config and once in invalidateTagsOnSave(). As mentioned, I believe that config entity base should override that and not return the single cache tag there.

The only other point that was not "fixed" is #49.2 with the menu dependency, but I agree with @WimLeers that we are not actually changing anything here, the new name makes it just a bit more obvious. We already were adding that specific cache tag that matches with the old entity cache tag.

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -223,6 +224,7 @@ public function save() {
     $this->storage->write($this->name, $this->data);
+    Cache::invalidateTags($this->getCacheTags());
     $this->isNew = FALSE;

Thinking about that method, that has logic to only clear the cache tag on update. Should we do the same here, no point in clearing a cache tag for a new config ... right?

Thanks for the review! Will fix those points.

Fixed that; this uncovered an instance of self::method() instead of static::method(); i.e. it wasn't using late static binding while it should be.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -411,4 +421,25 @@ public function getConfigDependencyName() {
   +   * config system already invalidates them.
   +
   +   */
   

   nitpick: Remove the line without the "8"

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -125,7 +125,7 @@ public function getCurrentRouteMenuTreeParameters($menu_name) {
   -        $this->cache->set($cid, $parameters, CacheBackendInterface::CACHE_PERMANENT, array('menu:' . $menu_name));
   +        $this->cache->set($cid, $parameters, CacheBackendInterface::CACHE_PERMANENT, array('config:system.menu.' . $menu_name));
   
   @@ -255,7 +255,7 @@ public function build(array $tree, $level = 0) {
          // Set cache tag.
   -      $build['#cache']['tags'][] = 'menu:' . $menu_name;
   +      $build['#cache']['tags'][] = 'config:system.menu.' . $menu_name;
          return $build;
        }
   

   It is still sad to couple menu trees to menu entities, even if indirectly.

1. Fixed.
2. You already said that in #49.2; I replied in #53, and Berdir agreed with my response in #68. This is a pre-existing problem; the only change is that it's now more obvious.

+++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
@@ -118,6 +131,10 @@ public function testSetData($data) {
   public function testSave($data) {
+    $this->cacheTagsInvalidator->expects($this->once())
+      ->method('invalidateTags')
+      ->with(['config:config.test']);
+

Can we make this a bit more clear for insert/update? At least document that we only expect a single call, even better if we could test it separately, e.g. to separate test methods, where one explicitly ensures that invalidateTags() is never called. This unit test would also pass if only saving a new config would clear the tag :)

Done.

From IRC:

berdir: WimLeers: ah.. last point: add a ->expects($this->never())  to testSaveNew
WimLeers: berdir: fair!
WimLeers: should've thought of that

Rebased. 3-way merge FTW.

CR created. IS updated.

Alex Pott remarked in IRC that LanguageConfigOverride had not yet been updated. After a discussion with Alex Pott and Berdir on how that should work, this is what I ended up with.

+++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
@@ -66,7 +66,9 @@
-   * @return \Drupal\Core\Config\Config
+   * Must invalidate the cache tags associated with the configuration object.
+   *
+   * @return $this
    *   The configuration object.

super-nitpick: $this doesn't need a description.

Feel free to update, but I think this is finally RTBC.

Change record looks good as well, if maybe a bit too detailed for a change record :)

Yay!

Fixed the super nitpick.

I think we need to do something in ConfigFactory::rename() too since this does not use the Config object.

Fixed, and added a unit test for it.

+++ b/core/tests/Drupal/Tests/Core/Config/ConfigFactoryTest.php
@@ -0,0 +1,87 @@
+  /**
+   * @covers ::rename
+   */
+  public function testRename() {
+    $old = new Config($this->randomMachineName(), $this->storage, $this->eventDispatcher, $this->typedConfig);
+    $new = new Config($this->randomMachineName(), $this->storage, $this->eventDispatcher, $this->typedConfig);
+
+    $this->storage->expects($this->exactly(2))
+      ->method('readMultiple')
+      ->willReturnMap([
+        [[$old->getName()], $old->getRawData()],
+        [[$new->getName()], $new->getRawData()],
+      ]);
+
+    $this->cacheTagsInvalidator->expects($this->once())
+      ->method('invalidateTags')
+      ->with($old->getCacheTags());
+
+    $this->configFactory->rename($old->getName(), $new->getName());
+  }

Can you add another mock for the storage->rename() call, then we have that fully covered, and not just 50% of the method :)

Sure, done.

We discussed in IRC if the cache tag invalidation should be moved to the config factory, we were not sure, but decided to keep it there for now.

This looks good to me again, rename is properly addressed, I can't think of anything else that would be missing.

Committed 5859ca2 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Follow-up: #2407765: Wherever simple config is used to render output to end user, associate their cache tags.

This CR wasn't published yet. https://www.drupal.org/node/2406455 is now live.