[HttpFoundation] Add File\Stream for size-unknown BinaryFileResponse #21188
Conversation
da3c7a4 to
363977b
Compare
| public function getSize() | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I am not sure that this a the best solution. Returning false is not covered by the documentation. What if we removed the method here and then used some kind of instanceof Stream check when handling the response instead?
There was a problem hiding this comment.
I think it's a doc issue. Looking at the source, getSize is just the same as filesize, or 'size' as returned by fstat.
Here is the doc for filesize that tells about false: http://php.net/filesize
This means that we have to handle false here, regardless of Stream.
There was a problem hiding this comment.
Makes sense then. However, did you manage to call getSize() without getting a warning in that case? Otherwise, we may want to silence the operation.
There was a problem hiding this comment.
nope, let's wait an issue report if that ever happens :)
There was a problem hiding this comment.
Ah well I see that we already throw an exception when the file does not exist.
|
👍 Status: Reviewed |
|
👍 |
| * | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| class Stream extends File |
There was a problem hiding this comment.
Shouldn't this file be moved as a fixture file instead?
There was a problem hiding this comment.
This file is the feature :)
|
Can you add a not in the CHANGELOG and submit a PR on the docs? |
363977b to
9befa70
Compare
|
@fabpot done |
| @@ -4,6 +4,7 @@ CHANGELOG | |||
| 3.3.0 | |||
| ----- | |||
|
|
|||
| * added the `File\Stream` class for size-unknown `BinaryFileResponse` | |||
There was a problem hiding this comment.
I think we need to be more explicit about the use-case.
9befa70 to
8011209
Compare
|
Thank you @nicolas-grekas. |
…ryFileResponse (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [HttpFoundation] Add File\Stream for size-unknown BinaryFileResponse | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19738 | License | MIT | Doc PR | symfony/symfony-docs#7343 Replaces #19740. Native "getSize" is reported to return false on error, so using false as return type doesn't break the signature. Commits ------- 8011209 [HttpFoundation] Add File\Stream for size-unknown BinaryFileResponse
Replaces #19740. Native "getSize" is reported to return false on error, so using false as return type doesn't break the signature.