Add Less compiler and pre-eval AST visitors for CSS var support by lippserd · Pull Request #365 · Icinga/ipl-web

lippserd · 2026-04-09T22:10:33Z

WikimediaLessCompiler wraps Less_Parser with a clean compile() API and normalizes parser exceptions into RuntimeException. PreEvalVisitor establishes a base for visitors that hook into the pre-compile() phase, receiving the raw AST before variables and mixins are resolved.

CssVarVisitor replaces every Less color variable reference with a var(--name, fallback) call, enabling runtime theming through CSS custom properties while preserving Less fallback resolution. DetachedRulesetCallVisitor expands a named detached ruleset into a configured Less template, making it possible to wrap styles in constructs like @media blocks without modifying the Less source.

The wikimedia/less.php dependency is bumped from ^3.2.1 to ^5.5 to align with the isPreEvalVisitor hook and other APIs these visitors rely on.

refs Icinga/icingaweb2#5492
refs Icinga/icinga-php-thirdparty#115

`WikimediaLessCompiler` wraps `Less_Parser` with a clean `compile()` API and normalizes parser exceptions into `RuntimeException`. `PreEvalVisitor` establishes a base for visitors that hook into the pre-`compile()` phase, receiving the raw AST before variables and mixins are resolved. `CssVarVisitor` replaces every Less color variable reference with a `var(--name, fallback)` call, enabling runtime theming through CSS custom properties while preserving Less fallback resolution. `DetachedRulesetCallVisitor` expands a named detached ruleset into a configured Less template, making it possible to wrap styles in constructs like `@media` blocks without modifying the Less source. The `wikimedia/less.php` dependency is bumped from `^3.2.1` to `^5.5` to align with the `isPreEvalVisitor` hook and other APIs these visitors rely on.

Copilot

Pull request overview

Introduces a Less compilation wrapper and pre-eval AST visitors to enable CSS custom-property theming and detached-ruleset templating, alongside a dependency bump to a newer wikimedia/less.php version that supports the required hooks.

Changes:

Add WikimediaLessCompiler with a compile() API that normalizes parser exceptions.
Add PreEvalVisitor plus CssVarVisitor and DetachedRulesetCallVisitor for pre-compile AST transformations.
Add comprehensive PHPUnit coverage for the visitors and bump wikimedia/less.php to ^5.5.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`composer.json`	Bumps `wikimedia/less.php` to a newer major version required by the pre-eval visitor hook.
`src/Less/WikimediaLessCompiler.php`	Adds a small compiler wrapper around `Less_Parser` with exception normalization.
`src/Less/PreEvalVisitor.php`	Adds a base visitor for pre-eval traversal (incl. workaround for import accept/visit dispatch).
`src/Less/CssVarVisitor.php`	Adds pre-eval rewrites from Less color variables to CSS `var(--x, fallback)` calls.
`src/Less/DetachedRulesetCallVisitor.php`	Adds pre-eval expansion of a named detached ruleset into a configured Less template.
`tests/Less/LessVisitorTestCase.php`	Adds a reusable test helper for compiling Less and asserting CSS output.
`tests/Less/CssVarVisitorTest.php`	Adds extensive test coverage for `CssVarVisitor` behaviors and edge cases.
`tests/Less/DetachedRulesetCallVisitorTest.php`	Adds test coverage for template wrapping and replacement behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T10:48:56Z

+        $parserOptions = [
+            'compress' => $minify,
+        ];
+
+        try {
+            return (new Less_Parser($this->parserOptions + $parserOptions))


compile() merges options using the array union operator ($this->parserOptions + $parserOptions), so any compress value provided in $parserOptions passed to the constructor will win over the $minify argument. If callers expect compile(..., $minify) to control minification, switch to an override merge order (e.g., merge then set 'compress' => $minify last) so $minify reliably takes precedence.

Suggested change

$parserOptions = [

'compress' => $minify,

];

try {

return (new Less_Parser($this->parserOptions + $parserOptions))

$parserOptions = $this->parserOptions;

$parserOptions['compress'] = $minify;

try {

return (new Less_Parser($parserOptions))

Claude and I agree.

Copilot · 2026-04-17T10:48:56Z

+        $t = tmpfile();
+        fwrite($t, $less);
+        try {
+            $root = (Less_Tree::$parse ?? new Less_Parser())
+                ->parseFile(stream_get_meta_data($t)['uri'], returnRoot: true);
+        } finally {
+            fclose($t);
+        }


tmpfile() can return false on failure. The current code passes the result straight to fwrite()/stream_get_meta_data()/fclose(), which will raise warnings or errors if a temp file cannot be created. Handle the failure case explicitly and throw a RuntimeException with a clear message when tmpfile() fails.

Copilot · 2026-04-17T10:48:56Z

+    /**
+     * Parsed template mixin definition, injected at root level once per run by {@see run()}
+     *
+     * @var Less_Tree_Mixin_Definition
+     */
+    protected readonly Less_Tree_Mixin_Definition $mixinDef;
+


This visitor stores a Less_Tree_Mixin_Definition object in $mixinDef, but the visitor is cloned on each run(). Because PHP cloning is shallow, the clone and prototype will share the same $mixinDef instance unless you deep-copy it. Either implement __clone() to clone/rebuild $mixinDef, or document/ensure that Less.php will never mutate the mixin definition object during compilation (to avoid cross-run state leakage).

Oof, not sure what the probablity of $mixinDef being mutated is. I'd leave this up to @lippserd to decide.

Copilot · 2026-04-17T10:48:57Z

+        foreach (array_reverse($this->mixinParams) as $ignoreVars) {
+            if (in_array($v->name, $ignoreVars, true)) {
+                return $v;
+            }
+        }


visitVariable() calls array_reverse($this->mixinParams) on every variable node visit, which allocates a reversed copy each time. Since this runs during AST traversal, it can become a hot path on large stylesheets. Iterate the stack backwards by index (or maintain it in the needed order) to avoid repeated array copies.

Is it even necessary to reverse the stack as seemingly the order isn't relevant for the loop?

Copilot · 2026-04-17T10:48:57Z

+use Less_Exception_Parser;
+use Less_Parser;
+use PHPUnit\Framework\TestCase;


Project tests consistently extend ipl\Tests\Web\TestCase (see tests/TestCase.php and e.g. tests/FilterTest.php). This new base test case extends PHPUnit\Framework\TestCase directly, which diverges from that convention. Consider extending the project TestCase instead to keep a single common base and allow future shared setup/teardown.

Suggested change

use Less_Exception_Parser;

use Less_Parser;

use PHPUnit\Framework\TestCase;

use ipl\Tests\Web\TestCase;

use Less_Exception_Parser;

use Less_Parser;

nilmerg

With the bump to wikimedia/less.php v5, it complains about empty variable values as noted by @sukhwinder33445 in Icinga/icingaweb2#5492.

Icinga Notifications Web might be responsible but either way, I think \ipl\Web\LessRuleset should ignore empty properties since its interface isn't strict enough when handling arrays.

nilmerg · 2026-04-17T11:26:14Z

+        $t = tmpfile();
+        fwrite($t, $less);
+        try {
+            $root = (Less_Tree::$parse ?? new Less_Parser())


This might warrant a comment: Falling back to a new parser instance causes a template being parsed with default configuration options.

nilmerg · 2026-04-17T11:31:47Z

+    /**
+     * Parsed template mixin definition, injected at root level once per run by {@see run()}
+     *
+     * @var Less_Tree_Mixin_Definition
+     */
+    protected readonly Less_Tree_Mixin_Definition $mixinDef;
+


Oof, not sure what the probablity of $mixinDef being mutated is. I'd leave this up to @lippserd to decide.

nilmerg · 2026-04-17T11:36:46Z

+        foreach (array_reverse($this->mixinParams) as $ignoreVars) {
+            if (in_array($v->name, $ignoreVars, true)) {
+                return $v;
+            }
+        }


Is it even necessary to reverse the stack as seemingly the order isn't relevant for the loop?

cla-bot bot added the cla/signed label Apr 9, 2026

lippserd mentioned this pull request Apr 10, 2026

composer remove wikimedia/less.php Icinga/icinga-php-thirdparty#115

Merged

lippserd force-pushed the less branch from d61516a to 5dde9e4 Compare April 10, 2026 12:54

lippserd force-pushed the less branch from 5dde9e4 to 3ced7b8 Compare April 10, 2026 13:02

lippserd added this to the 0.14.0 milestone Apr 10, 2026

lippserd mentioned this pull request Apr 10, 2026

Migrate Less compilation to ipl\Web\Less Icinga/icingaweb2#5492

Open

lippserd marked this pull request as ready for review April 10, 2026 14:41

lippserd requested a review from nilmerg April 10, 2026 14:41

nilmerg requested a review from Copilot April 17, 2026 10:43

Copilot started reviewing on behalf of nilmerg April 17, 2026 10:44 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

nilmerg requested changes Apr 17, 2026

View reviewed changes

Conversation

lippserd commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

nilmerg Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

nilmerg Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

nilmerg Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

nilmerg left a comment

Choose a reason for hiding this comment

Uh oh!

nilmerg Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

nilmerg Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

nilmerg Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lippserd commented Apr 9, 2026 •

edited

Loading