-
Notifications
You must be signed in to change notification settings - Fork 119
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
Default Accept-Language
to I18n.default_locale
#50
Conversation
Currently AutoLocale tries to set `I18n.locale` to nil when the Accept-Language header is missing, which leads to "nil is not a valid locale" error when `I18n.config.enforce_available_locales` is true (default). This commit fixes the problem by defaulting the header value to `I18n.default_locale`.
I18n.locale = http_accept_language.compatible_language_from(I18n.available_locales) | ||
hal = http_accept_language | ||
hal.header ||= I18n.default_locale.to_s | ||
I18n.locale = hal.compatible_language_from(I18n.available_locales) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work as well?
I18n.locale = http_accept_language.compatible_language_from(I18n.available_locales) || I18n.default_locale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DouweM Thanks for taking a look! I'm afraid it wouldn't, because compatible_language_from
may return nil when the Accept-Language header is given and contains no available locales. Applying a fallback to those cases as well will spoil the enforce_available_locales
concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knu I'm not sure I understand. If none of the locales in Accept-Language
are supported by the website, why wouldn't you want to fall back to the default locale?
With your code, I18n.locale
can still end up being set to nil
, which, as your write, leads to a "nil is not a valid locale" error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is expected behavior when enforce_available_locales
is set to true and the client only lists unsupported locales, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I would assume you would want to still present your website to the user using the default locale (usually English) rather than error out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it's your call to decide how HttpAcceptLanguage::AutoLocale behaves. You could easily implement your own variant to meet your needs anyway, like refusing a fallback by returning 406, adding a Content-Language response header, etc.
Is there any concerns with merging this? /cc @DouweM |
Default `Accept-Language` to `I18n.default_locale`
Thanks @knu! |
@DouweM hey, could you please release a new version with this fix? Thanks! |
Currently AutoLocale tries to set
I18n.locale
to nil when the Accept-Language header is missing, which leads to "nil is not a valid locale" error whenI18n.config.enforce_available_locales
is true (default).This commit fixes the problem by defaulting the header value to
I18n.default_locale
.