Let a theme ship its own javascript files - #221
Conversation
Themes can already ship CSS, images and sounds, but not javascript, and
there is no JS equivalent of the cssfiles hook to fill the gap from
within a theme.
A theme now declares its scripts in info.php:
$theme_scripts = array('theme.js');
The files are resolved inside the theme directory and added to the page
alongside the theme's stylesheets, so they take part in the normal
script pipeline (versioning, cache busting) instead of having to be
wired up as external URLs by whoever installs the theme.
Only plain *.js file names are accepted: no directory separators, no
'..', no URL. A theme cannot pull in a script from an arbitrary path or
an external host. Unreadable files are skipped.
The info.php read is factored out of _coveredApps() into _themeInfo(),
so both declarations share one guarded include, cached per instance.
Like the covered-apps list, the script list is derived on demand and is
not serialized.
Themes without the declaration are unaffected.
There was a problem hiding this comment.
Pull request overview
Adds first-class support for themes to ship and register their own JavaScript assets, analogous to existing theme support for CSS/images/sounds. This integrates theme-provided scripts into Horde’s standard script pipeline (including caching/minification), while constraining script declarations to safe, theme-local *.js filenames.
Changes:
- Extend
Horde_Themes_Cacheto read and validate$theme_scriptsfrom a theme’sinfo.php. - Introduce
Horde_Script_File_ThemeDirto resolve script filesystem paths/URLs against the theme directory. - Emit declared theme scripts from
Horde_PageOutput::header()via a new_addThemeScripts()helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/Horde/Themes/Cache.php | Factors out theme info.php loading and adds validated $theme_scripts discovery via themeScripts(). |
| lib/Horde/Script/File/ThemeDir.php | New Horde_Script_File implementation for scripts located under a theme’s directory (themesfs/themesuri). |
| lib/Horde/PageOutput.php | Adds _addThemeScripts() and calls it during header() so theme scripts are included through the normal script pipeline. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ralflang
left a comment
There was a problem hiding this comment.
When shipping this we also need add support to the modern src/ assets framework.
|
@pierrefardel would you mind testing #222 for the modern stack? Should be equivalent functionality. |
Companion to @pierrefardel 's #221 original design
Companion to @pierrefardel 's #221 original design
Let a theme ship its own javascript files
Summary
Themes can already ship CSS, images and sounds. They cannot ship javascript.
This adds
$theme_scriptstoinfo.php, so a theme can declare the behaviourthat goes with its markup:
The files are resolved inside the theme directory and added to the page
alongside the theme's stylesheets. Themes without the declaration are
unaffected.
Why
Horde_Themes_Elementalready covers CSS (Horde_Themes_Css), images(
Horde_Themes_Image) and sounds (Horde_Themes_Sound). Javascript is the oneasset class missing, and there is no way to fill the gap from within a theme:
Horde has no JS equivalent of the
cssfileshook.For a theme that only changes colours this does not matter. It does as soon as a
theme restructures the shell, because some things no CSS selector can express —
wrapping a bare text node so it can be styled, or grouping siblings the
application emits as a flat list.
Today the only way to ship such a theme is to ask whoever installs it to add a
hook to their
hooks.php, an instance configuration file that is not part ofthe theme. The theme is then no longer installable as-is:
composer requireit,enable it, and half of it silently does not load.
What the patch does
Horde_Themes_Cache::themeScripts()— reads$theme_scriptsfrom thetheme's
info.phpand returns the declared files. Theinfo.phpread isfactored out of
_coveredApps()into_themeInfo(), so both declarationsshare one guarded include instead of two.
Horde_Script_File_ThemeDir— aHorde_Script_Filethat resolves againstthemesfs/themesuri, mirroringHorde_Script_File_JsDirwhich resolvesagainst an app's
js/directory.Horde_PageOutput::_addThemeScripts()— emits the files, called fromheader()right where the theme is already consulted for stylesheets.Roughly 30 lines of new logic; the rest mirrors code that already exists.
Scope and safety
Theme-directory files only. A declared entry must be a plain
*.jsfilename — no directory separators, no
.., no URL. A theme cannot pull a scriptfrom an arbitrary path or an external host, so this adds no CSP surface and no
third-party dependency. Unreadable files are skipped rather than emitted as
broken tags.
No new trust. Installing a theme already means executing its PHP:
info.phpis
included byHorde_Themes_Cache(as of the$theme_coverssupport), anda theme's CSS is already arbitrary. A theme is trusted code, like an installed
application; letting it ship a script file is strictly less powerful than what
it can already do.
Theme name guard reused. Theme names come from user prefs, so
_themeInfo()keeps the existing
preg_match('/^[A-Za-z0-9_-]+$/')guard before includinginfo.php.Caching.
themeScripts()is derived on demand and is not part of__serialize(), so a cachedHorde_Themes_Cacherecomputes it — same approachas
_coveredApps().Backwards compatible. No declaration means no script, which is the current
behaviour for every existing theme.
A note for reviewers testing locally
This adds a new class file, so a
composer dump-autoloadis needed before thepatch does anything: without it
_addThemeScripts()fatals on a missingHorde_Script_File_ThemeDir. Nothing to do in a normal install, where the classships with the
horde/corepackage.Testing
Exercised in a real install, dynamic view, with a theme declaring two scripts.
The install runs Core
e66c4242; both files this patch touches arebyte-identical there and at the branch point, so the change under test is the
one proposed here.
The declared files are emitted alongside the core scripts and, unlike a
hook-loaded external file, they go through the normal script pipeline:
They are versioned like any other script file, so theme javascript takes part in
Horde's cache busting — an external-URL workaround does not, and stays cached in
the browser after the theme is updated.
Themes without
$theme_scripts(default,dark) emit no additional scripttags.
The name filter was checked separately against
sub/dir.js,../../etc/passwd.js,..js,http://evil.tld/x.js,style.css,foo.js.php,a b.jsand the empty string: all rejected, only plain*.jsnames accepted.
Context
Follows the CSS custom property work in horde/base#111, horde/imp#69 and
horde/kronolith#73, and the
$theme_coversdeclaration in #179. Thosemade the layout and the calendar colours themable from CSS; this closes the
remaining gap for themes that also need to ship behaviour.