Skip to content

feat: Modern stack theme-owned javascript port - #222

Merged
ralflang merged 1 commit into
FRAMEWORK_6_0from
feat/theme-scripts-src-assets
Aug 30, 2026
Merged

feat: Modern stack theme-owned javascript port#222
ralflang merged 1 commit into
FRAMEWORK_6_0from
feat/theme-scripts-src-assets

Conversation

@ralflang

Copy link
Copy Markdown
Member

Companion to @pierrefardel 's #221 original design

@pierrefardel

Copy link
Copy Markdown
Contributor

Thanks for picking this up, and for the merge on #221.

I have to be upfront about one thing: I could not exercise #222 at runtime. On my
instance nothing reaches the modern stack — ChromeRendererDispatcher is only
referenced by its own tests, and the pages I use (nag/list.php, imp/dynamic.php,
the Kronolith views) all go through Horde_PageOutput in lib/. So a "tested OK"
from me would be worth very little here. What follows is a code review plus a
replay of the filtering logic, not a live test.

What I checked

Theme-facing API is unchanged. Still $theme_scripts = array('theme.js') in
info.php. My two themes need no edit, which is the important part for anyone
already on #221.

Validation is preserved. PhpThemeInfoReader carries the same two patterns and
the explicit .. check. I replayed the logic against the same inputs I used for
#221../evil.js, a/b.js, http://x/y.js, theme.css, plain, ..js, a
hostile theme name, plus a declared-but-missing file — and the outcome matches
case for case.

Per-app resolution is a cascade, not a replacement. This is the one that worried
me at first: my themes only ship themes/horde/<theme>/info.php, so resolving
against $content->app looked like it would silently drop my scripts everywhere
outside Horde itself. Reading discoverTheme() cleared that up — the 'horde'
level is always collected first and the app level layers on top, mirroring the CSS
cascade. Existing declarations keep working, and an app can now add its own. That
is strictly better than what #221 did, where I only ever read the horde theme.

What would help

Is there a supported way to route a real page through the modern stack today, or
is it still ahead of the wiring? If there is a flag or an entry point I have
missed, I am happy to run my two themes through it and report back properly — I
have a setup that ships four theme scripts across two themes, so it is a
reasonable exercise of the cascade.

Otherwise the review is as far as I can honestly take it, and it looks right to me.

@ralflang

Copy link
Copy Markdown
Member Author

The modern stack should be exercised by whups, wicked and in parts by the modernized smartmobile views of nag and passwd.

@pierrefardel

Copy link
Copy Markdown
Contributor

Thanks — that is the pointer I was missing.

None of those are deployed here: this is a groupware-only install (imp, kronolith,
turba, nag, ingo, mnemo), so whups and wicked never came with it, and the nag
src/ I can see does not reach ChromeRendererDispatcher either — its
Responsive/ controllers don't reference PageContent or the chrome renderers.

So I still cannot give you a runtime test, and I would rather say so than dress up
the code review as one. The review itself stands: the theme-facing API is
unchanged, the validation patterns are preserved case for case, and the per-app
cascade is a genuine improvement over #221.

Separately, and unrelated to this PR: our theme aims to cover the whole suite, so
whups and wicked are on our list to style anyway. If we get them installed I will
come back and exercise this properly — but please don't hold the PR for that, it
is not a short path on our side.

@ralflang
ralflang merged commit 345227b into FRAMEWORK_6_0 Aug 30, 2026
1 check failed
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