Problem/Motivation

Currently the cache header is created twice, once in the HTML main content renderer (\Drupal\Core\Render\MainContent\HtmlRenderer, only runs for controllers returning a render array and when the negotiated format is HTML) and once in \Drupal\Core\EventSubscriber\FinishResponseSubscriber (which runs for every response).

Proposed resolution

See "API changes" below, and comment #12.

Remaining tasks

None.

User interface changes

None.

API changes

  • Added CacheableResponseInterface, which must be implemented by responses that want X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts headers to be exposed.
  • Added CacheableResponseTrait, to make it easy for response types to become cacheable (implement the interface, use the trait).
  • Renamed RendererInterface::addDependency() to RendererInterface::addCacheableDependency() for consistency with CacheableResponseInterface, and because it then maps 100% to CacheableDependencyInterface (perfect symmetry, better DX).

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because improving overall cacheability (more responses should be cacheable) and improving DX.
Prioritized changes The main goal of this issue is performance/DX.
Disruption Disruptive for contrib/custom code that is using RendererInterface::addDependency(). We could theoretically add BC by letting Renderer::addDependency() redirect to Renderer::addCacheableDependency(). But, since the number of callers is likely still quite low at this point, that may not be worth it. Committer can choose.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Title: Consolidate ways of setting X-Drupal-Cache-Tags headers » Consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers
Wim Leers’s picture

Issue summary: View changes

Fixing + clarifying.

Wim Leers’s picture

anavarre’s picture

Issue tags: +D8 cacheability
Fabianx’s picture

So I thought we could just put the BubbleableMetadata on the response:

$response->data('_cache_metadata')

Then the finish subscriber can take that and the original route access requirements and merge those to create the header.

Wim Leers’s picture

#5: oooooooh! That makes a ton of sense. Yes, let's do that :) Can't believe I didn't think of that :)

EDIT: well, it accepts that work has to be done in the finish subscriber, inevitably. The way Alex phrased the question, I was trying to avoid doing that. Should've seen this though. Well-spotted!

Fabianx’s picture

larowlan’s picture

There is no ->data :(

Fabianx’s picture

Priority: Normal » Major

Response does not have a ParameterBag larowlan told me.

So we need our own class:

- DrupalResponse?
- CacheableResponse?
- ResponseWithCacheMetaData?
- MainContentViewResponse? -- my favorite so far
- HtmlRendererResponse?
- RenderArrayResponse?

This is a soft requirement for BigPipe in a clean way, so upping priority to major.

We will also need onBeforeResponseSend / onAfterResponseSend events for BigPipe, which can then use getCacheMetaData(), etc.

Note: The response also has a getMaxAge() function, so we need to ensure we don't clash ...

Fabianx’s picture

I like:

use Drupal\Core\Render\RenderResponse;

similar to AjaxResponse, etc :)

Wim Leers’s picture

But it's not only necessary for HTML/render array responses, e.g. it's also for REST responses. Already in HEAD as of today: #2471473: REST responses should have proper cache tags.

So we'd need it to be more generic, and used for all responses generated by Drupal…

Fabianx’s picture

I discussed this lengthily with Crell, who is responsible for this part of Drupal and the outcome was in short (I will discuss more with Wim) that we see the best way forward as:

- We can use a new Response object class as long as we:

* a) extend Response
* b) use interfaces and traits to do so

e.g.

class CacheableResponse extends Response implements CacheableResponseInterface {
  use CacheResponseTrait;
}

The same can be done for JSON and other responses.

- We use a check on instanceof CacheableResponseInterface for _any_ cache related data we set in FinishResponseSubscriber as final headers on the response.

That means that:

If you do:

return new Response('Hi');

you are yourself responsible for everything, but the default Response is uncacheable (Cache-Control: private, no-cache, no-store, etc.) so this is not a security issue.

This essentially makes Drupal's cache, etc. system opt-in instead of mandatory, which should be fine and was a goal of the WSCCI initiative in the first place.

90% of controllers will go via HtmlRenderer anyway, for the remainder (AjaxResponse, JsonResponse, RedirectResponse (?)) here is the plan (this does not apply to this issue, but I need to put it somewhere):

----

- renderRoot and renderPlain should return either a value object or the output of $renderer->getCacheableRenderArray() directly.

So that we make it absolutely clear that this is markup + render data.

While that ship for $renderer->render() has sailed and makes no sense, renderRoot() is a very special case and renderPlain() is what you should be using, but then you _need_ to take care of the added meta data.

Whatever we return needs to be easily added to our new CacheableResponse object, the simplest could be to have for the 90% case a:

- Create a $response = $renderer->createResponse($build) function

And/Or have things like:

$response->addMetaData($object1);
$response->addMetaData($object2);

OR something like:

  $response = CacheableResponse::createFromRenderArray($build);

etc.

But also support e.g.:

$data = $renderer->renderRoot($build);

$response = new CacheableResponse($data->getContent(), 200);
$response->addMetaData($data);

There is probably a better DX to be had here, but we already do this with $ajaxResponse->addAttachments($build['#attached']); in core right now.

So having ajaxResponse implement CacheableResponseInterface and AttachmentsResponseInterface is possible.

Note: That http_header in #attached can be removed from the super global then, because it is just taken from $build['#attached'] or from $data->getAttachments() and applied to the response on setting the MetaData.

We could also blacklist any responses not having metadata as uncacheable, but that is an implementation detail.

--

Summary:

- Use our own responses in Drupal for anything that opts-in to Cacheability / etc.
- Do all cache tags header adding _only_ for responses implementing that interface
- Add simple to use interfaces and traits, so creating CacheableJsonResponse is a matter of creating the class, implementing the interface and using a trait, then using e.g. $response->addCacheDependency($entity) so that REST does not need to re-implement all the X-Drupal-Cache-Tags, logic etc.
0
- Ensure we tell the user that renderRoot() / renderPlain() you have something you need to add to your CacheableResponse() - if you return it as a Response to the browser - by returning either a render array or a value object. Returning markup from those newly added functions (hence almost no BC break) is wrong.

Finally this makes BigPipe also easier as we can just add one more interface, implement it and do placeholder replacement when that interface is active or store as part of the cache metadata. (that is for another discussion however - just posted that so it is clear this is thought-wise taken care of).

--

Goals:

- Better and easier DX (see recent critical REST patch)
- Opting in for Cacheability and having the default be: Cache off is good for security as 99% of controllers using the appropriate base classes don't need to deal with it.
- Easier adding of dependencies
- Removing of super globals in process attached, but leave it in the render array where it belongs, have the caller deal with it, always.
( e.g. this could even work for scripts, add placeholders for the scripts in html_preprocess_page(), then replace as part of the HtmlResponse later.)

Wim Leers’s picture

Wow!

Crell++
Fabianx++

Crell’s picture

Thanks, Fabianx!

One note, I don't thinks this option will fly:

  $response = CacheableResponse::createFromRenderArray($build);

Because the createFromRenderArray() process would rely on multiple services. No sense replacing one global with another global service call. :-) But

$response = $renderer->createResponse($build);

Should suffice for the 90% case and be hard for the few people who need to do it themselves to get wrong. :-)

(We should also consider a better name than "metadata", as that's very non-descriptive, but we can bikeshed that later.)

Mile23’s picture

@Fabianx: - We can use a new Response object class as long as we:

* a) extend Response
* b) use interfaces and traits to do so

I COMPLETELY agree with very strict limitations on what sort of extensions can happen to Response. Because I disagree with the solution. :-)

Response is not a verb.

For a start, there is render(), renderRoot(), renderPlain(). That's three special cases with ambiguous names. I'm not entirely sure what the distinction is. There shouldn't be one.

There should be render($render_array). If you've rendered some piece in order to cache it away, then do this:

$build['#markup'] = new Response($renderer->render($my_local_build), 200, ['cache-control'=>'hell yeah']);

Now, in the render array, you have a record of all the special-case headers and response codes needed by everyone who has ever rendered a piece of the thing, in place. Anything that's not a Response is up for grabs. Cached snippets could in fact be serialized Responses.

You determine a policy for merging these things, return Response, and then you buy yourself a beer.

@Fabianx: take care of the added meta data.

It's not metadata. It's special concerns for the output. It's only 'metadata' in that it's hanging around in a render array that has itself become a turing machine.

There are only headers, content, and requirements.

Wim Leers’s picture

For a start, there is render(), renderRoot(), renderPlain(). That's three special cases with ambiguous names. I'm not entirely sure what the distinction is. There shouldn't be one.

Please read their docs. render() is the 99% case.

Fabianx’s picture

#15: Yes, you are right, if you return a new Response we should not bother changing that in any response subscriber, which we currently do - that is part of the proposal.

However we add a (caching) layer on top of Symfony's Response concept, which indeed we are even mapping to Varnish and other reverse proxies.

So that is why if you want to leverage Drupal's usage of this caching layer you need to implement CacheableResponseInterface in the future.

That makes Drupal's additional layer explicit instead of implicit, which is a good move and leads to less security bugs as you need to explicitly shoot you in the foot instead of it happening by accident. (i.e. caching of a path showing sensible data which you want to take care of yourself by using a normal Response() - which the page cache accidentally caches ...).

catch’s picture

Priority: Major » Critical
Issue tags: +Needs Drupal 8 critical triage

Discussed this with Fabianx in irc.

If we do #2472321: Move rest.module routes to /api, then we'll be sending Vary: accept in the case that a route has two possible formats.

In that case, the page cache needs to handle it.

Previously we had content negotiation up-front for page caching, but we don't want to do that on every page cache request since it's not needed.

However, if we use cache contexts, we can set a request_format cache context, whitelist this in the page cache (currently it strips all cache contexts), then rely on cache contexts (i.e. cache redirect) to get the correct item from cache.

That has no runtime impact on any other page cache responses (or not much more than an if statement anyway), and will work accurately for the ones that need it.

I think it's likely that we can't do this in a patch release since it'll be quite a large internal refactor of the page cache - i.e. it might break it until you run rebuild.php or similar, so bumping to critical, but adding triage tag to discuss with other committers. The impact of not doing this is we'd have no internal page caching at all for /api/ routes.

dawehner’s picture

Interesting, so basically once we know that certain routes have needed request formats, we can retrieve them, but don't need them on a HTML case.
So then that request_format cache context could then determine the format probably from some basic mapping.

Wim Leers’s picture

Issue tags: +D8 Accelerate Dev Days
Fabianx’s picture

#19 Yes, I will update IS today with the plan, probably making this a meta.

Mile23’s picture

Wim Leers’s picture

Rolling an initial version of this.

Wim Leers’s picture

Status: Active » Needs review
FileSize
19.58 KB

And voilà.

Note that #2471473: REST responses should have proper cache tags, which landed last week, already adds X-Drupal-Cache-Tags headers for REST responses (ResourceResponse). That's now also been updated to use this. Test coverage for that was added in that patch. So if this patch comes back green, we know for certain that this approach is working correctly for HTML/render array responses and non-HTML/non-render array responses. :)

This patch:

  1. Tries to follow the design explained in #12 (Fabianx + Crell) and #14 (Crell) as closely as possible.
  2. In doing so, I didn't introduce any new class for storing cacheability metadata; we're just reusing what already exists.
  3. The intent is to only generate X-Drupal-Cache-* headers in a single place; the most logical place for that is IMO FinishResponseSubscriber.
  4. Therefore, the setting of associated-but-not-contained-in-the-response-content cacheability metadata (such as the route's access result) should be done in a different event subscriber than FinishResponseSubscriber. At the same time, this makes it easy for AnonymousUserResponseSubscriber to run between the aforementioned subscriber and the FinishResponseSubscriber. (The AnonymousUserResponseSubscriber conditionally adds another cache tag, but needs all other cache tags on the response to already be set to make its conditional decision.)
  5. EDIT: one thing I forgot to mention: I called the method to add additional cacheability metadata CacheableResponseInterface::addDependency(), to make it similar to RendererInterface::addDependency(). It makes sense, too: a cacheable response's cacheability depends on all the cacheability metadata of all the things it is passed there.

Status: Needs review » Needs work

The last submitted patch, 24: response_cacheability_consolidation-2463009-24.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
23.05 KB
3.56 KB

Fixed that single failure. Test coverage expanded; this now also verifies that X-Drupal-Cache-(Contexts|Tags) headers are not set for Responses, only for CacheableResponses :)

Fabianx’s picture

Overall looks great, I think I will make this again a major task and push the Critical to a postponed follow-up (as the scope is here too much).

Two more goals had been for this issue:

a) Only do any Cache-Control headers if it is a cacheable response, this truly lets someone opt out of core's caching architecture, etc.
b) Have page cache honor Cache-Control header OR check for cacheable response as well.

Also happy to do those two in another follow-up if we think this needs more discussion.

EDIT: I will make them follow-ups ...

--

Overall +100 to the changes in the patch. That is a very clean architecture with the addDependency and the split of the finish response subscriber.

It looks even better patch wise than I imagined.

--

My only other request would be if we could have ->addDependency detect a (cacheable) render array automatically and convert to a metadata object so that its easier for users of CacheableResponses to use that.

Will likely find some nits to in proper review ...

Wim Leers’s picture

Priority: Critical » Major
FileSize
23.19 KB
1.68 KB

Overall looks great, I think I will make this again a major task and push the Critical to a postponed follow-up (as the scope is here too much).

Two more goals had been for this issue:

a) Only do any Cache-Control headers if it is a cacheable response, this truly lets someone opt out of core's caching architecture, etc.
b) Have page cache honor Cache-Control header OR check for cacheable response as well.

Also happy to do those two in another follow-up if we think this needs more discussion.

Let's do those in a follow-up issue. Then this can be major again, the follow-up can be critical. Let's cite @catch in #18 as the IS for that follow-up, and also as the reason for it being critical.

(Reason for it being a follow-up: the current patch has a very manageable scope, and actually does solve the original intent of this issue: see the IS.)

It looks even better patch wise than I imagined.

:)

My only other request would be if we could have ->addDependency detect a (cacheable) render array automatically and convert to a metadata object so that its easier for users of CacheableResponses to use that.

Done. I'm not convinced this is that much of an improvement, but it's also not harming anything.

Will likely find some nits to in proper review ...

Looking forward to those!

Fabianx’s picture

  1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -151,27 +153,27 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +    $default = new CacheableMetadata();
    +    $default->setCacheTags(['rendered']);
    +    $response->addDependency($default);
    

    I would argue we want to set that cache tag actually also for rest responses and such globally in the finishResponseSubscriber?

    Not sure ...

  2. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -109,14 +109,8 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    +      // Add rest settings config's cache tags.
    +      $response->addDependency($cache_tags = $this->container->get('config.factory')->get('rest.settings'));
    

    Not sure why we need the assignment?

    Is rest.settings of type cacheable dependency interface?

    Should it then be rest.cache_settings?

Initial review ...

Wim Leers’s picture

FileSize
23.18 KB
824 bytes
  1. No, because no rendering is involved in that whatsoever (AFAIK).
  2. LOL, bad refactoring. Fixed. Every ConfigBase subclass implements CacheableDepdendencyInterface, so also the REST settings config object.
Fabianx’s picture

Priority: Major » Critical

Setting back to critical, to avoid screwing with the stats until I get to properly set the scope of the follow-up ...

catch’s picture

I'm not 100% (or even 50%) on #2472321: Move rest.module routes to /api (see recent comments on #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use). This is a nice major issue to have by itself, the follow-up is only critical if we really do /api/, and it won't be critical because we did this issue - this just enables it.

Wim Leers’s picture

#32: that makes sense, thanks for clarifying!

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/CacheableResponseInterface.php
    @@ -0,0 +1,34 @@
    +  /**
    +   * Adds a dependency on an object: merges its cacheability metadata.
    +   *
    +   * E.g. when a response depends on some configuration, an entity, or an access
    +   * result, we must make sure their cacheability metadata is present on the
    +   * response. This method makes doing that simple.
    +   *
    +   * @param \Drupal\Core\Cache\CacheableDependencyInterface|mixed $dependency
    +   *   The dependency.
    +   */
    +  public function addDependency($dependency);
    

    My knee-jerk inclination is to split this into 2 methods: One for when we know it's a specific object that we can handle, and one for "other stuff we don't really comprehend". Which is what the docblock currently implies.

    If we know that it's either that object or a render array, for instance, then we can type hint and name both methods appropriately for stronger typing, clearer code paths, and fewer conditionals.

    IMO this method should also be called addCacheDependency(), since otherwise in context on a response object (as seen later in the patch) it's unclear that it's cache specific. I could read "$response->addDependency()" to mean all sorts of things that have nothing to do with caching.

  2. +++ b/core/lib/Drupal/Core/Cache/CacheableResponseTrait.php
    @@ -0,0 +1,40 @@
    +    if (!isset($this->cacheabilityMetadata)) {
    +      $this->cacheabilityMetadata = new CacheableMetadata();
    +    }
    

    Shouldn't this just be set by default on the property itself, rather than lazy-initializing? It's an empty object either way.

    That would also eliminate the |null on the return type of getCacheablility(), which is a good thing. ("Or null" returns lead to a glut of pointless IF statements, or random E_FATAL errors. Neither of those are good.)

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/AccessResultResponseSubscriber.php
    @@ -0,0 +1,51 @@
    +/**
    + * Response subscriber to bubble the route's access result's cacheability.
    + */
    +class AccessResultResponseSubscriber implements EventSubscriberInterface {
    

    As someone who only sort of groks the cacheability logic we have floating around now, this docblock does not help me much. I don't know what it should be since I don't grok it. :-) Probably this just needs an extra description sentence or two for background, and maybe a @see if relevant.

  4. +++ b/core/lib/Drupal/Core/EventSubscriber/AccessResultResponseSubscriber.php
    @@ -0,0 +1,51 @@
    +    $request = $event->getRequest();
    +    $response = $event->getResponse();
    +
    +    if ($response instanceof CacheableResponseInterface) {
    +      $access_result = $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT);
    +      $response->addDependency($access_result);
    +    }
    

    Consistency micro-op: Get just the response, check its interface, and return early if it doesn't match. Then continue and do the rest, including retrieving the request.

  5. +++ b/core/modules/rest/src/ResourceResponse.php
    @@ -17,7 +18,7 @@
    -class ResourceResponse extends Response {
    +class ResourceResponse extends CacheableResponse {
    

    I'd rather use the interface and trait directly, rather than the CacheableResponse. It's the same at runtime but more explicit.

Wim Leers’s picture

Thank you very much for your review, much appreciated!

  1. First: I see your overall POV.

    Second: note that we specifically designed RendererInterface::addDependency() to accept *anything*, but only if it implements CacheableDependencyInterface. See #2464877: Update RendererInterface::addDependency() to accept *any* object, not only CachableDependencyInterface objects for the rationale. (BUT! That method will be used very, very often. So it had to be concise. Here we could be more explicit.)

    Third: I also dislike the support for render arrays that was added in #28. I preferred what we had before, for the same reasons you cite: more strictness.

    So, I'm proposing:

    1. To rename addDependency() to ::addCacheDependency(), and to do that not only for CacheableResponseInterface, but also for RendererInterface
    2. To simply remove the render array support that was added in #28; the DX gain there is minimal, and is probably not worth the extra confusion/lack of strictness that #34 points out.
  2. I wanted to do that, but I can't, because I can't do $property = new CacheableMetadata(). Do you happen to know a trick to get around that? :)
  3. Good call; done. It was also very very poorly named. Fixed that too.
  4. I considered doing this too, but chose minimal changes in the first patch. Done now. :)
  5. Works for me, done.
Crell’s picture

+1 on the proposal in #35. If there's a common need for "given a render array, I want a cache metadata object out of it that I can put on a Response", then "render array -> CacheDependencyInterface" should be a stand-alone operation/service of its own that someone can leverage.

Re point 2: ... Carp. Good point. Normally you'd do that in the constructor but this is a trait so... Yeah, nothing's coming to mind. Maybe just document it? The getter should have the same "create empty object if needed" logic then so that NULL can never ever be returned, for the reasons discussed above.

Wim Leers’s picture

#36:

  1. Addressing that in the next reroll, to make it easier to review the changes for the next bullet.
  2. Haha, carp :P Going to remember that one. Or was it a typo? :D
Wim Leers’s picture

Now did point 2 of #35.1/#36.1:

To simply remove the render array support that was added in #28; the DX gain there is minimal, and is probably not worth the extra confusion/lack of strictness that #34 points out.

because

"render array -> CacheDependencyInterface" should be a stand-alone operation/service of its own that someone can leverage.

Doing this separately too, because this too should be of low contentiousness.

Wim Leers’s picture

FileSize
33.13 KB
15.25 KB

Finally, did point 1 of #35.1/#36.1. This is the most contentious change, so I wanted to do it separately.

I went with ::addCacheabilityDependency(), not ::addCacheDependency(). "cache dependency" doesn't make sense in my mind, "cacheability dependency" does. Would love to hear thoughts about this.

That is then also in perfect sync with CacheableDependencyInterface, which is the interface that classes that provide cacheability metadata implement.

While updating core, I grepped for >addDependency(, and found many more calls than I expected. Turns out that that's the method name used for config dependencies. Which makes a lot of sense: those are "true dependencies", so they deserve to use addDependency(), unprefixed. Here, we actually only care about cacheability, so the prefixed addCacheabilityDependency() is more accurate. For both responses and render arrays.

But on the other hand, there are serious reasons to dislike this change:

  1. Verbosity: "add dependency" is much simpler and sounds less scary. Many, many render arrays will use this method.
  2. Crell said in #34.1: it's unclear that it's cache specific, and I agreed with that — but does it have to be? If a response or a render array says it depends on some object, that's valuable information in and of itself: it signals that the render array/response depends on the information in the given object. And for now, the only consequence of that is adopting that information's cacheability metadata. But perhaps it will make sense to add more consequences in the future? For example, a feed (to signal that that information is also available as a feed, for example, i.e. automatically setting ['#attached']['feed']), or a header (to signal REST interactions with that information), or …
Wim Leers’s picture

@Fabianx, @catch: Filed that critical issue: #2476407: Use CacheableResponseInterface to determine which responses should be cached.

To clarify for those reading along: the original issue was major; catch & Fabianx expanded its scope (in #12 + #18), and *that* scope was critical. Better to commit a full solution for the original scope *now* rather than grow the patch to the expanded scope. So: separate issue for the critical part that it was expanded into. That's that issue I just mentioned.

Demoting to major.

Fabianx’s picture

Thanks Wim, I agree :).

Fabianx’s picture

#40:

Hmmmmmmmmmmm ... tricky.

It won't be purely cache specific as I very much would like to avoid having for Ajax responses to do:

$markup = $renderer->renderRoot($build);

$response = new AjaxResponse(200);
$response->addCommand('replace', $id, $markup); // not real syntax
$response->addAttachments($build['#attached']);
$response->addCacheabilityDependency(CacheableMetadata::createFromRenderArray($build));

That seems excessive.

Better would be IMHO at least for render arrays:

$markup = $renderer->renderRoot($build);
$response = new AjaxResponse(200);
$response->addRenderMetaData($build);

--

EDIT:

ALL THE ABOVE IS FOLLOW-UP HOWEVER.
This issue self-contained as it is already is way enough for what we need here. Little steps++

Therefore lets handle render arrays and how to add them to responses in a follow-up to improve their DX.

--

For every object that implements CacheableDependencyInterface it is probably fine to use just:

- addCacheabilityDependency()

and hence good to split the render array use case up to a follow-up. (as discussed above)

But while we are at naming things, what about to name the new function:

- addCacheInformation()

instead of addCacheabilityDependency()?

Not sure that is strong enough though.

Fabianx’s picture

+++ b/core/modules/rest/src/ResourceResponse.php
@@ -7,6 +7,9 @@
 
 namespace Drupal\rest;
 
+use Drupal\Core\Cache\CacheableResponse;

nit - CacheableResponse is unused now.

-- Found one more quick nit, so Alex does not need to fix it on commit :-D.

Status: Needs review » Needs work

The last submitted patch, 39: response_cacheability_consolidation-2463009-39.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
33.44 KB
16.32 KB

- addCacheInformation()

instead of addCacheabilityDependency()?

Not sure that is strong enough though.

I don't think that's a good idea, because:

  1. Indeed, it's not strong enough.
  2. After a very long discussion, we settled on the keyword "dependency" for CacheableDependencyInterface (in #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()")). Some calculated result that depends on other objects, which means the cached calculated result should be invalidated when the depended objects are invalidated: that's a dependency graph. So, any object that implements CacheableDependencyInterface is saying "if you depend on me, you can still be fully cacheable, because I'm a cacheable dependency". If your calculated result depends on an object that does not implement this interface, then either the calculated result is not cacheable, or you're on your own to figure out a way to guarantee the cached calculated result is invalidated when necessary.
    So: a lot of thought went into the naming there, and I'm cautious of throwing that away. The only change here is to be even slightly more specific, and to have 100% alignment: addCacheabilityDependency() + CacheableDependencyInterface.

Changes:

  1. Fixed #45.
  2. Actually, my explanation above just made me realize that we don't have 100% alignment. "add cacheability dependency" vs "cacheable dependency interface", "cacheability" vs. "cacheable". Let's make that 100% consistent.
Wim Leers’s picture

Issue summary: View changes
Issue tags: +DX (Developer Experience)

IS updated.

Wim Leers’s picture

Title: Consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers » Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers
Fabianx’s picture

RTBC from my side

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/CacheableResponseTrait.php
    @@ -0,0 +1,45 @@
    +  /**
    +   * The cacheability metadata.
    +   *
    +   * @var \Drupal\Core\Cache\CacheableMetadata
    +   */
    +  protected $cacheabilityMetadata = NULL;
    

    Nit: Technically the null default is redundant here, a that's what PHP does by default. I don't think we're doing that in most cases currently, so may as well not do it here.

    (Not a commit blocking comment.)

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
    @@ -133,19 +135,12 @@ public function onRespond(FilterResponseEvent $event) {
    +    // Expose the cache contexts and cache tags associated with this page in a
    +    // X-Drupal-Cache-Contexts and X-Drupal-Cache-Tags header respectively.
    +    if ($response instanceof CacheableResponseInterface) {
    +      $response_cacheability = $response->getCacheability();
    +      $response->headers->set('X-Drupal-Cache-Tags', implode(' ', $response_cacheability->getCacheTags()));
    +      $response->headers->set('X-Drupal-Cache-Contexts', implode(' ', $this->cacheContextsManager->optimizeTokens($response_cacheability->getCacheContexts())));
    

    Part of me wonders if this couldn't be internalized, or at least have some utility methods put on it. We have a response object, we know it has cache tags on it, we know the cache tags need to be part of the headers... so why do we need to do that externally? Why not add a primitive for that?

    In concept, Response::prepare() would be the ideal place for that. That allows you to futz with the request right before it gets sent, for header cleanup and stuff like that. However, as our default implementation is in a trait we can't silently inject additional behavior on prepare() (PHP doesn't have compile-time AOP) and implementers would need to remember to call some utility method from their own prepare() method. I don't know if that's better or worse than externalizing it like this.

    Not a commit blocker; just some design style musings. Thoughts welcome, including "stop over-thinking it". :-)

  3. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -151,27 +153,27 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +    // Set the generator in the HTTP header.
    +    list($version) = explode('.', \Drupal::VERSION, 2);
    +
    +    $response = new CacheableResponse($content, 200,[
    +      'X-Generator' => 'Drupal ' . $version . ' (https://www.drupal.org)'
    +    ]);
    

    Note: This means we only add the Drupal header on HTML responses, not on API responses. Is that good or bad? I could see arguments both ways. It also is now a little harder to remove globally if someone doesn't want to self-identify; changing it is probably off topic for this thread, though.

None of the above are commit blockers.

Regarding method naming, it's true we may add other dependent thingies in the future. If we do, they should have their own methods. PHP doesn't support method overloading (function foo(TypeA $a) {}, function foo(TypeB $b) {} is illegal), so we would either need separate methods or one method with a huge switch statement. You know my feeling on switch statements. :-)

Wim: Carp was deliberate. I'm trying to be family friendly in public. :-)

Wim Leers’s picture

FileSize
33.43 KB
570 bytes
  1. You're absolutely right. Fixed.
  2. This is out of scope; this patch already presents a vast improvement over HEAD. Logically and semantically speaking, the improvement here is that instead of putting the burden of setting and updating X-Drupal-Cache-(Contexts|Tags) headers in many places, the burden of updating the cacheability metadata is now on CacheableResponseTrait alone, and the burden of setting the actual headers is now in a single place. And that place is the "finish response" subscriber… which makes a ton of sense IMHO: if you don't want those headers, then you can just have your own "finish response" subscriber and do whatever the hell YOU want on your site :)
    I think it *would* be nice to have it happen automatically, but OTOH it's now also nice that you can easily opt out. Pros and cons either way.
    Plus, as you say, there's no nice/easy way to implement this right now.
  3. That's not true; because we also updated ResourceResponse to implement CacheableResponseInterface. I guess that's what you meant with "API response". As Fabianx said (per the discussion between you and Fabianx) in #12: this makes setting the X-Drupal-Cache-(Contexts|Tags) headers effectively opt-in. Yes, not every response has opted in yet. But that is okay.

RE: carp: Crell++ :D

Is that the magnificent smell of an impending RTBC? :P

Mile23’s picture

Status: Needs review » Needs work

OK, +1 from me, for all that's worth. I appreciate the difficulty of mapping this onto Drupal-think.

I'm not deep in the problem space but I still see a few things...

  1. +++ b/core/lib/Drupal/Core/Cache/CacheableResponseInterface.php
    @@ -0,0 +1,36 @@
    +  /**
    +   * Adds a dependency on an object: merges its cacheability metadata.
    +   *
    +   * E.g. when a response depends on some configuration, an entity, or an access
    +   * result, we must make sure their cacheability metadata is present on the
    +   * response. This method makes doing that simple.
    +   *
    +   * @param \Drupal\Core\Cache\CacheableDependencyInterface|mixed $dependency
    +   *   The dependency.
    +   */
    +  public function addCacheableDependency($dependency);
    

    It says it's simple, but I still don't know how. Maybe another sentence or two describing it? The @param is interface|mixed, which is kind of meaningless. Also maybe return $this for happy fluent interface action.

  2. +++ b/core/lib/Drupal/Core/Cache/CacheableResponseInterface.php
    @@ -0,0 +1,36 @@
    +  /**
    +   * Returns the cacheability metadata for this response.
    +   *
    +   * @return \Drupal\Core\Cache\CacheableMetadata
    +   */
    +  public function getCacheability();
    

    It says "Returns the cacheability metadata for this response" and it's a getter for cacheabilityMetadata which is of type CachableMetadata, so it should be getCachabilityMetadata(). Otherwise it looks like it should return a bool.

Crell’s picture

Wim: Cache headers, yes. I was referring to the 'X-Generator' header, which is being added in the HtmlRenderer class. :-)

I concur with both of Miles23's comments as well, although at this point we're just refining so yes an RTBC is probably in sight.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
33.5 KB
3.83 KB

#51:

  1. Fixed fluency. Not sure what I can still add the docs there. It's already quite lengthy. See RendererInterface::addCacheableDependency(), and as mentioned before: discussions at #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()") + #2464877: Update RendererInterface::addDependency() to accept *any* object, not only CachableDependencyInterface objects regarding its naming, documentation and why it accepts mixed. Let's not rehash that here.
  2. Good call, fixed.

#52: ohh! I misread! Let us please handle that in a separate issue, because it's out of scope here, but yes, that's a great point, and something that's bothered me for a long time. Can you open an issue for that?

Mile23’s picture

Status: Needs review » Needs work

Not trying to rehash anything, just that it would be great if the interface documented the promises it's making. I'd just really like to see another sentence about how to define a dependency, and/or maybe a @see or two.

I brought up the @param because if it were more clearly defined (just one interface, for instance) then the documentation might not be as important. But because the @param is interface|mixed, it's important to explain.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
35.01 KB
2.78 KB

Thanks for the clarification. Note that I'm fine with rehashing that, just not in this issue. If you want to rethink how that works (because I'll be the first to agree that it's imperfect), then please just file a new issue.

In any case: clarified that @param documentation, and did so on all related methods. I hope this is satisfactory :)

Mile23’s picture

Great, thanks. RTBC +1 from me, but I'm not sure I'm the guy to set the status dropdown. @crell? :-)

Wim Leers’s picture

@Mile23: you could be that guy :) Fabianx said it's RTBC in #48, Crell's comment in #49 were "no commit blockers", so it was only blocked on RTBC from your side :)

(I know it's very scary to RTBC an issue the first 20 times or so — so it's totally fine if you prefer not to do that for now :))

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

OK. :-)

It's just that Crell had some questions, it looked like. Maybe I'm wrong. Also I'm not sure this is the best architectural decision, but it works and it's decent.

It looks like the testbot has frozen in #55, but clicking on 'view' reveals that it passed.

Also: RTBC should always be scary, especially for a major decision like this. And I've done more than 20. :-)

Crell’s picture

X-Generator header issue here: #2477461: Move X-Generator header to its own listener

+1 (Subscribe!)

(I'm not wild about not type hinting addCacheableDependency() strictly, without the mixed, but it looks like resolving that would involve modifying the CacheableMetadata object, too, and that's scope creep.)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Just a load of nits. And In lieu of pressing re-test because #2466585: Decouple cache implementation from the renderer and expose as renderCache service landed (and quite a few other things since Apr 23) going to set back to needs work.

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/AnonymousUserResponseSubscriber.php
    @@ -60,14 +65,10 @@ public function onRespond(FilterResponseEvent $event) {
    -        $cache_tags = Cache::mergeTags($existing_cache_tags, $cache_tags);
    

    Can remove the Cache class from the use statements now.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
    @@ -10,6 +10,8 @@
     use Drupal\Core\Cache\Cache;
     use Drupal\Core\Cache\CacheableDependencyInterface;
    +use Drupal\Core\Cache\CacheableMetadata;
    

    Cache, CacheableDependencyInterface, CacheableMetaData are not used.

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
    @@ -133,19 +135,12 @@ public function onRespond(FilterResponseEvent $event) {
    -    $access_result = $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT);
    

    AccessAwareRouterInterface not used anymore.

  4. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -151,27 +153,27 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    -        $cache_contexts = Cache::mergeContexts($cache_contexts, $html[$region]['#cache']['contexts']);
    -        $cache_tags = Cache::mergeTags($cache_tags, $html[$region]['#cache']['tags']);
    

    Can remove the Cache class from the use statements now.

  5. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -151,27 +153,27 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    -    $response = new Response($content, 200,[
    

    Can remove the response class from the use statements now.

  6. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -109,14 +109,8 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    -        $cache_tags = Cache::mergeTags($existing_cache_tags, $cache_tags);
    

    Cache is not used anymore.

  • catch committed ea6548b on 8.0.x
    Issue #2463009 by Wim Leers: Introduce CacheableResponseInterface:...
catch’s picture

Status: Needs work » Fixed

Doh, I just committed/pushed this one as Alex reviewed. Let's open a follow-up for the nits..

Wim Leers’s picture

Wim Leers’s picture

  • alexpott committed e7b1e4e on 8.0.x
    Issue #2478543 by Wim Leers: Follow-up for #2463009: remove unused use...
Wim Leers’s picture

Follow-up committed :)

Status: Fixed » Closed (fixed)

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