[2.2] [HttpFoundation] Added BinaryFileResponse. #4546

Merged
merged 1 commit into from Oct 29, 2012

Conversation

Projects
None yet
Contributor

niklasf commented Jun 10, 2012

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 henrikbjorn and 1 other commented on an outdated diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ * Gets the file.
+ *
+ * @return File The file to stream
+ */
+ public function getFile()
+ {
+ return $this->file;
+ }
+
+ /**
+ * Automatically sets the Last-Modified header according the file modification date.
+ */
+ public function setAutoLastModified()
+ {
+ $this->setLastModified(\DateTime::createFromFormat('U', $this->file->getMTime()));
+ return $this;
@henrikbjorn

henrikbjorn Jun 10, 2012

Contributor

Missing newline before return cs.sensiolabs.org

@niklasf

niklasf Jun 10, 2012

Contributor

Thanks. Pushed.

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

@stof stof and 1 other commented on an outdated diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ $this->headers->set('Content-Type', $this->file->getMimeType() ?: 'application/octet-stream');
+ $this->headers->set('Accept-Ranges', 'bytes');
+ $this->headers->set('Content-Transfer-Encoding', 'binary');
+
+ if ('1.0' != $request->server->get('SERVER_PROTOCOL')) {
+ $this->setProtocolVersion('1.1');
+ }
+
+ $this->offset = 0;
+ $this->maxlen = -1;
+
+ if ($request->headers->has('X-Sendfile-Type') && self::$trustXSendfileTypeHeader) {
+ // 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')) {
@stof

stof Jun 10, 2012

Member

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

@niklasf

niklasf Jun 10, 2012

Contributor

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

@stof stof commented on the diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ public function sendContent()
+ {
+ if (!$this->isSuccessful()) {
+ parent::sendContent();
+ return;
+ }
+
+ $out = fopen('php://output', 'wb');
+ $file = fopen($this->file->getPathname(), 'rb');
+
+ stream_copy_to_stream($file, $out, $this->maxlen, $this->offset);
+
+ fclose($out);
+ fclose($file);
+ }
+
@stof

stof Jun 10, 2012

Member

it should be {@inheritDoc}

@niklasf

niklasf Jun 10, 2012

Contributor

Fixed. Thanks.

@Tobion

Tobion Jun 14, 2012

Member

{@inheritdoc} would be entirely correct.

@niklasf

niklasf Jun 14, 2012

Contributor

Alright, thanks. Fixed.

@stof stof commented on the diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ throw new \LogicException('The content cannot be set on a BinaryFileResponse instance.');
+ }
+ }
+
+ /**
+ * @{inheritDoc}
+ *
+ * return false
+ */
+ public function getContent()
+ {
+ return false;
+ }
+
+ /**
+ * Trust X-Sendfile-Type header.
@stof

stof Jun 10, 2012

Member

the curly brace should be on its own line

@niklasf

niklasf Jun 10, 2012

Contributor

Fixed.

@stof stof commented on the diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ }
+ }
+
+ /**
+ * @{inheritDoc}
+ *
+ * return false
+ */
+ public function getContent()
+ {
+ return false;
+ }
+
+ /**
+ * Trust X-Sendfile-Type header.
+ */
@stof

stof Jun 10, 2012

Member

broken indentation (using tabs ?)

@niklasf

niklasf Jun 10, 2012

Contributor

Accidantely used Drupal 2 space indentation. Fixed.

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

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

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

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

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

@jalliot jalliot and 1 other commented on an outdated diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpFoundation;
+
+use Symfony\Component\HttpFoundation\File\File;
+use Symfony\Component\HttpFoundation\File\Exception\FileException;
+
+/**
+ * BinaryFileResponse represents an HTTP response delivering a file.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
@jalliot

jalliot Jun 10, 2012

Contributor

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

@niklasf

niklasf Jun 10, 2012

Contributor

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

@jalliot

jalliot Jun 10, 2012

Contributor

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) :)

@niklasf

niklasf Jun 10, 2012

Contributor

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

@jalliot

jalliot Jun 10, 2012

Contributor

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

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

@jalliot jalliot and 1 other commented on an outdated diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ *
+ * @param SplFileInfo|string $file The file to stream
+ */
+ public function setFile($file, $autoValidation = true)
+ {
+ $file = new File((string) $file);
+
+ if (!$file->isReadable()) {
+ throw new FileException('File must be readable.');
+ }
+
+ $this->file = $file;
+
+ if ($autoValidation) {
+ $this->setAutoLastModified();
+ $this->setAutoEtag();
@jalliot

jalliot Jun 10, 2012

Contributor

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.

@niklasf

niklasf Jun 10, 2012

Contributor

Makes sense. Pushed.

@jalliot jalliot and 1 other commented on an outdated diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ $this->headers->set('Content-Length', $this->file->getSize());
+ $this->headers->set('Content-Type', $this->file->getMimeType() ?: 'application/octet-stream');
+ $this->headers->set('Accept-Ranges', 'bytes');
+ $this->headers->set('Content-Transfer-Encoding', 'binary');
+
+ if ('1.0' != $request->server->get('SERVER_PROTOCOL')) {
+ $this->setProtocolVersion('1.1');
+ }
+
+ $this->offset = 0;
+ $this->maxlen = -1;
+
+ if ($request->headers->has('X-Sendfile-Type') && self::$trustXSendfileTypeHeader) {
+ // Use X-Sendfile, do not send any content.
+ $this->headers->set($request->headers->get('X-Sendfile-Type'), $this->file->getPathname());
+ $this->maxlen = 0;
@jalliot

jalliot Jun 10, 2012

Contributor

Why opening the streams in that scenario?
You should set a property contentSent (or sth like that) that you'd set to true here. Then in sendContent you could return prematurely if the variable is true.

@niklasf

niklasf Jun 10, 2012

Contributor

Simply checking maxlen != 0, now.

@jalliot jalliot and 2 others commented on an outdated diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ public function setAutoEtag()
+ {
+ $this->setEtag(sha1_file($this->file->getPathname()));
+
+ return $this;
+ }
+
+ /**
+ * @{inheritdoc}
+ */
+ public function prepare(Request $request)
+ {
+ $this->headers->set('Content-Length', $this->file->getSize());
+ $this->headers->set('Content-Type', $this->file->getMimeType() ?: 'application/octet-stream');
+ $this->headers->set('Accept-Ranges', 'bytes');
+ $this->headers->set('Content-Transfer-Encoding', 'binary');
@jalliot

jalliot Jun 10, 2012

Contributor

You should add Content-Disposition too (sth like I did here) and maybe add a way to change the filename and the disposition.
Not sure it should be added with x-sendfile and Range though...

@niklasf

niklasf Jun 10, 2012

Contributor

Mhh .. not sure how to properly do it, since @fabpot said: "This is wrong as the method behavior is totally different from the parent. Why not set the content disposition without overriding this method?".

@jalliot

jalliot Jun 11, 2012

Contributor

Actually I would change the prototype of setFile to this:

public function setFile($file, $filename = '', $filenameFallback = '', $disposition = ResponseHeaderBag::DISPOSITION_ATTACHMENT);

$filename would default if not set to $file->getBasename() and $filenameFallback to a rawurlencoded version of it.
And then only setting the header in prepare (only if the user didn't already set a value). This would need 3 more properties and maybe getters/setters for them.
The autoETag and autoLastModified could be set in prepare as well

@niklasf

niklasf Jun 11, 2012

Contributor

Mhh ... not sure. As an example: I am trying to put this in Symfony for use in Drupal. All our use cases would not use any Content-Disposition header at all. I would like to make it easy to not have such a header. Would something like $autoDisposition = null that can be set to both DISPOSITION_ATTACHMENT and DISPOSITION_INLINE and would then have a Content-Disposition with the default filenames cover most of the use cases?

@jalliot

jalliot Jun 11, 2012

Contributor

Disposition is one thing but the important point for me is the filename. Why wouldn't you want the header in your application?

@niklasf

niklasf Jun 11, 2012

Contributor

Right. A header doesn't hurt, but that was not the entire point. There is already ResponseHeaderBag::makeDisposition($disposition, $filename, $filenameFallback = ''), because content dispositions are a more general concept. The only thing that BinaryFileResponse can provide beyond that (and that would make it useful to have some special handling here), is setting good defaults based on the file name it has. Thus a flag that decides whether to automatically set that or not would do it. Would you be happier with $autoDisposition = 'inline'? Or wasn't I able to convince you? ;)

@jalliot

jalliot Jun 11, 2012

Contributor

Yeah sure, I don't mind such a flag.
But I still think it would be nice to have a fluent API to set the filename, etc. (without having to use directly makeDisposition of course).
Anyway, let's see what @fabpot wants :)

@niklasf

niklasf Jun 14, 2012

Contributor

Looks like this is the remaining thing to figure out. Added the flag for now with default null and tests.

@ruimarinho

ruimarinho Jun 22, 2012

Contributor

Setting the filename like @jalliot mentioned would be a plus. Setting the disposition flag is not enough because you can't control the filename with precision as it is always based on the real filename. The ResponseHeaderBag::makeDisposition method works, but I still think such feature would fit perfectly on the scope of the BinaryResponse.

Looking forward for this merge. Thank you @niklasf!

@niklasf

niklasf Jun 26, 2012

Contributor

Thanks @ruimarinho . Yeah ... so I noticed while implementing the above flag, that makeDisposition doesn't set the Content-Disposition header. It just creates something that can be used as its value. So ... setting Content-Disposition headers is indeed a bit crufty. But still: Isn't that something valid for all responses (or rather ResponseHeaderBags)? I'd leave it for a seperate more general PR.

@ruimarinho

ruimarinho Jun 27, 2012

Contributor

Regarding the way ResponseHeaderBag works, you're right, but what I meant was that I am manually setting it after instantiating the BinaryFileResponse, because it's the only way I have to specify a custom filename.


$response = new BinaryFileResponse($document->getAbsolutePath());
$dispositionHeader = (new ResponseHeaderBag)->makeDisposition(ResponseHeaderBag::DISPOSITION_INLINE, $document->getId().'-myCustomGeneratedName'));

IMO, a better alternative would be passing $document->getId().'-myCustomGeneratedName') as an option of the class. What do you think?

@niklasf

niklasf Jun 27, 2012

Contributor

@jalliot: Alright. So we now have a chainable method setContentDisposition that also allows to override the filename.

@jalliot

jalliot Jun 27, 2012

Contributor

@niklasf Great!
I only quickly read the diff but it seems that you forgot to set a default value for the $disposition parameter (attachment?). You will have an exception thrown in ResponseHeaderBag otherwise.

@jalliot jalliot and 1 other commented on an outdated diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+
+namespace Symfony\Component\HttpFoundation;
+
+use Symfony\Component\HttpFoundation\File\File;
+use Symfony\Component\HttpFoundation\File\Exception\FileException;
+
+/**
+ * BinaryFileResponse represents an HTTP response delivering a file.
+ *
+ * @author Niklas Fiekas <niklas.fiekas@tu-clausthal.de>
+ * @author stealth35 <stealth35-php@live.fr>
+ * @author Igor Wiedler <igor@wiedler.ch>
+ */
+class BinaryFileResponse extends Response
+{
+ protected static $trustXSendfileTypeHeader;
@jalliot

jalliot Jun 10, 2012

Contributor

You need to set it to false by default (like done here for the trustProxy property in Request)

@jalliot

jalliot Jun 10, 2012

Contributor

BTW, if this PR is merged, a global configuration option could be set in FrameworkBundle to change the behaviour easily.

@niklasf

niklasf Jun 10, 2012

Contributor

Defaulting to "really" false, now.

Contributor

linniksa commented Jun 10, 2012

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

Contributor

niklasf commented Jun 10, 2012

@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)?

Contributor

linniksa commented Jun 10, 2012

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

@jalliot jalliot and 1 other commented on an outdated diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ * Automatically sets the ETag header according to the checksum of the file.
+ */
+ public function setAutoEtag()
+ {
+ $this->setEtag(sha1_file($this->file->getPathname()));
+
+ return $this;
+ }
+
+ /**
+ * @{inheritdoc}
+ */
+ public function prepare(Request $request)
+ {
+ $this->headers->set('Content-Length', $this->file->getSize());
+ $this->headers->set('Content-Type', $this->file->getMimeType() ?: 'application/octet-stream');
@jalliot

jalliot Jun 10, 2012

Contributor

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

@niklasf

niklasf Jun 10, 2012

Contributor

Good idea. Pushed.

Contributor

niklasf commented Jun 10, 2012

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

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

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

@jalliot jalliot and 1 other commented on an outdated diff Jun 10, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ * Automatically sets the ETag header according to the checksum of the file.
+ */
+ public function setAutoEtag()
+ {
+ $this->setEtag(sha1_file($this->file->getPathname()));
+
+ return $this;
+ }
+
+ /**
+ * @{inheritdoc}
+ */
+ 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);
@jalliot

jalliot Jun 10, 2012

Contributor

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');
}
@niklasf

niklasf Jun 11, 2012

Contributor

Oh ... indeed. Fixed.

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

Contributor

stealth35 commented Jun 11, 2012

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 stof and 1 other commented on an outdated diff Jun 11, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ */
+ public function __construct($file, $status = 200, $headers = array(), $public = true, $autoEtag = false, $autoLastModified = true)
+ {
+ parent::__construct(null, $status, $headers);
+
+ $this->setFile($file, $autoEtag, $autoLastModified);
+
+ if ($public) {
+ $this->setPublic();
+ }
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public static function create($file, $status = 200, $headers = array(), $public = true, $autoEtag = false, $autoLastModified = true)
@stof

stof Jun 11, 2012

Member

this method leads to a E_STRICT error

@niklasf

niklasf Jun 11, 2012

Contributor

Well spotted. Fixed. Thanks!

Contributor

niklasf commented Jun 11, 2012

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

Contributor

niklasf commented Jun 11, 2012

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

@fabpot fabpot and 1 other commented on an outdated diff Jun 11, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ /**
+ * {@inheritDoc}
+ *
+ * @throws \LogicException when the content is not null
+ */
+ public function setContent($content)
+ {
+ if (null !== $content) {
+ throw new \LogicException('The content cannot be set on a BinaryFileResponse instance.');
+ }
+ }
+
+ /**
+ * {@inheritDoc}
+ *
+ * return false
@fabpot

fabpot Jun 11, 2012

Owner

missing @ before return.

@niklasf

niklasf Jun 11, 2012

Contributor

Fixed. Thanks.

Contributor

stealth35 commented Jun 11, 2012

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

This pull request passes (merged ed60a2e7 into 0995b1f).

Contributor

linniksa commented Jun 11, 2012

@niklasf Thanks you for your work
If needed you may use linniksa@gmail.com

This pull request passes (merged 93821a17 into 0995b1f).

Contributor

niklasf commented Jun 14, 2012

@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.
Contributor

stealth35 commented Jun 14, 2012

Yeah sorry MimeTypeExtensionGuesser is for getting an extension with the Mime, forget about this, i'll take care aboute all MIME intégration later

Contributor

niklasf commented Jun 14, 2012

@stealth35: Awesome. Thanks a lot.

This pull request passes (merged 04485c24 into 7c91ee5).

This pull request fails (merged 944935b9 into d0e1547).

This pull request fails (merged 9932bfea into d0e1547).

@niklasf niklasf commented on the diff Jun 27, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ */
+ public function setAutoEtag()
+ {
+ $this->setEtag(sha1_file($this->file->getPathname()));
+
+ return $this;
+ }
+
+ /**
+ * Sets the Content-Disposition header with the given filename.
+ *
+ * @param string $disposition ResponseHeaderBag::DISPOSITION_INLINE or ResponseHeaderBag::DISPOSITION_ATTACHMENT
+ * @param string $filename Optionally use this filename instead of the real name of the file
+ * @param string $filenameFallback A fallback filename, containing only ASCII characters. Defaults to an automatically encoded filename
+ */
+ public function setContentDisposition($disposition, $filename = '', $filenameFallback = '')
@niklasf

niklasf Jun 27, 2012

Contributor

@jalliot Do you mean this one? The first parameter isn't optional, so that wouldn't be a problem.

@jalliot

jalliot Jun 27, 2012

Contributor

@niklasf Then the $autoDisposition name isn't correct (it implies a boolean value, which would lead to an exception).

@niklasf

niklasf Jun 28, 2012

Contributor

@jalliot Right. Thanks. Fixed it.

This pull request fails (merged 1a09690b into e0351c9).

fabpot referenced this pull request Jul 13, 2012

Closed

add BinaryFileResponse #3602

@ruimarinho ruimarinho and 2 others commented on an outdated diff Jul 18, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+
+ /**
+ * Sets the Content-Disposition header with the given filename.
+ *
+ * @param string $disposition ResponseHeaderBag::DISPOSITION_INLINE or ResponseHeaderBag::DISPOSITION_ATTACHMENT
+ * @param string $filename Optionally use this filename instead of the real name of the file
+ * @param string $filenameFallback A fallback filename, containing only ASCII characters. Defaults to an automatically encoded filename
+ */
+ public function setContentDisposition($disposition, $filename = '', $filenameFallback = '')
+ {
+ if ($filename === '') {
+ $filename = $this->file->getFilename();
+ }
+
+ if ($filenameFallback === '') {
+ $filenameFallback = rawurlencode($filename);
@ruimarinho

ruimarinho Jul 18, 2012

Contributor

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?

@niklasf

niklasf Jul 18, 2012

Contributor

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?)

@Crell

Crell Jul 22, 2012

Contributor

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.

@ruimarinho

ruimarinho Jul 22, 2012

Contributor

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.

Contributor

jalliot commented Aug 7, 2012

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

Contributor

niklasf commented Aug 7, 2012

@jalliot Thanks. Fixed.

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

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

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

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

@stloyd stloyd commented on the diff Aug 8, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+
+ if (substr($path, 0, strlen($pathPrefix)) == $pathPrefix) {
+ $path = $location . substr($path, strlen($pathPrefix));
+ break;
+ }
+ }
+ }
+ }
+ $this->headers->set($type, $path);
+ $this->maxlen = 0;
+ } elseif ($request->headers->has('Range')) {
+ // Process the range headers.
+ 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);
@stloyd

stloyd Aug 8, 2012

Contributor

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.

@niklasf

niklasf Aug 8, 2012

Contributor

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

@stloyd

stloyd Aug 8, 2012

Contributor

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

Contributor

Crell commented Aug 29, 2012

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?

Owner

fabpot commented Aug 29, 2012

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

Contributor

linniksa commented Sep 20, 2012

any news about merging this?

@stof stof commented on the diff Oct 13, 2012

...mfony/Component/HttpFoundation/BinaryFileResponse.php
+ */
+ public function setContent($content)
+ {
+ if (null !== $content) {
+ throw new \LogicException('The content cannot be set on a BinaryFileResponse instance.');
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ *
+ * @return false
+ */
+ public function getContent()
+ {
+ return false;
@stof

stof Oct 13, 2012

Member

It should return null, not false IMO

Member

stof commented Oct 13, 2012

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

Contributor

niklasf commented Oct 13, 2012

@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.

Contributor

Burgov commented Oct 16, 2012

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?

Contributor

sstok commented Oct 16, 2012

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

Contributor

niklasf commented Oct 18, 2012

@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))

Contributor

Crell commented Oct 18, 2012

@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.

Contributor

sstok commented Oct 18, 2012

@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.

Contributor

Crell commented Oct 18, 2012

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.

@fabpot fabpot added a commit that referenced this pull request Oct 29, 2012

@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

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

Owner

fabpot commented Oct 29, 2012

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment