Skip to content

[HttpKernel] Add basic support for language negotiation #43108

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

Merged
merged 1 commit into from
Oct 5, 2021

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Sep 20, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

Continuation of #36507. Thanks @GregoireHebert!

This PR adds two options to the framework configuration:

  • set_locale_from_accept_language: Makes the Request' locale automatically set based on the Accept-Language header (restricted by a new framework.enabled_locales config option which replaces framework.translator.enabled_locales).
    The explicit _locale request attribute always wins over the Accept-Language header when it's set.
  • set_content_language_from_locale: Sets the Content-Language Response header based on the Request' locale.

This is going to be useful for API Platform and related (e.g. Sylius/Sylius#11412).

@GregoireHebert
Copy link
Contributor

Thank you for taking over :-)

@stof
Copy link
Member

stof commented Sep 20, 2021

Using the Accept-Language should be opt-in IMO. Otherwise, this might break projects that already set the locale based on other rules (the preferences of a logged-in user for instance).

@chalasr chalasr force-pushed the lang-neg branch 2 times, most recently from d491c3b to 9f0e400 Compare September 20, 2021 14:43
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor

@chalasr chalasr force-pushed the lang-neg branch 6 times, most recently from 53f82a0 to 59317c0 Compare September 20, 2021 20:01
@chalasr chalasr changed the title [HttpFoundation] Add basic support for language negotiation [HttpKernel] Add basic support for language negotiation Sep 20, 2021
@chalasr chalasr added this to the 5.4 milestone Sep 20, 2021
@chalasr
Copy link
Member Author

chalasr commented Oct 5, 2021

This PR is ready.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nit-picking. 🙂

Looks good, thank you!

@chalasr
Copy link
Member Author

chalasr commented Oct 5, 2021

Thanks for the reviews! All fixed now

@derrabus
Copy link
Member

derrabus commented Oct 5, 2021

Thank you @GregoireHebert and @chalasr.

@derrabus derrabus merged commit 43f0161 into symfony:5.4 Oct 5, 2021
@chalasr chalasr deleted the lang-neg branch October 5, 2021 16:41
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 19, 2021
…(hiddewie)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[HttpKernel] Add basic support for language negotiation

From symfony/symfony#43108

Fixes #15891

I moved the documentation for the deprecated option with a deprecation notice at the old place.

The new options `set_content_language_from_locale` and `set_locale_from_accept_language` have been documented.

Commits
-------

c41db2f [HttpKernel] Add basic support for language negotiation
This was referenced Nov 5, 2021
nicolas-grekas added a commit that referenced this pull request Dec 1, 2021
…rs (maxhelias)

This PR was merged into the 5.4 branch.

Discussion
----------

[FrameworkBundle] ResponseListener needs only 2 parameters

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | -
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Introduced in 5.4 with : #43108

Commits
-------

6074226 ResponseListener needs only 2 parameters
fabpot added a commit that referenced this pull request Dec 1, 2021
…riate (nicolas-grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[HttpKernel] fix sending Vary: Accept-Language when appropriate

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

That's something I figured our while reviewing #44386 and that we missed in #43108:
the `Vary` header should be sent when we use `Accept-Language`, not when we send `Content-Language`.
/cc @chalasr

Commits
-------

afab34d [HttpKernel] fix sending Vary: Accept-Language when appropriate
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.

7 participants