Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Niklas Fiekas’s picture

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.

Crell’s picture

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.

Niklas Fiekas’s picture

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.

cosmicdreams’s picture

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));
Crell’s picture

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.

Crell’s picture

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

Retitling.

Crell’s picture

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

Niklas Fiekas’s picture

Status: Active » Needs review
FileSize
2.41 KB

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.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
2.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.

Anonymous’s picture

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.

Crell’s picture

Issue tags: +Needs reroll

I think we just need a reroll here.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
3.29 KB

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

Anonymous’s picture

Missed the comment changes out from update_test.module.

Anonymous’s picture

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.

RoSk0’s picture

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.

mtift’s picture

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.');
mtift’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Crell’s picture

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?

mtift’s picture

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...

mtift’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
6.06 KB

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

mtift’s picture

+++ 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.

Niklas Fiekas’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.05 KB

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) {
Niklas Fiekas’s picture

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

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

Thank you!!!

podarok’s picture

+1RTBC for #30
small change

webchick’s picture

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

amateescu’s picture

Category: feature » task

You are correct :)

xjm’s picture

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.

xjm’s picture

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.)

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
930 bytes
5.9 KB

Thanks for the review!

Crell’s picture

Status: Needs review » Reviewed & tested by the community

And again.

webchick’s picture

Title: Change file_transfer() to use BinaryFileResponse » Change 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.

webchick’s picture

Priority: Normal » Critical
Niklas Fiekas’s picture

Title: Change notice: Change file_transfer() to use BinaryFileResponse » Change 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.

Anonymous’s picture

Issue summary: View changes

Add link to the function.