Route's access result's cacheability not applied to the response's cacheability

1. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -111,8 +112,10 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st
   -        // Persist the 'exception' attribute to the subrequest.
   +        // Persist the 'exception' and access result attributes to the
   +        // subrequest.
            $sub_request->attributes->set('exception', $request->attributes->get('exception'));
   +        $sub_request->attributes->set(AccessAwareRouterInterface::ACCESS_RESULT, $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT));
   

   The documentation was and is still not helpful at all. Yes, we see what the code is doing, but there is nothing about the why.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -139,6 +159,30 @@ public function onRespond(FilterResponseEvent $event) {
   +      $existing_cache_contexts = explode(' ', $response->headers->get('X-Drupal-Cache-Contexts'));
   +      $cache_contexts = Cache::mergeTags($existing_cache_contexts, $cache_contexts);
   

   I'm glad that Cache::mergeTags works the same as Cache::mergeContexts

3. +++ b/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php
   @@ -84,6 +84,19 @@ public function test10() {
   +        '#markup' => "test18",
   

   nitpick hell: use single quotes ...

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -139,6 +159,30 @@ public function onRespond(FilterResponseEvent $event) {
+
+    // X-Drupal-Cache-Contexts
+    $cache_contexts = $cacheable_access_result->getCacheContexts();
+    if ($response->headers->has('X-Drupal-Cache-Contexts')) {
+      $existing_cache_contexts = explode(' ', $response->headers->get('X-Drupal-Cache-Contexts'));
+      $cache_contexts = Cache::mergeTags($existing_cache_contexts, $cache_contexts);
+    }
+    $response->headers->set('X-Drupal-Cache-Contexts', implode(' ', $this->cacheContexts->optimizeTokens($cache_contexts)));

Alright, you didn't got me ... $cache_contexts = Cache::mergeTags ... its certainly using the wrong method. In general I'm curious whether we at any time had more than 2 contexts/tags we merged at the same time, ... the API is a bit sad as its documentation is not optimal.

Yes, we merge >2 tags in several places. E.g. \Drupal\block\BlockViewBuilder::viewMultiple(). For ::mergeContexts(), I just made it analogous with ::mergeTags().

Well, we could have one method doing multiple and the default is just a simlpe wrapper, but these are just ideas.

RTBC + 1

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -139,6 +159,30 @@ public function onRespond(FilterResponseEvent $event) {
+  protected function updateDrupalCacheHeaders(Response $response, CacheableInterface $cacheable_access_result) {
+    // X-Drupal-Cache-Tags
...
+    // X-Drupal-Cache-Contexts
+    $cache_contexts = $cacheable_access_result->getCacheContexts();

For the record:

I am not yet happy about the special casing of the headers here.

I think we should always set the headers in the response subscriber and never in the main ContentHtml Thing, but that is out of scope for this particular issue, so should be a follow-up.

---

Still RTBC

#19: good ideas, but having them as separate methods keeps the door open for smarter validation in the future — they're conceptually different things, they're just both represented as strings.

I simply think you don't understand me at all.

Let's keep this focused on route's access result handling.

Yeah, no question, all I wanted is to throw out some general thoughts. Sorry

RTBC + 1

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 6c3303d and pushed to 8.0.x. Thanks!

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -139,6 +159,30 @@ public function onRespond(FilterResponseEvent $event) {
+    // X-Drupal-Cache-Tags
+    $cache_tags = $cacheable_access_result->getCacheTags();
+    if ($response->headers->has('X-Drupal-Cache-Tags')) {
+      $existing_cache_tags = explode(' ', $response->headers->get('X-Drupal-Cache-Tags'));
+      $cache_tags = Cache::mergeTags($existing_cache_tags, $cache_tags);
+    }
+    $response->headers->set('X-Drupal-Cache-Tags', implode(' ', $cache_tags));
+
+    // X-Drupal-Cache-Contexts
+    $cache_contexts = $cacheable_access_result->getCacheContexts();
+    if ($response->headers->has('X-Drupal-Cache-Contexts')) {
+      $existing_cache_contexts = explode(' ', $response->headers->get('X-Drupal-Cache-Contexts'));
+      $cache_contexts = Cache::mergeContexts($existing_cache_contexts, $cache_contexts);
+    }
+    $response->headers->set('X-Drupal-Cache-Contexts', implode(' ', $this->cacheContexts->optimizeTokens($cache_contexts)));

I think it is a shame to have to explode and reset these but I discussed with @Wim Leers in IRC and he couldn't see a way around this.

I created #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers as follow-up to #33 and #22 so we don't forget.