BoltTranslate / Translate

Provides translation for contenttypes.
Other
43 stars 37 forks source link

Redirect to locale based on Accept-Language header #53

Closed peterverraedt closed 8 years ago

peterverraedt commented 8 years ago

Use the Accept-Language header to determine the best locale, if the _locale parameter is not set in the url. More precisely:

Two remarks:

madc commented 8 years ago

My two cents: It doesn't feel right to change the default locale depending on the browser configuration. Also, I see a possible issue with #40. If the default locale does not have a locale slug anymore, this changes the website links depending on the browser language(, which is a very bad thing..). That means, mysite.com/de would exist for an "english" browser, but not for a "german" one.

I think, we should rather redirect to the preferred locale, than change the settings for default one. Beside being less error prone, this should also be easier to implement. Also, this definitely needs a configuration flag to turn it on/off.

SvanteRichter commented 8 years ago

First of all, Thank you @peterverraedt for the PR!

If I understand the objective correctly (not having looked at the code yet) this only redirects if there is no _locale in the url (like on / or a 404), so for example:

German visitior to "mysite.com/en" gets english site
English visitor to "mysite.com/de" gets german site
German visitor to mysite.com gets redirected to mysite.com/de
English visitor to mysite.com gets redirected to mysite.com/en

Am I correct in that?

peterverraedt commented 8 years ago

@SahAssar That is correct. I'll update the PR description to make this more clearly. @madc The 'default locale' has a double meaning, it would not be the same as the 'default locale' in #40. This PR would only change the 'default redirection locale', i.e. the locale to which is redirected to on /. Implementing #40 would still be possible, by either disabling this redirection, or by allowing that / either serves content in the 'first locale' or redirects to another locale homepage, e.g. /de.

madc commented 8 years ago

I see. Thanks for implementing this, its without doubt a useful feature. When do you think it could be ready to merge? I'd like to hold #40 back until you're done..

peterverraedt commented 8 years ago

@madc I only wanted to add a cleaner solution for the urlGenerator override. In 27a54a9 this is achieved, I believe. The code should now be ready to merge.

SvanteRichter commented 8 years ago

Alright, merging!