Skip to content

Add BinaryFileResponse::streamsEntireFile method #38280

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

iluuu1994
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets https://github.com/symfony/psr-http-message-bridge/issues/84
License MIT
Doc PR TODO

This method will return false when the client requests only a specific
range of a file.

See https://github.com/symfony/psr-http-message-bridge/issues/84

This method is needed for the PsrHttpFactory to know BinaryFileResponses can pass a stream of the given file to the psr response or if the content needs to be loaded into memory (because only a Range of the file is streamed). As mentioned in the issue above, I'm not sure if this change is acceptable in a patch because it does have a theoretical BC break as BinaryFileResponse is not final.

This method will return false when the client requests only a specific
range of a file.

See https://github.com/symfony/psr-http-message-bridge/issues/84
@iluuu1994 iluuu1994 force-pushed the binary-file-response-streams-entire-file branch from b1fa23e to fa7435c Compare September 23, 2020 14:37
@ro0NL
Copy link
Contributor

ro0NL commented Sep 23, 2020

to avoid the BC break in 4.4, reflection or a subclass in userland could be used.

@iluuu1994
Copy link
Contributor Author

@ro0NL Thanks for your response. Note that this is a bug in a Symfony package, not in one of my own. Yes, symfony/psr-http-message-bridge could also use reflection to get the two private properties offset and maxlen. We just need to remember that this package depends on the two properties even though they're private. If you prefer that solution I'm happy to close this PR and create one for symfony/psr-http-message-bridge.

@fabpot
Copy link
Member

fabpot commented Sep 24, 2020

I prefer a fix in symfony/psr-http-message-bridge instead. Thank you.

@fabpot fabpot closed this Sep 24, 2020
nicolas-grekas added a commit to symfony/psr-http-message-bridge that referenced this pull request Sep 29, 2020
…(iluuu1994)

This PR was merged into the 2.0-dev branch.

Discussion
----------

Fix BinaryFileResponse with range to psr response conversion

Closes #84

As requested by [Fabien](symfony/symfony#38280 (comment)).

I think using the slightly less optimal version of checking for a `Content-Range` header is better than relying on reflection. In theory, this could be slightly sub optimal when streaming whole files and setting the `Content-Range` manually but I'm assuming that's very rare in practice.

Commits
-------

5d5932d Fix BinaryFileResponse with range to psr response conversion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants