Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix BinaryFileResponse sending wrong Content-Length header for files modified by stream wrappers/filters#19740
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
fabpot commentedAug 26, 2016
@TravisCarden You did very well :) What about the suggestion made by@EclipseGc on the linked ticket? It looks like a better way to me at first glance. |
TravisCarden commentedAug 26, 2016
Sure. As long as no client code depends on |
| // Get the file size from a stream rather than from disk in case it is changed later by | ||
| // stream wrappers or stream filters. | ||
| $file =fopen($this->getPathname(),'rb'); | ||
| $fstat =fstat($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.
can't we callstat($this->getPathname()) directly 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.
ok, just read your linked issue.
any way to add a test case that would break when using the parent getSize()?
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.
To do that I think we'd need to write a test that is actually encrypting files so that the size is different on disc. I assume that's the best way to go about doing this?
…h header for files modified by stream wrappers/filters.
TravisCarden commentedSep 17, 2016
I was going to write a unit test for this, but I can't install the project dependencies on the 2.7 branch: Any thoughts? |
jakzal commentedSep 19, 2016
@TravisCarden works for me. Have you updated composer.json by any chance? |
TravisCarden commentedSep 19, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've got it working now. Not sure what was actually wrong, since I didn't change anything today. We'll chalk it up to gremlins. Thanks,@jakzal |
b5d8d75 to126b8acCompareTravisCarden commentedSep 20, 2016
Okay, here's an example of a unit test that fails without the patch and passes with it. It involves an annoyingly large fixture for such a small change, because it requires a stream filter, which in turn necessitates a stream wrapper; and I wasn't sure where those classes should go in the directory structure. They didn't get autoloaded in the test where I put them (hence the |
| // reliable way to get the effective size of the file is to actually read it. | ||
| ob_start(); | ||
| $size =readfile($this->getPathname()); | ||
| ob_end_clean(); |
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 will load the full response in memory, which means will break for contents that don't fit into memory.
This totally defeat the purpose of using streams imho.
When a stream wrapper/filter is there, we'd better use chunked encoding.
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.
You seem to be correct, of course. I think the code worked in the context of my project on account of incidental configuration details, but considering the matter abstractly it obviously doesn't seem like a solution suitable for generic application. Using chunked encoding in case of a wrapper/filter does seem like a sensible approach. I don't know what that would look like, though.
nicolas-grekas commentedOct 3, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
To switch chunked encoding on, one shouldnot send the |
nicolas-grekas commentedJan 6, 2017
Since it's impossible to know if a stream has a filter attached to it, the only solution is to make authors responsible for saying it. This is what#21188 tries to achieve. Closing this one. |
…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
I have attempted to followthe guidelines for submitting a patch, but this is my first attempt to the contribute to the Symfony community, so I welcome feedback. Thanks.