Skip to content
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

BinaryFileResponse doesn't work unless prepare is called #28237

Closed
iluuu1994 opened this issue Aug 22, 2018 · 10 comments
Closed

BinaryFileResponse doesn't work unless prepare is called #28237

iluuu1994 opened this issue Aug 22, 2018 · 10 comments

Comments

@iluuu1994
Copy link
Contributor

iluuu1994 commented Aug 22, 2018

Symfony version(s) affected: 3.4.14

Description

I'm using the HttpFoundation component to stream binary files. I'm using the basic snipped underneath which produces no output. The reason for this is this line.

stream_copy_to_stream has the following signature:

int stream_copy_to_stream(resource $source, resource $dest[, int $maxlength = -1[, int $offset = 0]])

The maxlen and offset properties are only actually ever set in the prepare method. Thus Symfony passes null to both of these parameters unless prepared is called. PHP will not do anything if these properties are null:

https://3v4l.org/MtBl7

How to reproduce

$response = new BinaryFileResponse($filepath);
$response->send();

Possible Solution

The solution is to call prepare on the response. The docs do mention the prepare method but don't say that it's mandatory.

$response = new BinaryFileResponse($filepath);
$response->prepare(Request::createFromGlobals());
$response->send();

Symfony should either:

  • Be clearer in the documentation that prepare is necessary
  • Throw an error when maxlength or offset are null
  • Default to -1 and 0 as the default values
@j4nr6n
Copy link
Contributor

j4nr6n commented Aug 24, 2018

Hey @iluuu1994

I apologize if I'm misunderstanding you. I tried to reproduce your error, but I wasn't able to.

One thing I wasn't clear on, is whether you are using the Symfony framework or just the HttpFoundation component. In the first situation, you can simply return your BinaryFileResponse object instead of calling ->send() on it. The ResponseListener in the HttpKernel component calls prepare() for you.

@j4nr6n
Copy link
Contributor

j4nr6n commented Aug 24, 2018

But the HttpFoundation component docs does mention prepare() a little further up the page.

@iluuu1994
Copy link
Contributor Author

iluuu1994 commented Aug 24, 2018

@WackyMole When using the Symfony kernel Symfony will always calls prepare on the response under the hood:

$response->prepare($event->getRequest());

I'm using the BinaryFileResponse in a non Symfony project. Try just the send but without prepare.

@j4nr6n
Copy link
Contributor

j4nr6n commented Aug 24, 2018

I did, and I do see your error in that case.

@iluuu1994
Copy link
Contributor Author

@WackyMole Ok 👍 The docs do not mention that prepare needs to be called. That would be a valid solution to this issue.

@j4nr6n
Copy link
Contributor

j4nr6n commented Aug 24, 2018

Well, it wasn't the error I thought it was. This works.

$response = new BinaryFileResponse(__DIR__.'/Test.txt');
$response->setContentDisposition(
    ResponseHeaderBag::DISPOSITION_ATTACHMENT,
    'filename.txt'
);
$response->send();

@iluuu1994
Copy link
Contributor Author

@WackyMole Can you try it completely without a Symfony kernel? In a standalone .php file with nothing but the lines from the previous comment?

@j4nr6n
Copy link
Contributor

j4nr6n commented Aug 25, 2018

Sure can. This was created as a single index.php and a test.txt with the only dependency being symfony/http-foundation@3.4.14. Setting the content disposition makes it a download rather than open in browser (at least in Chrome). I guess the alternative is to call prepare().

P.S. Do you know how many hello (x).txt files I have in my downloads directory now?

@iluuu1994
Copy link
Contributor Author

@WackyMole

P.S. Do you know how many hello (x).txt files I have in my downloads directory now?

😂

I can reproduce my problem with your example. The file is downloaded (as the content disposition header is set correctly) but the file is empty because stream_copy_to_stream doesn't work when the 3rd and 4th parameters are null.

@j4nr6n
Copy link
Contributor

j4nr6n commented Aug 26, 2018

Yeah, I noticed that after I commented. Something in prepare() sets those two values based on request headers. Perhaps those properties should have default values.

fabpot added a commit that referenced this issue Aug 27, 2018
…mpty file (wackymole)

This PR was merged into the 2.8 branch.

Discussion
----------

[HttpFoundation] Fix unprepared BinaryFileResponse sends empty file

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes, with the exception of preexisting, unrelated failures
| Fixed tickets | #28237
| License       | MIT
| Doc PR        |

When you call `BinaryFileResponse#sendContent()` without first calling `prepare()` the response is sent but the contents are empty. `prepare()` properly initializes the `$maxlen` and `$offset` properties. However, `sendContent()` doesn't do any sanity checking, and so, uses the uninitialized properties. This causes `stream_copy_to_stream()` to copy empty contents and the file that is sent, to contain nothing.

This change initializes the properties at definition instead of in `prepare()`.

> Additionally:
> - Bug fixes must be submitted against the lowest branch where they apply

~I'm not sure how early this bug exists, or how far back to go. I'll check to see if 2.7 and 2.8 are affected and report back.~

Commits
-------

dba8687 Instantiate $offset and $maxlen at definition
@fabpot fabpot closed this as completed Aug 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants