[HttpFoundation] add a default Content-Language to the Response #36507

GregoireHebert · 2020-04-20T17:40:47Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	~
License	MIT
Doc PR	symfony/symfony-docs# incoming

This PR will allow to add a default Content-Language header in the Response.
It also add a new available_locales entry under the framework configuration.
If this array is not empty, it will try to set the request locale according to the preferredLanguage (Accept-Language header), only if there isn't any _locale attribute.

Todo

Create documentation PR
Update changelog

nicolas-grekas

some quick comments

nicolas-grekas · 2020-04-20T18:03:27Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -81,6 +81,12 @@ public function getConfigTreeBuilder()
                ->scalarNode('ide')->defaultNull()->end()
                ->booleanNode('test')->end()
                ->scalarNode('default_locale')->defaultValue('en')->end()
+                ->arrayNode('available_locales')


there is enabled_locales already, can't we reuse it?

it's in the translator section.
what if the translator is not available?
Is it still relevant to reuse it?

My fear is that it will be confusing for users to have to configure this twice.

I suggest to make the translator option defaulting to the value of this new parameter if not set explicitly (and to not set it explicitly in the recipe). It’s already what we do for the default local IIRC.

or we could move the setting out of the translation subtree.
again, having twice the same setting is calling for troubles to me.

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/full.xml

src/Symfony/Component/HttpFoundation/Response.php

src/Symfony/Component/HttpKernel/EventListener/LocaleListener.php

nicolas-grekas · 2020-04-21T08:59:59Z

What's the purpose of this feature? HTTP negotiation is tricky, as explained on the doc of Varnish and on Cloudflare (which actively works against HTTP negotiation btw).

Most apps do localization by embedding the locale into URLs. This removes all issues with HTTP negotiation.

Thus the challenge I'm posting here: why? :)

GregoireHebert · 2020-04-21T12:49:29Z

Hello @nicolas-grekas :)

What's the purpose of this feature? HTTP negotiation is tricky, as explained on the doc of Varnish and on Cloudflare (which actively works against HTTP negotiation btw).

We can agree that, as defined in RFC 2616 the content language(s) should be expressed by the Content-Language Header. Which is not the case at the moment in Symfony. It could be one or multiple languages, but let stick to a default behaviour where our content is only made of one language.

Thus as corollary, we should be able to serve the data according to the Accept-Language Header as we are already able to serve it according to the Accept-Content Header. (which is used by API-Platform for content-negociation)

Having this managed by default allows us to stick closer to the HTTP behaviour,
and serve a content in the proper language. And for some edge cases, the translator should even rely on the Accept-Languages to fallback according to the user preferences (which is not the purpose of this PR, but could be another one).

It does not override the URL locale behaviour, which is still the best solution when using cloudflare.

or we could move the setting out of the translation subtree.
again, having twice the same setting is calling for troubles to me.

I applied @dunglas solution for now but find it "hacky".
If I find your solution fancier, it's BC-break right?
oh, or maybe you mean renaming available_locales to enabled_locales
with the actual behaviour, to deprecate the previous translation key?

nicolas-grekas · 2020-04-21T20:39:27Z

Instead of copying the request on the response, which might be just wrong, what about using the locale - the one that the locale-aware services manage?

GregoireHebert · 2020-04-22T09:08:18Z

I am not sure to follow your train of thought.
If by locale-aware you mean using the localeAwareInterface, yes sure I could do that, but the difference is that it always falls back with the default locale and the check on the parent request, which I am not sure is a good idea.

I'll check but I have the feeling that
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/EventListener/LocaleAwareListener.php#L49 is a mistake, where it should not force it that way... let me check the tests.

Oh ok, Let' say this:
Accept-Language: fr, en
available_locales: en, mi <-- here EN matches
default_locale: mi

Now I create a Request, that end up having an en locale thanks to the Accept-Language and available_locales (it could come from enabled_locales it's the same really)
If I use the LocaleAwareInterface or kernel.local_aware tag, for a master request, I'll end up with 2 setLocale calls. Once with the actual request local (en), and then a second time with the default locale (mi), which is wrong.

Was that an intended behaviour or is it missing a check upon wether it's a master request or not? I feel like this should expect to be fr https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Tests/EventListener/LocaleAwareListenerTest.php#L56

And anyway, it's a copy from the request, of course, we should be able to serve the result the closest what has been asked.

Something else to do maybe, could be to keep track of every langage used by the translator, (in addition to the classic behaviour of, "I want a language and you shall give me the most approaching result") and if it differs from what's expected, then setup the different languages used by the translator. (but in another translator related listener I suppose, and in another PR ^^).

WDYT?

nicolas-grekas · 2020-04-22T09:15:00Z

My thought is that HTTP negotiation is risky, look eg at this patch, which fixes a CVE. I'm not sure we should default to using HTTP negotiation at all.

The URL is a much better way to select the locale, both internally (the app) and for clients (browsers).

GregoireHebert · 2020-04-22T09:39:11Z

Alright, I let go this PR... It's a pity.
Still, could you have a look at the possible bug in the LocaleAwareListener?

@dunglas I think we should follow the same recommendation for API-Platform then.

fabpot · 2020-06-24T14:06:31Z

What's the status of this PR? @nicolas-grekas?

nicolas-grekas · 2020-06-24T14:11:10Z

My reasoning here is that this adds complexity, has risks (cache poisoning) and very specific benefits.
I don't think we should merge this into core. But maybe I missed something.

nicolas-grekas · 2020-06-24T14:11:37Z

Closing meanwhile.
Thanks for proposing @GregoireHebert

GregoireHebert · 2020-08-28T07:47:42Z

Hello @nicolas-grekas :)

I'd like to challenge you (and the community of course) again on this one :p
I heard your concerns and dug into it.

This is not really a discouraged usage of HTTP. When I look at the documentation you sent, it's more of a cache sharding behaviour that is to be expected. Not a cache poisoning.

I saw your accept-content revert commit, and it's not the same thing at all, it is against the specification itself, while accept-language is not.

Anyway in terms of risk of DDOS attack the fact that one could theorically ping every language authorized possible does not change regardless the way of doing it. URL nor Header.

All that being said, I don't see this as much as a risk for 2 reasons. It's part of the HTTP specification, and it's opt-in.
We need this for API-Platform, so we might end up adding this in it, but since Symfony is an HTTP Framework, we think it's worth it to have it directly in it.

Moreover, since the browsers are already set with one's prefered languages it's a win-win for everyone in my opinion.
WDYT ? :)

ogizanagi · 2021-07-04T09:51:27Z

Hi @GregoireHebert 👋🏻

This subject came back in the core team recently. Since this is opt-in and already targets to restrict the locales forwarded from the request to the response, we're open to re-discuss it.
I found myself creating a similar AcceptLanguageListener in multiple apps. This a core HTTP feature and many apps & libraries such as API-Platform would benefit from this being available in Symfony's core, so I'm in favor of it 👍🏻

Would you be up to work back on this?

GregoireHebert · 2021-07-04T13:47:23Z

Yup sure, with pleasure!
I might have free time this week :)

…n (GregoireHebert) This PR was merged into the 5.4 branch. Discussion ---------- [HttpKernel] Add basic support for language negotiation | 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). Commits ------- 904b54f [HttpKernel] Add basic support for language negotiation

[HttpFoundation] add a default Content-Language to the Response

bb52d5d

carsonbot added Status: Needs Review HttpFoundation Feature labels Apr 20, 2020

nicolas-grekas reviewed Apr 20, 2020

View reviewed changes

dunglas requested changes Apr 20, 2020

View reviewed changes

src/Symfony/Component/HttpKernel/EventListener/LocaleListener.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 20, 2020

nicolas-grekas added this to the next milestone Apr 21, 2020

GregoireHebert added 2 commits April 21, 2020 14:21

fix review

6cb2fe8

update changelogs

6f0d7a9

nicolas-grekas closed this Jun 24, 2020

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

nicolas-grekas mentioned this pull request Nov 12, 2020

WIP: Use Accept-Language to select the locale api-platform/core#3826

Closed

nicolas-grekas mentioned this pull request Jun 22, 2021

Improve support for primary language selection (according to header field “Accept-Language”) #41455

Closed

alanpoulain mentioned this pull request Jun 24, 2021

[ADR] How to deliver expected locale to the server Sylius/Sylius#12753

Closed

chalasr mentioned this pull request Sep 20, 2021

[HttpKernel] Add basic support for language negotiation #43108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] add a default Content-Language to the Response #36507

[HttpFoundation] add a default Content-Language to the Response #36507

GregoireHebert commented Apr 20, 2020 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas Apr 20, 2020

GregoireHebert Apr 20, 2020

nicolas-grekas Apr 20, 2020

dunglas Apr 20, 2020

nicolas-grekas Apr 21, 2020

nicolas-grekas commented Apr 21, 2020 •

edited

Loading

GregoireHebert commented Apr 21, 2020

nicolas-grekas commented Apr 21, 2020

GregoireHebert commented Apr 22, 2020 •

edited

Loading

nicolas-grekas commented Apr 22, 2020 •

edited

Loading

GregoireHebert commented Apr 22, 2020

fabpot commented Jun 24, 2020

nicolas-grekas commented Jun 24, 2020

nicolas-grekas commented Jun 24, 2020

GregoireHebert commented Aug 28, 2020

ogizanagi commented Jul 4, 2021 •

edited

Loading

GregoireHebert commented Jul 4, 2021

[HttpFoundation] add a default Content-Language to the Response #36507

[HttpFoundation] add a default Content-Language to the Response #36507

Conversation

GregoireHebert commented Apr 20, 2020 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Apr 20, 2020

Choose a reason for hiding this comment

GregoireHebert Apr 20, 2020

Choose a reason for hiding this comment

nicolas-grekas Apr 20, 2020

Choose a reason for hiding this comment

dunglas Apr 20, 2020

Choose a reason for hiding this comment

nicolas-grekas Apr 21, 2020

Choose a reason for hiding this comment

nicolas-grekas commented Apr 21, 2020 • edited Loading

GregoireHebert commented Apr 21, 2020

nicolas-grekas commented Apr 21, 2020

GregoireHebert commented Apr 22, 2020 • edited Loading

nicolas-grekas commented Apr 22, 2020 • edited Loading

GregoireHebert commented Apr 22, 2020

fabpot commented Jun 24, 2020

nicolas-grekas commented Jun 24, 2020

nicolas-grekas commented Jun 24, 2020

GregoireHebert commented Aug 28, 2020

ogizanagi commented Jul 4, 2021 • edited Loading

GregoireHebert commented Jul 4, 2021

GregoireHebert commented Apr 20, 2020 •

edited

Loading

nicolas-grekas commented Apr 21, 2020 •

edited

Loading

GregoireHebert commented Apr 22, 2020 •

edited

Loading

nicolas-grekas commented Apr 22, 2020 •

edited

Loading

ogizanagi commented Jul 4, 2021 •

edited

Loading