Use CacheableResponseInterface to determine which responses should be cached

Blocked on a decision on:

- #2472321: Move rest.module routes to /api

Blocked on implementation:

- #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers

Therefore postponing for now.

As a reminder, a critical issue is one that:

* Renders [the] system unusable and has no workaround.
* Causes loss of data.
* Exposes security vulnerabilities.

Downgrading until some justification for critical is added to the IS.

Actually, I see that justification is provided, but that's only in the event we're doing /api and so far I count 3 framework maintainers against that.

I am marking this active as major (not critical task).

The other issue showed that even without the move of /api it is very easy to shoot yourself in the foot right now.

And if Core does not get it right, we cannot expect contrib/ to do so ...

i.e. as a Symfony developer I would never get the idea that if I do:

$string = 'This is a secret only valid for this user / request / etc.';
return new Response($string, 200);

that it then suddenly is cached in the page cache or external cache. As we only do it for anon its less of a problem (and hence not critical), but the DX of getting opted-in to all the cache headers when what I have might not be cacheable at all, is questionable at best and wrong at worst ...

I totally agree with @fabianx here, if you use raw response objects, you did not wanted the result to be cached, unless you set your headers for yourself.
The code producing the response object is the only one which can figure out, how long something is cacheable.

Here is a list of raw response objects:

core/modules/aggregator/tests/modules/aggregator_test/src/Controller/AggregatorTestRssController.php:34:    $response = new Response(); # Test doesn't matter
core/modules/ban/src/BanMiddleware.php:53:      return new Response(SafeMarkup::format('Sorry @ip has been banned', ['@ip' => $ip]), 403); # I guess we don't want to cache ban messages?
core/modules/ban/tests/src/Unit/BanMiddlewareTest.php:86:    $expected_response = new Response(200); # Test
core/modules/book/src/Controller/BookController.php:157:    return new Response(drupal_render($exported_book)); # Seems to make sense to cache
core/modules/image/src/Controller/ImageStyleDownloadController.php:141:        return new Response($this->t('Error generating image, missing source file.'), 404); #  If we cache this here, we might never update, once the image is there?
core/modules/image/src/Controller/ImageStyleDownloadController.php:180:      return new Response($this->t('Error generating image.'), 500);
core/modules/image/tests/src/Unit/PageCache/DenyPrivateImageStyleDownloadTest.php:53:    $this->response = new Response(); # Test
core/modules/node/tests/src/Unit/PageCache/DenyNodePreviewTest.php:53:    $this->response = new Response();  # Test
core/modules/rest/src/RequestHandler.php:69:          return new Response($content, 400, array('Content-Type' => $request->getMimeType($format))); # I guess we want to be able, at least theoretically, cache the responses, maybe for GET at least, but therefor serializer would need to bubble up the cache contexts of the access ...
core/modules/rest/src/RequestHandler.php:102:      return new Response($content, $e->getStatusCode(), $headers);
core/modules/rest/src/RequestHandler.php:124:    return new Response(\Drupal::csrfToken()->get('rest'), 200, array('Content-Type' => 'text/plain'));
core/modules/system/src/Controller/DbUpdateController.php:201:    return new Response($this->bareHtmlPageRenderer->renderBarePage($output, $title, 'maintenance_page', $regions)); # No cache
core/modules/system/src/Controller/SystemInfoController.php:72:    return new Response($output); # Tricky, the PHP page used to be really good to measure the "bootstrap"/routing and nothing else, but I think caching that result would allow you to measure more.
core/modules/system/src/CronController.php:55:    return new Response('', 204);
core/modules/views/src/ViewExecutable.php:1403:        $this->response = new Response('', 200);
core/modules/views/src/ViewExecutable.php:1704:      $this->response = new Response();

So what's the actionable here? dawehner's comments in #11 suggest there's maybe 2-3 direct Response usages right now we should cache, and the rest probably leave as is. Shall we spin up dedicated issues for those and make this a meta?

#12: I think the only thing left here is write a patch. I don't think we need dedicated metas as the conversion should be very straightforward, but I am also not opposed to a meta ...

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.

So, theoretically, the attached patch should fix (a)?

@dawehner mentioned in irc that we should be using cacheable responses for rest as well, so attached is a patch that does exactly that.

Looks great, I assume we need some tests though.

Fixes the test failures in #14, #15.
Optimized the if statement as suggested by @Wim Leers in #16.
Added a test as requested in #18 by @Fabianx.

Test looks great, now just need to get it green.

The second call to /system-test/respond-reponse is a cache-hit, while it's not supposed to be one. I have no idea why that happens though.
Locally that is the only remaining failure. I can't reproduce the other test failures.

#23: That is likely page cache interfering, which uses its own logic.

Maybe we need to try to add the logic there, too?

And we should have one test scenario with page_cache disabled at least.

I disabled page cache module and ran the tests again. Locally, this doesn't seem to work though. I can't figure out why these test don't work though.

The testCacheableResponseResponses in \Drupal\page_cache\Tests\PageCacheTest now passes locally.

I'm assuming that there'll be a lot of new failures though, so lets see where exactly.

+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -119,7 +120,8 @@ protected function pass(Request $request, $type = self::MASTER_REQUEST, $catch =
+    $response = $this->get($request);
+    if ($response instanceof CacheableResponse) {

I don't think that is right.

A non-cacheable response should never enter the page cache in the first place.

I think the simplest actually is to use a ResponsePolicy, which then works for page cache and the main subscriber out of the box.

I removed that snippet of code from the PageCache module and put it in a ResponsePolicy as suggested in #30. This should break at least the same amount of tests as before.

The current patch adds a CacheableJsonResponse. This also changes the DenyNonCacheableResponse.php to not do instanceof CacheableResponse it now does instanceof CacheableResponseInterface.
This way, the newly added CacheableJsonResponse works, as well as the original CacheableResponse and HtmlResponse.
I've also slightly changed the PageCacheTest so that passes now as well, this should reduce the amount of errors significantly.

FinishResponseSubscriber::onRespond() :

-    $is_cacheable = ($this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY);
+    $is_cacheable = ($response instanceof CacheableResponseInterface) && ($this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY);

Isn't that redundant with the addition of the DenyNonCacheableResponse policy ?

@yched, you're right, this is redundant now.

Removing needs test tag, because there's a test added in the patch.

For the record, this is an API change as existing responses will no longer be cached. So at least needs a change record for that...

1. +++ b/core/core.services.yml
   @@ -231,6 +231,11 @@ services:
   +  page_cache_no_cache_non_cacheable:
   

   That is a hilarious service name. Almost as good as block_block_block...

2. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -406,4 +406,47 @@ public function testFormImmutability() {
   +  /**
   +   * Tests the difference between having a controller return a Response object
   +   * versus returning a CacheableReponse
   +   */
   

   Nit: One line docblock.

My only other issue with #37 is that it's a deny, which feels like an opt-out type implementation rather than an opt-in. Is it just a quirk of the way the cache policy system works (which I don't fully grok) that it's done that way?

To clarify why I'm concerned in #40, if someone has a Response object of their own, and they set cache headers on it themselves, we should not be overriding that. The presence of CacheableResponseInterface should be the opt-IN to us touching cache headers at all. The default for a Response is to have no cache. If a controller doesn't opt-in, if I read this code correctly, we'd forcibly disable caching on it. Rather, we want to do nothing at all to caching and assume the controller dev knows what he's doing cache-wise.

+++ b/core/core.services.yml
@@ -231,6 +231,11 @@ services:
+  page_cache_no_cache_non_cacheable:

Does it make sense to talk about page cache inside core itself?

#40.1: Changed the service name to: page_cache_deny_non_cacheable.
#40.2: Fixed the docblock.
#42: At the same place in services.yml the following services were already defined: page_cache_request_policy, page_cache_response_policy, page_cache_no_cache_routes, page_cache_no_server_error. If this is not good practice, I can open a followup to move these to the page cache module.

Regarding @Crell's comment in #40 & #41:
I honestly don't have a correct answer for this, I followed the suggestion from @Fabianx in #30. I'm going to try to answer it, as far as I understand this.

Cacheability doens't change if you return a renderable array from a controller, that'll be the most used use-case when creating new controllers in custom code, I think.
When you're actually doing something like: return new Reponse('Test', 200); there isn't any cache metadata and thus refusing to cache that controller is a good solution, imho. If you're already setting the correct headers for caching (X-Drupal-Cache, X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts) I'm assuming you're pretty well known with the entire caching layer and so returning a CacheableResponse instead of the Response shouldn't be that hard to figure out.

I think I correctly understood what @Fabianx explained about this in irc, so here's an updated patch.
Patch removes the DenyNonCacheableResponse response policy and only adds cacheability headers in the FinishReponseSubscriber when the response is a cacheable response. Didn't change any of the tests but everything should still pass.

( X-POST with #45 )

Discussed in IRC; I indeed forgot that DENY would remove any cacheable headers set.

- Just add an early return when its not a CacheableResponseInterface response
- Leave page cache 100% alone in here as the following should still work:

return new Response('Hi, I have a cache tag', 200, ['X-Drupal-Cache-Tags' => ['a_nice_cache_tag'], 'Cache-Control' => 'public, max-age=600' ]);

Unrelated:

In a follow-up we could discuss to set a FrozenCacheableMetadata at this point to ensure immutability.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -125,7 +125,7 @@ public function onRespond(FilterResponseEvent $event) {
-    $is_cacheable = ($this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY);
+    $is_cacheable = ($response instanceof CacheableResponseInterface && $this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY);

No, this is not correct and has the same problems.

We want to early return above the other CacheableResponnseInterface check.

e.g.

-if ($response instanceof CacheableResponseInterface) {
+if (!($response instanceof CacheableResponseInterface)) {
+  return;
+}

Implements the early return mentioned in #47.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -115,11 +115,19 @@ public function onRespond(FilterResponseEvent $event) {
-    if ((Settings::get('reverse_proxy', FALSE) || Settings::get('send_cacheability_headers', FALSE)) && $response instanceof CacheableResponseInterface) {
+    if (Settings::get('reverse_proxy', FALSE) || Settings::get('send_cacheability_headers', FALSE)) {

That part was reverted, that is why the patch does not apply anymore.

Looks great!

Probably need to extend the test coverage a little for the 'leave responses that set cacheable data' alone part and ensure that standard responses by default are not cacheable.

My local install wasn't up to date with the latest commits. This interdiff is vs #45.

@Fabianx figured out that when there's no "Expires" header, everything breaks. So this makes sure that we always have one.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -120,6 +120,12 @@ public function onRespond(FilterResponseEvent $event) {
+        $expire_header = $response->headers->get('Expires');
+        if (!$expire_header) {
+          $this->setExpiresNoCache($response);
+        }

We can and should even call $this->makeResponseUncacheable()

or what it is called.

That is perfectly valid - as the default is uncacheable, so if no one has changed the Cache-Control value we mark it uncacheable.

#55
There already is \Drupal\Core\EventSubscriber\FinishResponseSubscriber::setResponseNotCacheable so I don't think that's a good idea. I can call out to that method instead though.

#56: Yes, that is what I meant.

So, this fixes #55

Nice, likely either the image tests are mistaken or they need to return a CacheableResponse instead.

This fixes the failures in ImageStylesPathAndUrlTest. I can't get the other 2 tests to work though.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -115,13 +115,22 @@ public function onRespond(FilterResponseEvent $event) {
   +    // If the current response isn't an implementation of the
   +    // CacheableResponseInterface, we're trusting that a Response is either
   +    // explicitly not cacheable or that caching headers are already set from
   +    // another place.
   +    if (!($response instanceof CacheableResponseInterface)) {
   +      if (!$this->isCacheControlCustomized($response)) {
   +        $this->setResponseNotCacheable($response, $request);
   +      }
   +      return;
   +    }
   

   I agree with the logic here. The negation in combination with the early return makes that a bit hard to read. I probably would try to write this in positive logic, i.e. (if ($response instanceof CacheableResponseInterface) { /* manipulate headers } else { /* leave them alone unless cache-control is not set at all */ }. YMMV.

2. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
   @@ -136,7 +136,7 @@ function _testImageFieldFormatters($scheme) {
   -      $this->assertTrue(strstr($this->drupalGetHeader('Cache-Control'),'private') !== FALSE, 'Cache-Control header was sent.');
   +      $this->assertTrue(strstr($this->drupalGetHeader('Cache-Control'),'public') !== FALSE, 'Cache-Control header was sent.');
   
   +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
   @@ -215,7 +215,7 @@ protected function doTestResponsiveImageFieldFormatters($scheme, $empty_styles =
   -      $this->assertTrue(strstr($this->drupalGetHeader('Cache-Control'), 'private') !== FALSE, 'Cache-Control header was sent.');
   +      $this->assertTrue(strstr($this->drupalGetHeader('Cache-Control'), 'public') !== FALSE, 'Cache-Control header was sent.');
   

   That does not seem to be correct. When images are served with the private stream wrapper, then I expect that Cache-Control should contain private not public. We want to allow browsers to store the file, but not intermediate proxies.

3. +++ b/core/modules/image/tests/modules/image_module_test/image_module_test.module
   @@ -10,7 +10,11 @@
   -    return array('X-Image-Owned-By' => 'image_module_test');
   +    return [
   +      'X-Image-Owned-By' => 'image_module_test',
   +      'Cache-Control' => 'no-cache, must-revalidate, post-check=0, pre-check=0',
   +      'Expires' => \DateTime::createFromFormat('j-M-Y H:i:s T', '19-Nov-1978 05:00:00 GMT')->format('D, d M Y H:i:s T'),
   +    ];
   

   I also suspect that this change might be wrong. instead I think that ImageStyleDownloadController::deliver() should be altered and those headers should be added to the list of default headers before they are passed to the BinaryFileResponse I think. And why add Cache-Control: no-cache here?

Re #46

return new Response('Hi, I have a cache tag', 200, ['X-Drupal-Cache-Tags' => ['a_nice_cache_tag'], 'Cache-Control' => 'public, max-age=600' ]);

What exactly do you expect to happen in that case in:

1. finish response subscriber?
2. the page cache middleware?

#65

1. While I agree that if (!($response instanceof CacheableResponseInterface)) { ... } reads kind of funky. I think this is a better solution over adding extra indentation in the following code blocks. So I haven't changed this yet. I think the documentation is sufficiently clear.
2. ok, changed the tests.
3. Moved similar logic to ImageStyleDownloadController::deliver()
   I added the cache control header to get tests to pass.

#66: If I understand it correctly, the finish response subscriber adds the headers it should add to every response: X-UA-Compatible, Content-language, X-Content-Type-Options and X-Frame-Options. After adding those headers it should stop executing (because it's not an instance of CacheableResponseInterface and the cache-control header is added. I'm not enterily sure what the page cache middleware should do, but because this is not a CacheableResponseInterface I'd assume it shouldn't be triggered.

The attached patch reintroduces the test failures found in #58. Working on getting those fixed.

Attached patch reduces the amount of failures from 6 to 4.

I spent an afternoon looking at the remaining 4 testfailures.
Attached interdiff is what I think could possible be a resolution to the remaining failures but it doesn't make a difference so the attched patch is still the one from #69. I'm going to need help to figure this out.

@Wim Leers is right, the failures in ConfigFormOverrideTest + ContainerRebuildWebTest are not related to this patch.

I can't figure out how to fix the failures in the other 3 tests.

I was on holidays for a week and spent a couple of hours with this patch. The attached interdiff is the only piece of code that actually is useful. Attached patch is still failing with the same 4 failures.

Like I mentioned in #71, I'll need help to figure out how to resolve those failures.

Looking at this now.

So, the BinaryFileResponse constructor has an optional argument $public = TRUE. If that is set, then the Cache-Control directive public will be set - regardless of the contents of the $headers parameter.

In HEAD we seem to deliver private image styles with Cache-Control: no-cache, but everything which is delivered through the FileDownloadController (even if it is an image attached to a node), gets Cache-Control: private.

Also the change to ShortcutSetsTest seem bogus.

The remaining failure in PageCacheTest is introduced because dynamic page cache doesn't vary by query parameters by default.
The attached patch fixes this by clearing caches between each call to system-test/set-header.

The last patch I uploaded, #82 didn't integrate all the changes @znerol did in #80. Attached patch does.

There is still one remaining failure in PageCacheTest on this line:
$this->assertEqual($this->drupalGetHeader('Foo'), 'bar', 'Custom header was sent.');. We could also clear caches (Cache::invalidateTags(['rendered']);) but then the comment preceding this would be wrong: // Check that authenticated users bypass the cache..

The problem in PageCacheTest was that the response generated in SystemTestController::setHeader didn't set the correct cacheability metadata, it now does and this fixes the remaining failures.

Fixes all the remarks made in #93, reverted the code in .8

Talked with @znerol about this, we're happier now.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -115,13 +115,30 @@ public function onRespond(FilterResponseEvent $event) {
   +    // If the current response isn't an implementation of the
   +    // CacheableResponseInterface, we assume that a Response is either
   +    // explicitly not cacheable or that caching headers are already set in
   +    // another place.
   +    if (!$response instanceof CacheableResponseInterface) {
   

   Isn't this an API change?

   For example, I'm pretty sure that this means that redirect responses are no longer cached, and there is no redirect response class in core that is cacheable then? That will break redirect.module's response caching, for example?

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -67,7 +68,7 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
              $error['error'] = $e->getMessage();
              $content = $serializer->serialize($error, $format);
   -          return new Response($content, 400, array('Content-Type' => $request->getMimeType($format)));
   +          return new Response($content, 400, ['Content-Type' => $request->getMimeType($format)]);
            }
          }
   

   No change except the array() format switch...

3. +++ b/core/modules/system/src/FileDownloadController.php
   @@ -52,17 +52,11 @@ public function download(Request $request, $scheme = 'private') {
    
   -      foreach ($headers as $result) {
   -        if ($result == -1) {
   -          throw new AccessDeniedHttpException();
   -        }
   +      if (in_array(-1, $headers) || empty($headers)) {
   +        throw new AccessDeniedHttpException();
          }
    
   -      if (count($headers)) {
   -        return new BinaryFileResponse($uri, 200, $headers);
   -      }
   -
   -      throw new AccessDeniedHttpException();
   +      return new BinaryFileResponse($uri, 200, $headers, $scheme !== 'private');
        }
   

   Same here, the only real difference seems to be the additional argument? That makes it a lot harder to review...

4. +++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
   @@ -259,13 +260,22 @@ public function authorizeInit($page_title) {
    
   +  public function respondWithReponse(Request $request) {
   +    return new Response('test');
   +  }
   +
   +  public function respondWithCacheableReponse(Request $request) {
   +    return new CacheableResponse('test');
   

   Missing docblocks.

Attached patch fixes most of @Berdir's problems in #98. Not sure about #98.1.

Thanks @Berdir for the review. Re #98.1, while I do consider this as an API change, it for sure has an impact on existing code/deployments. From manual testing I can confirm that indeed the Cache-Control header for redirect responses will prevent caching in reverse proxies with the patch. That was not the case before.

Not sure what to do about that. Maybe our various RedirectResponse classes should be subclasses of CacheableResponse?

Also I am not quite sure about the scope of this issue. What exactly is the motivation for all this?

Filed #2573923: Introduce a CacheableRedirectResponse and use it where appropriate

#101: The motivation is exactly like happened in that issue.

A redirect might be cached, even though we don't know how to invalidate it, etc.

So if someone uses a normal Symfony response then they are on their own to use cache headers, etc.

This is to both:

a) avoid bugs (caching something wrongly)

b) allow people to do with Symfony responses whatever they want - more freedom.

Patch no longer applied, reroll attached.

This looks like rc deadline material to me. The reason for rc deadline is because this is a potential behavior change, but a good one.

Already now I have seen code in the contrib wild west that returned a normal Response without cacheable metadata.

However now this will be cached both internally and externally for the set cache time, but there is no cacheable metadata with the usual problems of cache expiration, etc.

So this is really important to get in.

After long back and forth decided to make this critical.

I would have made this critical after rc deadline anyway (if it was not in, yet), so it is more honest to make it critical now.

Rewritten the IS and added beta evaluation to make the point way more clear.

CR next.

Proposed commit message:

Issue #2476407 by borisson_, znerol, Fabianx, Wim Leers, dawehner, Crell, Berdir: Using CacheableResponseInterface to better determine which responses should be cached/cacheable

Added a CR: https://www.drupal.org/node/2579995

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -115,13 +115,30 @@ public function onRespond(FilterResponseEvent $event) {
   +      // HTTP/1.0 proxies do not support the Vary header, so prevent any caching
   

   there are http/1.0 only proxies out there?

2. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -178,7 +178,7 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   -      return new BinaryFileResponse($uri, 200, $headers);
   +      return new BinaryFileResponse($uri, 200, $headers, FALSE);
   
   +++ b/core/modules/system/src/FileDownloadController.php
   @@ -59,7 +59,7 @@ public function download(Request $request, $scheme = 'private') {
          if (count($headers)) {
   -        return new BinaryFileResponse($uri, 200, $headers);
   +        return new BinaryFileResponse($uri, 200, $headers, $scheme !== 'private');
          }
    
   

   Does someone mind adding a comment about why we need FALSE here?

#114.2:
I didn't want to derail this issue by posting a patch to test things here. I was not sure why we need FALSE and not $scheme !== 'private' like in the other case. I did that in #2580147-3: Testing issue - Please ignore and the tests still pass. I have added a test to see if this can be made to fail. I am waiting for those tests to complete at the moment.

@dawehner, @hussainweb: regarding #114.2, @znerol explained this in #78.

Not sure about #114.1

@borisson_: I saw that, but I couldn't find anything on why $scheme is not used here. Did I miss something?

@dawehner, @hussainweb: regarding #114.2, @znerol explained this in #78.

Well, so can we add this to the patch?

Here it is. I am not entirely convinced by the explanation (I mean the original explanation of setting $public to FALSE). What if something else changes the header down the line? This will cause the FinishResponseSubscriber to not touch the Cache-Control header again.

I found a case where just passing FALSE to $public fails. I have been trying out this in a testing issue and here are the final patches. To see the test which fails, see interdiff-119-120-test.txt. The interdiff with the fix is in interdiff-119-120.txt. The corresponding patches are attached as well. The -test.patch should fail in \Drupal\image\Tests\ImageStylesPathAndUrlTest.

EDIT: Fixed typo.

Thank you @hussainweb!

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -178,7 +178,11 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
+      // already modified. We pass in FALSE for the $public parameter to make
+      // sure we don't change the headers.

I think we need to adapt the comment now

Expected failures and passes. :)

Here is the change as per #121.

Alright, well I still think we could improve the comment to explain why we don't want to change the headers but meh.

RTBC + 1

Just a question:

Should we use $schema === 'public' for $public == TRUE or is private our only private scheme we use in Core?

Overall I think our file system works in the way that anyone regardless of scheme should be able to set headers with Cache-Control to public or private.

e.g. it is perfectly fine to change access to a public resource to have a Cache-Control of private.

Given that this is a pre-existing bug and an edge-case only crazy people like me need however, I don't think we need to solve that here.

But in the future it should be the right fix to just add $public = FALSE, which means in essence:

"Do not make this file explicitly public, but do nothing and leave the headers as is."

Overall that flag is quite badly named and should have never been used in the first place.

It might be also that the need for that flag was during one of the conversions, where we removed our explicit setting of Cache-Control: public, in which case the patch here is completely correct.

This was more an analysis of the changes and does not affect RTBC status.

New proposed commit message:

Issue #2476407 by borisson_, hussainweb, znerol, Fabianx, Wim Leers, dawehner, Crell, Berdir: Using CacheableResponseInterface to better determine which responses should be cached/cacheable

Updating commit credit per #125.

This patch makes a lot of sense, and better to break this before RC than after, so pushed to 8.0.x and published the CR.

Two requests:

• The CR doesn't make it clear that the 99% of controllers that return render arrays are unaffected. If someone could work that into the CR, I'd appreciate it.

• +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
  @@ -115,13 +115,30 @@ public function onRespond(FilterResponseEvent $event) {
  +    if (!$response instanceof CacheableResponseInterface) {
  +      if (!$this->isCacheControlCustomized($response)) {
  +        $this->setResponseNotCacheable($response, $request);
  +      }
  +
  +      // HTTP/1.0 proxies do not support the Vary header, so prevent any caching
  +      // by sending an Expires date in the past. HTTP/1.1 clients ignore the
  +      // Expires header if a Cache-Control: max-age directive is specified (see
  +      // RFC 2616, section 14.9.3).
  +      if (!$response->headers->has('Expires')) {
  +        $this->setExpiresNoCache($response);
  +      }
  +      return;
  +    }
  

  This seems like it's roughly (but not exactly) duplicating code that also exists elsewhere. Not sure if there's a way to refactor this method to have a bit less duplication, but if there is, please open a followup for that.

Thanks, updated the CR:

https://www.drupal.org/node/2579995/revisions/view/8958033/8958223