Enable the Rails/I18nLocaleAssignment cop to scan all the application ruby files

[Senen]

References

Discussion from PR #4629

Objectives

Enable the Rails/I18nLocaleAssignment cop to scan all application ruby files.

This cop scans only the tests files by default, but we prefer to scan all application Ruby files, so when a developer uses the class method I18n.locale=, the cop will embrace using the method I18n.with_locale instead. By doing this way, the cop will help developers to avoid unexpected translation errors.

Quoting the Rails 6 guides:

I18n.locale can leak into subsequent requests served by the same thread/process if it is not consistently set in every controller. For example executing I18n.locale = :es in one POST requests will have effects for all later requests to controllers that don't set the locale, but only in that particular thread/process. For that reason, instead of I18n.locale = you can use I18n.with_locale which does not have this leak issue.

Now we enabled the cop for all application Ruby files; we have to remove the assignments at the controller level to set the request locale. As Rails 6 guides suggest [1], we can use the around_action controller callback to set each request locale without breaking the rule.

[1] https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests

[javierm]

After reading the comments at rails/rails#34356 and the commit messages from this pull request, if I understand correctly, we aren't changing the controllers in order to avoid leaks between requests, but in order to avoid the case where developers could set the locale somewhere else without realizing the possible consequences of doing so. Is that correct?

If that's the case, I'm fine with the changes 👌.

I've left a couple of comments as usual 😉.

[Senen]

After reading the comments at rails/rails#34356 and the commit messages from this pull request, if I understand correctly, we aren't changing the controllers in order to avoid leaks between requests, but in order to avoid the case where developers could set the locale somewhere else without realizing the possible consequences of doing so. Is that correct?

Interesting conversation, thanks for sharing it. Yes, that's the idea! Using the before_filter to set the locale is not wrong as the method I18n.locale= is thread-safe, but as CONSUL forks can uses gems with engines I thought it's better to use the new suggested way.

[Senen]

Hi @javierm,

What do you think about changing the name of the set_locale controller methods to switch_locale as the Rails 6 documentation example? It's not a big deal, it is just to follow Rails guides as much as we can 🤔

[javierm]

@Senen No strong preference, so whatever you prefer is fine 👌.