Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 wantX-Drupal-Cache-Tags
andX-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()
toRendererInterface::addCacheableDependency()
for consistency withCacheableResponseInterface
, and because it then maps 100% toCacheableDependencyInterface
(perfect symmetry, better DX).
Beta phase evaluation
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. |
Comment | File | Size | Author |
---|---|---|---|
#55 | response_cacheability_consolidation-2463009-55.patch | 35.01 KB | Wim Leers |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx commentedComment #2
Wim LeersFixing + clarifying.
Comment #3
Wim LeersAlso see #2458349-35: Route's access result's cacheability not applied to the response's cacheability: I don't see a simpler or cleaner way just yet.
Comment #4
anavarreComment #5
Fabianx CreditAttribution: Fabianx commentedSo 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.
Comment #6
Wim Leers#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!
Comment #7
Fabianx CreditAttribution: Fabianx commentedI changed my mind, this can be worked on directly using #5 and does not need to wait for #2450993: Rendered Cache Metadata created during the main controller request gets lost.
Closed #2463697: X-Drupal-Cache-Contexts and X-Drupal-Cache-Tags headers should not indicate emptiness by the empty string as header value as a duplicate of this.
Comment #8
larowlanThere is no ->data :(
Comment #9
Fabianx CreditAttribution: Fabianx for Acquia commentedResponse 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 ...
Comment #10
Fabianx CreditAttribution: Fabianx for Acquia commentedI like:
use Drupal\Core\Render\RenderResponse;
similar to AjaxResponse, etc :)
Comment #11
Wim LeersBut 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…
Comment #12
Fabianx CreditAttribution: Fabianx for Acquia commentedI 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.
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:
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:
OR something like:
etc.
But also support e.g.:
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.)
Comment #13
Wim LeersWow!
Crell++
Fabianx++
Comment #14
Crell CreditAttribution: Crell at Palantir.net commentedThanks, Fabianx!
One note, I don't thinks this option will fly:
Because the createFromRenderArray() process would rely on multiple services. No sense replacing one global with another global service call. :-) But
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.)
Comment #15
Mile23I 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.
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.
Comment #16
Wim LeersPlease read their docs.
render()
is the 99% case.Comment #17
Fabianx CreditAttribution: Fabianx for Acquia commented#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 ...).
Comment #18
catchDiscussed 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.
Comment #19
dawehnerInteresting, 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.
Comment #20
Wim LeersComment #21
Fabianx CreditAttribution: Fabianx for Acquia commented#19 Yes, I will update IS today with the plan, probably making this a meta.
Comment #22
Mile23Just a heads-up that work is being done here: #2393329: Replace all drupal_render calls with the service, and inject it, if possible.
Comment #23
Wim LeersRolling an initial version of this.
Comment #24
Wim LeersAnd 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:
X-Drupal-Cache-*
headers in a single place; the most logical place for that is IMOFinishResponseSubscriber
.FinishResponseSubscriber
. At the same time, this makes it easy forAnonymousUserResponseSubscriber
to run between the aforementioned subscriber and theFinishResponseSubscriber
. (TheAnonymousUserResponseSubscriber
conditionally adds another cache tag, but needs all other cache tags on the response to already be set to make its conditional decision.)CacheableResponseInterface::addDependency()
, to make it similar toRendererInterface::addDependency()
. It makes sense, too: a cacheable response's cacheability depends on all the cacheability metadata of all the things it is passed there.Comment #26
Wim LeersFixed that single failure. Test coverage expanded; this now also verifies that
X-Drupal-Cache-(Contexts|Tags)
headers are not set forResponse
s, only forCacheableResponse
s :)Comment #27
Fabianx CreditAttribution: Fabianx for Acquia commentedOverall 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 ...
Comment #28
Wim LeersLet'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.)
:)
Done. I'm not convinced this is that much of an improvement, but it's also not harming anything.
Looking forward to those!
Comment #29
Fabianx CreditAttribution: Fabianx for Acquia commentedI would argue we want to set that cache tag actually also for rest responses and such globally in the finishResponseSubscriber?
Not sure ...
Not sure why we need the assignment?
Is rest.settings of type cacheable dependency interface?
Should it then be rest.cache_settings?
Initial review ...
Comment #30
Wim LeersConfigBase
subclass implementsCacheableDepdendencyInterface
, so also the REST settings config object.Comment #31
Fabianx CreditAttribution: Fabianx for Acquia commentedSetting back to critical, to avoid screwing with the stats until I get to properly set the scope of the follow-up ...
Comment #32
catchI'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.
Comment #33
Wim Leers#32: that makes sense, thanks for clarifying!
Comment #34
Crell CreditAttribution: Crell at Palantir.net commentedMy 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.
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.)
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.
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.
I'd rather use the interface and trait directly, rather than the CacheableResponse. It's the same at runtime but more explicit.
Comment #35
Wim LeersThank you very much for your review, much appreciated!
Second: note that we specifically designed
RendererInterface::addDependency()
to accept *anything*, but only if it implementsCacheableDependencyInterface
. 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:
addDependency()
to::addCacheDependency()
, and to do that not only forCacheableResponseInterface
, but also forRendererInterface
$property = new CacheableMetadata()
. Do you happen to know a trick to get around that? :)Comment #36
Crell CreditAttribution: Crell at Palantir.net commented+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.
Comment #37
Wim Leers#36:
Comment #38
Wim LeersNow did point 2 of #35.1/#36.1:
because
Doing this separately too, because this too should be of low contentiousness.
Comment #39
Wim LeersFinally, 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 useaddDependency()
, unprefixed. Here, we actually only care about cacheability, so the prefixedaddCacheabilityDependency()
is more accurate. For both responses and render arrays.But on the other hand, there are serious reasons to dislike this change:
['#attached']['feed']
), or a header (to signal REST interactions with that information), or …Comment #40
Wim Leers@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.
Comment #41
Fabianx CreditAttribution: Fabianx for Acquia commentedThanks Wim, I agree :).
Comment #42
Fabianx CreditAttribution: Fabianx for Acquia commented#40:
Hmmmmmmmmmmm ... tricky.
It won't be purely cache specific as I very much would like to avoid having for Ajax responses to do:
That seems excessive.
Better would be IMHO at least for render arrays:
--
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.
Comment #43
Fabianx CreditAttribution: Fabianx for Acquia commentednit - CacheableResponse is unused now.
-- Found one more quick nit, so Alex does not need to fix it on commit :-D.
Comment #45
Wim LeersI don't think that's a good idea, because:
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 implementsCacheableDependencyInterface
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:
Comment #46
Wim LeersIS updated.
Comment #47
Wim LeersComment #48
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC from my side
Comment #49
Crell CreditAttribution: Crell at Palantir.net commentedNit: 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.)
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". :-)
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. :-)
Comment #50
Wim LeersX-Drupal-Cache-(Contexts|Tags)
headers in many places, the burden of updating the cacheability metadata is now onCacheableResponseTrait
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.
ResourceResponse
to implementCacheableResponseInterface
. 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 theX-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
Comment #51
Mile23OK, +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...
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.It says "Returns the cacheability metadata for this response" and it's a getter for
cacheabilityMetadata
which is of typeCachableMetadata
, so it should begetCachabilityMetadata()
. Otherwise it looks like it should return abool
.Comment #52
Crell CreditAttribution: Crell at Palantir.net commentedWim: 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.
Comment #53
Wim Leers#51:
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 acceptsmixed
. Let's not rehash that here.#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?
Comment #54
Mile23Not 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.
Comment #55
Wim LeersThanks 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 :)Comment #56
Mile23Great, thanks. RTBC +1 from me, but I'm not sure I'm the guy to set the status dropdown. @crell? :-)
Comment #57
Wim Leers@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 :))
Comment #58
Mile23OK. :-)
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. :-)
Comment #59
Crell CreditAttribution: Crell at Palantir.net commentedX-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.)
Comment #60
alexpottJust 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.
Can remove the Cache class from the use statements now.
Cache, CacheableDependencyInterface, CacheableMetaData are not used.
AccessAwareRouterInterface not used anymore.
Can remove the Cache class from the use statements now.
Can remove the response class from the use statements now.
Cache is not used anymore.
Comment #62
catchDoh, I just committed/pushed this one as Alex reviewed. Let's open a follow-up for the nits..
Comment #63
Wim LeersFollow-up filed with patch: #2478543: Follow-up for #2463009: remove unused use statements.
Comment #64
Wim LeersComment #66
Wim LeersFollow-up committed :)