[HttpFoundation] Fix unprepared BinaryFileResponse sends empty file #28278
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ro0NL
reviewed
Aug 27, 2018
| @@ -201,9 +201,6 @@ public function prepare(Request $request) | |||
|
|
|||
| $this->ensureIEOverSSLCompatibility($request); | |||
|
|
|||
| $this->offset = 0; | |||
There was a problem hiding this comment.
i think we should keep this in case of multiple calls to prepare().
Technically initializing the properties with default values can break some edgy code, relying on the the lazy null. Perhaps better to default on the calling side: $this->offset ?? 0?
There was a problem hiding this comment.
The null-coalesce operator isn't available until PHP 7.0. I thought 2.8 needed to support PHP 5.3.9+. I could use ?: but then if $offset is set to 0 by prepare(), the ternary will evaluate to false. That's probably not that big of an issue in this case since it would be set to 0 anyway.
I did restore the default values in prepare() though.
| { | ||
| $response = BinaryFileResponse::create(__DIR__.'/File/Fixtures/test.gif', 200); | ||
|
|
||
| $file = fopen(__DIR__.'/File/Fixtures/test.gif', 'r'); |
There was a problem hiding this comment.
I'd suggest to use file_get_contents here
nicolas-grekas
approved these changes
Aug 27, 2018
|
Thanks @nicolas-grekas, I've updated the target branch. |
ro0NL
approved these changes
Aug 27, 2018
fabpot
approved these changes
Aug 27, 2018
|
Thank you @WackyMole. |
fabpot
added a commit
that referenced
this pull request
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
This was referenced Aug 27, 2018
Merged
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When you call
BinaryFileResponse#sendContent()without first callingprepare()the response is sent but the contents are empty.prepare()properly initializes the$maxlenand$offsetproperties. However,sendContent()doesn't do any sanity checking, and so, uses the uninitialized properties. This causesstream_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().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.