-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[2.2] [HttpFoundation] Added BinaryFileResponse. #4546
Conversation
public function setAutoLastModified() | ||
{ | ||
$this->setLastModified(\DateTime::createFromFormat('U', $this->file->getMTime())); | ||
return $this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline before return cs.sensiolabs.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Pushed.
// Use X-Sendfile, do not send any content. | ||
$this->headers->set($request->headers->get('X-Sendfile-Type'), $this->file->getPathname()); | ||
$this->maxlen = 0; | ||
} else if ($request->headers->has('Range')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra space here (running the PHP-CS-Fixer would fix it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. PHP-CS-Fixer looks really useful. Ran it and pushed.
/** | ||
* BinaryFileResponse represents an HTTP response delivering a file. | ||
* | ||
* @author Fabien Potencier <fabien@symfony.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be you (and maybe @stealth35 and/or @igorw) ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ... I thought that was the standard header. Tell me in case you want different or no credits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ... thanks! Correct like that, or with an e-mail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks man ;)
You can use the same email I put in #2606 if needed.
|
||
if ($autoValidation) { | ||
$this->setAutoLastModified(); | ||
$this->setAutoEtag(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Pushed.
What about support X-Accel-Redirect (nginx)? |
@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)? |
@niklasf Because nginx not use full file path, this need X-Accel-Mapping header (http://rack.rubyforge.org/doc/Rack/Sendfile.html) |
public function prepare(Request $request) | ||
{ | ||
$this->headers->set('Content-Length', $this->file->getSize()); | ||
$this->headers->set('Content-Type', $this->file->getMimeType() ?: 'application/octet-stream'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should check for existence of this header. The user might want to specify something himself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Pushed.
@Partugal: Alright. Doing such a substitution now. Also added a test for that. |
public function prepare(Request $request) | ||
{ | ||
$this->headers->set('Content-Length', $this->file->getSize()); | ||
$this->headers->set('Content-Type', $this->file->getMimeType() ?: 'application/octet-stream', true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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');
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ... indeed. Fixed.
I think the MIME should be base on the extensions map, for an example with Client to server : Reverve MIME => libmagic |
/** | ||
* {@inheritDoc} | ||
*/ | ||
public static function create($file, $status = 200, $headers = array(), $public = true, $autoEtag = false, $autoLastModified = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method leads to a E_STRICT error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted. Fixed. Thanks!
@partugal: Thanks! Also added tests. Any e-mail you want to have in your credits? |
@stealth35: Yeah ... makes sense. How would I get that information? |
} | ||
|
||
if ($filenameFallback === '') { | ||
$filenameFallback = rawurlencode($filename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Thanks. Fixed. |
if (!$request->headers->has('If-Range') || $this->getEtag() == $request->headers->get('If-Range')) { | ||
$range = $request->headers->get('Range'); | ||
|
||
list($start, $end) = array_map('intval', explode('-', substr($range, 6), 2)) + array(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stloyd: Sorry, I am not sure what lines above you mean or what exactly you would factor out :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind =) It seems like not worth change ;-)
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? |
I'm going to review the PRs marked for 2.2 after the release of 2.1 (next week). |
any news about merging this? |
*/ | ||
public function getContent() | ||
{ | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should return null
, not false
IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stof +1
@niklasf can you fix my comment ? and is there anything else left before merging or is it ready ? |
@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. |
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? |
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 |
@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. @Drak Absolutely. What would you propose? (see #4546 (comment)) |
@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. |
@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. |
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. |
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.
Merged! Thanks everyone for your hard work on this one. The remaining points should now be addressed in follow-up PRs. |
Another stab at #3602, based on @stealth35's code at https://gist.github.com/1472230.