FEATURE: Localization fallbacks (server-side)

[everettbu]

Test 9

[github-actions]

This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days.

[maloyan4good]

Code Review

Verdict: NEEDS DISCUSSION
Confidence: HIGH

Summary

This PR introduces server-side localization fallbacks by replacing the Rails-level config.i18n.fallbacks = true setting with a custom FallbackLocaleList class that enforces a [user_locale, site_locale, :en] fallback chain. It also consolidates the pluralization initializer into the new i18n.rb initializer and adds an ensure_loaded! helper to the translate accelerator.

Findings

Priority	Issue	Location
P1	ensure_loaded! in FallbackLocaleList calls I18n.ensure_loaded! but the translate accelerator's ensure_loaded! is not thread-safe — it initializes @loaded_locales outside the mutex	lib/freedom_patches/translate_accelerator.rb:62-65
P1	FallbackLocaleList#ensure_loaded! reads I18n.locale at call time, but set_locale calls it after setting I18n.locale — if locale is not yet set (e.g. during boot/reload), I18n.locale defaults to :en, silently loading only English and skipping the site locale	config/initializers/i18n.rb:20-22
P2	FallbackLocaleList inherits from Hash but only overrides [] — other Hash methods (fetch, values_at, merge, etc.) will return unexpected results if called by the i18n backend internals	config/initializers/i18n.rb:12
P2	I18n.backend.class.send(:include, I18n::Backend::Fallbacks) uses .class which will include the module into the concrete backend class globally and permanently — if the backend is ever swapped or wrapped (e.g. in tests), this silently breaks	config/initializers/i18n.rb:8
P2	The translate accelerator's translate method caches by "\#{key}\#{config.locale}\#{config.backend.object_id}" but does not account for fallbacks — a cache hit for a missing key in the user locale will return the cached miss rather than falling through to the fallback	lib/freedom_patches/translate_accelerator.rb:68-76
P3	ensure_loaded! in the translate accelerator duplicates the guard already present in load_locale (which checks @loaded_locales.include? inside the mutex) — the outer check in ensure_loaded! is a TOCTOU race	lib/freedom_patches/translate_accelerator.rb:62-65

Details

[P1] has a TOCTOU race on

File: lib/freedom_patches/translate_accelerator.rb:62-65

The new method initializes @loaded_locales outside the LOAD_MUTEX and checks include? without holding the lock. load_locale itself is mutex-protected, but the guard check in ensure_loaded! is not, creating a race window in multi-threaded Rails servers (Puma).

# Current (racy)
def ensure_loaded!(locale)
  @loaded_locales ||= []
  load_locale locale unless @loaded_locales.include?(locale)
end

# Safer — delegate entirely to load_locale which already holds the mutex
def ensure_loaded!(locale)
  load_locale(locale)
end
# load_locale already returns early if locale is already loaded (inside the mutex)

[P1] in may silently use wrong locale during boot

File: config/initializers/i18n.rb:20-22

FallbackLocaleList#ensure_loaded! reads I18n.locale at call time. During a reload! cycle (triggered by execute_reload in the accelerator), ensure_all_loaded! is called which also calls I18n.fallbacks[locale] — but this path is fine. The risk is that set_locale calls I18n.fallbacks.ensure_loaded! after setting I18n.locale, which is correct for the request path. However, the method name ensure_loaded! on the fallback list is confusing because it conflates two responsibilities: computing the fallback list and triggering locale loading. Consider passing the locale explicitly:

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

And in set_locale:

I18n.fallbacks.ensure_loaded!(I18n.locale)

This makes the dependency explicit and avoids implicit global state reads.

[P2] Translation cache does not account for fallback misses

File: lib/freedom_patches/translate_accelerator.rb:68-76

The LRU cache key is "\#{key}\#{config.locale}". If a key is missing in the user locale and falls back to the site locale or :en, the result is cached under the user locale key — which is correct. But if the key is genuinely missing in all fallback locales, translate_no_cache raises/returns a missing-translation string, and that miss is cached. This was true before this PR too, but the new fallback chain makes it more likely to hit this path. Not a blocker, but worth noting.

Recommendation

Address the thread-safety issue in ensure_loaded! (P1) by delegating directly to load_locale which already handles the mutex guard. Clarify the FallbackLocaleList#ensure_loaded! API by passing the locale explicitly rather than reading I18n.locale implicitly. The Hash inheritance for FallbackLocaleList is fragile — consider a plain Struct or BasicObject subclass, or at minimum document which Hash interface the i18n backend depends on.

[mfeuerstein]

PR Review — approved

Reviewed 7 files. 0 high-severity issues found. Verdict: approved.

app/controllers/application_controller.rb (low)

• Reviewed app/controllers/application_controller.rb — looks good

lib/freedom_patches/translate_accelerator.rb (low)

• Reviewed lib/freedom_patches/translate_accelerator.rb — looks good

config/cloud/cloud66/files/production.rb (low)

• Reviewed config/cloud/cloud66/files/production.rb — looks good

config/initializers/pluralization.rb (low)

• Reviewed config/initializers/pluralization.rb — looks good

config/environments/profile.rb (low)

• Reviewed config/environments/profile.rb — looks good

config/initializers/i18n.rb (medium)

• Reviewed config/initializers/i18n.rb — looks good

config/environments/production.rb (low)

• Reviewed config/environments/production.rb — looks good

[ron-x5labs]

Code Review: FEATURE: Localization fallbacks (server-side)

Problem

Introduce server-side I18n locale fallbacks so that when a translation key is missing from a user's locale, the lookup falls through a chain of [user_locale, site_default_locale, :en] instead of returning a missing-translation string. Previously config.i18n.fallbacks = true made Rails fall back only to the process-global I18n.default_locale (:en).

Solution Reviewed

Removes config.i18n.fallbacks = true from the production/profile/cloud66 configs and replaces it with a new config/initializers/i18n.rb that includes I18n::Backend::Pluralization + I18n::Backend::Fallbacks into the backend, defines a custom FallbackLocaleList < Hash whose [] returns [locale, SiteSetting.default_locale.to_sym, :en].uniq.compact, and sets I18n.fallbacks = FallbackLocaleList.new. The deleted pluralization.rb initializer is folded into the new one. ApplicationController#set_locale now calls I18n.fallbacks.ensure_loaded! per request, and the translate accelerator gains an ensure_loaded!(locale) helper to on-demand-load each fallback locale.

Summary

The design is sound and the cutover is clean (the Rails config.i18n.fallbacks = true removal is actually required — Rails' i18n_railtie would otherwise overwrite the custom FallbackLocaleList). However there is one blocking correctness bug: the process-global LRU translation cache key omits the now-per-site fallback target, so on a multisite deployment two sites with different default_locale pollute each other's cached fallback translations. Fallbacks are not bypassed by the translate_no_cache alias (verified against i18n 0.7.0 — it delegates dynamically to config.backend.translate, which is Fallbacks#translate), so the mechanism works within web requests once the cache-key issue is fixed.

Files Reviewed

• config/initializers/i18n.rb — deeply reviewed
• lib/freedom_patches/translate_accelerator.rb — deeply reviewed
• app/controllers/application_controller.rb — deeply reviewed
• config/environments/production.rb — lightly reviewed (clean removal)
• config/environments/profile.rb — lightly reviewed (clean removal)
• config/cloud/cloud66/files/production.rb — lightly reviewed (clean removal)
• config/initializers/pluralization.rb — lightly reviewed (deleted; content moved verbatim into i18n.rb)

Verification

• ruby -c / test suite — skipped: no Ruby/Bundler runtime in the review environment. Changed files were read in full and confirmed well-formed; the i18n control flow (Fallbacks#translate, Config#backend sharing, cache delegation) was verified against the i18n 0.7.0 gem source on GitHub.

Notes on prior review findings (verified independently)

• The earlier P1 TOCTOU race in ensure_loaded! is not a real new bug: ensure_loaded! delegates to load_locale, which re-checks @loaded_locales.include? inside LOAD_MUTEX — the same double-checked-locking pattern the pre-existing translate method (line 68) already uses. @loaded_locales is initialized to [] by reload! at boot and is never nil at runtime, so the ||= [] is a no-op after boot.
• The P2 FallbackLocaleList < Hash fragility is not a real bug: i18n 0.7.0's Backend::Fallbacks#translate only calls I18n.fallbacks[locale].each — no fetch/values_at/merge/[]= is invoked, so overriding [] alone satisfies the contract.
• The P2 I18n.backend.class.send(:include) concern is not a real bug: I18n.backend lazily defaults to I18n::Backend::Simple and is never reassigned in this repo, so .class reliably targets Simple.

Issues Found

Blocking

• lib/freedom_patches/translate_accelerator.rb:72 — see inline comment (anchored to the translate method).

Non-blocking

• config/initializers/i18n.rb:17 — string/symbol locale mismatch defeats .uniq dedup (see inline).
• lib/freedom_patches/translate_accelerator.rb:64 — fallbacks no-op (and can poison the shared cache) in jobs/mailers/with_locale that set I18n.locale without calling ensure_loaded! (see inline).
• app/controllers/application_controller.rb:159 — ensure_loaded! propagates YAML load errors → 500 on every request, no graceful degradation (see inline).
• lib/freedom_patches/translate_accelerator.rb:56 — a missing fallback locale file is silently marked loaded with zero translations, no observability (see inline, anchored to load_locale).
• No tests cover the new fallback contract. spec/controllers/application_controller_spec.rb only asserts I18n.locale, not that I18n.fallbacks is a FallbackLocaleList, the chain order, or that ensure_loaded! loads each fallback locale. A regression to the chain order, .uniq.compact, or the load loop would go undetected.

Suggestions

• config/initializers/i18n.rb:1 — the # order: after 02-freedom_patches.rb comment implies an order: directive mechanism that does not exist; Rails loads initializers alphabetically, so the ordering holds today only by accident (digits sort before i). Since the only cross-dependency (I18n.ensure_loaded!) is resolved at request time, not load time, consider dropping the misleading comment.
• config/initializers/i18n.rb:17 — SiteSetting.default_locale.to_sym raises NoMethodError if default_locale is ever nil (the repo already guards this elsewhere, e.g. lib/tasks/db.rake). A nil guard (SiteSetting.default_locale&.to_sym) would match that defensive pattern.

Verdict

Request changes before merge — the per-site fallback target must enter the cache key (or the cache must be invalidated on default_locale change) to avoid serving wrong-language translations across multisite sites; the remaining items are quality/observability gaps worth addressing in the same pass.

[ron-x5labs]

Blocking — cache key omits the per-site fallback target, causing cross-site translation leakage on multisite.

A few lines below this, the LRU key (line 72) is "#{key}#{config.locale}#{config.backend.object_id}". I18n::Config#backend is a class variable (@@backend in i18n 0.7.0), shared across all threads and all multisite sites, so backend.object_id is identical for every site; @cache (line 71) is one shared LruRedux::ThreadSafeCache on the I18n singleton. Pre-PR, config.i18n.fallbacks = true made Rails fall back to the global I18n.default_locale (:en) — the same target for every site — so the key was sufficient. This PR changes the fallback target to the per-site SiteSetting.default_locale.to_sym (via FallbackLocaleList#[]), but the cache key still doesn't capture it.

Concrete failure: Site A (default_locale :de) and Site B (default_locale :fr) both have a Polish user (:pl). A key missing from pl.yml resolves via fallback. Whichever site first translates that key caches the result under "<key>:pl:<backend_id>"; the other site's Polish user then gets a cache hit and receives the wrong language (e.g. Site B's user sees German). The 300-entry LRU only churns on cold keys, so hot UI strings stay wrong until eviction. Separately, I18n.reload! (the only cache clearer, line 41) is wired by Rails to a FileUpdateChecker over I18n.load_path — never to a SiteSetting change — so within a single site a runtime default_locale change also serves stale cached fallback results.

Fix: include the resolved fallback list (or SiteSetting.default_locale, or a site id such as RailsMultisite::ConnectionManagement.current_db) in the cache key, and clear @cache when default_locale changes.

[ron-x5labs]

Non-blocking — string/symbol mismatch defeats the .uniq dedup, causing a redundant double-load of the site-default locale.

I18n.locale is a String here: set_locale assigns current_user.effective_locale / SiteSetting.default_locale, both strings (i18n 0.7.0's I18n.locale= stores the value verbatim, no to_sym). But SiteSetting.default_locale.to_sym and :en are Symbols. Array#uniq uses eql?/hash, and "en".eql?(:en) is false, so when the user locale equals the site default (or is en) the chain is e.g. ["en", :en, :en].uniq => ["en", :en] rather than [:en].

Downstream, ensure_loaded! (line 21) calls I18n.ensure_loaded! for both "en" and :en; the accelerator guards @loaded_locales with exact == equality (translate_accelerator.rb:48/64), so the same locale's YAML is parsed and loaded twice on the first request after every boot/reload, and @loaded_locales permanently holds mixed-type duplicates. Translations still resolve via the first matching fallback, so this is a perf/dedup defect, not a correctness break. (Note: i18n's own I18n::Locale::Fallbacks#[] normalizes with to_sym — this custom class should too.) Fix: [locale.to_sym, SiteSetting.default_locale.to_sym, :en].uniq.compact.

[ron-x5labs]

Non-blocking — fallbacks silently no-op (and can poison the shared cache) outside the HTTP request path.

This new ensure_loaded! is called only from ApplicationController#set_locale (application_controller.rb:159). Background jobs set I18n.locale = SiteSetting.default_locale directly (app/jobs/base.rb:151) without loading fallbacks, and I18n.with_locale blocks (lib/post_destroyer.rb:102, app/services/post_alerter.rb) and rake tasks do likewise. The accelerator's translate (line 68) on-demand-loads only config.locale, never the fallback chain. In such a context, Fallbacks#translate iterates the chain but each fallback locale's file was never loaded → throw(:exception, MissingTranslation) → the default handler returns the 'translation missing' string, which is .freeze'd and cached under key+config.locale on the shared global LRU. A job/console running before the matching web request thus caches 'translation missing' for that key+locale, defeating fallback resolution for web requests too until LRU eviction.

Fix: eager-load the fallback chain wherever I18n.locale is set for a non-request context (or have translate load I18n.fallbacks[config.locale] instead of only config.locale).

(Note: fallbacks are not bypassed by the translate_no_cache alias — it delegates dynamically to config.backend.translate, which is Fallbacks#translate once the module is included into the backend class. So the mechanism works in web requests once the cache-key issue is fixed.)

[ron-x5labs]

Non-blocking — ensure_loaded! propagates YAML load errors with no graceful handling, turning a single corrupt fallback file into a 500 on every request.

FallbackLocaleList#ensure_loaded! → I18n.ensure_loaded! → load_locale → I18n.backend.load_translations → load_file/load_yml, which re-raises malformed YAML as I18n::InvalidLocaleData (i18n 0.7.0 base.rb). None of load_locale, I18n.ensure_loaded!, or FallbackLocaleList#ensure_loaded! rescue it, and ApplicationController declares no rescue_from for I18n::InvalidLocaleData / Psych::SyntaxError. Because the fallback chain always includes :en and SiteSetting.default_locale, a single corrupt always-present fallback YAML (e.g. en.yml) takes down the entire site on every request — a broader blast radius than the previous lazy single-locale loading, which only loaded config.locale and only for the user's own locale. Consider skipping a broken fallback tier and continuing with the next (degrading gracefully) rather than failing the whole request, plus a log line.

[ron-x5labs]

Non-blocking — a missing/misnamed fallback locale file is silently marked loaded with zero translations, no observability.

Inside load_locale above this end: I18n.load_path.grep(Regexp.new("\\.#{locale}\\.yml$")) (line 56) returns [] when no file matches, and the overridden load_translations (lines 23-26) is a no-op for an empty list — yet @loaded_locales << locale (line 58) still runs. So a fallback locale whose file isn't on I18n.load_path (a default_locale whose [locale].yml is missing, or a typo'd locale) is permanently recorded as loaded with zero translations, never retried, with no error and no log. The only symptom is unexpected 'translation missing' strings. This PR's new ensure_loaded! (line 62) eagerly drives load_locale for every fallback locale, surfacing this silent degradation for the fallback chain. Consider checking that the grep matched at least one file (or that the locale loaded non-empty translations) and logging a warning when it didn't.

[ron-x5labs]

Code Review: FEATURE: Localization fallbacks (server-side)

Problem

Introduce server-side I18n locale fallbacks so that when a translation key is missing from a user's locale, the lookup falls through a chain of [user_locale, site_default_locale, :en] instead of returning a missing-translation string. Previously config.i18n.fallbacks = true made Rails fall back only to the process-global I18n.default_locale (:en).

Solution Reviewed

Removes config.i18n.fallbacks = true from the production/profile/cloud66 configs and replaces it with a new config/initializers/i18n.rb that includes I18n::Backend::Pluralization + I18n::Backend::Fallbacks into the backend, defines a custom FallbackLocaleList < Hash whose [] returns [locale, SiteSetting.default_locale.to_sym, :en].uniq.compact, and assigns it to I18n.fallbacks. A new ensure_loaded!(locale) on the translate accelerator preloads fallback locales; ApplicationController#set_locale calls I18n.fallbacks.ensure_loaded! on every request.

Summary

The core design is sound and the initializer/ordering/module-includes are correct. However, there is one blocking multisite cache-isolation bug: the fallback chain is now per-site (depends on SiteSetting.default_locale) but the translate accelerator's shared LRU cache key does not include the site, so two sites with different default locales can serve each other's cached fallback translations. Several non-blocking gaps remain around error resilience, job-path coverage, and missing tests.

Files Reviewed

• config/initializers/i18n.rb — deeply reviewed (NEW, high risk: defines fallback chain + global I18n.fallbacks wiring)
• lib/freedom_patches/translate_accelerator.rb — deeply reviewed (high risk: shared cache + new ensure_loaded!)
• app/controllers/application_controller.rb — deeply reviewed (medium risk: per-request ensure_loaded! call)
• config/environments/production.rb — lightly reviewed (removal of config.i18n.fallbacks)
• config/environments/profile.rb — lightly reviewed (removal of config.i18n.fallbacks)
• config/cloud/cloud66/files/production.rb — lightly reviewed (removal of config.i18n.fallbacks)
• config/initializers/pluralization.rb — lightly reviewed (DELETED; content preserved verbatim in i18n.rb:3-5; no external references found)

Verification

• grep 'def locale=' lib/ app/ config/ — confirmed no freedom patch overrides I18n::Config#locale= (i18n 0.7.0 setter symbolizes via locale.to_sym), so I18n.locale is always a Symbol; the string/symbol .uniq concern raised during review was a false positive and dropped.
• grep 'rescue_from' app/controllers/application_controller.rb — confirmed no rescue_from for I18n::InvalidLocaleData / Psych::SyntaxError.
• grep -r 'config.i18n.fallbacks' config/ — confirmed clean cutover: no remaining occurrences in any environment file.
• grep -r 'FallbackLocaleList|ensure_loaded|fallbacks' spec/ — confirmed zero test coverage for the new fallback feature.
• No bundle/rake available in this environment; typecheck/build skipped.

Verdict

Recommend changes before merge — the multisite cache-poisoning bug is a tenancy-isolation defect that should be fixed (cache key must include the site discriminator) before this ships.

[ron-x5labs]

🔴 Blocking — multisite cache poisoning: cross-site translation leakage.

The fallback chain is now per-site ([locale, SiteSetting.default_locale.to_sym, :en]), so translate_no_cache produces site-dependent results for the same (key, locale). But the translate accelerator caches results in a single process-wide LruRedux::ThreadSafeCache on the I18n module singleton, keyed only "#{key}#{config.locale}#{config.backend.object_id}" (translate_accelerator.rb:72). I18n::Config#backend is a class variable (@@backend in i18n 0.7.0) → object_id is identical for every multisite site, and config.locale collides across sites whenever two sites have a user with the same locale.

Result: site A (default fr) warms a cache entry whose fallback resolved to a French string; site B (default es) with a de-locale user hits the same key and gets site A's French fallback served — a tenancy-isolation failure reachable via every no-arg I18n.t('key') call.

Pre-PR this was safe because config.i18n.fallbacks = true fell back to the global I18n.default_locale (:en) — site-independent. This PR introduces the site-dependence that breaks the cache invariant.

# translate_accelerator.rb — include the site discriminator:
k = "#{key}#{config.locale}#{config.backend.object_id}#{SiteSetting.default_locale}"

Or make @cache per-site (keyed by RailsMultisite::ConnectionManagement.current_db).

[ron-x5labs]

🟡 Non-blocking — corrupt fallback YAML → 500 on every request, no circuit breaker.

I18n.fallbacks.ensure_loaded! → I18n.ensure_loaded! → load_locale → backend.load_translations → load_file, which re-raises malformed YAML as I18n::InvalidLocaleData / Psych::SyntaxError. This runs on every request via set_locale. There is no rescue_from for those classes in ApplicationController (verified: only RenderEmpty, RateLimiter::LimitExceeded, PG::ReadOnlySqlTransaction, Discourse::NotLoggedIn/NotFound/InvalidAccess/ReadOnly). Because :en is unconditionally in the chain, a single corrupt en.yml takes down every request for every user and every multisite site. load_locale skips the @loaded_locales << locale push on exception, so there's no circuit breaker — it retries and 500s on every request until the file is fixed.

# Wrap the preload so a corrupt file degrades gracefully:
I18n.fallbacks.ensure_loaded! rescue nil
# or add a dedicated rescue_from for I18n::InvalidLocaleData / Psych::SyntaxError

[ron-x5labs]

🟡 Non-blocking — fallbacks only wired into the web request path; jobs / with_locale / rake are silently half-wired.

ensure_loaded! is called only from set_locale. Background jobs (app/jobs/base.rb:151) set I18n.locale = SiteSetting.default_locale but never call ensure_loaded!, and the accelerator's translate (translate_accelerator.rb:67) only load_locales config.locale (the primary) — never the rest of the fallback chain. So in a job, a key missing from the primary locale falls through to :en (or the site default), which was never loaded → MissingTranslationData instead of the fallback string. Email/notification translation regresses vs. the web path. Same gap affects I18n.with_locale blocks and rake tasks.

# Option A — preload in jobs too (app/jobs/base.rb after I18n.locale = ...):
I18n.fallbacks.ensure_loaded!

# Option B — have translate load the whole chain, not just the primary:
# translate_accelerator.rb translate():
I18n.fallbacks[config.locale].each { |l| load_locale(l) unless @loaded_locales.include?(l) }

[ron-x5labs]

🟡 Non-blocking — missing locale file silently marked loaded with zero translations.

load_locale (line 56) greps I18n.load_path.grep(/\.#{locale}\.yml$/); when no file matches it returns [], and the overridden load_translations (lines 23-26) is a no-op for an empty list — yet @loaded_locales << locale (line 58) still runs. So a fallback locale whose yml is missing (a default_locale with no [locale].yml, or a typo) is permanently recorded as loaded with zero translations, never retried, with no error and no log. The new ensure_loaded! force-loads the full chain on every request, so this silent-empty-load now happens eagerly per request. Only symptom: unexpected 'translation missing' strings with no operator signal.

# In load_locale, log when a locale resolves to no files:
files = I18n.load_path.grep(Regexp.new("\\.#{locale}\\.yml$"))
Rails.logger.warn("I18n: no translation files found for locale #{locale}") if files.empty?
I18n.backend.load_translations(files)

[ron-x5labs]

🟡 Non-blocking — FallbackLocaleList replaces I18n::Locale::Fallbacks, dropping the .map / .defaults= plugin API.

In i18n 0.7.0 the default I18n.fallbacks is an I18n::Locale::Fallbacks instance whose documented API includes .map(:ca => :"es-ES"), .defaults=, and parent-locale computation. FallbackLocaleList defines only [] and ensure_loaded!. A plugin using the documented I18n.fallbacks.map(...) to register custom mappings (e.g. zh_TW -> zh_CN, which this file's own TODO anticipates) would now raise NoMethodError. Core Discourse doesn't use these methods (verified by grep), so this is a plugin-facing contract change.

# Subclass and override only [] to preserve the extension API:
class FallbackLocaleList < I18n::Locale::Fallbacks
  def [](locale)
    [locale.to_sym, SiteSetting.default_locale.to_sym, :en].uniq.compact
  end
  # ...ensure_loaded!...
end

[ron-x5labs]

🟡 Non-blocking — no test coverage for the new fallback feature.

grep -r 'FallbackLocaleList|ensure_loaded|fallbacks' spec/ returns zero matches; the accelerator itself has no existing specs either. A feature that changes translation loading on every request, touches the shared multisite LRU cache, and runs in all environments should have regression guards. Critical flows to cover:

1. Fallback chain order: FallbackLocaleList[locale] == [locale, default_locale.to_sym, :en] with dedup.
2. Multisite cache isolation — assert two sites with different default_locale don't share cached fallback translations (the blocking bug above).
3. ensure_loaded! idempotency — repeated calls don't reload.
4. Missing-locale-file behavior — ensure_loaded! doesn't crash when a fallback yml is absent.
5. Job-path fallback — a job that hits a missing key resolves via the fallback chain.

Note: spec/spec_helper.rb sets I18n.locale = :en (Symbol) before each test, so the real request-path locale flow is never exercised — adding tests here would also close that gap.

[ron-x5labs]

Code Review: FEATURE: Localization fallbacks (server-side)

Problem

Introduce server-side I18n fallbacks so a translation key missing from a user's locale resolves through [user_locale, site_default_locale, :en] instead of returning a missing-translation string. Previously config.i18n.fallbacks = true (set in production/profile/cloud66 configs) made Rails fall back only to the global I18n.default_locale (:en).

Solution Reviewed

Removes config.i18n.fallbacks = true from the three environment configs and replaces it with a new config/initializers/i18n.rb that folds in the deleted pluralization.rb initializer, includes I18n::Backend::Pluralization + I18n::Backend::Fallbacks into the backend, defines a FallbackLocaleList < Hash whose [] returns [locale, SiteSetting.default_locale.to_sym, :en].uniq.compact, and assigns it to I18n.fallbacks. ApplicationController#set_locale now calls I18n.fallbacks.ensure_loaded! per request, and the translate accelerator gains ensure_loaded!(locale) to on-demand-load each fallback locale. The config.i18n.fallbacks = true removal is actually required — Rails' i18n railtie would otherwise overwrite the custom FallbackLocaleList.

Summary

The design is sound and the cutover is clean, but there is one blocking tenancy-isolation bug: the shared, process-wide translation cache key does not capture the now-per-site fallback target, so on a multisite deployment two sites with different default_locale serve each other's cached fallback translations. Several non-blocking gaps remain around job-path coverage, error resilience, the dropped I18n::Locale::Fallbacks API, and missing tests.

Files Reviewed

• config/initializers/i18n.rb — deeply reviewed (NEW, high risk)
• lib/freedom_patches/translate_accelerator.rb — deeply reviewed (high risk: shared cache + new ensure_loaded!)
• app/controllers/application_controller.rb — deeply reviewed (medium risk: per-request ensure_loaded!)
• config/environments/production.rb — lightly reviewed (clean removal)
• config/environments/profile.rb — lightly reviewed (clean removal)
• config/cloud/cloud66/files/production.rb — lightly reviewed (clean removal)
• config/initializers/pluralization.rb — lightly reviewed (deleted; content moved verbatim into i18n.rb)

Verification

• i18n 0.7.0 gem source (lib/i18n.rb, config.rb, backend/fallbacks.rb, locale/fallbacks.rb) read from GitHub — confirmed: Config#locale= symbolizes (locale.to_sym), Config#backend is @@backend (shared class var), I18n.translate delegates to backend.translate -> Backend::Fallbacks#translate iterates I18n.fallbacks[locale], and I18n::Locale::Fallbacks exposes .map/defaults=/parent-locale compute.
• grep -r 'FallbackLocaleList|ensure_loaded|fallbacks' spec/ — zero matches (no test coverage).
• grep of app/jobs/base.rb, lib/post_destroyer.rb, lib/tasks/db.rake — confirm non-request paths set I18n.locale without ensure_loaded!.
• grep rescue_from app/controllers/application_controller.rb — no handler for I18n::InvalidLocaleData/Psych::SyntaxError.
• ls config/locales — regional variants present (pt_BR, zh_TW, ...) and base pt.yml exists (relevant to parent-locale fallback).
• No Ruby/Bundler runtime available in this environment; ruby -c/test suite skipped. All claims above are grounded in the read gem source and repo greps.
• A prior review on this PR flagged a string/symbol .uniq dedup double-load as non-blocking; verified FALSE and dropped — I18n::Config#locale= always symbolizes, so all three chain entries are Symbols and dedup is correct.

Verdict

Request changes before merge — the multisite cache-poisoning bug is a tenancy-isolation defect (wrong-language strings served across sites) and must be fixed (add the site discriminator to the cache key, or make the cache per-site) before this ships; the remaining items are quality/coverage gaps worth addressing in the same pass.

[ron-x5labs]

🔴 Blocking — multisite cache poisoning: cross-site translation leakage.

translate caches the no-arg result in a process-wide LruRedux::ThreadSafeCache (@cache on the I18n singleton, line 71) keyed "#{key}#{config.locale}#{config.backend.object_id}" (line 72). I18n::Config#backend is a class variable (@@backend in i18n 0.7.0), so backend.object_id is identical for every multisite site, and @cache is shared across all threads — the key is effectively key + locale. Pre-PR, config.i18n.fallbacks = true fell back to the global :en, so the cached result was site-independent. This PR makes the fallback target the per-site SiteSetting.default_locale (FallbackLocaleList#[]), so the resolved string for the same (key, locale) now differs per site, but the cache key still doesn't capture the site. Result: site A (default_locale :de) warms "key:pl:<backend>" with a German fallback string; site B (default_locale :fr) with a :pl user hits that entry and is served German instead of French — a tenancy-isolation failure on every I18n.t('key').

# translate_accelerator.rb — include the site discriminator in the key:
k = "#{key}#{config.locale}#{config.backend.object_id}#{SiteSetting.default_locale}"

(Verified against i18n 0.7.0: Config#locale= symbolizes, Config#backend is @@backend, and I18n.translate delegates to backend.translate -> Backend::Fallbacks#translate, so the cached value is the fallback-resolved string.)

[ron-x5labs]

🟡 Non-blocking — fallbacks are wired into the web request path only.

ensure_loaded! is called solely from set_locale. Background jobs set I18n.locale = SiteSetting.default_locale directly (app/jobs/base.rb:151) and I18n.with_locale blocks (lib/post_destroyer.rb:102) / rake tasks (lib/tasks/db.rake) do the same, without preloading the fallback chain. The accelerator's translate (line 68) on-demand-loads only config.locale (the primary), never the rest of I18n.fallbacks[locale], so a key missing from the primary locale in a job yields a MissingTranslation/'translation missing' string rather than the fallback — emails and notifications regress versus the web path. Either call I18n.fallbacks.ensure_loaded! wherever I18n.locale is set for a non-request context, or have translate load the whole I18n.fallbacks[config.locale] chain.

[ron-x5labs]

🟡 Non-blocking — no circuit breaker: a corrupt fallback YAML 500s every request.

ensure_loaded! -> load_locale -> backend.load_translations -> load_file re-raises malformed YAML as I18n::InvalidLocaleData / Psych::SyntaxError, and set_locale runs this on every request. ApplicationController declares no rescue_from for those classes, and :en is unconditionally in the fallback chain, so a single corrupt en.yml takes down every request for every user (and every multisite site) until the file is fixed. load_locale skips the @loaded_locales << locale push on exception, so there is no caching of the failure — it reloads and 500s on every request. Consider wrapping the preload (I18n.fallbacks.ensure_loaded! rescue nil) or adding a rescue_from so a corrupt file degrades to missing-translation strings rather than a hard 500.

[ron-x5labs]

🟡 Non-blocking — no test coverage for the new fallback feature.

grep -r 'FallbackLocaleList|ensure_loaded|fallbacks' spec/ returns zero matches, and the accelerator has no existing specs. A feature that changes translation loading on every request and touches the shared multisite LRU cache should have regression guards: (1) FallbackLocaleList[locale] == [locale, default_locale.to_sym, :en] with dedup; (2) multisite cache isolation (the blocking bug above); (3) ensure_loaded! idempotency; (4) missing-locale-file behavior doesn't crash; (5) job-path fallback resolves via the chain.

[ron-x5labs]

🟡 Non-blocking — FallbackLocaleList drops the I18n::Locale::Fallbacks extension API.

Pre-PR, config.i18n.fallbacks = true gave Rails an I18n::Locale::Fallbacks instance whose documented API includes .map(:ca => :es), .defaults=, and parent-locale computation (e.g. :"es-MX" -> [:"es-MX", :es, :en]). FallbackLocaleList < Hash overrides only []/ensure_loaded!, so I18n.fallbacks.map(...) now raises NoMethodError — notable because this file's own TODO (fallback zh_TW to zh_CN) anticipates exactly that kind of mapping. Core has no current callers (verified by grep), so this is plugin-facing. Subclass and override only [] to preserve the API:

class FallbackLocaleList < I18n::Locale::Fallbacks
  def [](locale)
    [locale.to_sym, SiteSetting.default_locale.to_sym, :en].uniq.compact
  end
  def ensure_loaded!; self[I18n.locale].each { |l| I18n.ensure_loaded! l }; end
end

[ron-x5labs]

Code Review: FEATURE: Localization fallbacks (server-side)

Problem

Introduce server-side I18n fallbacks so a translation key missing from a user's locale resolves through [user_locale, site_default_locale, :en] instead of returning a missing-translation string. Previously config.i18n.fallbacks = true (set in production/profile/cloud66 configs) made Rails fall back only to the global I18n.default_locale (:en).

Solution Reviewed

Removes config.i18n.fallbacks = true from the three environment configs and replaces it with a new config/initializers/i18n.rb that folds in the deleted pluralization.rb initializer, includes I18n::Backend::Pluralization + I18n::Backend::Fallbacks into the backend, defines a FallbackLocaleList < Hash whose [] returns [locale, SiteSetting.default_locale.to_sym, :en].uniq.compact, and assigns it to I18n.fallbacks. ApplicationController#set_locale now calls I18n.fallbacks.ensure_loaded! per request, and the translate accelerator gains ensure_loaded!(locale) to on-demand-load each fallback locale. The config.i18n.fallbacks = true removal is actually required — Rails' i18n railtie would otherwise overwrite the custom FallbackLocaleList.

Summary

The design is sound and the cutover is clean, but there is one blocking tenancy-isolation bug: the shared, process-wide translation cache key does not capture the now-per-site fallback target, so on a multisite deployment two sites with different default_locale serve each other's cached fallback translations. Several non-blocking gaps remain around job-path coverage, error resilience, the dropped I18n::Locale::Fallbacks API, and missing tests.

Files Reviewed

• config/initializers/i18n.rb — deeply reviewed (NEW, high risk)
• lib/freedom_patches/translate_accelerator.rb — deeply reviewed (high risk: shared cache + new ensure_loaded!)
• app/controllers/application_controller.rb — deeply reviewed (medium risk: per-request ensure_loaded!)
• config/environments/production.rb — lightly reviewed (clean removal)
• config/environments/profile.rb — lightly reviewed (clean removal)
• config/cloud/cloud66/files/production.rb — lightly reviewed (clean removal)
• config/initializers/pluralization.rb — lightly reviewed (deleted; content moved verbatim into i18n.rb)

Verification

• i18n 0.7.0 gem source (lib/i18n.rb, config.rb, backend/fallbacks.rb, locale/fallbacks.rb) read from GitHub — confirmed: Config#locale= symbolizes (locale.to_sym), Config#backend is @@backend (shared class var), I18n.translate delegates to backend.translate -> Backend::Fallbacks#translate iterates I18n.fallbacks[locale], and I18n::Locale::Fallbacks exposes .map/defaults=/parent-locale compute.
• grep -r 'FallbackLocaleList|ensure_loaded|fallbacks' spec/ — zero matches (no test coverage).
• grep of app/jobs/base.rb, lib/post_destroyer.rb, lib/tasks/db.rake — confirm non-request paths set I18n.locale without ensure_loaded!.
• grep rescue_from app/controllers/application_controller.rb — no handler for I18n::InvalidLocaleData/Psych::SyntaxError.
• ls config/locales — regional variants present (pt_BR, zh_TW, ...) and base pt.yml exists (relevant to parent-locale fallback).
• No Ruby/Bundler runtime available in this environment; ruby -c/test suite skipped. All claims above are grounded in the read gem source and repo greps.
• A prior review on this PR flagged a string/symbol .uniq dedup double-load as non-blocking; verified FALSE and dropped — I18n::Config#locale= always symbolizes, so all three chain entries are Symbols and dedup is correct.

Verdict

Request changes before merge — the multisite cache-poisoning bug is a tenancy-isolation defect (wrong-language strings served across sites) and must be fixed (add the site discriminator to the cache key, or make the cache per-site) before this ships; the remaining items are quality/coverage gaps worth addressing in the same pass.

[ron-x5labs]

Note: the review was accidentally submitted twice due to a shell command error (the submission command ran twice).

• Canonical review: id 4997825751 (CHANGES_REQUESTED) — contains all 5 inline findings at the diff anchors. Treat this as the review of record.
• Duplicate: id 4997826011 (CHANGES_REQUESTED) — its 5 inline comments were deleted. The review body could not be dismissed/edited/deleted with the available token permissions (GitHub returns 404 for those operations). Please disregard it.

Apologies for the duplicate.