Skip to content

[HttpKernel] Revert #45476 to fix missing Request in RequestStack for StreamedResponse #51391

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

DaDeather
Copy link
Contributor

Q A
Branch 6.3
Bug fix yes
New feature no
Deprecations no
Tickets Fix #46743
License MIT

Revert #45476 and add test to ensure Request availability while using StreamedResponse callback.

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Aug 16, 2023

Reverting #45476 will break streamedresponses for other Runtimes like Roadrunner, Swoole (php-runtime/runtime#115), is there any possibility todo this only when specific Runtime is used? /cc @nicolas-grekas.

Addition: The problem also is that it would behave differently between Roadrunner, Swoole, now as they could then still run into #46743 where this listener is required to be disabled. I understand @derrabus concern about #51139 but to have full support for such runtimes I don't see another way then make HttpKernel aware of a Response class specially it already has dependency to HttpFoundation.

@nicolas-grekas
Copy link
Member

Thanks for the reminder @alexander-schranz

@DaDeather DaDeather deleted the revert-streamedresponselistener-deprecation branch August 16, 2023 08:36
@derrabus
Copy link
Member

derrabus commented Aug 16, 2023

I understand @derrabus concern about #51139 but to have full support for such runtimes I don't see another way then make HttpKernel aware of a Response class specially it already has dependency to HttpFoundation.

The other way would be to document that the request stack is empty when StreamedResponse's callback is executed. This is fine imho.

Note that the breaking change was to un-wire the deprecated listener which has happened without prior deprecation in a minor release. We could wire it again, but make it opt-out by introducing a config flag into FrameworkBundle. Yeah, I know how we don't like those.

@nicolas-grekas
Copy link
Member

document that the request stack is empty when StreamedResponse's callback is executed

This ship has sailed IMHO. Also it's very hard to deal with, because any services can be used in that callback. I think #51396 is pragmatic and makes sense also.

@derrabus
Copy link
Member

I think #51396 is pragmatic and makes sense also.

LGTM.

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.

[HttpKernel] "Deprecation" of StreamedResponseListener causes RequestStack to be empty within StreamedResponse callback
5 participants