Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment #1
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedIntresting. 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.
Comment #2
Crell CreditAttribution: Crell commentedYou 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.
Comment #3
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedI 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.
Comment #4
cosmicdreams CreditAttribution: cosmicdreams commentedSo while this issue moves forward for Symfony we still have 13 instances of fread in Drupal. Should we fix these?
Comment #5
Crell CreditAttribution: Crell commentedThe 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.
Comment #6
Crell CreditAttribution: Crell commentedRetitling.
Comment #7
Crell CreditAttribution: Crell commentedSymfony should be all up to date now, so this issue can continue! Niklas, you still interested?
Comment #8
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMaybe we should be dropping file_transfer(), since now it would be a mere wrapper around returning a BinaryFileResponse.
Comment #10
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedForgot a few use statements and one comment block.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commented#10: 1561362-binary-file-response-10.patch queued for re-testing.
Comment #14
Crell CreditAttribution: Crell commentedI think we just need a reroll here.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedHere's hoping that this one passes through without any random test fails...
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedMissed the comment changes out from update_test.module.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedTypo in my comments for that module, sorry. Once more...
Comment #19
RoSk0#17: binaryfileresponse_conversion-1561362-17.patch queued for re-testing.
Comment #21
mtiftThe most recent patch failed twice when ImageStylesPathAndUrlTest added a 'private' scheme and tested for 'no-cache, private' on line 153.
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'.
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:
Comment #22
mtiftComment #24
Crell CreditAttribution: Crell commentedSwitching 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?
Comment #25
mtiftGiven 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:
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...
Comment #26
mtiftThis patch should (hopefully) fix the remaining error messages, which were stating that "PrivateStream::stream_cast is not implemented."
Comment #27
mtiftRemoved a rogue tab.
Comment #29
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMinor fix: Type hints can't be used for primitives like int.
Comment #30
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMinor doc changes. I think this is good to go now.
Comment #31
Crell CreditAttribution: Crell commentedUnless the bot objects, I think #30 is good to go.
Thank you!!!
Comment #32
podarok+1RTBC for #30
small change
Comment #33
webchickIs this actually a feature..? This seems more like a straight refactoring task, or do I misunderstand?
Comment #34
amateescu CreditAttribution: amateescu commentedYou are correct :)
Comment #35
xjm#30: 1561362-binary-file-response-30.patch queued for re-testing.
Comment #37
xjmNice patch! However, the docblock for the
stream_cast()
method doesn't quite meet the minimum documentation requirements.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.)Comment #38
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for the review!
Comment #39
Crell CreditAttribution: Crell commentedAnd again.
Comment #40
webchickCommitted and pushed to 8.x. Thanks!
This will need a change notice.
Comment #41
webchickComment #42
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedCreated a change notice: http://drupal.org/node/1957078.
Comment #43.0
(not verified) CreditAttribution: commentedAdd link to the function.