Skip to content

[1.x] fix: resolve translation references in implicitly-loaded fallback catalogues - #5024

Open
gianniguida wants to merge 1 commit into
flarum:1.xfrom
gianniguida:gg/fix-translator-fallback-catalogue-parse-1x
Open

[1.x] fix: resolve translation references in implicitly-loaded fallback catalogues#5024
gianniguida wants to merge 1 commit into
flarum:1.xfrom
gianniguida:gg/fix-translator-fallback-catalogue-parse-1x

Conversation

@gianniguida

Copy link
Copy Markdown
Contributor

Backport of #5023 to 1.x. See that PR for the full write-up; summary below.

Symptom

Compiled locale JS assets (e.g. admin-en.js) sometimes contain unresolved translation references — literal values like:

"core.admin.nav.dashboard_button":"=> core.admin.dashboard.title"

so the admin UI displays raw => core.admin.… strings. Only the fallback locale (en) is affected; the poisoned compiled asset persists until a cache flush (its revision exists, so it is never recompiled). Found in production on Flarum 1.8.19 (community.sbb.ch) and confirmed end-to-end there: touching getCatalogue('de') before recompiling the English admin locale asset deterministically produces the broken asset; removing that single line produces a clean one.

Root cause

Translator::getCatalogue() gated =>-reference resolution on a locale-keyed flag (! isset($this->catalogues[$locale])). But Symfony's Translator::loadFallbackCatalogues() (symfony/translation v5.4), when loading any other locale, pre-stores the original en catalogue in $this->catalogues['en'] while attaching only a copy to the requesting catalogue. Loading de first therefore stores a raw, never-parsed en original; a later getCatalogue('en') finds the locale already set, skips parsing, and returns it raw. Flarum\Frontend\AddTranslations then compiles the literal => … strings into the locale JS asset.

Deterministic repro (locale de, fallback ['en'], non-debug, cache dir — mirroring LocaleServiceProvider):

$t->getCatalogue('en');
$t->getCatalogue('en')->get('core.admin.nav.dashboard_button'); // "Dashboard"            ✔

// fresh translator, fresh cache:
$t->getCatalogue('de');   // implicit fallback load of 'en'
$t->getCatalogue('en')->get('core.admin.nav.dashboard_button'); // "=> core.admin.dashboard.title"  ✘

Fix

Track parsed state per catalogue object instead of per locale name, and walk the fallback chain on every getCatalogue() call — the raw stored original and its parsed copy share a locale name, so a locale-keyed flag cannot distinguish them. Per-object tracking parses each catalogue object exactly once; parseCatalogue() is unchanged.

Differences from the 2.x fix, for PHP 7.3 compatibility: SplObjectStorage (contains()/attach()) instead of WeakMap, untyped private property.

Tests

New TranslatorReferenceResolutionTest (PHPUnit 9 style), wired like LocaleServiceProvider:

  • en loaded directly → resolved (regression guard).
  • en first loaded implicitly as de's fallback, then requested directly → resolved (failed before this fix with '=> bar' vs 'Resolved').
  • de's attached fallback copy resolved, via getFallbackCatalogue() and trans() fallthrough (regression guard).

Full unit suite and PHPStan pass on 1.x.

Context

Surfaced while working on FriendsOfFlarum/redis#34 — multi-instance cache invalidation increases recompile frequency, raising exposure — but reproducible on a bare single-instance install.

🤖 Generated with Claude Code

…alogues

Backport of the 2.x fix to 1.x.

Flarum's Translator::getCatalogue() only ran `=>`-reference resolution
(parseCatalogue()) when the requested locale was not yet present in
$this->catalogues. But Symfony's Translator::loadFallbackCatalogues()
pre-stores the *original* fallback catalogue in $this->catalogues while
attaching only a fresh copy to the requesting locale's catalogue.

So loading e.g. 'de' first stored a raw, never-parsed 'en' original;
a later getCatalogue('en') found the locale already set, skipped
parsing, and returned it with unresolved references. Consumers that
snapshot the catalogue - most damagingly Frontend\AddTranslations,
which compiles the locale JS assets - then emitted literal
"=> core.admin.dashboard.title" strings, and the poisoned compiled
asset persisted until a cache flush (revisions gate recompiles).

Track parsed state per catalogue *object* (SplObjectStorage, since 1.x
must stay PHP 7.3-compatible) instead of per locale name, and walk the
fallback chain on every access. The raw stored original and its parsed
copy share a locale name, so a locale-keyed flag cannot distinguish
them; per-object tracking parses each catalogue object exactly once.
parseCatalogue() is unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gianniguida
gianniguida requested a review from a team as a code owner September 2, 2026 14:05
gianniguida added a commit to gianniguida/redis that referenced this pull request Sep 7, 2026
…ntract

Review follow-up. No behaviour change (src/ diff vs the fix commit is
comments only).

- Formatter: justify leaving storage/formatter/* by parity with core's own
  runtime refresh (Formatter::flush() and the Formatter extender only forget
  the cache entry) rather than by the production log line, which is an
  E_WARNING that also occurs on healthy parse-only requests. The renderer
  class is autoloaded DURING unserialize(), so the deletion window is
  microseconds. Document the residuals: same-hash rebuilds rewrite the live
  class file in place, and concurrent cache misses each rebuild it.
- Views: core does delete storage/views/* on the acting pod for extensions
  that register views; the reason other pods can leave them is that compiled
  views are keyed by source path and expire by source mtime.
- OPcache: drop the unsupported "classes fail during the swap" rationale;
  the targeted invalidation is belt-and-braces (Symfony re-invalidates on
  rewrite; no-op from the CLI). Note that the per-SAPI double apply now buys
  little.
- DISTRIBUTED_CACHE.md: the manifest race is reduced (fewer writers), not
  eliminated, and core's single flush is itself a dozen sequential writes;
  attribute the racing-refill half of the residual to this extension's
  settings layer; state the real per-apply cost, including that Saved
  triggers a full apply core does not need; point operators at the core
  1.x fallback-catalogue bug (flarum/framework#5024) that propagation
  cannot fix. README FAQ updated.
- Test: always seed rev-manifest.json with a revision for the asset
  sentinel (a re-added flush is a no-op against an empty manifest, which is
  what a second run inherited), name the sentinel forum.js, assert
  flarum.formatter is forgotten, clean seeded files up in tearDown only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@gianniguida

Copy link
Copy Markdown
Contributor Author

Adding context on why I think this qualifies under the 1.x critical-fixes-only policy, with what we've learned running it in production since the PR was opened.

It's user-visible breakage that doesn't heal itself. Once the raw => core.… strings are baked into admin-en.js / forum-en.js, the asset's revision exists, so core never recompiles it. English users see the broken UI until an admin clears the cache — and the very next admin action can re-poison it. On our forum this presented as "the admin panel is unreadable in English after saving any setting", which was escalated as a production incident before we understood it was core.

It's not a multi-instance edge case. The trigger is simply: a non-English catalogue gets compiled before the English one is touched, in the same request. That happens on a single-instance install whenever the site's default locale isn't en and catalogues have just been cleared — i.e. right after cache:clear, an extension toggle, or a settings save (Extend\Locales::onEnable/onDisableLocaleManager::clearCache()). Every multilingual forum whose default locale isn't English is exposed on every admin action. Multi-instance setups just roll the dice N times instead of once.

There is no admin-level workaround. The only mitigation short of this patch is site-level PHP: a middleware that calls getCatalogue('en') before anything else in the request (we ship one, with a control test that reproduces the bug against the installed core). That's not something a forum operator can do, and it costs an extra catalogue load + parse on every non-English request forever.

The fix is already accepted on 2.x (#5023, merged 2026-09-02). This backport is the same logic with SplObjectStorage in place of WeakMap for PHP 7.3; parseCatalogue() itself is untouched, and the new test fails before / passes after. Risk surface is one method in Translator.

Happy to adjust anything for 1.x conventions. If a 1.8.20 is on the table at all, I'd argue this belongs in it: deterministic, silent, persists until manual intervention, and every non-English-default forum is affected.

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.

1 participant