Updated: Comment #N

Problem/Motivation

Cache::invalidateTags() is called from ThemeSettingsForm::submitForm(), PerformanceForm::submitForm(), etc. However, that doesn't get run when the theme settings are changed from an API call, for example, within UserPictureTest::testPictureOnNodeComment(). In general, cache invalidation needs to be triggered by configuration or content changes, regardless of whether it's a form submission that triggers the change.

Other examples can be found by searching HEAD for where Cache::invalidateTags() or cache_invalidate_tags() is called.

Proposed resolution

A cache tag per configuration name, like config:system.performance.

Remaining tasks

None.

User interface changes

None.

API changes

  1. Added AccessResult::cacheUntilConfigurationChanges(ConfigBase $configuration) (a sister method of public function cacheUntilEntityChanges(EntityInterface $entity)).
  2. Added ConfigBase::getCacheTags() (just like EntityInterface::getCacheTags()). It ensures that each cache tag is of the form config:<config object name>, so e.g. config:system.performance for the performance settings configuration object. When a configuration object is updated or deleted, this is used to invalidate the appropriate cache tags, as you'd expect.
  3. Added StaticMenuLinkOverridesInterface::getCacheTags().

Because of point 2, all cache tags of config entities have changed accordingly, e.g. menu:<menu name> has become config:system.menu.<menu name>. However, you should have been using the getCacheTags() method on configuration entities already anyway, in which case this cache tag change will be picked up by your existing code automatically.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because Config System imports/syncs today do not result in the necessary cache clears, making it seem as if the import/sync didn't work correctly. On top of that, changing simple config through the UI does not clear any caches whatsoever, and hence simple config that affects e.g. render cached output, doesn't cause that render cached output to be invalidated.
Issue priority Critical because this breaks cacheability, makes Drupal seem broken.
Prioritized changes The main goal of this issue is security/performance.
Disruption Zero disruption.
Files: 
CommentFileSizeAuthor
#87 simple_config_cache_tags-2040135-87.patch89.33 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,924 pass(es). View

Comments

effulgentsia’s picture

Issue tags: +D8 cacheability

tagging

xjm’s picture

Issue summary: View changes
Issue tags: +Configuration system
Wim Leers’s picture

Title: Decouple cache invalidation from form submissions » Decouple cache invalidation from form submissions: otherwise the Configuration System will appear broken
Priority: Normal » Major
Issue summary: View changes

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

I think this at least major.

effulgentsia’s picture

Subscribers might be good for other reasons, but is it possible / would it make sense to solve this issue purely with a cache tag per config file, and have the config system itself invalidate that tag when the file is changed? Not sure if that's a workable idea, just throwing it out there.

Wim Leers’s picture

#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.

Wim Leers’s picture

Title: Decouple cache invalidation from form submissions: otherwise the Configuration System will appear broken » Cache tags for config entities

Renaming for clarity.

Wim Leers’s picture

Title: Cache tags for config entities » Cache tags for config entities (necessary to ensure config changes invalidate dependent render cache items)
effulgentsia’s picture

Title: Cache tags for config entities (necessary to ensure config changes invalidate dependent render cache items) » Cache tags for config entities (necessary to ensure config changes invalidate dependent render cache items) and non-entities

Also adding non-entities to the title, for config objects like system.performance and menu_link.static.overrides.

Wim Leers’s picture

Title: Cache tags for config entities (necessary to ensure config changes invalidate dependent render cache items) and non-entities » Cache tags for config non-entities
Priority: Major » Critical
Related issues: +#2342651: Cache tags for *all* config entities

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!

catch’s picture

Component: cache system » configuration system
xjm’s picture

Title: Cache tags for config non-entities » Caches dependent on simple config are only invalidated on form submissions
Category: Task » Bug report
Issue tags: +Needs tests

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

xjm’s picture

Issue tags: +Triaged D8 critical
Berdir’s picture

Issue tags: +Ghent DA sprint

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 :)

catch’s picture

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.

effulgentsia’s picture

How about a d) simple_config:PROVIDER?

effulgentsia’s picture

#15 might not be very different from #13.b. In core, I think only system and user modules define more than one simple config per module.

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?

Would it need to? Or, can ConfigEntityBase implement a getCacheTags() that returns the same format as b), so that whatever in the config system does the clearing could apply the same logic for both simple and entity?

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Now working on this.

Wim Leers’s picture

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 :)

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
55.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,449 pass(es), 19 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 19: simple_config_cache_tags-2040135-19.patch, failed testing.

swentel’s picture

+++ 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.

olli’s picture

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.

Wim Leers’s picture

#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 :)

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
60.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,485 pass(es). View
5.75 KB

And green. Tests were already added in #19.

Wim Leers’s picture

FileSize
62.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,477 pass(es), 17 fail(s), and 13 exception(s). View
2.25 KB

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

Wim Leers’s picture

FileSize
62.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,483 pass(es). View
822 bytes

Noticed a flaw in #25 after uploading.

The last submitted patch, 25: simple_config_cache_tags-2040135-25.patch, failed testing.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

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.

Wim Leers’s picture

FileSize
63.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,596 pass(es). View
2.95 KB

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

Solely docs changes, so leaving at RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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.

Wim Leers’s picture

Status: Needs review » Needs work

I like that idea.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
59.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,505 pass(es), 99 fail(s), and 0 exception(s). View
10.55 KB
Wim Leers’s picture

FileSize
59.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,492 pass(es), 99 fail(s), and 0 exception(s). View
759 bytes

Noticed a small mistake.

The last submitted patch, 32: simple_config_cache_tags-2040135-32.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 33: simple_config_cache_tags-2040135-33.patch, failed testing.

Berdir’s picture

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..

Berdir’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
70.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
29.16 KB
  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 :)

Berdir’s picture

  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?

Wim Leers’s picture

FileSize
70.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,539 pass(es), 7 fail(s), and 0 exception(s). View
1.96 KB
  1. Ah, that makes much more sense :)
  2. Indeed, great catch, fixed!
catch’s picture

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.

The last submitted patch, 38: simple_config_cache_tags-2040135-38.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: simple_config_cache_tags-2040135-40.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
73.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
5.23 KB

Easy fix.

I consider this RTBC'able.

Status: Needs review » Needs work

The last submitted patch, 44: simple_config_cache_tags-2040135-44.patch, failed testing.

Wim Leers’s picture

Issue summary: View changes

Added beta evaluation

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
75.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,534 pass(es). View

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.)

Berdir’s picture

Status: Needs review » Needs work

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()" ?

dawehner’s picture

  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!

Berdir’s picture

Status: Needs work » Needs review
FileSize
72.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,148 pass(es), 1 fail(s), and 0 exception(s). View
4.25 KB

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.

Status: Needs review » Needs work

The last submitted patch, 50: simple_config_cache_tags-2040135-50.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
73.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,363 pass(es). View
1.3 KB

Messed up the merge and a weird typo.

Wim Leers’s picture

FileSize
73.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch simple_config_cache_tags-2040135-53.patch. Unable to apply patch. See the log in the details link for more information. View
876 bytes

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 :)

Status: Needs review » Needs work

The last submitted patch, 53: simple_config_cache_tags-2040135-53.patch, failed testing.

Berdir’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
72.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,432 pass(es), 4 fail(s), and 0 exception(s). View
1.44 KB

Rebased, and updated those two other instances.

Status: Needs review » Needs work

The last submitted patch, 56: simple_config_cache_tags-2040135-56.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
72.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,442 pass(es). View
2.97 KB
Berdir’s picture

+++ 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...

Wim Leers’s picture

FileSize
80.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
13.71 KB

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

Status: Needs review » Needs work

The last submitted patch, 60: simple_config_cache_tags-2040135-60.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
83.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,505 pass(es), 55 fail(s), and 276 exception(s). View
3.92 KB

Status: Needs review » Needs work

The last submitted patch, 62: simple_config_cache_tags-2040135-62.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
79.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,941 pass(es), 1 fail(s), and 46 exception(s). View
10.5 KB
berdir: WimLeers: I'd suggest to revert at least the config injection, I think most fails are because of that
WimLeers: berdir: agreed

Done.

Status: Needs review » Needs work

The last submitted patch, 64: simple_config_cache_tags-2040135-64.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
77.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,480 pass(es). View

And this will be green again, at last!

Wim Leers’s picture

FileSize
1.74 KB

Forgot the interdiff for #66 in my enthusiasm.

Berdir’s picture

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.

Berdir’s picture

+++ 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?

Wim Leers’s picture

Status: Needs review » Needs work

Thanks for the review! Will fix those points.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
79.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,836 pass(es). View
5.9 KB

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.

dawehner’s picture

  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.

Wim Leers’s picture

FileSize
79.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,847 pass(es). View
751 bytes
  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.
Berdir’s picture

+++ 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 :)

Wim Leers’s picture

FileSize
79.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch simple_config_cache_tags-2040135-75.patch. Unable to apply patch. See the log in the details link for more information. View
1.43 KB

Done.

Wim Leers’s picture

FileSize
79.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch simple_config_cache_tags-2040135-76.patch. Unable to apply patch. See the log in the details link for more information. View
657 bytes

From IRC:

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

The last submitted patch, 75: simple_config_cache_tags-2040135-75.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 76: simple_config_cache_tags-2040135-76.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
80.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,891 pass(es). View

Rebased. 3-way merge FTW.

Wim Leers’s picture

Issue summary: View changes

CR created. IS updated.

Wim Leers’s picture

FileSize
85.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,885 pass(es). View
5.89 KB

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.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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 :)

Wim Leers’s picture

FileSize
85.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,885 pass(es). View
759 bytes

Yay!

Fixed the super nitpick.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
89.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,912 pass(es). View
3.75 KB

Fixed, and added a unit test for it.

Berdir’s picture

+++ 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 :)

Wim Leers’s picture

FileSize
89.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,924 pass(es). View
711 bytes

Sure, done.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 5859ca2 on 8.0.x
    Issue #2040135 by Wim Leers, Berdir: Caches dependent on simple config...
Wim Leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture

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