Config overrides should provide cacheability metadata

Have ConfigFactoryOverrideInterface extend CacheableDependencyInterface

I think the fundamental idea here is solid.

If we decide that it's valid to have config overrides that don't require any cacheability metadata at all, then we could possibly add an instanceof check in ConfigFactory.php. That would remove the API change

-1, because it allows implementations to not think about cacheability. And given config overrides can have huge impact on a site, and therefore also huge cache poisoning, I don't think that's acceptable. IOW: we must force all config overrides to implement this, and to therefore think about this.
The total number of config overrides is very low anyway, so disruption will still be minimal.

Now, let's talk about the sole config override in core: \Drupal\language\Config\LanguageConfigFactoryOverride. I first wanted to point out its inability to figure out how it was actually negotiated, but then I realized this always uses the interface language. Just to be one hundred percent certain: it is impossible to introduce a new "config language" language type, right? (Which would then be used instead of the interface language.)

catch pointed out for my question at the bottom of #2 that the module introducing such a new "config language" language type could also just override the LanguageConfigFactoryOverride service and change that. Plus it's an extremely esoteric need.

So: a definitive +1 to this issue, as long as every config override is required to implement CacheableDependencyInterface.

-1, because it allows implementations to not think about cacheability.

Well on the other hand there is a good reason that you should write software in a way that not everything couples to everything in general.
For this config overrides are special already given their limited usecase.

In talking to pfrenssen, I realized something:

Then ConfigFactory.php can collect the cache contexts from the overrides and apply them to the config object. This is an API change for all config overrides.

(Emphasis mine.)

This is not something you can just do, because:

abstract class ConfigBase implements CacheableDependencyInterface {

That just gives you getters, not setters. At #2512718-29: EntityManager::getTranslationFromContext() should add the content language cache context to the entity (and in IRC), Fabian said:

That would mean config extends CacheableMetadata instead of just implementing CacheableDependencyInterface, but that is not even an API change.

And that's what the quote in the IS is based on. But it's wrong/oversimplifying. CacheableMetadata has setters that do not yet have an interface. So we'd need to add such an interface (no idea what it'd be named), and have CacheableMetadata and ConfigBase implement it.

Well on the other hand there is a good reason that you should write software in a way that not everything couples to everything in general.

For software targeting the web, it's fine to couple everything to the cacheability metadata subsystem. Because only then you can do proper HTTP caching.

(Only the things that DO NOT depend on any request context don't need cacheability metadata. If you don't like cacheability metadata in config overrides, then the only solution is to disallow config overrides to depend on request context.)

[...] Plus it's an extremely esoteric need.

I bet it takes way less to summon Satan himself...

Started working on this. I like the pirate day idea from @catch :)

+++ b/core/modules/config/src/Tests/CacheContextConfigOverrideTest.php
@@ -0,0 +1,36 @@
+    $config_factory = $this->container->get('config.factory');

Suggested re-org:

$config_factory = $this->container->get('config.factory');
$config = $theme = $config_factory->get('system.theme');
$theme = $config->get('default');
$this->assertEquals(['pirate_day'], $config->getCacheContexts());

-----

Looks like a GREAT start!

+++ b/core/lib/Drupal/Core/Cache/ExtensibleCacheContextsInterface.php
@@ -0,0 +1,27 @@
+interface ExtensibleCacheContextsInterface {

This is the final form we agreed upon in #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity:

interface RuntimeCacheContextsDependencyInterface {

  public function addRuntimeCacheContexts(array $cache_contexts);

}

Btw, I wrote a draft change record mentioning this:

https://www.drupal.org/node/2525764

WIP, implemented the actual adding of cache contexts to the config object. This will still fail since we are missing a PirateCacheContext.

I'm wondering since ConfigFactoryOverrideInterface now implements CacheableDependencyInterface we are not only adding the required getCacheContexts() method to all config overrides, but also getCacheTags() and getCacheMaxAge() which are both unused in the scope of this ticket. What shall we do with these?

Cross-posted with @plach, will address that and implement the cache context.

So is there really no use-case for config entities needing cache tags.

e.g. domain module overrides config.site_name per domain.

So it adds the cache context, but it also would like that when config.domains changes that config.site_name is invalidated and likely other overrides.

Yes, it could do so itself, but is that such an esoteric need?

Similar when I want to expire data that is using config.site_name after 10 min, because I change site_name every 10 min dynamically.

Yes, I could use a cache context, but I don't want 1000 variations, I want to invalidate as the old site name, never comes back ...

I am still to go full interface and full override and have RuntimeCacheableDependencyInterface. That gives the most flexibility.

And yes if the config entity is saved I want config.domains to be invalidated as well, that is okay and a trade off I make.

Edit:

Thought about it some more and we _need_ to go full interface.

Use case:

A dynamic config override, yes it can probably be expressed as a cache context, but:

- It fills up the DB with 1000s of Cache::Permanent variations
- If it is so dynamic that it can't be cached, it cannot say: max-age=0, which is a MUST in some situations

And an always changing cache contexts => infinite variations

There is a reason we have this CacheableDependencyInterface and the same we need the RuntimeCacheableDependencyInterface.

Thanks for providing some good examples of overrides setting cache tags. It's not critical though. Shall we make a separate issue for this and refer to it in the @todo messages?

#16: Why would we introduce an Interface that is incomplete to only extend it later?

I think I would rather do it all together. Already now the config overrides have CacheableDependencyInterface, so why would we only go half-way?

Not being able to set max-age=0 could be critical under certain circumstances (e.g. sensitive data).

Or rather: Setting max-age=0 on the ConfigOverride (as you can now) and it not being taken into account would be critical.

But introducing an Interface that only has getCacheContexts() is fragile IMHO and misses the point of that this should provide cacheability metadata.

So then we can best extend the interface with two additional methods addRuntimeCacheTags() and setRuntimeCacheMaxAge(), and change the interface name to RuntimeCacheableDependencyInterface.

Now that we make it complete, would there be any need to remove existing cache contexts / tags, or change existing ones?

#15 runs counter to what catch said in #2512718-45: EntityManager::getTranslationFromContext() should add the content language cache context to the entity. Let's get his feedback.

EDIT: Note that an especially gnarly consequence of that is that it then becomes possible for entities, upon saving, to invalidate the run-time cache tags as well. (Config objects do not have this consequence, because config overrides are only applied to immutable config, not to editable config, and calling ImmutableConfig::save() results in an exception, hence no run-time cache tags are invalidated).

#19: I was not able to find a use-case for that, if there really is we would need to add that later, but it would make things more complex.

#20: Lets go full interface for config and just cache contexts for entities for now - that should resolve the concern.

As a follow-up I suggest that route derived entities are made / wrapped immutable, too.

OK, I already added the cache tags support but I'll remove it again. This will have the strange consequence for now that the config override objects will need to implement an unused getCacheTags() method.

Posting work in progress.

Fabianx replied to #20 at #2512718-72: EntityManager::getTranslationFromContext() should add the content language cache context to the entity. I'd still like catch's POV too.

#23: you can keep it in for now if that's easier (less work to remove it later than add it back later?).

+++ b/core/lib/Drupal/Core/Config/ConfigBase.php
@@ -269,7 +291,7 @@ public function merge(array $data_to_merge) {
@@ -286,4 +308,25 @@ public function getCacheMaxAge() {

@@ -286,4 +308,25 @@ public function getCacheMaxAge() {
     return Cache::PERMANENT;
   }

This needs to be fixed, too and cache tags too obivously.

--

#23: Not sure what you want to remove, but for now lets go full interface with cache tags and cache contexts and max-age here.

Implemented the remaining changes.

+++ b/core/modules/config/tests/config_override_test/src/PirateDayCacheContextConfigOverride.php
@@ -55,8 +56,6 @@ public function getCacheContexts() {
     return [];

@@ -64,8 +63,6 @@ public function getCacheTags() {
     return Cache::PERMANENT;

I think this should return 'pirate-day-tag' and 24 hours ...

--

Looks great overall.

Great points @FabianX, as great as it would be, Pirate Day does not last forever. Also fixed the test failure.

This needs work:

1. @berdir discovered that the cacheability metadata that is being set in this issue is not being applied correctly to ConfigEntities. ConfigEntities do not actually embed the Config object on which they are based, but instead the individual config values are applied. We need to ensure that the overridden values are applied properly.
2. The interface that is shared between this issue and #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity has been updated again. We should adapt it to incorporate the changes.
3. Since this issue and #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity have some overlap and we have been playing ping-pong with the interface we now decided to consider #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity as the master issue. Any changes to the interface will be done there first and backported to this issue. It would be nice to provide a second patch that already includes all the code from that issue to ensure that everything works as expected when the two issues are eventually merged.

#30.1: Why are our tests passing then?

What is a config entity and how as a user would I use them / get them?

Going to work on this again.

@FabianX, will make sure we have tests for the missing functionality. Will start with that. Config entities are one of the two entity types we have, the counterweight of content entities. I'm sure you're familiar with them :)

Backported the relevant changes to the interface, as well as the trait that was added from #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity. I've also uploaded a patch that contains the code from both issues, since this is now depending on the other issue.

The points from #30 and #31 still need to be addressed.

This is a version of the patch that is a diff between this issue and the parent issue #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity. In case the parent issue gets committed this is the one to continue work on.

+++ b/core/modules/config/tests/config_override_test/src/Cache/PirateDayCacheContext.php
@@ -0,0 +1,52 @@
+    $is_pirate_day = static::isPirateDay() ? 'yarr' : 'nay';
...
+   * To ease testing this is determined with a global variable rather than using
+   * the traditional compass and sextant.

ROTFL :-D

---

Looks all great, I have no idea what is missing, but as berdir knows and we still need a failing test for the other config entities, this should be able to make progress.

+++ b/core/lib/Drupal/Core/Config/ConfigBase.php
@@ -269,21 +272,21 @@ public function merge(array $data_to_merge) {
    */
   public function getCacheTags() {
-    return ['config:' . $this->name];
+    return Cache::mergeTags(['config:' . $this->name], $this->cacheTags);
   }

this should be updated to call $this->getCacheTagsForInvalidation() instead, so that the custom override of that method are considered.

What's missing are the add* calls in ConfigEntityStorage to pass the config cacheability metadata to the config entity and some tests for that.

+++ b/core/modules/config/tests/config_override_test/src/Cache/PirateDayCacheContext.php
@@ -0,0 +1,52 @@
+    $is_pirate_day = static::isPirateDay() ? 'yarr' : 'nay';
...
+   * To ease testing this is determined with a global variable rather than using
+   * the traditional compass and sextant.

lol, @pfressen++

1. +++ b/core/lib/Drupal/Core/Cache/MutableCacheableDependencyTrait.php
   @@ -0,0 +1,60 @@
   +   * Runtime cache contexts.
   

   s/Runtime c/C/

2. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -210,6 +218,20 @@ protected function loadOverrides(array $names) {
   +      $this->cache[$cache_key]->addCacheContexts($override->getCacheContexts());
   +      $this->cache[$cache_key]->addCacheTags($override->getCacheTags());
   +      $this->cache[$cache_key]->setCacheMaxAgeIfLower($override->getCacheMaxAge());
   

   Can be simplified to

   $this->cache[$cache_key]
     ->addCacheContexts(…)
     ->addCacheTags(…)
     ->setCacheMaxAgeIfLower(…);
   

3. +++ b/core/modules/config/src/Tests/CacheContextConfigOverrideTest.php
   @@ -0,0 +1,43 @@
   +  public function testConfigOverride() {
   

   Missing docblock.

4. +++ b/core/modules/config/src/Tests/CacheContextConfigOverrideTest.php
   @@ -0,0 +1,43 @@
   +    // Check that the cacheable properties are correct.
   

   s/cacheable/cacheability/

5. +++ b/core/modules/config/tests/config_override_test/src/Cache/PirateDayCacheContext.php
   @@ -0,0 +1,52 @@
   + * Defines the PirateDayCacheContext service that allows to cache the booty.
   

   LOL

6. +++ b/core/modules/config/tests/config_override_test/src/Cache/PirateDayCacheContext.php
   @@ -0,0 +1,52 @@
   +   * To ease testing this is determined with a global variable rather than using
   +   * the traditional compass and sextant.
   

   LOLLLLLLLLLLLL

7. +++ b/core/modules/config/tests/config_override_test/src/PirateDayCacheContextConfigOverride.php
   @@ -0,0 +1,68 @@
   + * Test implementation of a cache context providing config override.
   

   Isn't it the other way around?
   It's fine, but it's missing a dash between "context" and "providing". Right now, it reads as if it's a cache context providing a config override, which makes no sense :P

8. +++ b/core/modules/config/tests/config_override_test/src/PirateDayCacheContextConfigOverride.php
   @@ -0,0 +1,68 @@
   +    $overrides = array();
   

   s/array()/[]/

Quoting @berdir from #36:

this should be updated to call $this->getCacheTagsForInvalidation() instead, so that the custom override of that method are considered.

getCacheTagsForInvalidation() is not part of ConfigBase but of ConfigEntityBase. @berdir confirmed in IRC that this does not need to be changed.

+++ b/core/modules/config/src/Tests/CacheContextConfigOverrideTest.php
@@ -0,0 +1,43 @@
+/**
+ * Tests configuration overrides that set cache contexts.
+ *
+ * @group config
+ */
+class CacheContextConfigOverrideTest extends KernelTestBase {

This is testing more than only cache contexts now. The class name and documentation should be updated.

Re the issue summary:

If we decide that it's valid to have config overrides that don't require any cacheability metadata at all, then we could possibly add an instanceof check in ConfigFactory.php. That would remove the API change, but support for it would still be a contributed project blocker (and while interface language is a required cache context, IMO language config overrides should set a good example here).

It is also true that config overrides may be used in whatever language needed. See LanguageManagerInterface::setConfigOverrideLanguage(). This is not used very widely in Drupal core. User module uses it to send the right language emails to people (which depending on how its rendered may or may not depend on cached things I guess). And DateFormatter::dateFormat() uses it to let people format dates in whatever language needed. So assuming that across the same request overrides from only one language are used is not entirely true.

This addresses the missing test coverage for config entities. 3 versions of the patch:

1. Failing patch to prove that cacheability metadata is now tested correctly for config entities. Includes code from the parent issue.
2. Full patch including the code from the parent issue #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity so it can be tested on the bot.
3. The do-not-test version is the best one to review since it does not contain the code from the parent issue. Since it depends on the parent issue it is non-functional on its own.

Next up is to address the remaining remarks from #38 and #40.

@Gabor do you think we are missing test coverage for the existing use cases?

And the interdiff.

Fixed remaining remarks. This is ready for review.

The big patch includes the code from the parent issue #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity so it can be tested on the bot. The smaller patch only includes the code from this issue and is the best to review.

Would we need negative tests to see if the overrides are not present when it is not Pirate Day? And do we need to test any bubbling up of the cache contexts?

#48

1. I disagree; setCacheMaxAge() means "set the given value, i.e. overwrite whatever the current value is" (that's also what CacheableMetadata::setCacheMaxAge() does), whereas setCacheMaxAgeIfLower() does not have that problem. An alternative would be addCacheMaxAge().
2. Indeed, I was thinking the same. But agreed that for the purposes of this test that is not necessary. However, it would clarify the intended usage. Perhaps just add a comment explaining that? (i.e. because we're just simulating it and it's not actually time-based in this test implementation, that we can't actually do that.)

Another complete review. Mostly nits.

1. +++ b/core/lib/Drupal/Core/Config/ConfigBase.php
   @@ -28,8 +30,9 @@
   -abstract class ConfigBase implements CacheableDependencyInterface {
   +abstract class ConfigBase implements CacheableDependencyInterface, MutableCacheableDependencyInterface {
   

   We can remove CacheableDependencyInterface here because MutableCacheableDependencyInterface extends that.

2. +++ b/core/modules/config/src/Tests/CacheabilityMetadataConfigOverrideTest.php
   @@ -0,0 +1,76 @@
   +  public function testConfigOverride() {
   

   So this test is first testing using the theme configuration (a "plain" config object) and then testing the placed block config entity (a config entity).

   I think it should be clarified that it's testing those two distinct things; at first sight it seems you're testing the same thing twice.

   Perhaps split this up in testPlainConfigOverride() and testConfigEntityOverride()? (Better names TBD.)

3. +++ b/core/modules/config/src/Tests/CacheabilityMetadataConfigOverrideTest.php
   @@ -0,0 +1,76 @@
   +    // Check that the cacheability properties are correct.
   

   Nit: s/properties are/metadata is/

4. +++ b/core/modules/config/src/Tests/CacheabilityMetadataConfigOverrideTest.php
   @@ -0,0 +1,76 @@
   +    // overridden correctly. This ensures that the metadata is correctly applied
   

   Nit: s/ensures/verifies/

5. +++ b/core/modules/config/src/Tests/CacheabilityMetadataConfigOverrideTest.php
   @@ -0,0 +1,76 @@
   +    // Check that our call to action message is appealing to filibusters.
   +    $this->assertEqual($block->label(), 'Draw yer cutlasses!');
   

   LOLLLLL :D

6. +++ b/core/modules/config/src/Tests/CacheabilityMetadataConfigOverrideTest.php
   @@ -0,0 +1,76 @@
   +    // Check that the cacheability metadata properties are correct.
   

   Nit: s/properties are/is/

7. +++ b/core/modules/config/tests/config_override_test/config_override_test.services.yml
   @@ -7,3 +11,7 @@ services:
   +  config_override_test.pirate_day_cache_context_overrider:
   +    class: Drupal\config_override_test\PirateDayCacheContextConfigOverride
   

   Nit: "override" vs "overrider". The interface is "override", so let's use that everywhere.

Addressed the remarks and the failures. There are two patches: the large one includes the parent issue #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity and is intended for the testbot. The 'do-not-test' patch is the one to review, it only contains the code of this issue, but is non-functional on its own since it depends on the parent issue.

@berdir suggested IRL to also provide an integration test that checks the correct bubbling of the cache context. Will work on that now.

Discussed the proposed solution in this issue with @catch, @alexpott, @effulgentsia, @Gábor Hojtsy and @Berdir. Both @catch and @alexpott approve of this solution, so we can work on finishing this patch, notably the additional test coverage that we want.

This is split off from #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity, to make the scope of both more manageable/easier to review. The IS of that issue has been rewritten completely to capture the agreed upon solution.

I updated the IS here to point to the other IS.

I can't find any more complaints. IMO this is RTBC. But the other patch needs to go in first. I updated the IS. The only thing is missing, is a CR.

1. +++ b/core/lib/Drupal/Core/Config/ConfigBase.php
   @@ -10,7 +10,8 @@
   +use Drupal\Core\Cache\MutableCacheableDependencyInterface;
   +use Drupal\Core\Cache\MutableCacheableDependencyTrait;
   
   @@ -28,8 +29,9 @@
   +  use MutableCacheableDependencyTrait;
   

   These have all been renamed in the other issue.

2. +++ b/core/modules/config/src/Tests/CacheabilityMetadataConfigOverrideTest.php
   @@ -0,0 +1,84 @@
   +  public function testConfigOverride() {
   ...
   +  public function testConfigEntityOverride() {
   

   Yay, better/clearer!

3. +++ b/core/modules/config/tests/config_override_test/config/install/block.block.call_to_action.yml
   @@ -0,0 +1,26 @@
   +  label: 'Shop for cheap now!'
   

   Spam in core! <3

Thanks Wim! Backported upstream changes.

In addition to the CR this still needs an integration test on @berdir's request.

Adding an issue tag for that integration test then :)

#2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity has just landed! :)

To do:

1. We'll want to reroll this; simply reuploading the "do-review-but-do-not-test" patch from #56 should be enough :)
2. We already have a change record (https://www.drupal.org/node/2525764), but I think we can improve clarity there: it should explicitly mention that ConfigOverrideInterface now extends CacheableDependencyInterface etc. etc.
3. And finally, we still want that integration test.

The integration test took some time, 3 individual bugs were discovered during while writing it. I wouldn't have been able to bring this to conclusion without the great help of @berdir, he should get commit credit.

1. In BlockViewBuilder::viewMultiple() the cache contexts from the entity were not included in the #cache property of the render array. This was critical to fix in this issue, because it prevented the cache contexts from being invoked.
2. Apparently if a block doesn't have any content its cache contexts are being ignored. This was discovered since we are using an empty test block. This is probably a bug though since the block title is rendered, so it makes complete sense to apply the cache contexts. This might be a relic from our (Drupal 6/7) past when blocks without content were not rendered at all. This probably is out of scope for this issue, but we should fix this in a followup. For the moment I'm working around it by making sure the block has content before launching the test.
3. Third, in AccessResult::cacheUntilEntityChanges() when the cacheability metadata is taken from the entity and applied to the AccessResult only the cache tags are considered:
   

     public function cacheUntilEntityChanges(EntityInterface $entity) {
       $this->addCacheTags($entity->getCacheTags());
       return $this;
     }
   

   It seems like we should also copy the cache contexts, since the entity might vary on a parameter that affects accessibility, for example the block visibility might be toggled in a config override. This seems to be out of scope for this issue though, but let's discuss this and create a followup if needed.

1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -210,6 +218,21 @@ protected function loadOverrides(array $names) {
   +  protected function propagateCacheableDependencyOverrides($cache_key) {
   +    foreach ($this->configFactoryOverrides as $override) {
   +      $this->cache[$cache_key]
   +        ->addCacheContexts($override->getCacheContexts())
   +        ->addCacheTags($override->getCacheTags())
   +        ->mergeCacheMaxAge($override->getCacheMaxAge());
   

   The way this API is defined right now makes it impossible to have different cacheablity metadata for different config objects.

   Is that really what we want?

   If not, then we should either pass the config name to those methods (which mean they can't implement CacheableDependencyInterface anymore, but that's fine IMHO. nobody will call them in a generic way).

   Or we use the pattern that we've used in other places and pass in a CacheableMetadata object to loadOverrides().

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -186,7 +186,21 @@ protected function doLoadMultiple(array $ids = NULL) {
   +
   +      // Add cacheability metadata to the record.
   +      $records[$id]['cacheContexts'] = $config->getCacheContexts();
   +      $records[$id]['cacheTags'] = $config->getCacheTags();
   +      $records[$id]['cacheMaxAge'] = $config->getCacheMaxAge();
   

   I'm not sure if this approach is correct. I think we should call the methods the return value of mapFromStorageRecords instead of relaying on implementation details here (how the property is named + that properties are just assigned like that).

3. +++ b/core/modules/block/src/BlockViewBuilder.php
   @@ -67,7 +67,10 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
              'keys' => ['entity_view', 'block', $entity->id()],
   -          'contexts' => $plugin->getCacheContexts(),
   +          'contexts' => Cache::mergeContexts(
   +            $entity->getCacheContexts(),
   +            $plugin->getCacheContexts()
   +          ),
   

   This is the crucial fix that we discovered in the web/integration test.

Config overrides aren't my thing, so this is a general review, feel free to ignore if not relevant.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -186,7 +186,21 @@ protected function doLoadMultiple(array $ids = NULL) {
   +      $key = array_search('config:' . $config->getName(), $records[$id]['cacheTags']);
   

   Could there ever be an instance where array_search doesn't find this item? Should we allow for that?

2. +++ b/core/modules/config/tests/config_override_integration_test/src/Cache/ConfigOverrideIntegrationTestCacheContext.php
   @@ -0,0 +1,35 @@
   +    $state = \Drupal::state()->get('config_override_integration_test.enabled', 'no');
   
   +++ b/core/modules/config/tests/config_override_integration_test/src/CacheabilityMetadataConfigOverride.php
   @@ -0,0 +1,73 @@
   +    $state = \Drupal::state()->get('config_override_integration_test.enabled', 'no');
   

   Any reason not to use a boolean here?

3. +++ b/core/modules/config/tests/config_override_test/config_override_test.services.yml
   @@ -1,4 +1,8 @@
   +    class: Drupal\config_override_test\Cache\PirateDayCacheContext
   

   nice!

4. +++ b/core/modules/config/tests/config_override_test/src/Cache/PirateDayCacheContext.php
   @@ -0,0 +1,52 @@
   + * Defines the PirateDayCacheContext service that allows to cache the booty.
   

   needs more yarr

5. +++ b/core/modules/config/tests/config_override_test/src/Cache/PirateDayCacheContext.php
   @@ -0,0 +1,52 @@
   +   * To ease testing this is determined with a global variable rather than using
   +   * the traditional compass and sextant.
   

   <3

From #62:

1. The way this API is defined right now makes it impossible to have different cacheablity metadata for different config objects. Is that really what we want?

   To me it looks wrong. The current implementation is applying the cacheability metadata of all config overrides to all config objects. That throws out the granularity we depend on. This is not currently tested.

   @Wim Leers, what is your opinion of this?

2. I think we should call the methods the return value of mapFromStorageRecords instead of relaying on implementation details here [...]

   Very good suggestion. mapFromStorageRecords() returns entity objects so we can set the values using the proper API instead.

3. This is the crucial fix that we discovered in the web/integration test.

   It is, thanks a lot for the help!

From #63:

1. Could there ever be an instance where array_search doesn't find this item? Should we allow for that?

   There shouldn't be since ConfigBase::getCacheTags() always returns the cache tag in this format. Technically nothing is stopping a contrib module from implementing its own variation of this method and returning another cache tag format. I don't think this would be recommended but you never know the exotic use cases contrib can dream up.
   Ideally we should have a method like getOwnCacheTag() but we could also simply throw an exception, forcing contrib do deal with it themselves if they decide to steer away from the recommended course.

2. Any reason not to use a boolean here?

   I originally had a boolean but changed it to a string at some point, will revert to a boolean again, it makes more sense.

3. Credit to @catch for the great idea for the test use case :)
4. See PirateDayCacheContext::getContext().
5. :)

#59: thank you very much for your thoroughness!

1. +1. See my reply to #62.2.
2. Still looking into this. Will answer in my next comment.
3. Replying to this in my next comment.

Also doing a code review, will be in my next comment.

#62

1. Interesting question! I wonder to what extent ConfigFactoryOverrideInterface was designed with this in mind though. It seems like it's designed to override all config objects in the same, consistent way. The single, parameterless getCacheSuffix() method seems to further indicate that?

   But I think it'd indeed work, because ConfigFactory only does static caching.

2. So I think you're saying This should not live here, but in an overridden mapFromStorageRecords() method?
3. Yep, that fix makes total sense. It's entirely analogous to EntityViewBuilder, that also applies the cache contexts of the entity. Now that a block (i.e. a placed block config entity) can also have cache contexts and max-age and not only cache tags, we need this. Which gets me to my point: this should also merge the max-age of the block entity!

#63: :)

Implemented first suggestion of #62-1.

Keelhauled the remaining remarks. I had to rush this because I have little time left before I embark on my return voyage, so please excuse any typos or other problems.

This is a review of the #66 + #67 interdiffs. Still working on what I promised in #65.

#66:

+++ b/core/lib/Drupal/Core/Config/ConfigFactoryOverrideInterface.php
@@ -7,12 +7,10 @@
-interface ConfigFactoryOverrideInterface extends CacheableDependencyInterface {
+interface ConfigFactoryOverrideInterface {

@@ -60,4 +58,51 @@ public function getCacheSuffix();
+  public function getCacheContexts($name);
...
+  public function getCacheTags($name);
...
+  public function getCacheMaxAge($name);

Let's then just add getCacheableMetadata($name) instead. We're doing it for analogous reasons as we did that in #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified (i.e. parameter-dependent cacheability metadata), so we should end up with an analogous solution.

#67:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -184,25 +184,35 @@ protected function doLoadMultiple(array $ids = NULL) {
+    $entities = $this->mapFromStorageRecords($records, $configs);
+
+    // Add cacheability metadata to the entities.
+    foreach ($entities as $id => $entity) {
+      $entity->addCacheContexts($configs[$id]->getCacheContexts());

Why do that here; why not add ConfigEntityStorage::mapFromStorageRecords(), have that call the parent (i.e. the default implementation) and then just add the cache tags to it?

Then ConfigEntityStorage::doLoadMultiple() doesn't need to change at all.

1. No 100% sure. The difference is that we then don't merge it, we assign it again using addCacheContexts(). So we'd create an object just to get the value out again. We definitely need to profile this becuase this is all in the critical path, we're loading lots and lots of config on pages.

2. And add what cache tags exactly? ;) We need the config objects. We tried to pass it to the map method, but the problem is that it would be another override-parent-method-with-optional-argument hack.

Addressing test failures, will now look at the remarks.

#69 has been fully answered by #70, leaving that as is, unless @Wim Leers disagrees.

#71:

1. I'm also no big fan of cacheUntilEntityChanges(), I like the idea of deprecating it in favor of a better named one, what about addCacheabilityMetadata()? Or doesn't that convey the action well enough? Would it be OK to defer this to a followup issue?
2. Ok will reword it in the next patch.
3. Yes this is perfectly fine. The original cache tag is always added automatically when the cache tags are retrieved (see ConfigBase::getCacheTags()).
4. Ok I'll file a followup issue and link it here, so that this workaround can be removed when the bug is fixed.
5. Will do! This suggestion got lost but it is indeed needed, my apologies!
6. OK!

Quoting myself in #65:

#59:
[…]

2. Still looking into this. Will answer in my next comment.
3. Replying to this in my next comment.

Also doing a code review, will be in my next comment.

#59.2 + #65.2 + #71.4: Had to wrap up today before I could get to this. Will do tomorrow.

#59.3 + #65.3: addressed at the bottom, in my reply to #71.1

#70.1: Good point. But I think API similarity is important here, if it doesn't cost us perf-wise.
#70.2: Ahhh! I didn't realize that. That makes a lot of sense, thanks! :)

#71.1 + #73.1: So the rationale for cacheUntilEntityChanges() dates back to the days where EntityInterface only had cache tags (it predates the existence of CacheableDependencyInterface). It's type-hinted to EntityInterface:

public function cacheUntilEntityChanges(EntityInterface $entity) {

Similarly, we have:

public function cacheUntilConfigurationChanges(ConfigBase $configuration) {

What about deprecating and redirecting both to a new addCacheableDependency() method, like we have on several interfaces now?

Test failures were due to #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified getting in, fixed those, still have to look at the other remarks.

#75: +1 to use addCacheableDependency() instead.

Addressed all remarks from comment #71. Still need to do #75+#78+#81.

Started implementing addCacheableDependency(). I thought it would be nice to use the RefinableCacheableDependencyTrait for this, and pass in a CacheableMetaData object.

I ran out of time before I was able to test it. This will probably fail horribly.

Although our stable testbot did not yet return with a result (after 4h), the CI testbot has numerous Undefined property: Drupal\Core\Access\AccessResultAllowed::$getCacheMaxAge.

Here is a quick fix so we get better test results.

Started addressing the failures in the unit tests. Covered ContentTranslationManageAccessCheckTest and AccessResultTest.

addCacheableDependency() should work with CacheableDependencyInterface, not CacheableMetadata. See the last few comments in #2525910: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case, which is doing the same now.

Not sure it is a good idea to already add that method to the interface here and implement it. In that other issue and also in #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity, we explicitly moved that part to a follow-up to only have to do the minimal changes as part of fixing the criticals. If we implement that method separately but in a consistent way then it's easy to move the method to the common interface/trait and use it for AccessResult and CacheableMetadata.

@Wim Leers already created an issue for letting AccessResult implement RefinableCacheableDependencyInterface and use the trait. That means we can limit the scope of this and defer all those changes to that issue: #2526326: Update CacheableMetadata & AccessResult to use RefinableCacheableDependency(Interface|Trait).

Trying to help @pfrennsen, because he indicated he would not have time for this today anymore. Restarting from #82 then as per #89 and adding a standalone addCacheableDependency(CacheableDependencyInterface $other_object)to AccessResult with the same implementation that #2525910: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case uses (I guess it can also be argued that access results may be numerous on a page, so the same merge optimizations may be in order, but we can obviously simplify it). That allows us to have a common trait / interface for them in #2526326: Update CacheableMetadata & AccessResult to use RefinableCacheableDependency(Interface|Trait). Not sure what kind of testing do we want on this method, its just bolted on at the moment.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -319,14 +274,15 @@ public function cachePerUser() {
   +   *
   +   * @deprecated Use this::addCacheableDependency() instead.
       */
      public function cacheUntilEntityChanges(EntityInterface $entity) {
   -    $this->addCacheTags($entity->getCacheTags());
   -    // @discuss Currently only the cache tags are copied from the entity, but
   -    //   the contexts are ignored. Shouldn't we include these, since we might
   -    //   vary on a parameter that affects accessibility? A good example would be
   -    //   block visibility.
   -    // $this->addCacheContexts($entity->getCacheContexts());
   +    $metadata = CacheableMetadata::createFromObject($this);
   +    $metadata->addCacheTags($entity->getCacheTags());
   +    $metadata->addCacheContexts($entity->getCacheContexts());
   +    $this->addCacheableDependency($metadata);
   +
   

   We still want this part, except that it's now a one line call to pass $entity to addCacheableDependency().

2. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -337,9 +293,33 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
   +   *
   +   * @deprecated Use this::addCacheableDependency() instead.
       */
      public function cacheUntilConfigurationChanges(ConfigBase $configuration) {
   -    $this->addCacheTags($configuration->getCacheTags());
   +    $metadata = CacheableMetadata::createFromObject($this);
   +    $metadata->addCacheTags($configuration->getCacheTags());
   +    $this->addCacheableDependency($metadata);
   +
   +    return $this;
   +  }
   

   Same here.

Updated with those, which ensures the new method is at least indirectly tested. Also edited the deprecation note with more info on the scope of depreciation (in D9).

It helps to actually pass on the variable we got passed :D

Copying the current approach from the other issue, more or less. without the optimizations for cache tags/contexts since AccessResult dos not use Cache::mergeContexts() right now and adding that makes those tests fail.

Also fixed and updated the test a bit. Didn't make much sense anymore what we tested there, so I just dropped some of the old tests for the now deprecated methods and instead add some new tests for addCacheableDependency().

Latest changes from @Gábor Hojtsy and @Berdir look great, just fixed a few nits. I think this is ready for final review.

Added two change record drafts: https://www.drupal.org/node/2532870 and https://www.drupal.org/node/2532882 please review.

Profiling results, tested 200 requests with a concurrency of 1 as authenticated user.

node/1

HEAD:

              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   592  654  21.7    660     692
Waiting:      564  624  21.1    630     661
Total:        592  654  21.7    660     692

Patch:

              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   599  659  18.2    663     683
Waiting:      571  630  17.7    634     652
Total:        599  659  18.2    663     683

node/1/edit

HEAD:

              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   597  656  20.9    663     685
Waiting:      570  627  20.3    634     654
Total:        597  656  20.9    663     685

Patch:

              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   601  659  19.5    664     685
Waiting:      573  630  19.0    635     654
Total:        601  659  19.5    664     685

admin/config

HEAD:

              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   709  781  22.1    785     817
Waiting:      679  749  21.8    753     782
Total:        709  781  22.1    785     817

Patch:

              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   714  779  23.1    784     816
Waiting:      683  747  22.8    751     783
Total:        714  779  23.1    784     816

I mostly have nitpicks and relatively-nitpicky things. But those relatively-nitpicky things I feel are actually quite important because they affect future code quite a bit, since after this critical issue/patch, it won't be changed again.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -332,9 +334,52 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
   +  public function addCacheableDependency($other_object) {
   

   Same concerns here as at #2525910-108: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case.

2. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -319,10 +319,12 @@ public function cachePerUser() {
   +   *   this::addCacheableDependency() instead.
   
   @@ -332,9 +334,52 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
   +   *   this::addCacheableDependency() instead.
   

   The this:: in there looks super bizarre. Let's s/this::/::/, no?

3. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -126,6 +126,10 @@ protected function doGet($name, $immutable = TRUE) {
   +      // Propagate cache contexts to the config object.
   +      $this->propagateCacheableDependencyOverrides($cache_key, $name);
   
   @@ -183,6 +187,10 @@ protected function doLoadMultiple(array $names, $immutable = TRUE) {
   +        // Propagate cacheable dependencies to the config object.
   +        $this->propagateCacheableDependencyOverrides($cache_key, $name);
   

   The comments are inconsistent.

4. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -210,6 +218,23 @@ protected function loadOverrides(array $names) {
   +  protected function propagateCacheableDependencyOverrides($cache_key, $name) {
   

   Actually, this is a bad method name too.

   We're not propagating cacheable dependencies; overridden config is a cacheable dependency, and a config overrider is NOT a cacheable dependency (it can't be a cacheable dependency because it's not a value object; it's logic). What happens here is we're propagating the config-specific cacheability metadata from the config overrider onto the overridden config.

   (A simpler example: a render array that only shows a child if a certain access result indicates it's allowed; then that access result is a cacheable dependency of the render array.)

   So, I think in this case it is actually propagateConfigOverriderCacheabilityOntoConfig() or something like that.

5. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -210,6 +218,23 @@ protected function loadOverrides(array $names) {
   +      $this->cache[$cache_key]
   +        ->addCacheContexts($override->getCacheContexts($name))
   +        ->addCacheTags($override->getCacheTags($name))
   +        ->mergeCacheMaxAge($override->getCacheMaxAge($name));
   
   +++ b/core/lib/Drupal/Core/Config/ConfigFactoryOverrideInterface.php
   @@ -58,4 +58,51 @@ public function getCacheSuffix();
   +  public function getCacheContexts($name);
   ...
   +  public function getCacheTags($name);
   ...
   +  public function getCacheMaxAge($name);
   

   I'd like to repeat #69.1. Also see Berdir's reply in #70.1.

   But it still feels like it'd easier to understand.

   Given the recent changes,
   it'd mean the cited code could then actually be changed to:

   $this->cache[$cache_key]->addCacheableDependency($override->getCacheableMetadata($name));
   

   Also: it'd mean slightly fewer function calls.

   But I think the similarity of interfaces for similarity of reasons is the strongest argument for #69.1.

6. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryOverrideInterface.php
   @@ -58,4 +58,51 @@ public function getCacheSuffix();
   +   * These identify a specific variation/representation of the object.
   +   *
   +   * Cache contexts are tokens: placeholders that are converted to cache keys by
   +   * the @cache_contexts_manager service. The replacement value depends on the
   +   * request context (the current URL, language, and so on). They're converted
   +   * before storing or retrieving an object in cache.
   +   *
   ...
   +   * When this object is modified, these cache tags will be invalidated.
   

   These comments feel like overkill/unnecessary repetition.

7. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -184,11 +184,35 @@ protected function doLoadMultiple(array $ids = NULL) {
   +      $entity->addCacheContexts($configs[$id]->getCacheContexts());
   +      $entity->mergeCacheMaxAge($configs[$id]->getCacheMaxAge());
   

   Nit: Can be chained. Not sure if that's actually better here though.

8. +++ b/core/modules/config/tests/config_override_integration_test/src/Cache/ConfigOverrideIntegrationTestCacheContext.php
   @@ -0,0 +1,43 @@
   +  public function getContext() {
   +    // Default to the 'disabled' state.
   +    $state = \Drupal::state()->get('config_override_integration_test.enabled', FALSE) ? 'yes' : 'no';
   +    return 'config_override_integration_test.' . $state;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCacheableMetadata() {
   +    return new CacheableMetadata();
   +  }
   

   Normally, things that depend on State need to specify max-age = 0 (because it can change at any time).

   If this should not specify max-age = 0, we should document why not.

9. +++ b/core/modules/config/tests/config_override_test/src/Cache/PirateDayCacheContext.php
   @@ -0,0 +1,65 @@
   +  }
   +}
   

   Nit: needs \n in between.

#104:

1. Removed the optimization.
2. Fixed. this:: is weird indeed, more common is to use self:: or indeed just the shorthand ::.
3. Fixed.
4. Fixed. Renamed to propagateConfigOverriderCacheability().
5. Discussed this with @Wim Leers on IRC. In order for this to work we need to add both RefinableCacheableDependencyInterface::addCacheableMetadata() and ConfigOverriderInterface::getCacheableMetadata(). Wim suggested to add the addCacheableMetadata() method in addition to the existing ones (addCacheContexts() etc). In the case of getCacheableMetadata() I removed the original methods since they are now redundant.

   In converting the patch it does seem like using a CacheableMetadata object actually results in fewer function calls, and it is less code in total, the patch is smaller now.

6. Fixed.
7. Fixed.
8. Fixed.
9. Fixed.

I had to rush a bit, didn't have time to run all relevant tests, and still have to address remarks from #105. Will do that tomorrow!

Thanks! Looking great :)

1. +++ b/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyInterface.php
   @@ -52,4 +54,14 @@ public function addCacheTags(array $cache_tags);
   +   * Adds the passed in cacheability metadata to the object.
   +   *
   +   * @param \Drupal\Core\Cache\CacheableMetadata $metadata
   +   *   The cacheability metadata to add.
   +   *
   +   * @return $this
   +   */
   +  public function addCacheableMetadata(CacheableMetadata $metadata);
   

   This should be named addCacheableDependency(), mirrored after \Drupal\Core\Cache\CacheableResponseInterface::addCacheableDependency().

   Also, its parameter should not be typehinted to CacheableMetadata, but to nothing, just like that other example. In case the passed in parameter implements CacheableDependencyInterface, we have cacheability metadata, otherwise, we assume max-age = 0. Just like that other example.

2. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -225,12 +225,9 @@ protected function loadOverrides(array $names) {
   -  protected function propagateCacheableDependencyOverrides($cache_key, $name) {
   +  protected function propagateConfigOverriderCacheability($cache_key, $name) {
   

   The method is renamed, but the docblock isn't updated accordingly.

3. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryOverrideInterface.php
   @@ -59,50 +59,11 @@ public function getCacheSuffix();
   +  public function getCacheableMetadata($name);
   

   This is now missing docs for the $name parameter. Let's bring that back :)

4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -196,8 +196,9 @@ protected function doLoadMultiple(array $ids = NULL) {
   -      $entity->addCacheContexts($configs[$id]->getCacheContexts());
   -      $entity->mergeCacheMaxAge($configs[$id]->getCacheMaxAge());
   +      $entity
   +        ->addCacheContexts($configs[$id]->getCacheContexts())
   +        ->mergeCacheMaxAge($configs[$id]->getCacheMaxAge());
   

   This can now be simplified to:

   $entity->addCacheableDependency($configs[$id]);
   

5. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -196,8 +196,9 @@ protected function doLoadMultiple(array $ids = NULL) {
          // Remove the self-referring cache tag that is present on Config objects
          // before setting it. A ConfigEntity doesn't need this since it will be
   

   Except we still have this, which is why the previously commented code looks like it does.

   Can't we simplify this too? Can't we move this logic to ConfigEntity::addCacheTags(), so that that simply filters out cache tags that are returned by ConfigEntity::getCacheTagsToInvalidate()?

   Or… even simpler, and more generically, can't we change RefinableCacheableDependencyTrait::addCacheTags() from

   $this->cacheTags = Cache::mergeTags($this->cacheTags, $cache_tags);
   

   to:

   $this->cacheTags = Cache::mergeTags($this->cacheTags, array_diff($cache_tags, $this->getCacheTags()));
   

   (Just playing devil's advocate here. I'm not sure whether that's actually clearer. The current code confused @beejeebus in #105, so trying to think of alternatives.)

Not quit sure if the reason why we exclude that cache tag is clear from the comment.

We do this because things like renaming or creating duplicates of an entity would otherwise keep that cache tag which would then still reference the old object. Not a huge deal, but IIRC it came up in manual testing or caused some test fails. So it's not just a performance optimization or so.

That said, moving that logic into the entity sounds nice, I like that. Not quite sure what that means for DateFormat, though. Because the cache tag to invalidate for that is 'rendered', so code like the above would *not* filter them out, which would in turn result in them being added to all kinds of things...

Regarding #5: This indeed warrants some more detailed explanation.

This is an edge case that is only present in ConfigEntities. Most cacheable objects are standalone, but config entities are 100% dependant on their Config, and thus always inherit the cache tags from the Config object. The cache tag of a ConfigEntity object is identical to the cache tag of the Config object it is derived from.

A ConfigEntity must inherit all the cache tags from its parent Config object, but when you call Config::getCacheTags() this also includes the tag that refers to the Config object. This is how the self-referring tag ends up in a ConfigEntity. This doesn't happen for any other cacheable objects since they are not so closely coupled to another cacheable object.

It is useless for a cacheable object to reference itself in its cache tags, none of the cacheable objects do this. ConfigEntity shouldn't be an exception.

Now this whole debate is probably rather academic, the presence of the self-referring tag is not causing any harm, but this will have some impact on performance since this self-referring cache tag might get merged hundreds of times when bubbling up.

I like the suggestion to move this to ConfigEntity::getCacheTags() though.

edit: @berdir is correct in #108, we did encounter some test failures when this was originally added, I forgot about that. We should investigate a bit.

Do the terminology discussions in #2525910: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case affect how we should introduce new APIs in this patch? The names of public functions, etc? For consistency.

@Gabor Hojtsy, yes we should ensure to be consistent. I will follow up on that issue and backport any changes to the interfaces.

Currently, the two issues are still consistent. But, yes, we should keep them in sync. Ideally, the other issue will land soon, that'd make things easier :)

Just some drive by review ...

1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -210,6 +218,20 @@ protected function loadOverrides(array $names) {
   +  protected function propagateConfigOverriderCacheability($cache_key, $name) {
   +    foreach ($this->configFactoryOverrides as $override) {
   +      $this->cache[$cache_key]->addCacheableMetadata($override->getCacheableMetadata($name));
   

   Did we discussed about making it plural by default to save a little bit of function calls?

   Beside from that, why do overrides not implement \Drupal\Core\Cache\CacheableDependencyInterface?

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -184,11 +184,36 @@ protected function doLoadMultiple(array $ids = NULL) {
   +    // Add cacheability metadata to the entities.
   +    foreach ($entities as $id => $entity) {
   

   I think some explanation about why would be nice

3. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
   @@ -694,6 +731,19 @@ public function testLoadMultipleAll() {
   +      ->will($this->returnValue([]));
   ...
   +      ->will($this->returnValue(['config:foo']));
   ...
   +      ->will($this->returnValue(Cache::PERMANENT));
   ...
   +      ->will($this->returnValue('foo'));
   
   @@ -703,6 +753,18 @@ public function testLoadMultipleAll() {
   +      ->will($this->returnValue([]));
   ...
   +      ->will($this->returnValue(['config:bar']));
   ...
   +      ->method('getCacheMaxAge')
   ...
   +      ->method('getName')
   
   @@ -742,6 +804,18 @@ public function testLoadMultipleIds() {
   +      ->will($this->returnValue([]));
   ...
   +      ->will($this->returnValue(['config:foo']));
   ...
   +      ->will($this->returnValue('foo'));
   

   In case we still have to adapt things here, let's just use ->willReturn(...) it is so much better to read

1. plural would make it more complex since we pass along the name of the object. Not quite sure how that would work. I'd say we should keep it unless we have numbers from profiling that show we have a problem there? And about the interface, same reason, the additional argument. We want to be able to allow an override that only every acts on block config entities to not apply the cache context or whatever to every config in the system.

Did not apply anymore in BlockViewBuilder, reroll first.

Working on addressing at least some of the feedback.

Resolving #107 (except /5) and #114 (except /1), those two are still in discussion.

1. RefinableCacheableDependencyInterface::addCacheableMetadata(CacheableMetadata $metadata) renamed to RefinableCacheableDependencyInterface::addCacheableDependency($other_object)
2. Adapted implementation in RefinableCacheableDependencyTrait.php, eg. it did not return $this before even though the interface mandates it
3. Discussed propagateConfigOverriderCacheability() with Wim and the name is confusing, because we never call them overrideRs, we call them overrides. The interface, etc. Only tests have overriders (which provide overrides BTW). So renamed to propagateConfigOverriderCacheability()
4. Removed inline comments around it because they were not added value. Updated phpdoc.
5. Restored the $name phpdoc in ConfigFactoryOverrideInterface
6. Simplified the adding of dependency info in ConfigEntityStorage
7. Simplified the propagation of cache info in ViewUI (and removed a bunch of unused uses which are now obsolete)
8. Using willReturn now in ConfigEntityStorageTest (that was quite a bit of changes, not entirely sure it should be in scope here, but well)

#114.1: Berdir already explained this in #115. But in case it wasn't clear yet: because CacheableDependencyInterface only works for value objects (i.e. the same tags/contexts/max-age are returned always); but config overrides are not a value object: they apply different cacheability metadata to different config (i.e. different tags/contexts/max-age are returned).

Interdiff looks great! Since you applied my suggestion in #107.4, but did not address #107.5, I'm curious to see if that'll cause any failures.

+++ b/core/modules/views_ui/src/ViewUI.php
--- a/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
@@ -581,20 +581,20 @@ public function testSaveNoMismatch() {
-      ->will($this->returnValue($config_object));
+      ->willReturn($config_object);

I don't think the changes in this file, all similar to the one I quoted here, are actually necessary? (EDIT: you say the same in your point 8.)

They're harmless though. So I'm fine with this personally. EDIT: @dawehner asked for this in #114.3, Gábor just updated the existing ->will() calls as well, for local consistency. Makes sense. Just a bit distracting is all.

Cannot work on this anymore today, cannot assign back to pfrenssen, so just unasssigning.

Well yeah I did not asked about changing existing ones, just new ones would be nice to do it right from the start.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -332,9 +334,33 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
   +      $this->maxAge = Cache::mergeMaxAges($this->maxAge, $other_object->getCacheMaxAge());
   
   +++ b/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyTrait.php
   @@ -36,6 +36,27 @@
   +      if ($this->maxAge === Cache::PERMANENT) {
   +        $this->maxAge = $other_object->getCacheMaxAge();
   +      }
   +      elseif (($max_age = $other_object->getCacheMaxAge()) && $max_age !== Cache::PERMANENT) {
   +        $this->maxAge = Cache::mergeMaxAges($this->maxAge, $max_age);
   +      }
   ...
   +    else {
   +      // Not a cacheable dependency, this can not be cached.
   +      $this->maxAge = 0;
   +    }
   

   This difference needs at least some explanation, but honestly I don't get why this is not part of Cache::mergeMaxAges

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -184,11 +184,36 @@ protected function doLoadMultiple(array $ids = NULL) {
   +    /** @var Config[] $configs */
   

   We use FQCNs here

Let's have a look how this can move forward.

@dawehner: re

#122/0: the file is a big mess; I can undo the work of fixing the existing ones, regardless of what we add here, the file has all kinds of array and willReturn() or not approaches all over the place

#122/1: I guess it is sort of part of that, it is just doing it differently. CacheableMetadata::merge() does these same shortcuts, but it claims its called many times a request. We don't need to do the same shortcuts here.

#122/2: Easy to fix.

Addressed all of these.

It looks like all remarks have been addressed, except #107-5. Answers have been provided in #108 and #110, but we still need to give some more context and investigate whether the logic can be moved to a better place, or if this turns out not to be possible we should at least provide better documentation.

Short recap of this remark:

• In #107-5 @Wim Leers questions why we are manually filtering out the self-referring cache tags in ConfigEntityStorage::doLoadMultiple(). The documentation only mentions this is done for performance reasons. Maybe a better place for this is ConfigEntity::addCacheTags() or RefinableCacheableDependencyTrait::addCacheTags().
• In #108 @Berdir mentions that apart from the performance optimization this also impacts config entities that are cloned or renamed. This would change the cache tag, but the original cache tag would still be kept if it is not removed at this point.

Addressed the first test failure, and started to look into the second one but my time is up for today.

Fixing more places in ConfigEntityStorageTest for number of calls of getCacheTags, similar to 127.
Fixing a missing self-referential cache tag missing in tests in Views' plugin CacheTest.

Theoretically this should be green now. :)

So with the patch green again, the only thing not yet addressed is my remark in #107.4-5, which explored a solution to what @beejeebus raised as being confusing in #105. But… we can clean it up further in a non-critical follow-up.

So I think that if the reasons Berdir gave in #108 for the current implementation being the way it is, this can be committed.

Giving this a review.

Given the comment below is a minor problem and this is a critical issue, setting to RTBC status for now.

The patch looks fantastic!

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -184,11 +183,36 @@ protected function doLoadMultiple(array $ids = NULL) {
+      $entity->addCacheableDependency($configs[$id]);
...
+      // Remove the self-referring cache tag that is present on Config objects
+      // before setting it. A ConfigEntity doesn't need this since it will be
+      // dynamically generated in EntityInterface::getCacheTagsToInvalidate().
+      // The cache tags are merged during rendering, and having fewer tags
+      // available improves performance.
+      $cache_tags = $configs[$id]->getCacheTags();
+      $key = array_search('config:' . $configs[$id]->getName(), $cache_tags);
+      if ($key !== FALSE) {
+        unset($cache_tags[$key]);
+      }
+      $entity->addCacheTags($cache_tags);

Yeah, the removal and adding currently is a no-op.

As addCacheTags per design can only add tags and not set them.

Probably the "easiest" way to solve this would be:

$cacheable_metadata = CacheableMetadata::createFromObject($configs[$id]);

// getCacheTags()
// filter()
// setCacheTags()

$entity->addCacheableDependency($cacheable_metadata);

Could be fixed in a quick follow-up.

1. Shouldn't we be addressing #133 as part of this part rather than a followup?

2. +++ b/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyTrait.php
   @@ -36,6 +36,22 @@
   +  public function addCacheableDependency($other_object) {
   +    if ($other_object instanceof CacheableDependencyInterface) {
   +      $this->addCacheContexts($other_object->getCacheContexts());
   +      $this->addCacheTags($other_object->getCacheTags());
   +      $this->mergeCacheMaxAge($other_object->getCacheMaxAge());
   +    }
   +    else {
   +      // Not a cacheable dependency, this can not be cached.
   +      $this->maxAge = 0;
   +    }
   +    return $this;
   +  }
   

   I can't see any explicit test coverage of this logic - unlike the coverage for the similar change in AccessResult

3. +++ b/core/modules/config/tests/config_override_test/src/ConfigOverrider.php
   @@ -7,6 +7,8 @@
   +use Drupal\Core\Cache\Cache;
   

   Not used.

4. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -10,7 +10,8 @@
   +use Drupal\Core\Cache\Cache;
   +use Drupal\Core\Cache\CacheableDependencyInterface;
   

   Not used.

#134.

1. It's not essential, the current code is fine, the discussion is solely about making it more elegant.
2. #2526326: Update CacheableMetadata & AccessResult to use RefinableCacheableDependency(Interface|Trait) will make it significantly easier to test this, and I don't think we want to roll this into that. But, of course, fair point. Would you be okay with doing the AccessResult portion of #2526326 in this issue?
   

I'm happy with the reasons that @Wim Leers gives in #135. Once AccessResult uses the trait we have coverage. Points 3 and 4 can be fixed on commit.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -184,11 +183,36 @@ protected function doLoadMultiple(array $ids = NULL) {
+      // Remove the self-referring cache tag that is present on Config objects
+      // before setting it. A ConfigEntity doesn't need this since it will be
+      // dynamically generated in EntityInterface::getCacheTagsToInvalidate().
+      // The cache tags are merged during rendering, and having fewer tags
+      // available improves performance.
+      $cache_tags = $configs[$id]->getCacheTags();
+      $key = array_search('config:' . $configs[$id]->getName(), $cache_tags);
+      if ($key !== FALSE) {
+        unset($cache_tags[$key]);
+      }
+      $entity->addCacheTags($cache_tags);

I still don't feel we've addresses all the feedback surrounding this comment. There has been talk of moving it into getCacheTags and of the need to improve the documentation because it is not only about performance.

#137: but:

the only thing not yet addressed is my remark in #107.4-5, which explored a solution to what @beejeebus raised as being confusing in #105. But… we can clean it up further in a non-critical follow-up.

See bottom of #105 + #108. That's why I wrote It's not essential, the current code is fine, the discussion is solely about making it more elegant. in #135.

So… I'm not quite sure what you mean, @alexpott? Back to RTBC to get feedback.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -184,11 +183,36 @@ protected function doLoadMultiple(array $ids = NULL) {
+      // Remove the self-referring cache tag that is present on Config objects
+      // before setting it. A ConfigEntity doesn't need this since it will be
+      // dynamically generated in EntityInterface::getCacheTagsToInvalidate().
+      // The cache tags are merged during rendering, and having fewer tags
+      // available improves performance.

In #126 @pfrenssen:

In #108 @Berdir mentions that apart from the performance optimization this also impacts config entities that are cloned or renamed. This would change the cache tag, but the original cache tag would still be kept if it is not removed at this point.

Discussed with @xjm, @catch, @effulgentsia and @webchick and we agreed that this issue is critical because of the cache poisoning possibilities once contrib adds configuration overrriders.

Okay, agree then, lets fix #133.

@Wim: This is not about elegance, but currently the cache tag is not removed.

And yes as this is a bug that is "green" we need to add explicit test coverage for that.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -184,11 +183,36 @@ protected function doLoadMultiple(array $ids = NULL) {
+    foreach ($entities as $id => $entity) {
+      $entity->addCacheableDependency($configs[$id]);
+
+      // Remove the self-referring cache tag that is present on Config objects
+      // before setting it. A ConfigEntity doesn't need this since it will be
+      // dynamically generated in EntityInterface::getCacheTagsToInvalidate().
+      // The cache tags are merged during rendering, and having fewer tags
+      // available improves performance.
+      $cache_tags = $configs[$id]->getCacheTags();
+      $key = array_search('config:' . $configs[$id]->getName(), $cache_tags);
+      if ($key !== FALSE) {
+        unset($cache_tags[$key]);
+      }
+      $entity->addCacheTags($cache_tags);
+    }

@FabianX has a point, there is something wrong here. It looks like the cache tags are set in $entity->addCacheableDependency($configs[$id]), so the following code will be ineffective.

I will have some time to dig into this this afternoon.

For the moment let's try just remove it, if it is really ineffective then the tests should be green. We could also solve this part in a follow-up, since this doesn't affect the critical part of the issue.

#144: After alexpott's comment that this might affect clones, I am pretty sure we need to solve this properly to avoid the config entity cache tag being present.

My pseudo-code was:

$cacheable_metadata = CacheableMetadata::createFromObject($configs[$id]);

// getCacheTags()
// filter()
// setCacheTags()

$entity->addCacheableDependency($cacheable_metadata);

which should work.

@FabianX thanks! Will address it when I have some time this afternoon.

Actually since this code was ineffective the remark from @berdir in #108 that the self-referring tag broke existing test coverage is now also answered. Since #119 Drupal\views\Tests\Plugin\CacheTest started failing because the self-refererring tag was present:

Value

array(
  0 => 'config:views.view.test_cache_header_storage',
  1 => 'views_test_data:1',
)

is equal to value

array(
  0 => 'config:views.view.test_cache_header_storage',
  1 => 'config:views.view.test_view',
  2 => 'views_test_data:1',
).

This failure was fixed in #130.

Addresses all of the feedback regarding Config entities inheriting cache tags from their underlying Config objects. Adds full documentation, implements the suggested solution from Fabianx (that I arrived independently at), and fixed the brokenness in CacheTestthat it caused since #119 as @pfrenssen pointed out in #146 (which I failed to notice in my reviews).

Note that the duplication test is kinda ugly, because config entities can only be saved if they have an ID, but creating a duplicate removes the original ID (which is obviously necessary), but then leaves you with no way to specify a new ID. However, for the purposes of this test, that's actually not a problem.

Worse, the renaming test is impossible to write because renaming config entities is broken AFAICT — opened #2539116: Renaming Config entities does not actually rename them for that. Wrote the test coverage, but for now it will fail, so I commented it out with a @todo.

Interdiff is relative to #130, not #144.

Back to RTBC

#134 3. and 4. bits can be fixed without impeding RTBC status.