Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Add File\Stream for size-unknown BinaryFileResponse#21188
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
da3c7a4 to363977bCompare| publicfunctiongetSize() | ||
| { | ||
| returnfalse; | ||
| } |
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 am not sure that this a the best solution. Returningfalse is not covered by the documentation. What if we removed the method here and then used some kind ofinstanceof Stream check when handling the response instead?
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 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 aboutfalse:http://php.net/filesize
This means that wehave to handle false here, regardless of 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.
Makes sense then. However, did you manage to callgetSize() without getting a warning in that case? Otherwise, we may want to silence the operation.
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.
nope, let's wait an issue report if that ever happens :)
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 well I see that we already throw an exception when the file does not exist.
xabbuh commentedJan 8, 2017
👍 Status: Reviewed |
javiereguiluz commentedJan 8, 2017
👍 |
| * | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| class Streamextends File |
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.
Shouldn't this file be moved as a fixture file instead?
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 fileis the feature :)
fabpot commentedJan 8, 2017
Can you add a not in the CHANGELOG and submit a PR on the docs? |
363977b to9befa70Comparenicolas-grekas commentedJan 9, 2017
@fabpot done |
| 3.3.0 | ||
| ----- | ||
| * added the`File\Stream` class for size-unknown`BinaryFileResponse` |
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 we need to be more explicit about the use-case.
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.
better now?
9befa70 to8011209Comparefabpot commentedJan 9, 2017
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#7343Replaces#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
Uh oh!
There was an error while loading.Please reload this page.
Replaces#19740. Native "getSize" is reported to return false on error, so using false as return type doesn't break the signature.