Skip to content

FEATURE: Localization fallbacks (server-side)#2

Open
zaibkhan wants to merge 1 commit into
localization-system-prefrom
localization-system-post
Open

FEATURE: Localization fallbacks (server-side)#2
zaibkhan wants to merge 1 commit into
localization-system-prefrom
localization-system-post

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

The FallbackLocaleList object tells I18n::Backend::Fallbacks what order the
languages should be attempted in. Because of the translate_accelerator patch,
the SiteSetting.default_locale is *not* guaranteed to be fully loaded after the
server starts, so a call to ensure_loaded! is added after the locale is set for
the current user.

The declarations of config.i18n.fallbacks = true in the environment files were
actually garbage, because the I18n.default_locale was
SiteSetting.default_locale, so there was nothing to fall back to. *derp*
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Centralize I18n fallbacks, ensure lazy-loading
What’s good: Moves fallback configuration to a single initializer with a clear fallback chain and integrates with the translate accelerator to preload fallback locales.
Review Status: ✅ Safe to merge

Issues (Medium)

Severity Issue Why it matters
Medium Correctness — Potential NoMethodError when default_locale is nil …/initializers/i18n.rb
Calling to_sym on a nil default locale will raise NoMethodError (e.g., during boot or misconfiguration), preventing fallbacks from resolving. Guard the conversion to keep this path safe.
Medium Maintainability — Order-dependent fallback preload in ensure_loaded! …/initializers/i18n.rb
ensure_loaded! depends on the global I18n.locale; if called before I18n.locale is set in set_locale, it may preload fallbacks for the previous locale and miss the intended one for the current request. Accept an optional locale to remove ordering dependence.

Showing up to 2 medium issue(s). See inline suggestions for more details.

Key Feedback (click to expand)
  • Needs improvement: Avoid order dependence on I18n.locale when ensuring fallbacks are loaded and guard against nil default locale to prevent boot-time NoMethodError.
  • Testing: Add a controller/request spec verifying the fallback chain order [user locale → site default → en] is honored when a key is missing in the user locale. Include a test for the edge case where SiteSetting.default_locale is temporarily nil (or unset during boot) to ensure no crash and that compact/uniq logic behaves correctly.
  • Documentation: Consider a brief comment in the initializer noting that FallbackLocaleList intentionally replaces I18n::Locale::Fallbacks and only needs to respond to #[], plus that ensure_loaded! is required due to on-demand loading in the translate accelerator.
  • Compatibility: Overriding I18n.fallbacks with a custom object may affect gems expecting I18n::Locale::Fallbacks; this is likely fine since only #[] is used by I18n::Backend::Fallbacks, but worth noting for future integrations.
  • Performance: Fallback preloading runs once per unique locale due to @loaded_locales; negligible overhead after warm-up.
  • Open questions: Is SiteSetting.default_locale guaranteed non-nil at boot? In set_locale, is I18n.locale assigned before calling I18n.fallbacks.ensure_loaded!?

Confidence: 4/5 — Looks good; minor fixes (2 medium)

Sequence Diagram

sequenceDiagram
    participant Controller
    participant I18nFallbacks as I18n.fallbacks
    participant I18n
    Controller->>I18nFallbacks: ensure_loaded!()
    I18nFallbacks->>I18n: ensure_loaded!(locale)
    loop each fallback locale
        I18n->>I18n: load_locale(locale)
    end
Loading

React with 👍 or 👎 if you found this review useful.

# user locale, site locale, english
# TODO - this can be extended to be per-language for a better user experience
# (e.g. fallback zh_TW to zh_CN / vice versa)
[locale, SiteSetting.default_locale.to_sym, :en].uniq.compact

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: Calling to_sym on a nil default locale will raise NoMethodError (e.g., during boot or misconfiguration), preventing fallbacks from resolving. Guard the conversion to keep this path safe.

Suggested change
[locale, SiteSetting.default_locale.to_sym, :en].uniq.compact
[locale, (SiteSetting.default_locale && SiteSetting.default_locale.to_sym), :en].compact.uniq

[locale, SiteSetting.default_locale.to_sym, :en].uniq.compact
end

def ensure_loaded!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: ensure_loaded! depends on the global I18n.locale; if called before I18n.locale is set in set_locale, it may preload fallbacks for the previous locale and miss the intended one for the current request. Accept an optional locale to remove ordering dependence.

Suggested change
def ensure_loaded!
def ensure_loaded!(locale = I18n.locale)
self[locale].each { |l| I18n.ensure_loaded! l }
end

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