Skip to content

Make turbo stream format subscriber opt-in and not enabled automatically #214

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

Closed
wants to merge 1 commit into from

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented Jan 11, 2022

Q A
Bug fix? yes
New feature? yes
Tickets Fix #...
License MIT

The subscriber should be opt-in. As it currently manipulates the requestFormat which is not expected by all applications (e.g. Sulu or any other application, working with $request->getRequestFormat() for getting its template). A controller handling the turbo can activate the listener with:

my_route:
    defaults:
       turbo: true

This indicates this routes handles turbo route and so request format is set to turbo, for all other routes it behaves the symfony way and the request format is not manipulated in this case.

@dunglas
Copy link
Member

dunglas commented Jan 11, 2022

In my opinion, symfony/symfony#44980 is a better fix. It should allow removing this listener entirely.

@alexander-schranz
Copy link
Contributor Author

@dunglas when then:

$request->setFormat(TurboStreamResponse::STREAM_FORMAT, TurboStreamResponse::STREAM_MEDIA_TYPE);
$request->setRequestFormat(TurboStreamResponse::STREAM_FORMAT);

Can be removed from this listener I'm totally in for that change :)

@alexander-schranz
Copy link
Contributor Author

Tested your changes with sulu (without symfony/ux):

$request->getPreferredFormat(); // turbo_stream
$request->getRequestFormat(); // html

This would match my expected behaviour 👍 and would so not longer crash sulu's website controller here for turbo requests.

@dunglas
Copy link
Member

dunglas commented Jan 11, 2022

Maybe should we merge symfony/symfony#44980 as a bug fix in Symfony then, to not have to wait for 6.1 to be released.

@alexander-schranz
Copy link
Contributor Author

@dunglas that would be great :)

@chalasr
Copy link
Member

chalasr commented Jan 11, 2022

Well, symfony/symfony#44980 really is a feature... I'm for merging both PRs. We can remove the listener once this package supports 6.1+ only, as usual.

@dunglas
Copy link
Member

dunglas commented Jan 11, 2022

Actually it's a bug fix for Turbo. The question is: do we merge fixes needed for Symfony UX in the maintenance branches of the monorepo. In my opinion, we should at least in this case as this patch is very very small and cannot break anything.

@dunglas
Copy link
Member

dunglas commented Jan 11, 2022

@alexander-schranz could you try this patch instead, please? #217

@nicolas-grekas
Copy link
Member

Closing in favor of #217
Thanks for pushing this forward.

@alexander-schranz alexander-schranz deleted the patch-1 branch January 12, 2022 12:51
chalasr added a commit that referenced this pull request Jan 12, 2022
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Turbo] Simplify stream negotiation

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Tickets       | n/a
| License       | MIT

This patch replaces the ad-hoc content negotiation system for Turbo Streams used by the bundle. HttpFoundation's native content negotiation features are now used instead. This greatly simplifies the code, which is security-sensitive, and improves DX (content negotiation is now consistent with how it is handled in the rest of the framework).

Replaces #214. Needs #215 and symfony/symfony#44980.

Commits
-------

b6de1e3 [Turbo] Simplify stream negotiation
symfony-splitter pushed a commit to symfony/ux-turbo that referenced this pull request Jan 12, 2022
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Turbo] Simplify stream negotiation

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Tickets       | n/a
| License       | MIT

This patch replaces the ad-hoc content negotiation system for Turbo Streams used by the bundle. HttpFoundation's native content negotiation features are now used instead. This greatly simplifies the code, which is security-sensitive, and improves DX (content negotiation is now consistent with how it is handled in the rest of the framework).

Replaces symfony/ux#214. Needs symfony/ux#215 and symfony/symfony#44980.

Commits
-------

b6de1e3 [Turbo] Simplify stream negotiation
marcuswebapp added a commit to marcuswebapp/ux that referenced this pull request Aug 1, 2023
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Turbo] Simplify stream negotiation

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Tickets       | n/a
| License       | MIT

This patch replaces the ad-hoc content negotiation system for Turbo Streams used by the bundle. HttpFoundation's native content negotiation features are now used instead. This greatly simplifies the code, which is security-sensitive, and improves DX (content negotiation is now consistent with how it is handled in the rest of the framework).

Replaces symfony/ux#214. Needs symfony/ux#215 and symfony/symfony#44980.

Commits
-------

b6de1e3 [Turbo] Simplify stream negotiation
symfony-splitter pushed a commit to symfony/ux-turbo that referenced this pull request Sep 22, 2023
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Turbo] Simplify stream negotiation

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Tickets       | n/a
| License       | MIT

This patch replaces the ad-hoc content negotiation system for Turbo Streams used by the bundle. HttpFoundation's native content negotiation features are now used instead. This greatly simplifies the code, which is security-sensitive, and improves DX (content negotiation is now consistent with how it is handled in the rest of the framework).

Replaces symfony/ux#214. Needs symfony/ux#215 and symfony/symfony#44980.

Commits
-------

b6de1e3 [Turbo] Simplify stream negotiation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants