This function has used fread() since at least D5, with a comment explaining:

// Transfer file in 1024 byte chunks to save memory usage.

But according to Crell, that reason is based on a myth. Apparently the memory usage is the same, and readfile() is faster.

Files: 
CommentFileSizeAuthor
#38 1561362-binary-file-response-38.patch5.9 KBNiklas Fiekas
PASSED: [[SimpleTest]]: [MySQL] 53,436 pass(es).
[ View ]
#38 1561362-binary-file-response-38-interdiff.txt930 bytesNiklas Fiekas
#30 1561362-binary-file-response-30.patch5.8 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#30 1561362-binary-file-response-30-interdiff.txt2.37 KBNiklas Fiekas
#29 binary-file-response-conversion-1561362-29.patch6.05 KBNiklas Fiekas
PASSED: [[SimpleTest]]: [MySQL] 53,228 pass(es).
[ View ]
#27 binaryfileresponse_conversion-1561362-26.patch6.06 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 53,218 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#26 binaryfileresponse_conversion-1561362-26.patch6.06 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 53,161 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#26 interdiff.txt2.25 KBmtift
#21 binaryfileresponse_conversion-1561362-21.patch5.11 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 53,209 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#17 binaryfileresponse_conversion-1561362-17.patch3.85 KBchrisdolby
FAILED: [[SimpleTest]]: [MySQL] 53,127 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#16 binaryfileresponse_conversion-1561362-16.patch3.85 KBchrisdolby
FAILED: [[SimpleTest]]: [MySQL] 51,358 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#15 binaryfileresponse_conversion-1561362-14.patch3.29 KBchrisdolby
FAILED: [[SimpleTest]]: [MySQL] 51,344 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#10 1561362-binary-file-response-10-interdiff.txt2.13 KBNiklas Fiekas
#10 1561362-binary-file-response-10.patch3.9 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] 51,313 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#8 1561362-binary-file-response.patch2.41 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] 49,962 pass(es), 93 fail(s), and 1 exception(s).
[ View ]

Comments

Intresting. When we have #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel we can use StreamedResponses and might even remove file_transfer(). Not sure if it's worth doing this now.

You guys are thinking along the same lines I am. ;-)

I actually would like to see a subclass of StreamedResponse that is added upstream in Symfony that you pass a file name to, or a file stream, and it internally decides if it should use X-Sendfile, readfile(), fpassthru(), etc. Then we can replace our code with that.

I was planning to work on that soon now that the article is published, but if one of you wants to send that PR upstream to Symfony first, go for it. Just keep me in the loop so we don't duplicate work. :-)

I don't think it's worth changing now until we get further into kernel-land.

I tried to start a discussion about this in the Symfony queue: https://github.com/symfony/symfony/pull/4323. Generally there might be a BinaryFileResponse in Symfony at some point (but that requires some work and time). There's also https://github.com/igorw/IgorwFileServeBundle that got mentioned on the pull request, that supports pure php, sendfile and xsendfile via DI. We should probably evaluate that.

So while this issue moves forward for Symfony we still have 13 instances of fread in Drupal. Should we fix these?

Targets
    String 'fread'
Found usages  (14 usages)
    Unclassified usage  (14 usages)
        d8  (14 usages)
            C:\Users\cweber\Sites\d8\core\includes  (3 usages)
                bootstrap.inc  (1 usage)
                    (1975: 17) $bytes .= fread($fh, max(4096, $count));
                common.inc  (1 usage)
                    (913: 14) $chunk = fread($fp, 1024);
                file.inc  (1 usage)
                    (1348: 15) print fread($fd, 1024);
            C:\Users\cweber\Sites\d8\core\lib\Drupal\Component\Archiver  (6 usages)
                ArchiveTar.php  (6 usages)
                    (118: 29) $data = fread($fp, 2);
                    (706: 33) while ($v_data = @fread($v_file_from, 1024))
                    (853: 27) $v_block = @fread($this->_file, 512);
                    (1012: 31) while (($v_buffer = fread($v_file, 512)) != '') {
                    (1765: 17) if (fread($this->_file, 512) == ARCHIVE_TAR_END_BLOCK) {
                    (1768: 21) elseif (fread($this->_file, 512) == ARCHIVE_TAR_END_BLOCK) {
            C:\Users\cweber\Sites\d8\core\lib\Drupal\Core\StreamWrapper  (2 usages)
                LocalStream.php  (2 usages)
                    (232: 18) * Support for fread(), file_get_contents() etc.
                    (243: 12) return fread($this->handle, $count);
            C:\Users\cweber\Sites\d8\core\modules\aggregator\tests  (1 usage)
                aggregator_test.module  (1 usage)
                    (54: 11) $feed = fread($handle, filesize($file_name));
            C:\Users\cweber\Sites\d8\core\modules\openid  (1 usage)
                openid.inc  (1 usage)
                    (545: 14) $bytes = fread($f, $num_bytes);
            C:\Users\cweber\Sites\d8\core\vendor\symfony\http-foundation\Symfony\Component\HttpFoundation\Tests  (1 usage)
                RequestTest.php  (1 usage)
                    (692: 33) $this->assertEquals("", fread($retval, 1));

The Symfony BinaryFileResponse PR has been merged, so we can start using it as soon as #1834594: Update dependencies (Symfony and Twig) follow up fixes for Composer goes in.

Title:Change file_transfer() to use readfile()Change file_transfer() to use BinaryFileResponse

Retitling.

Symfony should be all up to date now, so this issue can continue! Niklas, you still interested?

Status:Active» Needs review
StatusFileSize
new2.41 KB
FAILED: [[SimpleTest]]: [MySQL] 49,962 pass(es), 93 fail(s), and 1 exception(s).
[ View ]

Maybe we should be dropping file_transfer(), since now it would be a mere wrapper around returning a BinaryFileResponse.

Status:Needs review» Needs work

The last submitted patch, 1561362-binary-file-response.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.9 KB
FAILED: [[SimpleTest]]: [MySQL] 51,313 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
new2.13 KB

Forgot a few use statements and one comment block.

Status:Needs review» Needs work
Issue tags:-WSCCI, -Proudly Found Elsewhere, -kernel-followup

The last submitted patch, 1561362-binary-file-response-10.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+WSCCI, +Proudly Found Elsewhere, +kernel-followup

The last submitted patch, 1561362-binary-file-response-10.patch, failed testing.

Issue tags:+Needs reroll

I think we just need a reroll here.

Status:Needs work» Needs review
StatusFileSize
new3.29 KB
FAILED: [[SimpleTest]]: [MySQL] 51,344 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

Here's hoping that this one passes through without any random test fails...

StatusFileSize
new3.85 KB
FAILED: [[SimpleTest]]: [MySQL] 51,358 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

Missed the comment changes out from update_test.module.

StatusFileSize
new3.85 KB
FAILED: [[SimpleTest]]: [MySQL] 53,127 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

Typo in my comments for that module, sorry. Once more...

Status:Needs review» Needs work
Issue tags:-WSCCI, -Proudly Found Elsewhere, -kernel-followup, -Needs reroll

The last submitted patch, binaryfileresponse_conversion-1561362-17.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+WSCCI, +Proudly Found Elsewhere, +kernel-followup, +Needs reroll

The last submitted patch, binaryfileresponse_conversion-1561362-17.patch, failed testing.

StatusFileSize
new5.11 KB
FAILED: [[SimpleTest]]: [MySQL] 53,209 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

The most recent patch failed twice when ImageStylesPathAndUrlTest added a 'private' scheme and tested for 'no-cache, private' on line 153.

$this->assertEqual($this->drupalGetHeader('Cache-Control'), 'no-cache, private', 'Cache-Control header was set to prevent caching.');

The 'Cache-Control' header being sent is 'must-revalidate, no-cache, post-check=0, pre-check=0, private', which seems to be a combination of the $default_header defined in bootstrap.inc + 'private'.

$default_headers = array(
    'Expires' => 'Sun, 19 Nov 1978 05:00:00 GMT',
    'Last-Modified' => gmdate(DATE_RFC1123, REQUEST_TIME),
    'Cache-Control' => 'no-cache, must-revalidate, post-check=0, pre-check=0',
    'ETag' => '"' . REQUEST_TIME . '"',
  );

I can make this individual test pass by checking for those three additional headers (must-revalidate, post-check=0, and pre-check=0), but it seems like changing the tests might break other things. So I'm not sure if the solution is changing the test or changing the headers, but I've attached a new patch that changes the test to see if that breaks other stuff:

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageStylesPathAndUrlTest.php
-      $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'no-cache, private', 'Cache-Control header was set to prevent caching.');
+      $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'must-revalidate, no-cache, post-check=0, pre-check=0, private', 'Cache-Control header was set to prevent caching.');

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, binaryfileresponse_conversion-1561362-21.patch, failed testing.

Switching to Symfony responses changes a lot of our headers, so any tests that rely on precise header strings are going to break. In such cases, manually verify that the headers that are getting sent are what we want them to be, then adjust the test to confirm that. Perhaps just a substr() check for no-cache?

Given the available assertions, I haven't come up with a more elegant way to check for the existence of "no-cache" that is better than something like this, which is a double negative of sorts:

// Checking that the 'Cache-Control' header contains 'no-cache'
$this->assertNotEqual(strpos($this->drupalGetHeader('Cache-Control'), 'no-cache'), FALSE, 'Cache-Control header was set to prevent caching.');

Now I'm trying to track down the source of the other errors:

finfo::file(): Drupal\Core\StreamWrapper\PrivateStream::stream_cast is not implemented!

It seems a rather cryptic message given that the text "stream_cast" does not exist in the Drupal codebase (based on my grepping). But I'm working on it...

Status:Needs work» Needs review
StatusFileSize
new2.25 KB
new6.06 KB
FAILED: [[SimpleTest]]: [MySQL] 53,161 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

This patch should (hopefully) fix the remaining error messages, which were stating that "PrivateStream::stream_cast is not implemented."

StatusFileSize
new6.06 KB
FAILED: [[SimpleTest]]: [MySQL] 53,218 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

+++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.phpundefined
@@ -339,6 +339,24 @@ public function stream_close() {
+   * Fixes warnings messages like this:
+   *   finfo::file() [finfo.file]: Drupal\Core\StreamWrapper\PrivateStream::stream_cast is not implemented!
+[TAB was here]

Removed a rogue tab.

Status:Needs review» Needs work

The last submitted patch, binaryfileresponse_conversion-1561362-26.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new6.05 KB
PASSED: [[SimpleTest]]: [MySQL] 53,228 pass(es).
[ View ]

Minor fix: Type hints can't be used for primitives like int.

-  public function stream_cast(int $cast_as) {
+  public function stream_cast($cast_as) {

StatusFileSize
new2.37 KB
new5.8 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Minor doc changes. I think this is good to go now.

Status:Needs review» Reviewed & tested by the community

Unless the bot objects, I think #30 is good to go.

Thank you!!!

+1RTBC for #30
small change

Is this actually a feature..? This seems more like a straight refactoring task, or do I misunderstand?

Category:feature» task

You are correct :)

Status:Reviewed & tested by the community» Needs work
Issue tags:+WSCCI, +Proudly Found Elsewhere, +kernel-followup

The last submitted patch, 1561362-binary-file-response-30.patch, failed testing.

Nice patch! However, the docblock for the stream_cast() method doesn't quite meet the minimum documentation requirements.

+++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.phpundefined
@@ -339,6 +339,20 @@ public function stream_close() {
+   * Signals that stream_select() is not supported by returning false.
+   *
+   * @param int
+   *   Can be STREAM_CAST_FOR_SELECT or STREAM_CAST_AS_STREAM.
+   *
+   * @return false
+   *
+   * @see http://php.net/manual/en/streamwrapper.stream-cast.php

The @param needs to have the variable after the datatype, and the return value needs documentation.
http://drupal.org/node/1354#param
http://drupal.org/node/1354#return

Also, minor: In the summary, FALSE should be all caps. (Lowercase is correct for the @return though.)

Status:Needs work» Needs review
StatusFileSize
new930 bytes
new5.9 KB
PASSED: [[SimpleTest]]: [MySQL] 53,436 pass(es).
[ View ]

Thanks for the review!

Status:Needs review» Reviewed & tested by the community

And again.

Title:Change file_transfer() to use BinaryFileResponseChange notice: Change file_transfer() to use BinaryFileResponse
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed and pushed to 8.x. Thanks!

This will need a change notice.

Priority:Normal» Critical

Title:Change notice: Change file_transfer() to use BinaryFileResponseChange file_transfer() to use BinaryFileResponse
Priority:Critical» Normal
Status:Active» Fixed
Issue tags:-Needs change record

Created a change notice: http://drupal.org/node/1957078.

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

Issue summary:View changes

Add link to the function.