Allow all available locales for template lookups #45169
Conversation
| @@ -19,7 +19,7 @@ class PathParser # :nodoc: | |||
| def build_path_regex | |||
| handlers = Template::Handlers.extensions.map { |x| Regexp.escape(x) }.join("|") | |||
| formats = Template::Types.symbols.map { |x| Regexp.escape(x) }.join("|") | |||
| locales = "[a-z]{2}(?:[-_][A-Z]{2})?" | |||
| locales = I18n.available_locales.map { |x| Regexp.escape(x) }.join("|") | |||
There was a problem hiding this comment.
You can use Regexp.union
There was a problem hiding this comment.
Thanks a lot! That is a great tip. I was (half) blindly just copying what was done above, but Regexp.union sure makes it nicer. I changed this now and took the liberty to adjust the two lines above as well for consistency reasons.
|
Hey @oneiros thanks for taking time to read the discussion. When I've researched about the issue, at some point this behavior was decided to be changed for a explicit REGEX ( I don't know why 🤷♂️ ) I think that it would be best that if Or maybe to union it too with the locales = Regexp.union( I18n.available_locales.map { |x| Regexp.escape(x) } + [/[a-z]{2}(?:[-_][A-Z]{2})?/] ) What do you think about it ? PS: Some of tests related to this didn't pass. |
I think that is the best way. This makes it 100% backwards compatible to the behavior introduced in Rails 7.0. And this should work for rare edge cases, where With this change I can also remove a lot of code I needed to add to existing tests. And this should also fix the failing tests outside of Thank you so much for the suggestion! |
oneiros
force-pushed
the
allow_all_available_locales_for_template_lookup
branch
from
cf6dcca
to
b5650f2
Compare
|
Greaat work @oneiros ! |
|
Thanks! I think this is a great fix to the issue without introducing regressions for folks who have views which they don't have in I18n.avalable_locales. Would you mind squashing the two commits into one (and remove the |
Following the discussion here: https://github.com/rails/rails/pull/44174/files#r785160819 Background: The `i18n` gem is relatively lax when it comes to naming locales. It does not enforce any standard. Thus it is possible to have e.g. per tenant locales (think `en_tenant1`, `en_tenant2` etc.). This also worked for translated templates up until rails 6.1. Rails 7 changed the template lookup and enforced a naming scheme for locales. This poses a problem for legacy apps that use non-standard locale names. This commit changes the way locale names are detected in template file names. In addition to the previously used regexp it also allows all known locales from `I18n.available_locales`. This makes it backwards compatible to rails 7.0 behavior while also allowing non-standard locale names. Thanks to jvillarejo for the great idea. Also introduce the usage of `Regexp.union`, a wonderful suggestion by casperisfine.
oneiros
force-pushed
the
allow_all_available_locales_for_template_lookup
branch
from
b5650f2
to
12c1289
Compare
Done. Thank you so much for accepting this. It will be a great help.
Oops. I did not know that. Thanks for the heads up. BTW, does this mean you plan to backport this to 7.0.x? This would of course be absolutely phenomenal. But if it is not possible, we might be able to live with a monkeypatch until 7.1 comes out. Thanks again to everyone who helped out with this! It is greatly appreciated. |
…es_for_template_lookup Allow all available locales for template lookups (cherry picked from commit a5b2b61)
…ales-for-template-lookup Backport (#45169): Allow all available locales for template lookups.
Summary
This is a follow up to #44154 and especially the discussion here: https://github.com/rails/rails/pull/44174/files#r785160819
This PR tries to implement the idea from that discussion, to just use
I18n.available_localesto build a list of allowed locales in a template file name.Other Information
Background: The
i18ngem is relatively lax when it comes to naming locales. It does not enforce any standard. Using non-standard locale names also worked fine with translated templates up until (and including) rails 6.1.Rails 7 changed the template lookup and enforced a naming scheme for locales. This poses a problem for legacy apps that use non-standard locale names.
In my case, I am currently trying to upgrade an app to rails 7 that is big on multitenancy and uses i18n to allow for per-tenant text changes and layout differences. Thus the tenant name is sometimes part of the locale. I worked on another app (not yet on rails 7) that used non-standard locale names to provide an alternative version of the site in "simple language", something that was required by law in this case.
Stemming from the discussion in the ticket mentioned above, the naming rules have already been relaxed a little. But as the discussion in the PR shows, this is not enough for all apps.
This PR is only a suggestion. I would be glad to have any solution that would let me keep the locale names in the aformentioned apps. And of course I am happy to help in any way I can.