This is split off from #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity to keep the scope manageable, please read the IS of that issue too!

Problem/Motivation

While discussing #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity we realised the configuration overrides do not provide cache contexts.

This does not currently manifest itself as a bug in core, but that is only because configuration overrides depend on the interface language, and interface language is a required cache context.

However as soon as you add domain module, organic groups or any other type of configuration override, then you have cache poisoning.

Proposed resolution

Have ConfigFactoryOverrideInterface extend CacheableDependencyInterface.

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.

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

Alex Pott also noticed that we have a bug with getCacheSuffix() - but I'll let him update the issue with that or open a spin-off (I think it was that the order, and hence cache key, could vary inconsistently).

While contributed project blockers are not usually critical, in this case the contrib module can port in every other aspect, but would have a security vulnerability until we fix this issue in core and it's updated to use it - and we have no way to stop people doing cache poisoning things in config overrides because really that's the entire point of them (arbitrarily changing config values based on whatever).

Remaining tasks

  1. Yep.
  2. Unpostpone #2526326: Update CacheableMetadata & AccessResult to use RefinableCacheableDependency(Interface|Trait).

User interface changes

Nope.

API changes

  • ConfigFactoryOverrideInterface now extends CacheableDependencyInterface.
  • ConfigBase now implements RefinableCacheableDependencyInterface

Data model changes

Nope.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Issue priority Critical because it allows cache poisoning through config overrides.
Prioritized changes The main goals of this issue are to enhance security and performance.
Files: 
CommentFileSizeAuthor
#158 interdiff.txt2.36 KBpfrenssen
#158 2524082-157.patch47.1 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,868 pass(es). View
#119 2524082-119.patch46.65 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2524082-119.patch. Unable to apply patch. See the log in the details link for more information. View
#116 2524082-116.patch41.93 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,605 pass(es), 1 fail(s), and 0 exception(s). View
#106 interdiff.txt20.48 KBpfrenssen
#106 2524082-106.patch42 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,376 pass(es), 1 fail(s), and 0 exception(s). View
#95 interdiff.txt1.58 KBGábor Hojtsy
#95 2524082-95.patch38.08 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 64,600 pass(es), 19,417 fail(s), and 3,628 exception(s). View
#93 2524082-93.patch37.8 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,237 pass(es). View
#93 interdiff-82-to-93.patch1.77 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,226 pass(es). View
#87 interdiff.txt6.97 KBpfrenssen
#87 2524082-87.patch47.03 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,180 pass(es), 78 fail(s), and 0 exception(s). View

Comments

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Issue tags: +D8 Accelerate London
Wim Leers’s picture

Issue summary: View changes
Issue tags: +API change

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

Wim Leers’s picture

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.

dawehner’s picture

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

Wim Leers’s picture

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.

Wim Leers’s picture

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

plach’s picture

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

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

pfrenssen’s picture

FileSize
10.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,299 pass(es), 0 fail(s), and 10 exception(s). View

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

Fabianx’s picture

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

jibran’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 2524082-8.patch, failed testing.

plach’s picture

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

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
12.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,296 pass(es), 1 fail(s), and 3 exception(s). View
7.39 KB

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?

pfrenssen’s picture

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

Fabianx’s picture

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.

pfrenssen’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 13: 2524082-12.patch, failed testing.

Fabianx’s picture

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

pfrenssen’s picture

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?

Wim Leers’s picture

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

Fabianx’s picture

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

Fabianx’s picture

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

pfrenssen’s picture

FileSize
13.96 KB
6.34 KB

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.

Wim Leers’s picture

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

Fabianx’s picture

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

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
15.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,339 pass(es), 1 fail(s), and 0 exception(s). View
5.68 KB

Implemented the remaining changes.

Fabianx’s picture

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

Status: Needs review » Needs work

The last submitted patch, 26: 2524082-26.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
15.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,346 pass(es). View
4 KB

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

pfrenssen’s picture

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.
Fabianx’s picture

Issue tags: +Needs tests

#30.1: Why are our tests passing then?

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

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

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

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
16.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,447 pass(es). View
41.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,480 pass(es). View
7.26 KB

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.

pfrenssen’s picture

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.

Fabianx’s picture

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

Berdir’s picture

Status: Needs review » Needs work
+++ 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.

plach’s picture

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

Wim Leers’s picture

  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()/[]/

pfrenssen’s picture

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.

pfrenssen’s picture

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

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config, +sprint
Gábor Hojtsy’s picture

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.

pfrenssen’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
43.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,495 pass(es), 3 fail(s), and 0 exception(s). View
44.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,483 pass(es), 5 fail(s), and 0 exception(s). View
16.57 KB

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?

pfrenssen’s picture

FileSize
5.42 KB

And the interdiff.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
FileSize
44.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,484 pass(es), 5 fail(s), and 0 exception(s). View
16.68 KB
3.83 KB

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?

The last submitted patch, 43: 2512718+2524082-43-config-entity-test-only.patch, failed testing.

The last submitted patch, 43: 2512718+2524082-43.patch, failed testing.

catch’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/MutableCacheableDependencyTrait.php
    @@ -0,0 +1,60 @@
    +  public function setCacheMaxAgeIfLower($max_age) {
    

    I know this was discussed a lot, but I'm not sure if we need the IfLower. When you set max age, it is always the maximum age, and other places we merge max ages we always take min($max_ages) - so it seems implied pretty well already. When I first saw the method name, I thought 'how do you know if it's lower when you set it?'.

  2. +++ b/core/modules/config/tests/config_override_test/src/PirateDayCacheContextConfigOverride.php
    @@ -0,0 +1,77 @@
    +    return PirateDayCacheContext::PIRATE_DAY_MAX_AGE;
    

    This could probably be dynamically set to 'seconds until pirate day', unless it's pirate day, in which case it could be 'seconds until midnight'. But completely unnecessary to do that of course.

Wim Leers’s picture

#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.)
Wim Leers’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 45: 2512718+2524082-45.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
49.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,523 pass(es). View
21.74 KB
11.13 KB

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.

Wim Leers’s picture

Issue summary: View changes

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.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Wim Leers’s picture

Issue summary: View changes

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

pfrenssen’s picture

FileSize
52.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,528 pass(es). View
21.75 KB
1.93 KB

Thanks Wim! Backported upstream changes.

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

Wim Leers’s picture

Issue tags: +Needs tests

Adding an issue tag for that integration test then :)

Wim Leers’s picture

Status: Needs review » Needs work

#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.
pfrenssen’s picture

Issue tags: -Needs tests
FileSize
23.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,076 pass(es), 15 fail(s), and 0 exception(s). View
31.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,091 pass(es). View
11.17 KB

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.

pfrenssen’s picture

Status: Needs work » Needs review

The last submitted patch, 59: 2524082-59-test-only.patch, failed testing.

Berdir’s picture

  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.

larowlan’s picture

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

pfrenssen’s picture

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. :)
Wim Leers’s picture

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

pfrenssen’s picture

FileSize
34.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,088 pass(es). View
11.09 KB

Implemented first suggestion of #62-1.

pfrenssen’s picture

FileSize
34.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,095 pass(es), 5 fail(s), and 0 exception(s). View
5.18 KB

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.

Status: Needs review » Needs work

The last submitted patch, 67: 2524082-67.patch, failed testing.

Wim Leers’s picture

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.

Berdir’s picture

Issue tags: +Needs profiling

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.

catch’s picture

  1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
    @@ -322,6 +322,11 @@ public function cachePerUser() {
       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());
         return $this;
       }
     
    

    Wondering a bit if we should add cacheByEntityCacheability() (better names welcome) and use that instead of this?

    For BC cacheUntilEntityChanges() could be deprecated and call through to that method.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryOverrideInterface.php
    @@ -58,4 +58,51 @@ public function getCacheSuffix();
    +   * 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 an object in cache.
    +   *
    

    Nit: is it worth rewording to 'They're converted before storing or retrieving an object in cache' or similar?

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -184,11 +184,35 @@ 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 = $config->getCacheTags();
    +      $key = array_search('config:' . $configs[$id]->getName(), $cache_tags);
    +      if ($key !== FALSE) {
    +        unset($cache_tags[$key]);
    +      }
    +      $entity->addCacheTags($cache_tags);
    +    }
    

    Is it OK to remove this?

    I can see code applying cache metadata from config objects, then expecting the config's own cache tag to be included there - so when they invalidate it the rendered stuff gets invalidated.

  4. +++ b/core/modules/config/src/Tests/CacheabilityMetadataConfigOverrideIntegrationTest.php
    @@ -0,0 +1,65 @@
    +
    +    // @discuss If our block does not contain any content then the cache context
    +    // is not bubbling up and the test fails. This probably indicates a bug
    +    // since the title of the block is rendered, so the context is useful.
    +    \Drupal::state()->set('block_test.content', 'Needs to have some content');
    +
    +    $this->drupalLogin($this->drupalCreateUser());
    +  }
    

    Sounds like a bug yes.

  5. +++ b/core/modules/config/tests/config_override_test/src/Cache/PirateDayCacheContext.php
    @@ -0,0 +1,52 @@
    +  const PIRATE_DAY_MAX_AGE = 86400;
    

    Per #48/49 this could use a comment to explain why 86400 wouldn't work for a real implementation, same as the one for $GLOBALS['is_pirate_day']

  6. +++ b/core/modules/config/tests/config_override_test/src/PirateDayCacheabilityMetadataConfigOverride.php
    @@ -0,0 +1,100 @@
    +   *
    +   * @return bool
    +   *   TRUE if the merchant ship will be boarded. FALSE if we drink rum instead.
    +   */
    +  protected function isCacheabilityMetadataApplicable($name) {
    +    return in_array($name, ['system.theme', 'block.block.call_to_action']);
    +  }
    +
    

    This is a good change.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
35.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,069 pass(es), 16 fail(s), and 0 exception(s). View
2.26 KB

Addressing test failures, will now look at the remarks.

pfrenssen’s picture

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

Status: Needs review » Needs work

The last submitted patch, 72: 2524082-72.patch, failed testing.

Wim Leers’s picture

Quoting myself in #65:

#59:
[…]

  1. Still looking into this. Will answer in my next comment.
  2. 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?

pfrenssen’s picture

FileSize
35.88 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,125 pass(es). View
1.69 KB
pfrenssen’s picture

Status: Needs work » Needs review
Fabianx’s picture

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

Status: Needs review » Needs work

The last submitted patch, 76: 2524082-76.patch, failed testing.

pfrenssen queued 76: 2524082-76.patch for re-testing.

catch’s picture

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

Makes sense to me.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
36.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,123 pass(es). View
2.88 KB

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

pfrenssen’s picture

FileSize
40.63 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
5.45 KB

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.

Gábor Hojtsy’s picture

FileSize
40.63 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 97,857 pass(es), 280 fail(s), and 0 exception(s). View
436 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 84: 2524082-84.patch, failed testing.

The last submitted patch, 83: 2524082-83.patch, failed testing.

pfrenssen’s picture

FileSize
47.03 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,180 pass(es), 78 fail(s), and 0 exception(s). View
6.97 KB

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

pfrenssen’s picture

Status: Needs work » Needs review
Berdir’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 87: 2524082-87.patch, failed testing.

pfrenssen’s picture

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

pfrenssen’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,226 pass(es). View
37.8 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,237 pass(es). View

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.

Berdir’s picture

  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.

Gábor Hojtsy’s picture

FileSize
38.08 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 64,600 pass(es), 19,417 fail(s), and 3,628 exception(s). View
1.58 KB

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

Status: Needs review » Needs work

The last submitted patch, 95: 2524082-95.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
38.08 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,241 pass(es), 2 fail(s), and 0 exception(s). View
583 bytes

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

Status: Needs review » Needs work

The last submitted patch, 97: 2524082-97.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
40.98 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,260 pass(es). View
5.07 KB

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

pfrenssen’s picture

FileSize
41.18 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,252 pass(es). View
1.5 KB

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

pfrenssen’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue tags: -Needs change record

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

pfrenssen’s picture

Issue tags: -Needs profiling

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
Wim Leers’s picture

Status: Needs review » Needs work

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.

beejeebus’s picture

ahoy me hearties, here be a drive-by review :-)

trying to figure out if i have any clue how caching and config work these days, so these comments may not be worth much.

+      // This is called many times per request, so avoid merging unless
+      // absolutely necessary.
+      if (empty($this->contexts)) {
+        $this->contexts = $other_object->getCacheContexts();
+      }

this seemed suspicious to me - why wouldn't merging contexts be fast when the the merge has an empty array? so i looked at Cache::mergeContexts() and i see this:

    \Drupal::service('cache_contexts_manager')->validateTokens($cache_contexts);

does it matter that we don't call that method in the 'fast path'? i have no idea, but validateTokens seems important?

+      // 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();

this doesn't make sense to me. we remove a tag because we know the entity system will just add it back? how does that help performance? do we not de-dupe before doing expensive stuff with cache tags?

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
42 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,376 pass(es), 1 fail(s), and 0 exception(s). View
20.48 KB

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

Wim Leers’s picture

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

Berdir’s picture

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

Status: Needs review » Needs work

The last submitted patch, 106: 2524082-106.patch, failed testing.

pfrenssen’s picture

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.

Gábor Hojtsy’s picture

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.

pfrenssen’s picture

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

Wim Leers’s picture

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

dawehner’s picture

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

Berdir’s picture

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
41.93 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,605 pass(es), 1 fail(s), and 0 exception(s). View

Did not apply anymore in BlockViewBuilder, reroll first.

Status: Needs review » Needs work

The last submitted patch, 116: 2524082-116.patch, failed testing.

Gábor Hojtsy’s picture

Assigned: pfrenssen » Gábor Hojtsy

Working on addressing at least some of the feedback.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
46.65 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2524082-119.patch. Unable to apply patch. See the log in the details link for more information. View
16.05 KB

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)

Wim Leers’s picture

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

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

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

dawehner’s picture

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

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

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

Gábor Hojtsy’s picture

FileSize
42.61 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,666 pass(es), 6 fail(s), and 0 exception(s). View
6.85 KB

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

Status: Needs review » Needs work

The last submitted patch, 124: 2524082-123.patch, failed testing.

pfrenssen’s picture

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.
pfrenssen’s picture

FileSize
2.55 KB
44.45 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,660 pass(es), 4 fail(s), and 0 exception(s). View

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

pfrenssen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 127: 2524082-127.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
45.69 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,804 pass(es), 9 fail(s), and 0 exception(s). View
3.08 KB

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

Wim Leers’s picture

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.

Fabianx’s picture

Assigned: pfrenssen » Fabianx

Giving this a review.

Fabianx’s picture

Assigned: Fabianx » Unassigned
Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

Wim Leers’s picture

#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?
alexpott’s picture

Status: Needs work » Reviewed & tested by the community

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.

alexpott’s picture

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

The last submitted patch, 130: 2524082-130.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

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

alexpott’s picture

Issue tags: +Triaged D8 critical

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.

Fabianx’s picture

Issue tags: +Needs tests

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.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
+++ 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.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
45.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,804 pass(es), 13 fail(s), and 0 exception(s). View
1.15 KB

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.

Fabianx’s picture

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

pfrenssen’s picture

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

Status: Needs review » Needs work

The last submitted patch, 144: 2524082-144.patch, failed testing.

Wim Leers’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
47.29 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,822 pass(es), 4 fail(s), and 0 exception(s). View
6.95 KB

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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

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

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
47.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,824 pass(es), 4 fail(s), and 0 exception(s). View
2.38 KB

Thanks @Berdir for pointing out I was renaming config entities all wrong. (Turns out it's both more intuitive than expected *and* less, depending on how you look at it.) Closed #2539116: Renaming Config entities does not actually rename them.

Fixed the nits in #134.3 + 4.

@Berdir++

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Aaaand back to RTBC, thanks for fixing the nits, too.

The last submitted patch, 148: 2524082-148.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
    @@ -332,9 +334,33 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
    +   * @param \Drupal\Core\Cache\CacheableDependencyInterface|mixed $other_object
    
    +++ b/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyInterface.php
    @@ -52,4 +52,18 @@ public function addCacheTags(array $cache_tags);
    +   * @param \Drupal\Core\Cache\CacheableDependencyInterface|mixed $other_object
    

    should we typehint for |object instead? mixed includes more than that

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -184,11 +184,41 @@ protected function doLoadMultiple(array $ids = NULL) {
    +      // 3. Fewer cache tags is better for performance.
    

    It is always better to either have a 3 or 5 dot list :P

pfrenssen’s picture

FileSize
47.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,836 pass(es), 4 fail(s), and 0 exception(s). View
809 bytes

Quickly sneaked in @dawehner's suggestion about using `object` instead of `mixed`.

dawehner’s picture

@pfrenssen
This has been there in more than one place.

pfrenssen’s picture

FileSize
47.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,849 pass(es), 4 fail(s), and 0 exception(s). View
871 bytes

@dawehner oops! I have to sneak another nitpick in the RTBC issue while @alexpott is not looking...

The last submitted patch, 150: 2524082-150.patch, failed testing.

pfrenssen’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
47.1 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,868 pass(es). View
2.36 KB

Fixing test failures.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#158 are trivial fixes.

The last submitted patch, 154: 2524082-154.patch, failed testing.

The last submitted patch, 156: 2524082-156.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 61603f5 and pushed to 8.0.x. Thanks!

  • alexpott committed 61603f5 on 8.0.x
    Issue #2524082 by pfrenssen, Gábor Hojtsy, Wim Leers, Berdir, Fabianx,...

The last submitted patch, 119: 2524082-119.patch, failed testing.

plach’s picture

There are a couple of draft CR mentioning this. Should they be published?

Wim Leers’s picture

Yes. Which ones aren't published?

plach’s picture

Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, glad to see this one landing :) Thanks all especially @pfrenssen for sticking to it :)

Status: Fixed » Closed (fixed)

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

The last submitted patch, 148: 2524082-148.patch, failed testing.