Posted by sreynen on May 4, 2012 at 3:49pm
18 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | file system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | kernel-followup, Proudly Found Elsewhere, WSCCI |
Issue Summary
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.
Comments
#1
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.
#2
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.
#3
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.
#4
So while this issue moves forward for Symfony we still have 13 instances of fread in Drupal. Should we fix these?
TargetsString '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));
#5
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.
#6
Retitling.
#7
Symfony should be all up to date now, so this issue can continue! Niklas, you still interested?
#8
Maybe we should be dropping file_transfer(), since now it would be a mere wrapper around returning a BinaryFileResponse.
#9
The last submitted patch, 1561362-binary-file-response.patch, failed testing.
#10
Forgot a few use statements and one comment block.
#11
The last submitted patch, 1561362-binary-file-response-10.patch, failed testing.
#12
#10: 1561362-binary-file-response-10.patch queued for re-testing.
#13
The last submitted patch, 1561362-binary-file-response-10.patch, failed testing.
#14
I think we just need a reroll here.
#15
Here's hoping that this one passes through without any random test fails...
#16
Missed the comment changes out from update_test.module.
#17
Typo in my comments for that module, sorry. Once more...
#18
The last submitted patch, binaryfileresponse_conversion-1561362-17.patch, failed testing.
#19
#17: binaryfileresponse_conversion-1561362-17.patch queued for re-testing.
#20
The last submitted patch, binaryfileresponse_conversion-1561362-17.patch, failed testing.
#21
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.');
#22
#23
The last submitted patch, binaryfileresponse_conversion-1561362-21.patch, failed testing.
#24
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?
#25
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:
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...
#26
This patch should (hopefully) fix the remaining error messages, which were stating that "PrivateStream::stream_cast is not implemented."
#27
+++ 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.
#28
The last submitted patch, binaryfileresponse_conversion-1561362-26.patch, failed testing.
#29
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) {
#30
Minor doc changes. I think this is good to go now.
#31
Unless the bot objects, I think #30 is good to go.
Thank you!!!
#32
+1RTBC for #30
small change
#33
Is this actually a feature..? This seems more like a straight refactoring task, or do I misunderstand?
#34
You are correct :)
#35
#30: 1561362-binary-file-response-30.patch queued for re-testing.
#36
The last submitted patch, 1561362-binary-file-response-30.patch, failed testing.
#37
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
@returnthough.)#38
Thanks for the review!
#39
And again.
#40
Committed and pushed to 8.x. Thanks!
This will need a change notice.
#41
#42
Created a change notice: http://drupal.org/node/1957078.
#43
Automatically closed -- issue fixed for 2 weeks with no activity.