Private file and image style downloads throw 500 error when page cache is active

This is also true of image style downloads, the first time they are generated. I was really hoping there was a better way to fix this than to explicitly check for certain response types before serving from the cache, but for right now, it looks like there isn't. See https://github.com/symfony/symfony/issues/9128

Rerolled and added an exclusion for StreamedResponse as per the Symfony issue linked above. I also moved the exclusions into drupal_set_page_cache, since that's where the determination of the page's cacheability is currently happening (drupal_page_is_cacheable()). Last but not least, I removed the previous changes to the x-drupal-cache headers, which didn't seem to be needed. Let's see if this passes.

Rerolled based on znerol's patch in comment 8 (please ignore the one I submitted in between), and just added the StreamedResponse as I mentioned in comment 13. Interdiff is against the patch in comment 8.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -81,7 +83,7 @@ public function onRespond(FilterResponseEvent $event) {
   -    $is_cacheable = drupal_page_is_cacheable();
   +    $is_cacheable = drupal_page_is_cacheable() && !($response instanceof BinaryFileResponse) && !($response instanceof StreamedResponse);
   

   Sadly there is no interface or method we can call but indeed have to hardcode it here. It would be great to explain why we do this here.

2. +++ b/core/modules/file/lib/Drupal/file/Tests/DownloadTest.php
   @@ -53,7 +53,27 @@ function testPublicFileTransfer() {
   +  public function testPrivateFileTransfer() {
   ...
   +  public function testPrivateFileTransferWithPageCache() {
   

   I wonder whether we could name the first one like "testPrivteFileTransferWithoutPageCache"

Here is a reroll for psr-4. I also tested this manually as I just hit the same issue. I think this patch looks good.

25: 2218463-25.patch queued for re-testing.

Reroll.

Comment makes sense, tests fail without the fix, let's get this in.

The CDN module in Drupal 7 has a "Far Future expiration" setting. In short, it serves a file with optimal caching headers. Including binary files.

I agree with this:

As a consequence the page cache is active for private file downloads in Drupal 8 while it was not on D7 and therefor we need to disable it explicitely on private file downloads.

But AFAICT this patch affects *all* binary responses, not just those for private files and image styles?

I think that the overarching problem is this:

 if ($is_cacheable && $this->config->get('cache.page.max_age') > 0) {
      if (!$this->isCacheControlCustomized($response)) {
        $this->setResponseCacheable($response, $request);
      }
    }
    else {
      $this->setResponseNotCacheable($response, $request);
    }

    // Store the response in the internal page cache.
    if ($is_cacheable && $this->config->get('cache.page.use_internal')) {
      drupal_page_set_cache($response, $request);
      …

$is_cacheable controls both whether caching headers will be set and whether the response is going to be stored in the page cache. It should be possible to have caching headers without storing the response in the page cache.

Hmm, that's a very good point. So we need to do something like this instead?

EDIT: Quick patch disclaimer.

1. +++ b/core/includes/common.inc
   @@ -2684,8 +2686,20 @@ function _drupal_bootstrap_full($skip = FALSE) {
   + *   Returns TRUE if the response has been recorded in the cache, FALSE
   

   Not sure about using 'recorded'. Saved, stored, set.. or something.

2. +++ b/core/includes/common.inc
   @@ -2684,8 +2686,20 @@ function _drupal_bootstrap_full($skip = FALSE) {
   +    return FALSE;
   
   @@ -2709,6 +2723,8 @@ function drupal_page_set_cache(Response $response, Request $request) {
   +  return TRUE;
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -100,8 +94,7 @@ public function onRespond(FilterResponseEvent $event) {
   +    if ($is_cacheable && $this->config->get('cache.page.use_internal') && drupal_page_set_cache($response, $request)) {
   

   It is not the best thing relying on the return value of a function known for just setting. Works for me though.

   Although this does not really just seem like a restriction from the backend. This is more a flaw/shortcoming in how Symfony deals with binary and streamed responses? Also looks like this is not likely to change at any point soon.

That looks much better, thank you! :)

Yep +1 on the latest patch. Includes everything we have discussed from #30 onwards.

Committed 0dc696d and pushed to 8.x. Thanks!