Proposed commit message:

Issue #2512718 by Berdir, pfrenssen, Wim Leers, Fabianx, dawehner, catch, effulgentsia, plach, Gábor Hojtsy: EntityManager::getTranslationFromContext() should add the content language cache context to the entity

Problem/Motivation

Entities and config both have the concept of variants of the canonical entity/config:

  • entities can have translations
  • config can have overrides
  • … other objects in D8 contrib can have similar patterns

Drupal 8 has a ParamConverter that translates an entity based on the negotiated language, i.e. based on the request context. It uses EntityManager::getTranslationFromContext() for that. Besides in a ParamConverter, EntityManager::getTranslationFromContext() is also called in other places, most notably EntityViewBuilder (which is used to render entities of all types).

So EntityManager::getTranslationFromContext() uses the negotiated language, but only if:

  • the entity implements TranslatableInterface, and
  • the entity does actually have translations, and
  • the $langcode parameter for getTranslationFromContext() is not set

In other words: in case all those conditions are met, then the entity actually varies by the content language cache context ('languages:' . LanguageInterface::TYPE_CONTENT, but it has no way to communicate that to the places where the rendering happens: the entity as upcasted in the paramconverter cannot possibly know/affect the code where the entity will actually be rendered; and similarly yet more narrowly the EntityViewBuilder calls getTranslationFromContext() yet has no way of knowing whether the content language cache context is actually necessary.

This has at least a couple of implications:

  1. #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) relies on the route cache context, which uses the raw parameters. This expects the upcasted parameters to be predicatable based on request context (at least in regards to cache contexts) - but they are not. See recent test failures in that issue, specifically #2429617-133: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) compared to the preceding patch.
  2. In fact, anything that relies on the route cache context is impacted in this way, leading to potential information disclosure problems as well.

Proposed resolution

Introduce a new RefinableCacheableDependencyInterface extends CacheableDependencyInterface that can be implemented by classes (objects) that have variants (translations, overrides …) that depend on request context (which means they need to add cache contexts, e.g. the content language cache context), on configuration (which means they need to add cache tags) or on time (which means they need to add max-age, e.g. Pirate day only lasts a day, so config overrides only last a single day).

This new interface allows those objects to receive additional cacheability metadata (hence the interface only has adders, no setters).

Concretely, this then allows EntityManager::getTranslationFromContext() to do $entity->addCacheContexts(['languages:' . LanguageInterface::TYPE_CONTENT]); when the translation of the entity was in fact based on the negotiated content language.

The one problem this then introduces is for entities: when saving an entity, we invalidate its cache tags. But it may now contain additional cache tags, which we do not want to invalidate. So, we also add EntityInterface::getCacheTagsToInvalidate(). This problem does not exist for Config, because config overrides are only applied to ImmutableConfig, not to editable Config. (If necessary, we can always extract this into a separate interface later, without API changes.)

Remaining tasks

  • Validate the proposed solution
  • Write a patch
  • Review it

User interface changes

None

API changes

  1. The following interface was added:
    interface RefinableCacheableDependencyInterface extends CacheableDependencyInterface {
    
      public function addCacheContexts(array $cache_contexts);
      public function addCacheTags(array $cache_tags);
      public function addCacheMaxAge($max_age);
    
    }
    
  2. Added EntityInterface::getOriginalCacheTags()

Data model changes

None

CommentFileSizeAuthor
#134 interdiff.txt3.13 KBpfrenssen
#134 2512718-134.patch30.53 KBpfrenssen
#125 interdiff.txt3.56 KBpfrenssen
#125 2512718-125.patch30.51 KBpfrenssen
#123 2512718-123-interdiff.txt17.65 KBBerdir
#123 2512718-123.patch30.54 KBBerdir
#119 2512718-117-interdiff.txt2.7 KBBerdir
#119 2512718-117.patch30.69 KBBerdir
#110 interdiff.txt1.1 KBpfrenssen
#110 2512718-110.patch27.94 KBpfrenssen
#107 interdiff.txt2.92 KBpfrenssen
#107 2512718-107.patch27.85 KBpfrenssen
#94 2512718-runtime-cache-context-93-interdiff.txt7 KBBerdir
#94 2512718-runtime-cache-context-93.patch27.9 KBBerdir
#91 2512718-runtime-cache-context-91-interdiff.txt846 bytesBerdir
#91 2512718-runtime-cache-context-91.patch25.08 KBBerdir
#86 2512718-runtime-cache-context-86-interdiff.txt13.69 KBBerdir
#86 2512718-runtime-cache-context-86.patch25.08 KBBerdir
#84 2512718-runtime-cache-context-84-interdiff.txt18.14 KBBerdir
#84 2512718-runtime-cache-context-84.patch25.12 KBBerdir
#77 2512718-runtime-cache-context-77-interdiff.txt17.3 KBBerdir
#77 2512718-runtime-cache-context-77.patch25.13 KBBerdir
#69 2512718-runtime-cache-context-68-interdiff.txt6.62 KBBerdir
#69 2512718-runtime-cache-context-68.patch13.55 KBBerdir
#62 2512718-runtime-cache-context-61-interdiff.txt7.53 KBBerdir
#62 2512718-runtime-cache-context-61.patch11.77 KBBerdir
#62 2512718-runtime-cache-context-61-test-only.patch5.17 KBBerdir
#58 2512718-runtime-cache-context-57-test-only.patch2.52 KBBerdir
#57 2512718-runtime-cache-context-57.patch6.86 KBBerdir
#57 2512718-runtime-cache-context-57-interdiff.txt2.52 KBBerdir
#56 2512718-runtime-cache-context-55-interdiff.txt856 bytesBerdir
#56 2512718-runtime-cache-context-55.patch4.34 KBBerdir
#55 2512718-runtime-cache-context-55-interdiff.txt856 bytesBerdir
#55 2512718-runtime-cache-context-55.patch4.34 KBBerdir
#51 2512718-runtime-cache-context-49.patch4.22 KBBerdir
#33 2512718-33.patch6.13 KBpfrenssen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Basically, this issue says "ParamConverters should only load the canonical object associated with the param, and not have any context-dependent logic". Because doing so means that the parameters upcasted by ParmConverters effectively depend on cache contexts themselves, but they have no way to propagate those.

In other words: ParamConverters containing context-based logic (rather than self-contained logic) breaks cacheability of any responses that use such ParamConverters.

dawehner’s picture

And yet that patch did not remove a single line of code in a controller that needed to get the language because (via Wim Leers),

Well that is not entirely true. Places where you would have to reintroduce it:
Access checking, title callback, block contexts etc.all they need to iterate on the proper object or you have for example a potential security issue.

Yeah no question, views_ui is a workaround.

dawehner’s picture

Issue summary: View changes

Removed the statement about the single line of code. The line is there, because its a generic API, but there are other places out there.

Gábor Hojtsy’s picture

@dawehner: right, access for example may be different per language, so it will make whatever to be cached different per language, so the question if it helps if the language loading happens later(?)

Also worth thinking about what happens to config entity loading then which is done in the request language too (and not at all handled with getTranslationFromContext()), also have no easy API to reload them in the request language. Also if we are to reload them in the request language at all times anyway, how does that help with caching? The output will be different based on conditions other than the request.

catch’s picture

Also if we are to reload them in the request language at all times anyway, how does that help with caching?

Anything that happens during the rendering process can set the language cache context, and that will work with SmartCache due to #cache_redirect.

The param converter is completely outside/before rendering, so there is no opportunity to set cache contexts there at all.

With entity access, we already have the ability to set cache contexts after #2287071: Add cacheability metadata to access checks - so an entity access checker that cares about the specific translation can both get the translation, and set the cacheability metadata there.

Gábor Hojtsy’s picture

@catch:

Anything that happens during the rendering process can set the language cache context, and that will work with SmartCache due to #cache_redirect.

Right, I think you'll have a harder time for config entities then because they don't know the language they were loaded in (or any variance in them based on overrides for the matter).

Fabianx’s picture

Basically it is the old rule:

- Whenever you use something external to your class, you must add a cache context.

The way I can see this is for language is:

'language' is already a cache key on the entity cache array.

Hence when I specify that I want to render this entity in german I need to specify:

render_with_language($entity, 'de');

But in case the language gets detected automatically:

render_with_language($entity, NULL);

Then the cache context would need to be added to the render array.

--

I think however the entity is already loaded in the right language ...

Gábor Hojtsy’s picture

@FabianX, @all: I think we need a thorough examination of the different things affected, eg. @dawehner brought up access checkers, those can now provide cache context. What about title callbacks he mentioned? Other things tightly connected to the request handling?

Gábor Hojtsy’s picture

@FabianX: also note that configuration may have arbitrary overrides, so it may be render_with_og_override(), render_with_domain_override(), render_with_time_of_day_override() or render_with_og_and_time_of_day_override(), etc. with the "little problem" that where the rendering happens it does not know which overrides are actually applicable / applied at that time. So looks like config objects loaded by the factory in general should be able to tell what overrides provided data that was merged in.

dawehner’s picture

Yeah I mean in general the problem is not necessarily only cache contexts, its the fact that you actually load the right translation.
In case you don't do that, as for example contrib authors give a shit about translations, you are screwed on a multilingual site.

Gábor Hojtsy’s picture

@dawehner: yeah the idea of solving the 80% case in the route upcasting was to make it work for those who don't care about it :/

Fabianx’s picture

Title: Numerous ParamConverters in core break the route cache context » Numerous ParamConverters in core break the route / url cache context
Issue tags: +Needs tests, +D8 Accelerate

I am gonna play devils advocate here.

The reason the test fails with smart cache is that the browsers language header is used for language negotiation without killing the page cache / smart cache.

However that will also kill any varnish caching we want to do - so that is not a good argument to block release on.

SmartCache could and should work with just [url] as we seriously talk about CDN edge caching.

There are good reasons for smart cache using [route] however:

- Access checks run (more secure)
- Theoretically smart cache could make use of negotiated data, but can't because the [route] cache context would be changing then (see below)

So we are in an interesting situation here:

- Smart cache uses information that is by now similar to [url] with some very little exceptions (language neg via browser), but normalized to which parameters are supported. (This makes it possible to avoid cache busting via ?bust=12345 e.g.)

I am however not sure it is worth blocking release on that as we pretty much take cache busting for granted even with Akamai. Some consider it even a feature.

- On the other hand it runs after a time where things have already been negotiated, so it would be great to be able to use that information.

Here the problem is that the [route] cache context if it used the upcasted parameters would still be changing.

However I don't think it would be wrong to have a [negotiated_route] cache key, which is statically set (e.g. on the request itself) when smart cache first tries to get things from cache.

Yes, that is a cache key, because it is based on negotiated information, not on external information.

So using a cache context is semantically wrong here as all information is already known by that point and _always_ runs, so the cache key is computed.

Compare views:

setPage() => use cache key
automatically derive page based on some external data => use cache context

It then also does not matter that entities are changed during the request, etc. That is of no concern for the cacheability.

So we need to stop mixing things here:

a) A param converter negotiating based on user and request.language => either need to set max-age=0 OR set [ user, 'headers:Content-Language' ], which in the end would ideally result in Vary: Content-Language for external proxies.

However I cannot see why that could not live on the request object, where access checks run during routing already put their cacheability data.

And I am even totally okay that some negotiator somewhere calls out to \Drupal::request()->getCurrentRequest()->get('cacheable_metadata')->addMaxAge(0); (or however we called that for access checks).

This should still be fixed, but is a major bug in my book, not demoting, yet, though.

That entities are loaded in the right language, however I cannot see as problematic.

I also think we could use our usual pattern here of checking all run negotiators / upcasters / ?, extend our own version of them with CacheableDependencyInterface and if they don't implement it => max-age=0. I think that is a good trade-off that we do elsewhere to be secure and fast.

b) Smart cache failing tests, yes smart cache can make use of negotiated information, but that is a cache 'key' at this point, not a cache context as it runs _AFTER_ said negotiation.

This can be fixed in smart cache itself and just means smart cache needs to store the initially negotiated cache key somewhere - be it on the request object or somewhere else.

Then the number of failures is correct, too.

Crell’s picture

I confess I'm a bit lost here. Can someone provide a failing test to demonstrate the issue? There seem to be a number of subtle bits being discussed I don't quite grok. Thanks.

Berdir’s picture

The reason the test fails with smart cache is that the browsers language header is used for language negotiation without killing the page cache / smart cache.

We discussed this. It has nothing to do with the header. The tests use an URL prefix for the language. browser neg in HEAD disables page cache and that also disables smartcache on those pages.

Just writing down some thoughts:

The *only* failing tests with smartcache were some routing tests that just return dummy context. We have many, many test that are accessing e.g. translated nodes. That's exactly the same scenario, really. But those are passing just fine. And the reason is that all real page callbacks have at least *one* thing that's being cached on the page. That means the render cache adds the default contexts, they bubble up to smartcache and /de/node/1 and /en/node/1 are cached differently by smartcache despite being the same route object (which I think is weird, the real URL should be reflected *somehow* in there or at least the route cache context. The reason that doesn't happen I guess is that we cut that part of the URL of and the routing system never sees it.)

Unless I'm missing something, that means fixing those tests should be pretty easy: Smartcache needs to ensure the same default cache contexts as the render cache, so that those are present even if there's nothing that's using render caching on a page.

For the more advanced use cases, like views ui that possibly loads the view from tempstore..
a) AFAIK, all those use cases are admin only, which we don't cache right now anyway.
b) If something doesn't work, they can call the page cache kill switch if nothing else works? If it's non-admin, they possibly already need to do that anyway?
c) Alternatively, special implementations could always move their logic to the controller.

I guess we could also somehow add a way for them to add cacheability metadata to the request or so. But for example for the views UI case, there's no context for that anyway, so that would have no choice but to disable caching anyway.

So.. so far, I really think that we can solve this default cache contexts + documentation on the param converters to define what they should and shouldn't do, and if they're doing something that's not recommended, what options/alternatives that they have?

Wim Leers’s picture

Unless I'm missing something, that means fixing those tests should be pretty easy: Smartcache needs to ensure the same default cache contexts as the render cache, so that those are present even if there's nothing that's using render caching on a page.

That would be an interesting "solution" :) But, alas, it's not a waterproof solution. The fact that almost all other tests pass merely points to our weakness: by default, the content language used is the same as the interface language. That's why all those other tests pass. If you'd have only two content languages but five interface languages, and hence would configure a different/more complex language negotiation setup, then those tests would also fail.


yeah the idea of solving the 80% case in the route upcasting was to make it work for those who don't care about it :/

This makes sense.

In that case, the more feasible solution would probably be to make parameter converters specify cache contexts.

@catch, thoughts?

Fabianx’s picture

Okay, to show again in very simple words:

In my opinion this issue is not critical:

1. Smartcache can use a cache key of the negotiated result. If [route] does loose information (e.g. translation prefix), then it should in addition use $request->getPathInfo(), which is protected against query string busting, because it is only the original path of the request.

[ edit: to clarify the route cache context itself should include getPathInfo() in the result. ]

2. Yes, param converters should have cache contexts, but that is only affecting code that runs before that of which we have two:

a) Page Cache - should use the original request.url so should be fine as there is only anonymous user and just 'dumb' anyway, so if you use page cache you cannot depend on anything else than [url] anyway.

b) Edge site caching, likely 8.1 material, so not release blocking - unless adding cacheable metadata to param converters is an API change that cannot be done in 8.1.

However given the interest of all parties on edge site caching, this can go in also as major.

dawehner’s picture

Reminds me on #2420523: Upcasting should not depend on the current user (make upcasting language agnostic) which has been explored as part of #2286971: Remove dependency of current_user on request and authentication manager.

You know I just fear that code like access checkers would be a too high risk or basically a more back to Drupal 7 world, where most contrib modules don't have support by default,
because, well, let's call it the english problem.

Crell’s picture

We discussed this during the WSCCI meeting today, and catch tried to explain the issue here. Assuming I understood correctly, here's my recommendation:

1) Add a CacheContextAwareParamConverterInterface (needs a better name) that param converters can implement to opt-in to providing cache contexts. That interface has a single method, contexts($value, $definition, $name, array $defaults) (same signature as convert()). It returns an array of cache contexts that are relevant if it's used.

2) ParamConverterManager then checks for that interface on any converter that actually gets used. If a converter it used and has that interface, it calls contexts() and merges the result into a $defaults['_cache_contexts'] array property. That gets returned all the way back up the routing system and merged into $request->attributes.

3) FinishResponseSubscriber, when assembling the Response, takes anything found in $request->attributes['_cache_contexts'] and merges that in.

I don't like putting more data into $request->attributes, but I don't know of an alternative. The approach above, though, has no BC breakage and puts the onus of cache-safety of param converter behavior on the param converter, not on controllers to implicitly know they need to replicate it. That is better DX for the 99% case.

dawehner’s picture

I wonder whether it would be worth implementation \Drupal\Core\Cache\CacheableDependencyInterfaceinstead of adding a new one ...

Crell’s picture

@dawhenher: The problem there is it assumes a value object and doesn't pass any data to the methods, so it would only work with hard-coded lists. But perhaps the converter could return an object implementing that...?

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Issue tags: +D8 Accelerate London

Working on this during the DA Accelerate critical sprint in London.

Fabianx’s picture

From D8 EU Criticals:

- Any cacheable metadata that is found during the param converters needs to be added as a required cache context, so its present on all things.
- The required cache contexts are injected in the renderer right now.

=> We need a required_cache_contexts service that has an interface to add new required cache contexts.

Disregard, discussed in IRC.

Cache Contexts are still needed for levels that come before param converters.

Wim Leers’s picture

- The required cache contexts are injected in the renderer right now.

Either I misheard due to the many Hangouts problems I was having, but I completely disagree with this.

That would massively break caching. E.g. the "powered by" block would not have the content language cache context on the /user URL, but it would have on the /node/1 URL. Hence cache misses.


So that brings us to the reason we catch and I said in #2: paramconverters shouldn't deal with variants, they should only deal with the canonical object (the downside being that all code needing to use translations must do so explicitly). Not going that way, which is the direction this issue is headed in now, means that problem doesn't exist, but means a new problem: how do we get all the things that depend on the parameter to also have the right cache context?

This is why I agree with #20 that the only sensible solution IF we want to have paramconverters still do translation, is:

3) FinishResponseSubscriber, when assembling the Response, takes anything found in $request->attributes['_cache_contexts'] and merges that in.

I don't like putting more data into $request->attributes, but I don't know of an alternative. The approach above, though, has no BC breakage and puts the onus of cache-safety of param converter behavior on the param converter, not on controllers to implicitly know they need to replicate it. That is better DX for the 99% case.

… but then we can still have broken caches: how will blocks using the upcasted node entity know that they're using a translated entity, and that they should hence set the content language cache context?

Wim Leers’s picture

Copy/pasting a relevant bit from IRC, about a potential solution for the bottom part of #25 that didn't turn out to be a complete solution:

11:29:39 Fabianx-screen: WimLeers: Should not everything the rendersn entity add content_language?
11:29:48 Fabianx-screen: by default.
11:29:51 WimLeers: everything that renders an entity you mean?
11:30:02 Fabianx-screen: yes
11:30:09 WimLeers: Fabianx-screen: ok, two things about that Q:
11:30:15 WimLeers: 1) YES! ABSOLUTELY! See https://www.drupal.org/node/2512712 to fix core :)
11:30:16 Druplicon: https://www.drupal.org/node/2512712 => Entities rendered by EntityViewBuilder should vary by content language cache context #2512712: Entities rendered by EntityViewBuilder should vary by content language cache context => 4 comments, 2 IRC mentions
11:31:19 WimLeers: 2) Yes! But it's impossible to do it for *everything*! What if you have a view or a block that receives the current node as a plugin context, and it doesn't actually render the entity, but uses the given entity to find the terms it references, and then shows those referenced terms?
11:31:49 WimLeers: IOW: I think there are use cases where we're not doing entity rendering, but just *using* an entity, and we *also* need the cache context there.
11:35:41 Fabianx-screen: WimLeers: Yes, that is a problem.
plach’s picture

I spoke a bit with @Wim about this: I think at this stage allowing every tiny bit of our codebase to specify its dependencies (cache contexts) might be nearly impossible, instead we could assume the "provider" of the problematic piece of code (paramconverter) is responsible for defining additional required cache contexts as a last-resort measure.

In the case of language.module, we could make it responsible for declaring the language cache contexts as required (i.e. extend renderer.config.required_cache_contexts). Depending on whether we have an explicit language negotiation configuration for content language this could be declared as required or not (so in the default case, the case that HEAD has as well, we could just make the interface language cache context required). This actually would be a better way to address the translation-upcasting-but-cache-contexts-are-missing problem, as language types can be defined by modules, which may introduce similar issues in the contrib space.

(credits to @Wim for pre-reviewing/improving my comment :)

Wim Leers’s picture

In discussion with @dawehner about #27 and preceding comments, I think we need one small addendum to #27: we would be caching too many variations of some things.

If we were to implement #27, then if (and only if!) content language negotiation is different from interface language negotiation, then the content language cache context would be a required cache context. Which means that even things that do not ever care about content language, would still be varied by it. For example: the "powered by" block would be cached per content language, even though that makes no sense.

But AFAICT that's a reasonable (and expected/unavoidable) cost to pay for still having the translation happening during parameter upcasting, which means that we don't need to force controllers to think about that.

IOW: we keep the current DX at the cost of having less efficient caching for some things.

(I think this is the best possible solution yet.)

Fabianx’s picture

Quick update from IRC:

- We upcast the [route], so we need to upcast the [route] cache context as well.

e.g. all the cache contexts added by the numerous param converters, need in the end be in [route], so when route is used all the variatons caused by the param converters are in the [route] cache context as well.

This even would work nicely with the hierarchy of things, if [route] gets reduced to [url], before that all the cache contexts of [route] are merged in. In that case we would not even need to change $request. (Yes, that would be cache contexts for cache contexts ...)

Though changing $request and having [route] take that into account would also be fine (and having the ResponseSubscriber take it from the $request) if we think that cache contexts on cache contexts are too crazy.

--

For config entities, we already require that you add the cacheable dependency when using e.g. config('site_name').

=> When e.g. domain module upcasts the config object to be dependent on the chosen domain, it needs to add the cache contexts to it.

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

e.g.

  $domain_site_name = $this->domainService->getSiteName();
  $config->applyOverride('site_name', $domain_site_name);
  $config->addCacheContexts(['domain']);

--

Catch points out the above won't work, what needs to happen is that:

ConfigFactoryOverrideInterface extends CacheableDependencyInterface, so that you must specify cache tags, cache contexts and cache max age.

Fabianx’s picture

So #20 works, but we need to have the route cache context, add the new parameter as well (when returning the value), not just the FinishResponseSubscriber. That solves both cases then as both depend on the immutable-by-policy $request->attribute(_route_cacheable_metadata); (like access already does)

Catch will split off config() to its own issue - as we need to do it regardless. (#2524082: Config overrides should provide cacheability metadata)

Fabianx’s picture

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
pfrenssen’s picture

FileSize
6.13 KB

Here's the little bit of work I had done so far, based on #20. This does not account for comments 21-30. This is incomplete and untested.

Wim Leers’s picture

To keep us all on the same page:

Proposal A (#2, catch/Wim): "paramconverters should only return the canonical object" (i.e. no entity translation)
Voted away because requires changes in all controllers
Proposal B (#20, catch/Crell): "paramconverters return cache contexts, that end up being merged and stored in a request attribute and then applied to the Response in the FinishResponseSubscriber"
Voted away because it doesn't work for things that depend on parameters but are also render cached individually (#25)
Proposal C (#27, plach): "don't change paramconverters, but require modules providing such paramconverters to update the required cache contexts accordingly"
Still viable, but less than ideal because it causes unnecessary cache variations and therefore a less effective cache (#28)
Proposal D (#29, Fabianx/catch): (an enhanced version of B) "paramconverters return cache contexts, that end up being merged and stored in a request attribute and then used by the route cache context (unsure how exactly, but the route cache context would like hash the cache contexts associated with the paramconverters), which means the route cache context then *does* correctly vary
This is possible, but is extremely hacky and quite questionable. It doesn't suffer from the problems in neither proposal B nor C, but lack the elegance of proposal D
catch’s picture

Title: Numerous ParamConverters in core break the route / url cache context » Entity::getTranslationFromContext() should add the content language cache context to the entity

Discussed this even more in irc.

We have mainly discussed three solutions today:

1. paramconverters set cacheability metadata, which is then added to the required cache contexts for everything on the page.

This has the disadvantage that something like the 'powered by Drupal' block might get varied by contexts that aren't relevant. Also paramconverters need to change.

2. paramconverters set cacheability metadata, which is then added to the route cache context - so that anything using the route cache context also gets the cache contexts from the paramconverters too.
This has the same disadvantage as #1.
The advantage is that only things that apply the route cache context get varied.
However there's a further disadvantage, consider the following:

A method in one class gets a node from the route parameters.

It then passes the node as parameter to a method on some other class.

That then creates a cacheable render array using the node. It has no idea the node came from the route, so it doesn't apply the cache context - still cache poisoning.

3. Just add more required cache contexts via a service - then config overrides and language negotiation can opt in.
This means more cache proliferation - but no poisoning.

I think there is an option #4.

We add a method to entities to set cache contexts. They already implement CacheableDependencyInterface. Then getTranslationFromContext() can call that method and add the content language cache context.

Berdir in irc just suggested something like addRuntimeCacheContext() or addDynamicCacheContext()

Then everywhere that uses the entity for rendering can get the cache contexts using the already existing Entity:getCacheContexts() - which already exists, and is already a requirement.

The additional advantage of this is it is similar to the approach in #2524082: Config overrides should provide cacheability metadata (thinking through that is where this idea eventually came from).

catch’s picture

Title: Entity::getTranslationFromContext() should add the content language cache context to the entity » EntityManager::getTranslationFromContext() should add the content language cache context to the entity
Wim Leers’s picture

Proposed interface:

RuntimeObjectVariantInterface {
  public function setVariantOrigin($cache_context);
}

Note that #2524082: Config overrides should provide cacheability metadata would need to use the same pattern.

Wim Leers’s picture

Extra background info for #35/#37:

  • In D7, an entity was always the canonical object, and you had to use field_get_items() with the $langcode parameter set to get the translated fields. So, in order to translate an entity, you had to figure out which language to display the entity in, which meant looking at the context. Which meant that controllers *did* think about context. (This is presumably also the reasoning Gábor explained in #5, #7 and particularly #12: it's a feature of D8 that all code automatically gets the translated entity, this makes sure that all of contrib actually works with translated entities.)
  • In D8, the "object carries the translation". This explains the better DX. But it also explains the lack of knowing the cache context to vary by. So, to fix that — we realized only now — we should just update ::getTranslationFromContext() to set the "content language" cache context, whenever it is actually used, and then the "object carries the translation AND the reason why".

This is why #37 proposes such generic names, because anything that does something like this could then add this cache context.

dawehner’s picture

It feels wrong that neither the classname nor the method name indiciates that this is about cache contexts ... in case this is not clear, people will not implement it.

Wim Leers’s picture

Talking this through with @plach, @dawehner and @berdir, this is what we ended up with:

interface RuntimeCacheableDependencyInterface extends CacheableDependencyInterface {

  public function addRuntimeCacheContext($cache_context);

}
plach’s picture

Component: routing system » cache system
Issue tags: +Entity Field API, +sprint

Moving to the right component.

effulgentsia’s picture

Is there any use-case for run-time cache tags or run-time max age? If so, should this be addRuntimeCacheableMetadata(CacheableMetadata $cacheable_metadata)?

effulgentsia’s picture

Or even:

interface MutableCacheableDependencyInterface extends CacheableDependencyInterface {

  public function addCacheableMetadata(CacheableMetadata $cacheable_metadata);

}
Wim Leers’s picture

#42: right, with the omission of "variant" (if you compare #41 to #37), the genericness of the naming suggests that should be possible. The concern with that though is that then an entity upon saving would actually invalidate those run-time cache tags as well.

catch’s picture

So I thought about a pirate module config override for #2524082: Config overrides should provide cacheability metadata.

On pirate day, it changes the default theme configuration to the 'pirate' theme.

Initially I'd suggested that might use max age (i.e. set the max age as seconds until pirate day, then on pirate day, set it as seconds until the end of pirate day).

But actually that use case can be solved much better with a cache context - the is_it_pirate_day_or_not cache context - which just returns a bool as a cache context value.

In irc, Wim Leers pointed out that if we add a cache tag to an entity, then this could then end up being invalidated if that entity is saved during that request - don't think we want that.

So I'd be hesitant to support anything other then cache contexts, unless there's a vey concrete use-case - since even with cache contexts the potential for mis-use is quite big. i.e. you could add user-specific stuff (like history module's information) to entity objects and set the user cache context on them and it will 'work' but ruin entity cache hit rates - when hook_entity_prepare_view() (or javascript) is a much better option.

effulgentsia’s picture

+1 to #45. In that case, ExtensibleCacheContextsInterface::addCacheContext()? With no need for ExtensibleCacheContextsInterface to extend CacheableDependencyInterface?

Fabianx’s picture

Yes, that was the feeling / idea of #8.

So I fully agree with option 4).

I like that entities get that information, it also means when things are not displayed that any negotiation does not affect the result.

However is there anything else than entities that can be upcasted?

--

On the interface, I am okay to only have a setter / getter for cache context here, because anything else gets quickly confusing.

TL;DR: +1 on the approach, carry on.

dawehner’s picture

We discussed that here at the sprint:

  1. There are usecases for passing max-age, given your code mutating could be lead to non cacheable stuff
  2. Given that there are uscases, passing CacheableMetadata seems fine
  3. On top of that we can avoid the save problematic, by adding a new parameter, see
    http://3v4l.org/DJVbd
Wim Leers’s picture

Note that #48 was discussed before #45/#46. So it's mostly irrelevant at this point. (The "we" in #48 are Daniel & I.)

catch’s picture

#47 upcasting I itself isn't the problem, it's whether something can be upcasted differently based on request context. I.e. Drupal 7 field translation didn't have this problem because there was no concept of current translation.

So anything else that which runs into the same issue will have to add CacheableDependencyInterface and a setter I think. Since those objects can also be passed as method params to things that don't know they came from the route.

Berdir’s picture

Status: Active » Needs review
FileSize
4.22 KB

Getting started with this.

I really think it's important to stress also on the method name that it's just for the current runtime and isn't persisted, so I went with the name that we discussed here for now in #40, it's easy to change.

This seems to be basically working, adding the line in EntityManager results in the language_content cache context being added to the page. Needs tests, documentation and so on, of course.

Note that, as hinted by comment in #47, this will actually *not* fix the test fails for smartcache because the test controller there isn't actually adding the cacheable metadata to the response. But then it's just the test being broken.

We also need to check every usage of ->getCurrentLanguage(LanguageInterface::TYPE_CONTENT) to identify other places where this cache context needs to be added... views for example has a query substitution for different types, so that will also need the language context or smartcache will be broken there. That's a pretty nice example for the really quite limited test coverage that we have for something like smartcache + multilingual.

dawehner’s picture

I totally agree that the method name should explain better when you would use it.

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -1336,4 +1336,11 @@ public function hasTrustedData() {
+  /**
+   * {@inheritdoc}
+   */
+  public function addRuntimeCacheContexts(array $cache_contexts) {
+    $this->storage->addRuntimeCacheContexts($cache_contexts);
+  }

Lovely ViewsUI!

effulgentsia’s picture

Good arguments for "Runtime" in the name, but can we rename RuntimeCacheableDependencyInterface to RuntimeCacheContextDependencyInterface? So that someone who wanted to could create RuntimeCacheTagsDependencyInterface if for whatever reason they disagreed with #45.

Status: Needs review » Needs work

The last submitted patch, 51: 2512718-runtime-cache-context-49.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.34 KB
856 bytes

Limiting the cache context so we have fewer tests to adapt, performance impact should be small.

This nicely shows that we're actually doing the whole logic to identify which language should be used twice, in the param converter and again in EntityViewBuilder since that doesn't know that it already happened. We tried to change that in #2073217: Remove the $langcode parameter from the entity view/render system a long time ago but it's more complicated than you'd think.

Berdir’s picture

I was also able to manually reproduce the real problem (not the test fail) with the smartcache patch:

1. Enable two language
2. Configure language negotiation, disable interface url negotation so it's always the default language (or use something else like user for it) and explicitly enable url negotation for the content language.
2. Create a node with a translations. The first translation you access will be cached, switching the language will show the node in the same language.

Will create a test for this today.

Berdir’s picture

Here is a very basic test. That might have some fails in other entity types, I've not tested them all yet. Not quite sure when and what is adding the language_url cache context, will need to look that up.

This is not the scenario described above since that would only fail when combined with smartcache so I'm not sure we really need that here (would be a useful thing to test in a non-critical follow-up maybe).

Berdir’s picture

Damn. That interdiff should be a test-only patch.

The last submitted patch, 57: 2512718-runtime-cache-context-57.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 58: 2512718-runtime-cache-context-57-test-only.patch, failed testing.

plach’s picture

+++ b/core/lib/Drupal/Core/Cache/RuntimeCacheableDependencyInterface.php
@@ -0,0 +1,13 @@
+interface RuntimeCacheableDependencyInterface {
+
+  public function addRuntimeCacheContexts(array $cache_contexts);
+
+}

The RuntimeCacheableDependencyInterface name (or any variation along those lines) is confusing if the interface is not extending CacheableDependencyInterface.

Btw, I guess missing PHP docs are intentional for now.

Berdir’s picture

Ok, that test was broken. This should be better.

Also fixed a number of things we found in those failing tests:

* Some entity types have additional contexts, so I made it so that it can be overridden.
* 4 of those entity types have no view page and we're just testing their forms there. They did not have any cacheable metadata in HEAD. We discussed this and decided to just add it to EntityForm instead of skipping them. Seems to work fine and will at some far point in the future allow us to cache forms with smartcache...
* CommentViewBuilder unknowingly dropped the cache contexts, so we changed that to just remove the keys but not remove any cacheable metadata.

Thanks for the review. Yeah, I just wanted to get started and not lose any time on naming/documentation since I thought we might change that anyway again.

The last submitted patch, 62: 2512718-runtime-cache-context-61-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 62: 2512718-runtime-cache-context-61.patch, failed testing.

plach’s picture

Issue summary: View changes

Updated the IS and wrote a draft change record for this:

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

Aside from the minor optimization below, the patch looks great to me, once it turns out green I am RTBC +1 on it.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -119,6 +119,11 @@ public function view(EntityInterface $entity, $view_mode = 'full', $langcode = N
+      foreach ($entities as $entity) {
+        if ($entity instanceof TranslatableInterface && count($entity->getTranslationLanguages()) > 1) {
+          $entity->addRuntimeCacheContexts(['languages:' . LanguageInterface::TYPE_CONTENT]);
+        }
+      }

We probably don't need this if we track that a NULL parameter was passed, and pass a NULL $langcode value in this case to EntityManager::getTranslationFromContext().

plach’s picture

Issue summary: View changes
plach’s picture

Issue summary: View changes
Fabianx’s picture

Sorry for interfering, but

#2524082-15: Config overrides should provide cacheability metadata

TL;DR:

At least for config overrides there is a need to go full interface, I bet for entities there is, too.

If I upcast an entity to have a very dynamic value (yes there are other ways for entities specifically), then I need to specify max-age=0, cache contexts are not enough. That entity must never be cached.

Similar if I change it based on some dynamic value, I might need to invalidate it where ever it is used and I should be okay if my value is invalidated when the entity is saved, that is a trade-off to make.

e.g. Suppose I am domain module and I add the config.site_name to all entities that are upcasted, so I need to vary per domain, but additionally if a domain site name changes I want the entity to expire.

( Yes, there is hook_entity_storage_load() and hook_entity_view_alter(), but there might be reasons why upcasting is better).

Also entities are not the only thing that could have this problem.

Berdir’s picture

Status: Needs work » Needs review
FileSize
13.55 KB
6.62 KB

Ok, renamed the class as discussed, added some documentation.

Also changed EntityForm to only add the metadata for existing entities and also for double-protection against the bug discovered with the test fail above, I made sure that Entity::getCacheTags() doesn't return a bogus cache tag for a new entity.

While discussing EntityViewBuilder, we also discovered another problem there and that's that $langcode might not match the actual entity langcode that is used due to fallbacks. So I refactored that a bit to only use the content language for the list #langcode key (not sure what the purpose of that exactly is, actually), so it's passing NULL to the getTranslationFromContext() issue which ensures that the cache context is added and changed the calls below to use the language from the entity object.

This *might* have some new test fails but I doubt we have been relying on this behavior anywhere.

Wim Leers’s picture

Status: Needs review » Needs work

Patch is looking great already :)

I only have nitpicks for now:

  1. +++ b/core/lib/Drupal/Core/Cache/RuntimeCacheContextsDependencyInterface.php
    @@ -0,0 +1,27 @@
    +   * @param array $cache_contexts
    
    +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -50,6 +50,13 @@
    +   * @var array
    

    s/array/string[]/

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -608,4 +618,10 @@ public function getConfigTarget() {
    +  }
     }
    

    Missing newline.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
    @@ -101,6 +101,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    // Ensure that edit forms have the correct cacheable metadata so they can be
    

    s/cacheable/cacheability/

  4. +++ b/core/modules/comment/src/CommentViewBuilder.php
    @@ -71,11 +71,9 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
    +    // keep the cacheable metadata, so it can bubble up.
    

    s/cacheable/cacheability/

  5. +++ b/core/modules/comment/src/Tests/CommentTranslationUITest.php
    @@ -33,6 +33,20 @@ class CommentTranslationUITest extends ContentTranslationUITestBase {
    +   * @var array
    

    s/array/string[]/

  6. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
    @@ -34,6 +38,15 @@
    +   * @var array
    
    +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentTranslationUITest.php
    @@ -18,6 +18,13 @@
    +   * @var array
    
    +++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
    @@ -22,6 +22,23 @@
    +   * @var array
    
    +++ b/core/modules/shortcut/src/Tests/ShortcutTranslationUITest.php
    @@ -19,6 +19,13 @@
    +   * @var array
    

    s/array/string[]/

pfrenssen’s picture

In comments 15-19 of #2524082: Config overrides should provide cacheability metadata arguments are put forward that it should not only be possible to add cache contexts but also cache tags and set the max age on runtime. This would mean to change the name of the new interface to RuntimeCacheableDependencyInterface with the following methods:

  1. addRuntimeCacheContexts()
  2. addRuntimeCacheTags()
  3. setRuntimeCacheMaxAge()
Fabianx’s picture

As discussed in IRC this issue could go ahead with RuntimeCacheContextInterface for now, while the other one should go full interface.

Wim pointed out that entities are not immutable (even though upcasted ones should IMHO be as follow-up), while config entities is immutable, hence does not have the problem of being accidentally saved. (config entities themselves are not immutable, but that is not where the cacheable information lives)

That is likely the best compromise to move both forward, also for entities there is entity_view() so theoretically I could do $entity->my_private_cache_tags = ['foo'] and then on entity_view() apply them if my private property is present (or something like that) ...

Edit: If two interfaces seem confusing another possibility would be to have addRuntimeCacheTags() return \InvalidArgumentException() always, so it cannot be mis-used.

yched’s picture

Minor :

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -50,6 +50,13 @@
   /**
+   * Runtime cache contexts.
+   *
+   * @var array
+   */
+  protected $runtimeCacheContexts = [];

@@ -440,7 +447,7 @@ public function referencedEntities() {
   public function getCacheContexts() {
-    return [];
+    return $this->runtimeCacheContexts;
   }

@@ -608,4 +618,10 @@ public function getConfigTarget() {
+  /**
+   * {@inheritdoc}
+   */
+  public function addRuntimeCacheContexts(array $cache_contexts) {
+    $this->runtimeCacheContexts = Cache::mergeContexts($this->runtimeCacheContexts, $cache_contexts);
+  }

Since RuntimeCacheContextsDependencyInterface is designed for more use cases than just entities, and the implementations here in Entity are fairly boilerplate, would it be worth providing a RuntimeCacheContextsDependencyTrait ?

Wim Leers’s picture

Fabianx’s picture

#73: Yes, I think a trait would be good.

effulgentsia’s picture

@pfrenssen, @Fabianx, @Wim Leers, @Berdir, and I just had a discussion about this issue, and we concluded that it does make sense to change RuntimeCacheContextsDependencyInterface to a full-blown RuntimeCacheableDependencyInterface that also includes adders for tags and max-age. We also discussed alternate names such as ModifiableCacheableDependencyInterface, but did not reach any firm conclusions about "runtime" vs. "modifiable" vs. something else.

So I'd be hesitant to support anything other then cache contexts, unless there's a vey concrete use-case

#2524082-15: Config overrides should provide cacheability metadata has some.

if we add a cache tag to an entity, then this could then end up being invalidated if that entity is saved during that request - don't think we want that

We could fix this by adding getOriginalCacheTags() to EntityInterface. Or split that out into something like OriginalCacheTagsAwareInterface. But the idea is that deciding what tags to add to a cached item and what tags to invalidate on save is actually two different things that deserve two different methods. We don't have many callers of the latter, so they can adjust to the new method name.

Berdir’s picture

Status: Needs work » Needs review
FileSize
25.13 KB
17.3 KB

Started to address this based on the discussions.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/MutableCacheableDependencyTrait.php
    @@ -0,0 +1,57 @@
    +  public function addCacheContexts(array $cache_contexts) {
    +    $this->cacheContexts = Cache::mergeContexts($this->cacheContexts, $cache_contexts);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function addCacheTags(array $cache_tags) {
    +    $this->cacheTags = Cache::mergeTags($this->cacheTags, $cache_tags);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function addCacheMaxAge($max_age) {
    +    $this->cacheMaxAge = Cache::mergeMaxAges($this->cacheMaxAge, $max_age);
    +  }
    

    These should all return $this;.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -350,6 +350,14 @@ public function referencedEntities();
    +   *   List of cache tags.
    

    s/List/Set/

  3. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -1339,8 +1339,29 @@ public function hasTrustedData() {
    +  public function addCacheMaxAge($max_age) {
    +    $this->storage->addCacheMaxAge($max_age);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getOriginalCacheTags() {
    +    return $this->storage->getOriginalCacheTags();
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function addCacheTags(array $cache_tags) {
    +    $this->storage->addCacheTags($cache_tags);
       }
    

    Same problem as earlier.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
    @@ -101,6 +101,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    // Ensure that edit forms have the correct cacheability metadata so they can be
    +    // cached.
    +    if (!$this->entity->isNew()) {
    +      \Drupal::service('renderer')->addCacheableDependency($form, $this->entity);
    +    }
    

    This seems od. Can't we add it to $form ?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -349,6 +350,14 @@ public function referencedEntities();
       /**
    +   * Returns the original cache tags of this entity, without mutations.
    +   *
    +   * @return string[]
    +   *   List of cache tags.
    +   */
    +  public function getOriginalCacheTags();
    

    We should make it clear to explain when to use getOriginalCacheTags() and when to use getCacheTags()

Status: Needs review » Needs work

The last submitted patch, 77: 2512718-runtime-cache-context-77.patch, failed testing.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -969,6 +969,7 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
         $langcode = $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
+        $entity->addRuntimeCacheContexts(['languages:' . LanguageInterface::TYPE_CONTENT]);

This is likely the reason for the test failures ...

Should be just addCacheContexts() now.

effulgentsia’s picture

We should make it clear to explain when to use getOriginalCacheTags()

Picking a good name might also help clarify. Since we're talking about an object that already implements CacheableDependencyInterface, we already have the concept that there are things that are cached that depend on this object. We want to be able to invalidate those dependents when the object is modified. To do so, we only need to invalidate at least one tag that is always part of the object. Because any tag that is always returned by the object's getCacheTags() method will be attached to every dependent, so invalidating only that tag is enough. There's no need to invalidate the other tags that come and go based on runtime variations (and in fact, we don't want to invalidate them, when those aren't the things that are getting modified).

So, some options:

  1. Name based on the concept of "always": e.g., getStaticCacheTags(), getStableCacheTags(), getDurableCacheTags().
  2. Name based on the concept of self-ness: e.g., getPrimaryCacheTags(), getBaseCacheTags(), getOwnCacheTags().
  3. Name based on the concept of using it for invalidation: e.g., getCacheTagsForInvalidation(), getCacheTagsToInvalidate().
  4. Change from a getter to an operation: e.g., invalidateCachedDependents() and have it invoke Cache::invalidateTags(), thereby making the getter not part of the interface.

#4 is my favorite for clarity, but it might not be the most optimal, because then for example, EntityViewBuilder::resetCache() would end up calling $entity->invalidateCachedDependents() for each entity and then need to invalidate some of its own tags, so Cache::invalidateTags() would get called multiple times instead of a single time with a combined tag list.

Given that, I don't have a strong favorite between 1-3. So curious what you all think.

Fabianx’s picture

I like #82.3 especially:

getCacheTagsForInvalidation()

To add to the brainstorming here is:

5. Name based on that its an identifier that is returned there: getCacheTagBaseId() // getCacheTagIdentifier() // getBaseCacheTag() - i.e. a list cache tag should not be added there, but injected on run time when the entity is actually part of a list.

6. Name based on identifier, but allow multiple: getIDCacheTags() // getCacheTagsIdentifiers() // getBaseCacheTags()

My favorite is still 82.3 :)

Berdir’s picture

Status: Needs work » Needs review
FileSize
25.12 KB
18.14 KB

Hopefully fixing the fatal errors. Will address the reviews tomorrow.

Status: Needs review » Needs work

The last submitted patch, 84: 2512718-runtime-cache-context-84.patch, failed testing.

Berdir’s picture

Fixed another stupid mistake :)

#71.1: That is the fancy way of setting it on $form :)
#71.2ff: I like getCacheTagsForInvalidation() as well, that is shown by the IDE when typing getCacheTags and is very explicit. Changed to that. This is a unique name either way, so a search and replace on it is trivial.

Berdir’s picture

Status: Needs work » Needs review
plach’s picture

+1 on ::getCacheTagsForInvalidation(): I think it's way more understandable if you don't know the involved code/logic very well.

Status: Needs review » Needs work

The last submitted patch, 86: 2512718-runtime-cache-context-86.patch, failed testing.

The last submitted patch, 33: 2512718-33.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
25.08 KB
846 bytes

such stupid. much wow. Uploaded the wrong patch last night.

Wim Leers’s picture

I think this patch looks great now.

  1. +++ b/core/lib/Drupal/Core/Cache/MutableCacheableDependencyInterface.php
    @@ -0,0 +1,52 @@
    +interface MutableCacheableDependencyInterface extends CacheableDependencyInterface {
    

    I think we want to add an @todo to CacheableMetadata and AccessResult, noting that they should use this trait instead. Should link to a follow-up issue. I created that issue for you: #2526326: Update CacheableMetadata & AccessResult to use RefinableCacheableDependency(Interface|Trait).

  2. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
    @@ -101,6 +101,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    // Ensure that edit forms have the correct cacheability metadata so they can be
    

    80 cols.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -349,6 +350,14 @@ public function referencedEntities();
    +   *   List of cache tags.
    

    s/List/Set/

  4. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
    @@ -34,6 +38,15 @@
    +  protected $defaultCacheContexts = ['languages:language_interface', 'theme' , 'user.permissions'];
    
    +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentTranslationUITest.php
    @@ -18,6 +18,11 @@
    +  protected $defaultCacheContexts = ['languages:language_interface', 'theme' , 'user.permissions', 'user.roles:authenticated'];
    
    +++ b/core/modules/shortcut/src/Tests/ShortcutTranslationUITest.php
    @@ -19,6 +19,11 @@
    +  protected $defaultCacheContexts = ['languages:language_interface', 'theme' , 'user'];
    

    Nit: unwanted space before the second comma.

Status: Needs review » Needs work

The last submitted patch, 91: 2512718-runtime-cache-context-91.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
27.9 KB
7 KB

This addressed the review and should fix the test failures.

I forgot to rename the overridden getCacheTagsMethods() (important to mention in the change record but I'm not aware of anything using this).

dawehner’s picture

I like the name getCacheTagsForInvalidation, especially because, if needed other object things could do something similar.

+++ b/core/lib/Drupal/Core/Cache/MutableCacheableDependencyInterface.php
@@ -0,0 +1,52 @@
+   *
+   * @throws \InvalidArgumentException
+   *   If a non-integer value is supplied.
+   */

I think we don't have to specify that, given that we will treat that as assertion in the future anyway, especially because its not part of the function itself.

Wim Leers’s picture

Only one remaining nit:

  1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -350,10 +350,18 @@ public function referencedEntities();
    +   * @see \Drupal\Core\Cache\CacheableDependencyInterface::getCacheTags()
    +   *
        */
    

    One newline too many.

  2. +++ b/core/modules/shortcut/src/Entity/Shortcut.php
    @@ -178,7 +178,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    -  public function getCacheTags() {
    +  public function getCacheTagsForInvalidation() {
    

    Oh heh, that'd do it, yes :)

Fabianx’s picture

Little comment on the interdiff of #86:

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -370,7 +370,7 @@ public function resetCache(array $entities = NULL) {
-      Cache::invalidateTags($this->getOriginalCacheTags());
+      Cache::invalidateTags($this->getCacheTags());

Was this changed back on purpose?

If yes, could we get a comment? If not, could we fix it back?

Berdir’s picture

$this is the view builder object not an entity. It's the thing that returns node_view for example. So yes, on purpose. I don't think it needs a comment, it might just be confusing since the context isn't clear in the interdiff.

Fabianx’s picture

#98: Thanks for the explanation, got it! :)

pfrenssen’s picture

+++ b/core/lib/Drupal/Core/Cache/MutableCacheableDependencyInterface.php
@@ -0,0 +1,52 @@
+  /**
+   * Adds the maximum age (in seconds).
+   *
+   * @param int $max_age
+   *   The max age to associate.
+   *
+   * @return $this
+   *
+   * @throws \InvalidArgumentException
+   *   If a non-integer value is supplied.
+   */
+  public function addCacheMaxAge($max_age);

The max age setting is not an array like the other settings, but an int. This makes it strange to allow to 'add' a max age. This makes it sound like it is possible to "add" a number of seconds to the already existing max age which doesn't make much sense.

Wouldn't it be better to change this to setCacheMaxAge() instead?

edit: This should of course still be calculated correctly using Cache::mergeMaxAges(), i.e. it shouldn't blindly set the max age, but compare it with the original one, and pick the lowest of the two.

Fabianx’s picture

#100: That is what:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Access%21...

does already, so I think yes.

( The only reason against set is that its still merged against the other value and the lower one wins.)

pfrenssen’s picture

Discussed the naming of the max age method with @berdir and @WimLeers. Since we are not actually "hard setting" the max age value this should be reflected somehow in the method name. The best compromise we came up with is setCacheMaxAgeIfLower(). There is some precedent of this pattern in core (e.g. SharedTempStore::setIfNotExists().

We can still bikeshed this if someone has a better suggestion.

Wim Leers’s picture

With "hard setting", @pfrenssen means "set and overwrite", which is what all CacheableMetadata setters actually do.

effulgentsia’s picture

setCacheMaxAgeIfLower() seems fine. But just brainstorming some options to keep the "add" prefix for parity with the others: addCacheMaxAgeMaximum(), addCacheMaxAgeConstraint(), addCacheMaxAgeRequirement()?

Wim Leers’s picture

I dislike the options in #104 — I think either of make most sense:

  • addCacheMaxAge()
  • setCacheMaxAgeIfLower()
Fabianx’s picture

+1 to setCacheMaxAgeIfLower

pfrenssen’s picture

Changed it to setCacheMaxAgeIfLower() and addressed remarks from #95 and #96.

pfrenssen’s picture

+++ b/core/lib/Drupal/Core/Cache/MutableCacheableDependencyTrait.php
@@ -0,0 +1,60 @@
+  /**
+   * Runtime cache contexts.
+   *
+   * @var array
+   */
+  protected $cacheContexts = [];
+
+  /**
+   * Cache tags.
+   *
+   * @var string[]
+   */
+  protected $cacheTags = [];
+
+  /**
+   * Cache max-age.
+   *
+   * @var int
+   */
+  protected $cacheMaxAge = Cache::PERMANENT;

Minor documentation issue, the documentation for $cacheContexts mentions it is for runtime, but the others don't. Would be better to be consistent.

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/Cache/MutableCacheableDependencyInterface.php
@@ -37,16 +37,15 @@ public function addCacheContexts(array $cache_contexts);
-   * @throws \InvalidArgumentException
-   *   If a non-integer value is supplied.

Not sure if we actually wanted to remove this. Until we have scalar type hints, this is actually still helpful.

Minor documentation issue, the documentation for $cacheContexts mentions it is for runtime, but the others don't. Would be better to be consistent.

Indeed; none of them should mention "runtime".

pfrenssen’s picture

FileSize
27.94 KB
1.1 KB

Fixed the documentation issues. Restored the @throws info.

Until we have scalar type hints, this is actually still helpful.

I also can't wait for the PHP7 strict_type goodness!!

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content
dawehner’s picture

Not sure if we actually wanted to remove this. Until we have scalar type hints, this is actually still helpful.

Well, I think this should be an assertion, can't it?

Wim Leers’s picture

Well, I think this should be an assertion, can't it?

Yes, let's add

// @todo Convert to assertion once https://www.drupal.org/node/2408013 lands

Like we have in several other places 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.

IS completely rewritten. This IS also applies to #2524082: Config overrides should provide cacheability metadata.

I hope I captured everything.

Wim Leers’s picture

Issue summary: View changes

Typo.

Berdir’s picture

Issue summary: View changes

Replaced getOriginalCacheTags() with getCacheTagsForInvalidation() in the issue summary.

Fabianx’s picture

Issue tags: +Needs draft change record updates

The change record will need to be updated, too.

Otherwise IS and approach look awesome!

catch’s picture

One question on that method name. All cache tags are for invalidation. What about getCacheTagsToInvalidate()?

Berdir’s picture

Issue tags: -Needs tests, -Needs draft change record updates
FileSize
30.69 KB
2.7 KB

Wrote some unit tests for the new methods.

ToInvalidate() also works for me, no strong opinion.

effulgentsia’s picture

+1 to #118. Also, while we're discussing naming, how about s/setCacheMaxAgeIfLower/reduceCacheMaxAgeTo/ and s/MutableCacheableDependency/RefinableCacheableDependency/? Because the interface by design does not allow for any mutating, but only mutating in one direction: towards an increasingly finer cache (more contexts, more tags, shorter age).

Wim Leers’s picture

#118: +1


#119: test coverage looks great. I agree that this patch now has sufficient test coverage, given that it already was adding integration tests in prior iterations.

  1. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -486,4 +487,59 @@ public function testPostLoad() {
    +    // tha.
    

    s/tha/that/

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -486,4 +487,59 @@ public function testPostLoad() {
    +  }
    +
    +
    +  /**
    

    2*\n is one too many.


Looks like we're down to bikeshedding names :)


#120: Ohh, I very much like RefinableCacheableDependencyInterface! Great explanation too.

I do not like reduceCacheMaxAge() — it's too close in meaning to "subtract", i.e. it almost feels like if the current max-age is 100 and you call reduceCacheMaxAge(10) that you'll end up with 90, instead of 10. I guess for that same reason, my suggestion of addCacheMaxAge() sounds like "add", i.e. addCacheMaxAge(10) results in 110. So setCacheMaxAgeIfLower() still seems to be the clearest one.

catch’s picture

Agreed with #121. Like definablerefinable, not keen on reduce.

What the method really does is setMaxMaxAge() but that would be silly ;)

Berdir’s picture

Issue summary: View changes
FileSize
30.54 KB
17.65 KB

Renamed getCacheTagsForInvalidation() to getCacheTagsToInvalidate().
Renamed MutableCacheableDependency to RefinableCacheableDependency.
Kept setCacheMaxAgeIfLower().

Updated the issue summary.

Wim Leers’s picture

Status: Needs review » Needs work

Besides a few small remaining details, I think this is RTBC!

  1. +++ b/core/lib/Drupal/Core/Cache/MutableCacheableDependencyTrait.php
    @@ -0,0 +1,60 @@
    +trait MutableCacheableDependencyTrait {
    

    This still needs to be renamed.

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -486,4 +487,59 @@ public function testPostLoad() {
    +    // tha.
    

    Nit: s/tha/that/

  3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -486,4 +487,59 @@ public function testPostLoad() {
    +    // There are no cache contexts by defaut.
    

    Nit: s/defaut/default/

pfrenssen’s picture

FileSize
30.51 KB
3.56 KB

Right! Good catch. I addressed these quickly since they affect my work over at the config overrides issue.

Berdir’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Wonderful! This is ready :) Given I've not rolled any patches, and was first pushing back against certain aspects of this solution, but was convinced by others that this is indeed a correct, solid, elegant approach, I think it's okay for me to RTBC this.

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyInterface.php
    @@ -0,0 +1,54 @@
    +  /**
    +   * Adds cache contexts.
    +   *
    +   * @param string[] $cache_contexts
    +   *   The cache contexts to be added.
    +   *
    +   * @return $this
    +   */
    +  public function addCacheContexts(array $cache_contexts);
    +
    +  /**
    +   * Adds cache tags.
    +   *
    +   * @param string[] $cache_tags
    +   *   The cache tags to be added.
    +   *
    +   * @return $this
    +   */
    +  public function addCacheTags(array $cache_tags);
    +
    +  /**
    +   * Sets the maximum age (in seconds).
    +   *
    +   * This only sets the max age if it is lower than the existing one.
    +   *
    +   * @param int $max_age
    +   *   The max age to associate.
    +   *
    +   * @return $this
    +   *
    +   * @throws \InvalidArgumentException
    +   *   Thrown if a non-integer value is supplied.
    +   */
    +  public function setCacheMaxAgeIfLower($max_age);
    +
    +}
    

    Sorry to stick on this, but these still bother me a bit. setCacheMaxAgeIfLower() I keep thinking 'If lower than what?'.

    What about:
    mergeCacheTags()
    mergeCacheContexts()
    mergeCacheMaxAge()

    In all three cases we merge the param into the existing values on the object.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
    @@ -101,6 +101,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     
    +    // Ensure that edit forms have the correct cacheability metadata so they can
    +    // be cached.
    +    if (!$this->entity->isNew()) {
    +      \Drupal::service('renderer')->addCacheableDependency($form, $this->entity);
    +    }
    

    Wouldn't we need to apply this for an add form for a translation too? Or is that technically an edit form even if the translation is new.

    Also adding this doesn't allow the forms to be cached, just correctly, were that to be the case.

Wim Leers’s picture

Wouldn't we need to apply this for an add form for a translation too? Or is that technically an edit form even if the translation is new.

We don't have a cache tag until the entity has been saved, because we don't have an entity ID until the entity has been saved.

alexpott’s picture

Status: Needs review » Needs work

This looks good - I think the mergeCache* method names are good as they match what's happening Drupal\Core\Cache\Cache::merge*

+++ b/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyInterface.php
@@ -0,0 +1,54 @@
+ * Allows to add cacheability metadata to an object for the current runtime.

Maybe "Allow cacheability metadata for the current request to be added to an object."

catch’s picture

We don't have a cache tag until the entity has been saved, because we don't have an entity ID until the entity has been saved.

No, but we might have a context?

Berdir’s picture

Talked with @catch about the names.

merge*() would be a bit sad for the CacheabilityMetadata and AccessResult objects which we want to use the interfaces too and they already have add methods for tags and context.

So we agreed on the compromise to just use merge for max age.

Wim Leers’s picture

What about:
mergeCacheTags()
mergeCacheContexts()
mergeCacheMaxAge()

Two reasons to not do that:

  1. then it is no longer an interface lifted from CacheableMetadata, so this would mean BC breaking changes to CacheableMetadata, BubbleableMetadata, FilterProcessResult, GeneratedLink and GeneratedUrl, or it'd mean maintaining BC yet having 3 deprecated methods on all of those.
  2. it'll be confusing that CacheableMetadata::merge() returns a new instance yet merge*() (these 3 methods) modify the object

No, but we might have a context?

Even for a newly created entity object, that therefore was not loaded from anywhere? i.e. for Node entities:

    $node = $this->entityManager()->getStorage('node')->create(array(
      'type' => $node_type->id(),
    ));

    $form = $this->entityFormBuilder()->getForm($node);
pfrenssen’s picture

Status: Needs work » Needs review
FileSize
30.53 KB
3.13 KB

Changed only setCacheMaxAgeIfLower() to mergeCacheMaxAge() and left the others unchanged. @WimLeers is correct in pointing out that this sets the value, and the related CacheableMetadata::merge() returns a modified object, but I think this is not so much of a problem. In the end these are different use cases, and the two interfaces are not depending on eachother. Using the same method names is a DX win, but since CacheableMetadata doesn't have a method for merging max ages which we could copy this doesn't impact DX so much.

@WimLeers do you think this is an acceptable compromise?

We don't have a cache tag until the entity has been saved, because we don't have an entity ID until the entity has been saved.

No, but we might have a context?

It looks like in the case of a translation this is indeed considered an edit form and the contexts and tags will be retained.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

@WimLeers do you think this is an acceptable compromise?

Yep, absolutely. I'd already said that in IRC, sorry for not yet saying it here.

AFAICT all committer feedback has been addressed, so back to RTBC!

Fabianx’s picture

RTBC + 1, looks great to me.

Proposed commit message:

Issue #2512718 by Berdir, pfrenssen, Wim Leers, Fabianx, dawehner, catch, effulgentsia, plach, Gábor Hojtsy: EntityManager::getTranslationFromContext() should add the content language cache context to the entity

due to the extensive discussions we had about it.

Fabianx’s picture

Issue tags: +Needs draft change record updates

The change record https://www.drupal.org/node/2525764 however is still outdated.

Probably the tag removal was x-post.

Fabianx’s picture

Issue summary: View changes
Berdir’s picture

Updated the change record a bit, also added a second one about the new getCacheTagsToInvalidate() method: https://www.drupal.org/node/2528342

alexpott’s picture

Ticking the credit boxes.

Fabianx’s picture

Issue tags: -Needs draft change record updates

Thanks, the updates look great!

catch’s picture

Status: Reviewed & tested by the community » Fixed

This looks great, and we have the config overrides issue still critical to flush out anything we might have missed.

Committed/pushed to 8.0.x, thanks!

  • catch committed a362252 on 8.0.x
    Issue #2512718 by Berdir, pfrenssen, Wim Leers, Fabianx, dawehner, catch...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks all!

Status: Fixed » Closed (fixed)

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