Fix BinaryFileResponse sending wrong Content-Length header for files modified by stream wrappers/filters #19740
Conversation
|
@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. |
|
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.
can't we call stat($this->getPathname()) directly instead?
There was a problem hiding this comment.
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.
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.
|
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? |
|
@TravisCarden works for me. Have you updated composer.json by any chance? |
|
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 to
126b8ac
Compare
|
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.
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.
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.
|
To switch chunked encoding on, one should not send the |
|
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#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
I have attempted to follow the guidelines for submitting a patch, but this is my first attempt to the contribute to the Symfony community, so I welcome feedback. Thanks.