[2.2] [HttpFoundation] Added BinaryFileResponse. by niklasf · Pull Request #4546 · symfony/symfony

niklasf · 2012-06-10T16:40:11Z

Another stab at #3602, based on @stealth35's code at https://gist.github.com/1472230.

Move things around a little, clean things up, looking how it has been done in StreamedResponse.
Add tests.
Make functions chainable.
Add a flag whether or not to trust the X-Sendfile-Type header.

henrikbjorn · 2012-06-10T16:45:00Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

Missing newline before return cs.sensiolabs.org

Thanks. Pushed.

travisbot · 2012-06-10T16:46:03Z

This pull request passes (merged 91341d82 into 9a5b275).

stof · 2012-06-10T16:48:21Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

extra space here (running the PHP-CS-Fixer would fix it)

Thanks. PHP-CS-Fixer looks really useful. Ran it and pushed.

travisbot · 2012-06-10T16:54:50Z

This pull request passes (merged eecad0b5 into 9a5b275).

travisbot · 2012-06-10T16:55:22Z

This pull request passes (merged 8ac6be6d into 9a5b275).

travisbot · 2012-06-10T16:58:18Z

This pull request passes (merged 18911f90 into 9a5b275).

travisbot · 2012-06-10T17:10:42Z

This pull request passes (merged 617c6614 into 9a5b275).

travisbot · 2012-06-10T18:10:47Z

This pull request passes (merged 7b2596cc into 7bec078).

jalliot · 2012-06-10T18:21:01Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

Should be you (and maybe @stealth35 and/or @igorw) ;)

Ah ... I thought that was the standard header. Tell me in case you want different or no credits.

The standard header is the copyright notice at the top of the file. The author information depends on the real author ;)
BTW, @stealth35's gist was actually based on my work in #2606 (which didn't have x-sendfile support) :)

Oh ... thanks! Correct like that, or with an e-mail?

Thanks man ;)
You can use the same email I put in #2606 if needed.

travisbot · 2012-06-10T18:45:08Z

This pull request passes (merged c66cce45 into 7bec078).

jalliot · 2012-06-10T18:46:39Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

Calculating the sha1 of the file can take a very long time depending on the size of the file. Not sure this is really needed in the general use case.
It would be good to split $autoValidation maybe: the lastModified header could be set by default and the ETag only if explicitly required.

Makes sense. Pushed.

linniksa · 2012-06-10T19:56:43Z

What about support X-Accel-Redirect (nginx)?

niklasf · 2012-06-10T20:41:10Z

@Partugal: So we support X-Sendfile-Type to pick the X-Sendfile header. What else would be needed to support X-Accel-Redirect (which we should definitely do)?

linniksa · 2012-06-10T21:29:41Z

@niklasf Because nginx not use full file path, this need X-Accel-Mapping header (http://rack.rubyforge.org/doc/Rack/Sendfile.html)

jalliot · 2012-06-10T22:23:37Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

You should check for existence of this header. The user might want to specify something himself.

Good idea. Pushed.

niklasf · 2012-06-10T22:45:38Z

@Partugal: Alright. Doing such a substitution now. Also added a test for that.

travisbot · 2012-06-10T22:48:04Z

This pull request passes (merged 15e7fc28 into 0995b1f).

travisbot · 2012-06-10T22:53:31Z

This pull request passes (merged 88dd7287 into 0995b1f).

jalliot · 2012-06-10T23:54:09Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

Actually, this does not change anything (I removed my initial comment because it was the wrong way to do it, and true is the default value).
You must do:

<?php if (!$this->headers->has('Content-Type')) { $this->headers->set('Content-Type', $this->file->getMimeType() ?: 'application/octet-stream'); }

Oh ... indeed. Fixed.

travisbot · 2012-06-11T03:20:56Z

This pull request passes (merged 98a07aed into 9a5b275).

stealth35 · 2012-06-11T07:47:35Z

I think the MIME should be base on the extensions map, for an example with xlsx that send an application/zip or a xlsx file MIME is application/vnd.openxmlformats-officedocument.spreadsheetml.sheet

Client to server : Reverve MIME => libmagic
Server to client : MIME => MIME map

stof · 2012-06-11T08:11:24Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

this method leads to a E_STRICT error

Well spotted. Fixed. Thanks!

niklasf · 2012-06-11T14:40:00Z

@partugal: Thanks! Also added tests. Any e-mail you want to have in your credits?

niklasf · 2012-06-11T14:41:39Z

@stealth35: Yeah ... makes sense. How would I get that information?

ruimarinho · 2012-07-18T14:20:59Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

When the original filename contains spaces, rawurlencode will convert those to %20, which in turns raises an exception (see ResponseHeaderBag). Maybe replacing spaces by underscores would be a good workaround?

Alright ... since also other characters are converted by rawurlencode, that one itself is probably a wrong default. How about having no custom logic here and introducing something better to https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/ResponseHeaderBag.php#L235, where I think it belongs? (In a seperate PR?)

I think that makes sense. Such sanity checking should be done as low-level as possible so that higher-level code doesn't need to re-implement it multiple times.

Agreed. I also wonder if it shouldn't just transliterate or ignore those non-ASCII characters when no fallback filename is explicitly provided. Unless the developer does this, it means that every malformatted filename will potentially prevent files from being downloaded.

jalliot · 2012-08-07T20:53:54Z

@niklasf You should backport the changes from 532334d and 3f51bc0

niklasf · 2012-08-07T21:07:10Z

@jalliot Thanks. Fixed.

travisbot · 2012-08-07T21:11:57Z

This pull request passes (merged 0393a7fc into 7dbadbf).

travisbot · 2012-08-07T21:18:54Z

This pull request passes (merged 7642f7c3 into 7dbadbf).

travisbot · 2012-08-08T05:42:32Z

This pull request passes (merged 7be713f3 into 7dbadbf).

travisbot · 2012-08-08T15:39:40Z

This pull request passes (merged 2f7bbbf into 7dbadbf).

stloyd · 2012-08-08T16:15:14Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

Here you use array_map + merge of additional array, few lines above you don't check that. IMO you could add small private method which could cover both cases.

@stloyd: Sorry, I am not sure what lines above you mean or what exactly you would factor out :/

Nevermind =) It seems like not worth change ;-)

Crell · 2012-08-29T05:43:29Z

It's not clear right now what work remains to be done on this PR. What's the blocker, so we can get this finished up and merged?

fabpot · 2012-08-29T05:45:23Z

I'm going to review the PRs marked for 2.2 after the release of 2.1 (next week).

linniksa · 2012-09-20T06:35:23Z

any news about merging this?

stof · 2012-10-13T17:05:19Z

src/Symfony/Component/HttpFoundation/BinaryFileResponse.php

It should return null, not false IMO

stof · 2012-10-13T17:09:12Z

@niklasf can you fix my comment ? and is there anything else left before merging or is it ready ?

niklasf · 2012-10-13T17:23:46Z

@stof Yes. That makes sense, especially because setContent() checks for null. The reason I did it like that is https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/StreamedResponse.php#L127. Should I change it anyway and maybe even open up a PR for StreamedResponse?

Also: I don't know of anything else that needs to be done before merging.

Burgov · 2012-10-16T09:38:33Z

This PR is pretty cool, but only supports sending physical files. I'm in the situation where I want to send files from Mongo GridFS with support for Partial Content (206).

A while back I made PR #5057 to enable support for this, which implements the 206 support on a more global level instead of only in one type of response. Can this be mixed somehow?

sstok · 2012-10-16T09:48:08Z

Maybe allowing setting an callback with setContent(), the callback will get the start and end of the requested range passed to it. Every time when sending content this callback will be executed and returns the content that must be send.

But it still would require an first call to set the total length of the content.

Or as the sendContent() already uses an stream, allowing set an custom stream instead of the
$file = fopen($this->file->getPathname(), 'rb'); one

niklasf · 2012-10-18T15:54:27Z

@sstok The first option you describe looks like https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/StreamedResponse.php. It doesn't yet support partial content (automatically), though.
The second option might be useful on it's own. The point we'd have to figure out: Curenntly we have convinience features that automatically use the filename or the last modification date of the file as headers. What is a clean and consistent way to support streams? Use the lowest common deminator for the constructor and the other features in chainable methods, that throw exceptions if not available?

@Drak Absolutely. What would you propose? (see #4546 (comment))

Crell · 2012-10-18T16:13:15Z

@sstok I think that's a separate issue. StreamedResponse can already do arbitrary streaming from arbitrary stream sources. BinaryFileResponse is a special case of that for when your payload is a physical file. Non-physical-files should already be covered by StreamedResponse. If there's something missing there, it should be added to that class.

sstok · 2012-10-18T18:37:18Z

@Crell I forgot about StreamedResponse :) but even then, StreamedResponse does not work with Content-Range.

Thinking about it, the current implementation already supports handling external files if it uses a custom stream wrapper. But not providing an open stream.

How about making an BinaryResponse class like StreamedResponce and then letting BinaryFileResponse extend that.

Crell · 2012-10-18T18:46:43Z

Hm. BinaryResponse extends StreamedResponse, and BinaryFileResponse extends BinaryResponse? I'd be OK with that, but I think that should be done in a follow-up PR. This issue already offers some nice new features we're waiting on in Drupal, and inserting an extra class in the hierarchy like that would not be a BC break.

@stealth35

This PR was merged into the master branch. Commits ------- 2f7bbbf [HttpFoundation] Added BinaryFileResponse. Discussion ---------- [2.2] [HttpFoundation] Added BinaryFileResponse. Another stab at #3602, based on @stealth35's code at https://gist.github.com/1472230. - Move things around a little, clean things up, looking how it has been done in StreamedResponse. - Add tests. - Make functions chainable. - Add a flag whether or not to trust the X-Sendfile-Type header. --------------------------------------------------------------------------- by Partugal at 2012-06-10T19:56:43Z What about support X-Accel-Redirect (nginx)? --------------------------------------------------------------------------- by niklasf at 2012-06-10T20:41:10Z @Partugal: So we support X-Sendfile-Type to pick the X-Sendfile header. What else would be needed to support X-Accel-Redirect (which we should definitely do)? --------------------------------------------------------------------------- by Partugal at 2012-06-10T21:29:41Z @niklasf Because nginx not use full file path, this need X-Accel-Mapping header (http://rack.rubyforge.org/doc/Rack/Sendfile.html) --------------------------------------------------------------------------- by niklasf at 2012-06-10T22:45:38Z @Partugal: Alright. Doing such a substitution now. Also added a test for that. --------------------------------------------------------------------------- by stealth35 at 2012-06-11T07:47:35Z I think the MIME should be base on the extensions map, for an example with `xlsx` that send an `application/zip` or a `xlsx` file MIME is `application/vnd.openxmlformats-officedocument.spreadsheetml.sheet` Client to server : Reverve MIME => libmagic Server to client : MIME => MIME map --------------------------------------------------------------------------- by niklasf at 2012-06-11T14:40:00Z @partugal: Thanks! Also added tests. Any e-mail you want to have in your credits? --------------------------------------------------------------------------- by niklasf at 2012-06-11T14:41:39Z @stealth35: Yeah ... makes sense. How would I get that information? --------------------------------------------------------------------------- by stealth35 at 2012-06-11T14:47:36Z use the `Symfony\Component\HttpFoundation\File\Mimetype\MimeTypeExtensionGuesser` it's the same map as Apache and if the extension don't exists use `$this->getMimeType` and finaly `application/octet-stream` --------------------------------------------------------------------------- by Partugal at 2012-06-11T15:46:41Z @niklasf Thanks you for your work If needed you may use linniksa@gmail.com --------------------------------------------------------------------------- by niklasf at 2012-06-14T10:58:19Z @stealth35: Sorry. I have to ask again. - So the first step would be using the map in `MimeTypeExtensionGuesser`? I don't see how I can access that, because the `guess()` method it has, is for guessing extensions from mime types, not the reverse. - Then, by `$this->getMimeType` you mean the getMimeType() method of the file? Sounds good. - `application/octet-stream` as the fallback. Alright. --------------------------------------------------------------------------- by stealth35 at 2012-06-14T11:00:33Z Yeah sorry `MimeTypeExtensionGuesser` is for getting an extension with the Mime, forget about this, i'll take care aboute all MIME intégration later --------------------------------------------------------------------------- by niklasf at 2012-06-14T13:12:22Z @stealth35: Awesome. Thanks a lot. --------------------------------------------------------------------------- by jalliot at 2012-08-07T20:53:54Z @niklasf You should backport the changes from 532334d and 3f51bc0 --------------------------------------------------------------------------- by niklasf at 2012-08-07T21:07:10Z @jalliot Thanks. Fixed.

fabpot · 2012-10-29T11:01:16Z

Merged! Thanks everyone for your hard work on this one.

The remaining points should now be addressed in follow-up PRs.

henrikbjorn reviewed Jun 10, 2012
View reviewed changes

stof reviewed Jun 10, 2012
View reviewed changes

jalliot reviewed Jun 10, 2012
View reviewed changes

stof reviewed Jun 11, 2012
View reviewed changes

fabpot mentioned this pull request Jul 13, 2012

add BinaryFileResponse #3602

Closed

ruimarinho reviewed Jul 18, 2012
View reviewed changes

fabpot mentioned this pull request Jul 26, 2012

[RFC] [HttpFoundation] Allow partial content to be sent #5057

Closed

[HttpFoundation] Added BinaryFileResponse.

2f7bbbf

stloyd reviewed Aug 8, 2012
View reviewed changes

stof reviewed Oct 13, 2012
View reviewed changes

fabpot merged commit 2f7bbbf into symfony:master Oct 29, 2012

fabpot mentioned this pull request Oct 29, 2012

Add documentation for the new BinaryFileResponse class symfony/symfony-docs#1866

Closed

niklasf mentioned this pull request Oct 29, 2012

[HttpFoundation] Return value of BinaryFileResponse::getContent() and StreamedResponse::getContent() #5864

Closed

jbafford mentioned this pull request Mar 30, 2013

Add documentation for the new BinaryFileResponse class (#1866) symfony/symfony-docs#2416

Merged

Basster mentioned this pull request Jul 13, 2023

[HttpFoundation] Allow BinaryFileResponse for non-successful responses #50966

Closed

Uh oh!

Conversation

niklasf commented Jun 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbot commented Jun 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbot commented Jun 10, 2012

Uh oh!

travisbot commented Jun 10, 2012

Uh oh!

travisbot commented Jun 10, 2012

Uh oh!

travisbot commented Jun 10, 2012

Uh oh!

travisbot commented Jun 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbot commented Jun 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linniksa commented Jun 10, 2012

Uh oh!

niklasf commented Jun 10, 2012

Uh oh!

linniksa commented Jun 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasf commented Jun 10, 2012

Uh oh!

travisbot commented Jun 10, 2012

Uh oh!

travisbot commented Jun 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbot commented Jun 11, 2012

Uh oh!

stealth35 commented Jun 11, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasf commented Jun 11, 2012

Uh oh!

niklasf commented Jun 11, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!