Skip to content

fix(core): resolve topbar/prefs app names with dgettext against ownin…#189

Merged
ralflang merged 2 commits into
horde:FRAMEWORK_6_0from
jcdelepine:fix/topbar
Jul 3, 2026
Merged

fix(core): resolve topbar/prefs app names with dgettext against ownin…#189
ralflang merged 2 commits into
horde:FRAMEWORK_6_0from
jcdelepine:fix/topbar

Conversation

@jcdelepine

Copy link
Copy Markdown
Contributor

Problem

Horde_Core_Topbar renders app names via plain _(), which uses the
currently active default gettext domain rather than the domain owning
the string. Since $params['name'] holds each app's raw msgid
(left untranslated at registry-parse time), non-current apps in the
menu silently fall back to the raw English name whenever the active
domain doesn't match.

Symptom

Topbar/prefs menu shows English app names ("Mail", "Calendar", "Task")
instead of localized ones, depending on which app is current. Confirmed
on two identical checkouts (same commit, same .mo files), each with a
different pattern — consistent with the active domain depending on
request execution order, not persisted state.

Fix

Use dgettext($app, $params['name']), resolved explicitly per app
instead of relying on the ambient default domain. For the prefs
submenu, translation happens in a pass over $prefs_apps before
uasort(), since the comparator only sees values, not app keys;
_sortByName() now just does strcoll() on already-resolved names.

Already-translated entries (Administration, Preferences, Help, etc.)
are unaffected — verified, not assumed.

Testing

Reproduced reliably on two independent installs from the same commit;
fix verified correct for kronolith/turba/imp/nag/mnemo in both the
topbar and preferences menu regardless of current app.

jcdelepine and others added 2 commits July 2, 2026 22:39
…g app's domain

_() in Topbar::getTree() and _sortByName() translates against the
ambient default gettext domain, not the domain of the app owning the
name. Non-current apps intermittently fall back to raw English msgids.
Use dgettext($app, ...) instead, resolved before sorting in the prefs
submenu since uasort()'s comparator has no access to the app key.
…lder

dgettext() only resolves against a domain that was bound this request,
but Horde binds an app's domain only when the app is pushed. Apps that
merely appear in the topbar/prefs menu (never pushed) therefore still
fell back to the raw English name. Bind the owning app's domain
(bindtextdomain + bind_textdomain_codeset) before dgettext(); dgettext()
leaves the active default domain untouched, so no save/restore is needed.

Resolve names where they originate, so headings and links -- which have
no dedicated app domain -- keep the ambient _() behaviour instead of
being forced through a per-app domain that would leave them untranslated.

Apply the same fix to the modern src/Topbar/TopbarBuilder.php, which had
the identical _() bug and is used by the desktop chrome renderer.

Add TopbarBuilder regression tests: a deterministic guard that app names
trigger per-app domain resolution while headings do not, plus a
behavioural test proving translation via the owning domain.
@TDannhauer

Copy link
Copy Markdown
Contributor

I pushed a follow-up commit to this branch (fix/topbar) that hardens the fix and extends it. The diagnosis here is correct; the change just needed two extra pieces to be robust.

1. dgettext($app, ...) needs the domain bound first

dgettext() only resolves against a domain that has been bound (bindtextdomain) in the current request, and Horde binds an app's domain only when that app is pushed (Horde_Registry::setTextdomain(), reached via pushApp() / setLanguageEnvironment()). Apps that merely appear in the topbar/prefs menu but were never pushed have no binding, so dgettext() returns the raw msgid — the same English fallback this PR is trying to remove.

Quick confirmation (de_DE.UTF-8, only horde bound, kronolith not pushed):

UNBOUND kronolith: dgettext('kronolith','Calendar') => Calendar   # raw msgid
current  _('Calendar') [horde domain] => Kalender                 # horde.mo happens to contain it
BOUND    kronolith: dgettext('kronolith','Calendar') => Kalender   # only after bindtextdomain()

This likely explains why it looked correct on the portal (block rendering calls setLanguageEnvironment(null, $app) per block app in Block/Collection.php, which binds those domains as a side effect) but would still show English on a page that doesn't render those blocks. The follow-up binds the owning app's domain before dgettext(). dgettext() doesn't touch the active default domain, so no textdomain() save/restore is needed.

2. Headings/links must keep _()

listApps() returns raw msgids for headings and links too, and their keys aren't real app domains — so dgettext($app, ...) in the render loop would leave heading names permanently untranslated. The follow-up resolves names where they originate: real apps via their own (now bound) domain, headings/links via the ambient _() as before. This also removes the double-translation of the already-resolved prefs_* / administration_* entries in the render loop.

3. Same bug in the modern builder

src/Topbar/TopbarBuilder.php (used by the desktop chrome renderer via DesktopChromeRenderer) had the identical _() usage and was untouched here. Both topbar paths are live, so the follow-up applies the same fix there.

Tests

Added TopbarBuilder regression tests:

  • testHeadingNamesBypassOwningAppDomainResolution — deterministic (no locale/.mo dependency): asserts app names trigger a per-app fileroot lookup while headings do not.
  • testResolvesAppNameViaOwningAppDomain — behavioural: builds a self-contained .mo fixture and proves the app name is translated via the owning domain even when it isn't the active default domain; skips gracefully where the environment can't do gettext translation at all.

Full TopbarBuilder suite is green. Happy to adjust naming/structure or split the modern-builder change into its own commit if you'd prefer.

@TDannhauer TDannhauer requested a review from ralflang July 3, 2026 05:20
@TDannhauer

TDannhauer commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

@jcdelepine , thanks for contributing.

I saw some gaps, and filled it. still i'm not the expert here, we need a review from @ralflang whether the general direction is right.

@ralflang

ralflang commented Jul 3, 2026

Copy link
Copy Markdown
Member

TBH I cannot reproduce this - my topbar has translated names for all apps, currently active or otherwise. Switching from base to kronolith to turba doesn't change this.

Before accepting I'd like to understand how your environment is different.

@ralflang

ralflang commented Jul 3, 2026

Copy link
Copy Markdown
Member

I understand the topbar itself never correctly did this but a side effect coupling the topbar to other stuff registry does in the pushapp/popapp cycle masked the problem.

@ralflang ralflang merged commit dfa4690 into horde:FRAMEWORK_6_0 Jul 3, 2026
1 check passed
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.

3 participants