add BinaryFileResponse #3602
Comments
|
#3375 also talks about adding this. |
|
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 |
|
As we have added JsonResponse, I think it belongs to the core as this is not easy to get it right. |
|
@fabpot +1 |
|
+1 |
|
@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 |
|
@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) |
|
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. 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. |
|
@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 |
|
@stof - Ah yes of course, my bad :) |
|
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? |
|
Thanks. So you're suggesting to use an |
|
You just need to specify the documentation that you need to explicitly set the XSendFileType header // true if you have use x headers detection, false by default
$response = new FileResponse('/var/www/1.txt', true);but imho its not needed |
|
other variants also have minuses:
|
|
just if you're sending files through php (without sendfile) you already have a problem |
|
Mhh ... yeah. Thanks, Partugal. |
|
unsupported options is ignored 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) only one situation is not covered: |
|
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? |
|
Request::trustSendfileType(); or allow specefy trusted headers: Request::setTrustedHeaders(array(
'HTTP_X_FORWARDED_FOR', 'X_SENDFILE_TYPE', // and more
)) |
|
Closing this issue as the discussion now happens in 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, thi 644A s 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.
Not sure if this belongs to the core but here is a previous discussion about adding support for simple binary file responses: see #2606
The text was updated successfully, but these errors were encountered: