Skip to content
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

Fall through to I18n.fallbacks when defined #180

Merged
merged 4 commits into from
Mar 10, 2018

Conversation

shioyama
Copy link
Owner

@shioyama shioyama commented Mar 8, 2018

Fixes #161.

This is necessary to avoid leakage when the default locale is changed
(even temporarily) in specs.
@shioyama
Copy link
Owner Author

shioyama commented Mar 9, 2018

@ingemar

# config/application.rb
config.i18n.default_locale = :sv
config.i18n.fallbacks = {
  en: %i[sv],
  de: %i[en sv],
}

And here's what I get with this PR:

irb(main):002:0> I18n.fallbacks[:sv]
=> [:sv]
irb(main):003:0> I18n.fallbacks[:en]
=> [:en, :sv]
irb(main):004:0> I18n.fallbacks[:de]
=> [:de, :en, :sv]
irb(main):005:0> Mobility.new_fallbacks[:sv]
=> [:sv]
irb(main):006:0> Mobility.new_fallbacks[:en]
=> [:en, :sv]
irb(main):007:0> Mobility.new_fallbacks[:de]
=> [:de, :sv, :en]

Important thing here is that you can also add model-specific fallbacks, and they will be "mixed in" with the default fallbacks:

irb(main):008:0> Mobility.new_fallbacks(de: [:fr])[:de]
=> [:de, :fr, :sv, :en]

This means that you have I18n.fallbacks, but in a model if you pass fallbacks: { de: [:fr] }, then French will be the first fallback, followed by whatever is in I18n.fallbacks. This is an edge case but it will be important because I will probably add support for "dynamic" fallbacks (e.g. fallbacks specified from user settings, etc.)

@darkcode85
Copy link

darkcode85 commented Mar 9, 2018

@shioyama I tested this and it works as expected and now Mobility respect the I18n.fallbacks 👍

For "dynamic" cases what I'm finally doing is to have a before_action and override the static fallbacks in this way: I18n.fallbacks[I18n.locale] = [I18n.locale, current_user.language.shortname] not sure if this is thread safe (it's difficult to me identify if the code is thread safe or not) but I it works using puma without issues.

@shioyama
Copy link
Owner Author

Great to hear that it works.

About dynamic cases, I can add support for passing a proc to fallbacks in the model. Then you could set:

class Post < ApplicationRecord
  extend Mobility
  translates :title, fallbacks: -> { current_user.language.shortname }
end

or something like that.

The point about thread safety is basically that if you change global state from one thread, there is the possibility that another thread will change the same data before you actually use your value. Basically mutable globals are bad for thread safety, and in this case I18n.fallbacks is a mutable global (you are changing it in/for one thread/user to a value that will not be appropriate in/for another thread/user).

But if you don't have a lot of requests, you may not notice this problem.

@darkcode85
Copy link

@shioyama that solution looks great for dynamic cases, just one question current_user or any variable set in application_controller.rb will be available to pass in the fallback block (fallbacks: -> { ... })? It is in the model scope so not sure if these variables will be available.

@shioyama
Copy link
Owner Author

Yes, you're right that won't work since current_user is not in scope.

Actually, in this case the safer way to do it would be to pass in the user's fallbacks to the getter method, like this (assuming a model Post and translated attribute title):

post.title(fallback: current_user.language.shortname)

this will try the current locale, and if it is nil will fallback to current_user.language.shortname. By passing shortname from the user directly to the getter method, you can avoid using a global (i.e. I18n.fallbacks or something else), which is bad for many reasons.

@darkcode85
Copy link

@shioyama thanks, yes I think that it will work although I believe that could be interesting to have an option to set it globally instead to have to pass it each time we call a translated attribute (not sure if set it globally and be thread safe is possible). I believe that if we have to pass the dynamic value in each getter method for all the translated attributes in the application code can be difficult to maintain.

@shioyama
Copy link
Owner Author

I believe that could be interesting to have an option to set it globally instead to have to pass it each time we call a translated attribute

In this case, I'd say it's better to explicitly pass in the fallbacks through the getter. Using globals for convenience is not something I would recommend, thread safety is one issue but not the only one.

Having an option to use a Proc for fallbacks on the other hand is something mobility could support. I'll think about it.

@shioyama shioyama deleted the default_to_i18n_fallbacks branch March 14, 2018 07:17
bricesanchez added a commit to refinery/refinerycms that referenced this pull request Apr 8, 2018
bricesanchez added a commit to refinery/refinerycms that referenced this pull request Apr 10, 2018
parndt pushed a commit to refinery/refinerycms that referenced this pull request May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants