Use I18n.locale as the only source of truth#71930
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ff72772ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8ff7277 to
c3618c4
Compare
I18n.locale as the only source of truth
Hamms
left a comment
There was a problem hiding this comment.
Great cleanup, thank you! I do have a couple of questions, but the only one I'm actually concerned about is the js_locale removal; everything else is just curiosity.
Speaking of curiosity, do you happen to know how Rails determines i18n.locale internally? I'm specifically interested to know more about precisely in what situation it deviates from value we end up with in request.locale, given all the work we do in the middleware that sets it.
| @@ -1,3 +1,2 @@ | |||
| - js_locale = request.locale.to_s.downcase.tr('-', '_') | |||
There was a problem hiding this comment.
why don't we need this anymore? Looks to me like it's still being referenced in the webpack_asset_path call just below
There was a problem hiding this comment.
We already have a helper method with the same name:
code-dot-org/dashboard/app/helpers/locale_helper.rb
Lines 18 to 21 in 76004b9
|
|
||
| protected def with_locale(&block) | ||
| I18n.with_locale(locale, &block) | ||
| I18n.with_locale(request.locale, &block) |
There was a problem hiding this comment.
Why don't we want the change to locale_helper.rb to apply here?
There was a problem hiding this comment.
I'm not sure I fully understand the question. Could you please clarify what you mean?
There was a problem hiding this comment.
Why do we want this to continue to use request.locale, rather than I18n.locale?
There was a problem hiding this comment.
request.locale determines which locale should be used for the current request based on the language cookie and, soon, also on the browser language. We then need to pass that resolved locale to the I18n backend so it can render the site using the correct locale. The next step would be to assign the language cookie value or browser language value directly through I18n.with_locale and remove request.locale entirely
| # end | ||
| require 'devise/custom_failure' | ||
| config.warden do |manager| | ||
| manager.failure_app = Devise::CustomFailure |
There was a problem hiding this comment.
I love that we can finally remove this! I would slightly prefer to see and the devise gem update bundled together in their own PR, but that's not a blocking concern
Previously, in many places we used
request.localeas if it were the application's actual locale. That is misleading becauserequest.localerepresents only the requested or inferred locale for the current request, not necessarily the final locale the app renders with.The real source of truth for rendering is
I18n.locale, since that is what the I18n backend actually uses to resolve translations and locale dependent behavior. When we rely onrequest.localein controllers, views, helpers, or other app logic, we risk using a locale value that does not match the one ultimately used for rendering.This change makes locale handling more consistent by using
I18n.localefor locale dependent application behavior and treatingrequest.localeonly as the request level input used to determine that final locale.