Skip to content
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

Add support for streamed response #50

Merged
merged 1 commit into from Mar 11, 2019

Conversation

Projects
None yet
7 participants
@danizord
Copy link
Contributor

danizord commented Sep 26, 2018

Here's a simple implementation of response streaming heavily inspired by Diactoros' response emitter, using HttpFoundation's StreamedResponse. It lacks support for Content-Range, but I can provide that in a future PR.

Closes #3

@danizord

This comment has been minimized.

Copy link
Contributor Author

danizord commented Sep 26, 2018

It seems like Travis is happy but fabbot.io failed with "Not Found" 🤔

@danizord

This comment has been minimized.

Copy link
Contributor Author

danizord commented Nov 14, 2018

Ping @stof @fabpot @dunglas in case you missed it 😄

@dunglas
Copy link
Member

dunglas left a comment

LGTM, can you just run PHP CS Fixer please.

@danizord

This comment has been minimized.

Copy link
Contributor Author

danizord commented Nov 21, 2018

@dunglas done :)

@@ -140,11 +154,12 @@ protected function getTemporaryPath()
*/
public function createResponse(ResponseInterface $psrResponse)
{
$response = new Response(
$psrResponse->getBody()->__toString(),
$response = new StreamedResponse(

This comment has been minimized.

@jvasseur

jvasseur Nov 23, 2018

I don't think we should use stream responses in every possible cases because StreamedResponse have some downsides like preventing the debug toolbar from being injected.

Can this be made configurable ?

@jdecool

This comment has been minimized.

Copy link

jdecool commented Feb 21, 2019

What about this PR ?

Is it still relevant ? This is a nice feature to add in this component.

@danizord

This comment has been minimized.

Copy link
Contributor Author

danizord commented Feb 21, 2019

Hey @jdecool! @jvasseur suggested to make it configurable, but I'm not sure how 🤔

Any suggestions?

@jdecool

This comment has been minimized.

Copy link

jdecool commented Feb 21, 2019

Maybe by creating a another method ?

Don't touch createResponse method and add a new createStreamedResponse ?

@jdecool

This comment has been minimized.

Copy link

jdecool commented Feb 24, 2019

@danizord do you need help to finish this PR ?

@nicolas-grekas nicolas-grekas force-pushed the danizord:feature/streamed-response branch from 0c60565 to 013db27 Mar 11, 2019

@nicolas-grekas nicolas-grekas force-pushed the danizord:feature/streamed-response branch from 013db27 to 7cc1605 Mar 11, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 11, 2019

I rebased the PR and made the behavior opt-in by adding a new bool $streamed argument to createResponse()

@Nyholm

Nyholm approved these changes Mar 11, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 11, 2019

Thank you @danizord.

@nicolas-grekas nicolas-grekas merged commit 7cc1605 into symfony:master Mar 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Mar 11, 2019

feature #50 Add support for streamed response (danizord)
This PR was merged into the 1.2-dev branch.

Discussion
----------

Add support for streamed response

Here's a simple implementation of response streaming heavily inspired by Diactoros' response emitter, using HttpFoundation's `StreamedResponse`. It lacks support for `Content-Range`, but I can provide that in a future PR.

Closes #3

Commits
-------

7cc1605 Add support for streamed response

@danizord danizord deleted the danizord:feature/streamed-response branch Mar 11, 2019

@danizord danizord restored the danizord:feature/streamed-response branch Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.