Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

add BinaryFileResponse #3602

Closed
fabpot opened this Issue · 22 comments

6 participants

@fabpot
Owner

Not sure if this belongs to the core but here is a previous discussion about adding support for simple binary file responses: see #2606

@fabpot
Owner

#3375 also talks about adding this.

@stof
Collaborator

One question is whether this should be added to the core or if we should link to IgorwFileServeBundle instead which already provides such features (and improve it according to the discussion on the PR)

cc @igorw

@fabpot
Owner

As we have added JsonResponse, I think it belongs to the core as this is not easy to get it right.

@fixe

+1

@ghost

@fabpot - I think we should do something like this which makes the component folder look much cleaner while maintaining BC (and even could make FC in 2.0) - https://github.com/drak/symfony/compare/response you can see how nice it looks by browsing the tree: https://github.com/drak/symfony/tree/response/src/Symfony/Component/HttpFoundation

@stof
Collaborator

@drak It does not maintain BC. As the BC class is extending the new class, it is BC only if you use the child class everywhere in the code (because of typehintings using the 2.0.x class names) and if the code uses this class, the user has to include the BC file everytime (which is also a BC break)

@ghost

Are we possibly talking cross terminology? I am meaning that apps which switch to 2.1 will continue to work. I do not think it is reasonable to have BC for things in a development branch which were introduced in the development branch and are unreleased.

What I've drafted is an easy way to introduce a legacy layer that ca be turned on and off. Anything using the old classes will continue to work, including type hinting - e.g. Symfony\Component\HttpFoundation\Request will work since that extends the moved class. Yes it does mean eventual refactor of code, but that is the point of a BC classes, they give time for the inevitable.

I think it's also a reasonable assumption that anyone upgrading to Symfony 2.1 will have to update their autoloader.php, that's a given, so there is no extra work here - the BC is just more clean than having empty stub classes but amounts to the same thing.

Please note the code I have referenced is not complete, it's just a quick move of files + legacy stub without the other necessary tweaks in order to demonstrate an idea.

@stof
Collaborator

@drak Any bundle typehinting the Request object somewhere would be broken if Symfony uses your new namespace for the class, meaning that Symfony will need to continue using the (non autoloadable) BC class to avoid a BC break
The inheritance works the other way: any typehint of the parent class will work if you pass the child class, not the opposite.

@ghost

@stof - Ah yes of course, my bad :)

@niklasf

I'd go for an iteration on this, but I am not sure how we should decide if X-Sendfile, X-Accel-Redirect, X-LIGHTTPD-send-file or native PHP should be used. How could this be done?

@niklasf

Thanks. So you're suggesting to use an X-Sendfile-Type header to make that decision. This has one flaw: On a server that doesn't support X-Sendfile, a malicious user could send that header himself. Then he gets a response with X-Sendfile and the path to the file. If we do not consider that path to be secret, it's ok, but that assumption could be a problem, especially with absolute paths.

@linniksa

You just need to specify the documentation that you need to explicitly set the XSendFileType header
or add parameter like this:

 // true if you have use x headers detection, false by default
$response = new FileResponse('/var/www/1.txt', true);

but imho its not needed

@linniksa

other variants also have minuses:
1. set send file type in config - more dangerous to file not actually send when changing some setting (for example move to another server, wrong module after update in apache, or some changes in lighttpd config)
2. secret key - really overkill in this situation

@linniksa

just if you're sending files through php (without sendfile) you already have a problem

@niklasf

Mhh ... yeah. Thanks, Partugal.
Now: I have a friend, who has a friend, who only has shared hosting ;) (The plan is to make this available for Drupal.)
And then: Should the caller decide about the method to deliver the file? This line of thought leads to dependency injection. That would be very close to what IgorwFileServeBundle is doing.
(Edited this around a little, made a couple false assumptions.)

@linniksa

unsupported options is ignored
if you properly set header no one will be able to override it
x-accel-redirect in nginx not use full file path, it use private locations described in config. in IgorwFileServeBundle i'm not see any code for this
doubling locations in 2 configs is bad pratice

ok for shared hosting you may not able to set headers, solution is add option like for esi (esi by the way have same problem)

framework:
    sendfile: ~ # only enable sendfile support, type of senfile set via headers

only one situation is not covered:
shared hosting with mod_sendfile and without able to set header

@Crell

If we have to fall back to manually specifying whether or not to use sendfile because auto-detecting it is too difficult, I think it would be better to follow the model set by Request::trustProxyData() and have something like FileResponse::useSendfile($use = TRUE); It's something that's going to be true or not for the entire request, period, so making a developer specify it every time (which means they either won't or will have to build up a DI setup on top of it) is a waste.

@fabpot, would you be OK with a manual flag for that?

@linniksa

Request::trustSendfileType();

or allow specefy trusted headers:


Request::setTrustedHeaders(array(
'HTTP_X_FORWARDED_FOR', 'X_SENDFILE_TYPE', // and more
))
@Crell

Just talked to @fabpot at Symfony Live. A static method that sets a trust flag, similar to on the Request method, should be fine. That's the tricky part here. @niklasf, can you update the PR accordingly?

@fabpot
Owner

Closing this issue as the discussion now happens in PR #4546.

@fabpot fabpot closed this
@fabpot fabpot referenced this issue from a commit
@fabpot fabpot merged branch niklasf/binary-file-response (PR #4546)
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.
7322696
@mmucklo mmucklo referenced this issue from a commit
@fabpot fabpot merged branch niklasf/binary-file-response (PR #4546)
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.
665f094
@hackzilla hackzilla referenced this issue from a commit
@fabpot fabpot merged branch niklasf/binary-file-response (PR #4546)
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.
707e289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.