Based on the discussion around file_transfer(): #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel.

8.x: file_transfer() is used by image_style_deliver() and file_download().

The kernel patch introduces StreamedResponses there to transfer files, inlining the code for streaming the files.

Proposed resolution

  • Make file_transfer() a helper function that returns a StreamedResponse.
  • Revert changes to core/includes/file.inc:
    diff --git a/core/includes/file.inc b/core/includes/file.inc
    index f204989..91e7eca 100644
    --- a/core/includes/file.inc
    +++ b/core/includes/file.incundefined
    @@ -5,6 +5,9 @@
      * API for handling file uploads and server file management.
      */

    +use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
    +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
    +use Symfony\Component\HttpFoundation\StreamedResponse;
    use Drupal\Core\StreamWrapper\LocalStream;

    /**
    @@ -2026,18 +2029,27 @@ function file_download() {
           $function = $module . '_file_download';
           $result = $function($uri);
           if ($result == -1) {
    -        return drupal_access_denied();
    +        throw new AccessDeniedHttpException();
           }
           if (isset($result) && is_array($result)) {
             $headers = array_merge($headers, $result);
           }
         }
         if (count($headers)) {
    -      file_transfer($uri, $headers);
    +      return new StreamedResponse(function() use ($uri) {
    +        $scheme = file_uri_scheme($uri);
    +        // Transfer file in 1024 byte chunks to save memory usage.
    +        if ($scheme && file_stream_wrapper_valid_scheme($scheme) && $fd = fopen($uri, 'rb')) {
    +          while (!feof($fd)) {
    +            print fread($fd, 1024);
    +          }
    +          fclose($fd);
    +        }
    +      }, 200, $headers);
         }
    -    return drupal_access_denied();
    +    throw new AccessDeniedHttpException();
       }
    -  return drupal_not_found();
    +  throw new NotFoundHttpException();
    }

  • Use the new file_transfer() in core/modules/update/tests/modules/update_test/update_test.module, instead of creating a custom StreamedResponse. Or is it for managed files only?
    -  readfile("$path/$project_name.$availability_scenario.xml");
    +  $file = "$path/$project_name.$availability_scenario.xml";
    +  if (!is_file($file)) {
    +    // Return an empty response.
    +    return new Response('', 200, array('Content-Type' =>  'text/xml; charset=utf-8'));
    +  }
    +  return new StreamedResponse(function() use ($file) {
    +    // Transfer file in 1024 byte chunks to save memory usage.
    +    if ($fd = fopen($file, 'rb')) {
    +      while (!feof($fd)) {
    +        print fread($fd, 1024);
    +      }
    +      fclose($fd);
    +    }
    +  }, 200, array('Content-Type' =>  'text/xml; charset=utf-8'));

Also note #1561362: Change file_transfer() to use BinaryFileResponse that is about making a StreamedFileResponse or another replacement, or something in Symfony upstream.

Comments

Niklas Fiekas’s picture

Title:Use file_transfer() as a helper functions instead of inlining file transfers» Use file_transfer() as a helper function instead of inlining file transfers
Niklas Fiekas’s picture

Issue summary:View changes

Updated issue summary.

Dave Reid’s picture

I don't understand why we can't simplify the kernel branch right now with this change? We're making more work and the kernel patch harder to review, only to undo it later.

Dave Reid’s picture

Status:Active» Fixed

Resolved with http://drupalcode.org/sandbox/Crell/1260830.git/commit/6340061 on the wscci kernel branch.

Status:Fixed» Closed (fixed)

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

Anonymous’s picture

Issue summary:View changes

.