From 5ca673db6b416d0cf034a9b266d4b1a716905f58 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 10 Mar 2023 04:21:37 +0000 Subject: [PATCH] Fix "Undefined property" warning when calc() is used with CSS var() == Less_Tree_Operation == In Less_Tree_Operation::operate(), we call Dimension::operate() with argument $b (named $other there), which Dimension expects to be a number with a unit, i.e. something we can perform math on. Change Operation::operate() to not call this if argument $b is something else, which avoids the reported PHP warning of $other->unit being undefined. Noting that the only class with a 'unit' property is Less_Tree_Dimension. As a side-effect, this immediately fixes another bug, namely that the "var()" right-hand side of `calc(100% - var(foo))` was completely ignored previously when it wasn't a number. For example, it can be a CSS keyword or CSS function like var() that the browser handles client-side. Noting that CSS also natively supports math. LESS should only handle math when the input uses PHP variables or literals, and thus effectively has a constant outcome. Previously, Dimention::operate() would implicitly read $other->value as NULL when $other value wasn't a numerical Dimension object, and thus the math operation silently failed before this commit, e.g.: > "100% - var(foo)" > "100 - NULL" (as %) > "100%" With the warning avoided/fixed, we now correctly preserve the equation in its entirely when $b is e.g. a var() call. == Less_Tree_Call == The above change on its own is insufficient, as that elevates the warning to a fatal error. The added test case exposes this, where `calc(100% - var(foo) - var(bar))` is parsed as: Call (calc) \-- Expression \-- Operation (-) |-- Operation (-_ | |- Dimension (100%) | |- Call (var foo) |-- Call (var bar) The above change, makes the first Operation pair no longer compile Dimension+Call down to a Dimension, instead preserving Operation as literal CSS output. This means the second Operation will now see a left-hand value of Operation, which doesn't have an "operate" method, and thus correctly triggers the `throw` statement that I cleaned up, because that's not something we can compute server-side. Fix this by recognising `calc()` higher up as something that server-side LESS math should not be performed on in the first place. This effectively backports https://github.com/less/less.js/pull/3162 (https://github.com/less/less.js/commit/a48c24c4dd3c13e00a20ece803237) for the similarly reported issue upstream at https://github.com/less/less.js/issues/974. That bugfix was originally released with less.js 3.0.0, but the part of that I'm backporting here doesn't change behaviour of any of our less.js 2.5.3 compliance tests, thus safe to backport. We already threw a fatal error when one of the arguments wasn't a Dimension or Color (no other class has the 'operate' method), so this only adds non-fatal outcomes to previously fatal outcomes. Bug: T331688 Change-Id: Idd443a76372682de4edddeb64ab5a65b23bbd3ce --- lib/Less/Environment.php | 9 ++++++++ lib/Less/Tree/Call.php | 23 +++++++++++++------ lib/Less/Tree/Color.php | 1 + lib/Less/Tree/Dimension.php | 1 + lib/Less/Tree/Operation.php | 11 ++++++--- test/Fixtures/less.php/css/T331688-calc.css | 6 +++++ test/Fixtures/less.php/css/T331688-op-var.css | 3 +++ test/Fixtures/less.php/less/T331688-calc.less | 10 ++++++++ .../less.php/less/T331688-op-var.less | 3 +++ 9 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 test/Fixtures/less.php/css/T331688-calc.css create mode 100644 test/Fixtures/less.php/css/T331688-op-var.css create mode 100644 test/Fixtures/less.php/less/T331688-calc.less create mode 100644 test/Fixtures/less.php/less/T331688-op-var.less diff --git a/lib/Less/Environment.php b/lib/Less/Environment.php index bfc38255..31ff8022 100644 --- a/lib/Less/Environment.php +++ b/lib/Less/Environment.php @@ -41,6 +41,8 @@ class Less_Environment { public static $mixin_stack = 0; + public static $mathOn = true; + /** * @var array */ @@ -93,7 +95,14 @@ public function copyEvalEnv( $frames = [] ) { return $new_env; } + /** + * @return bool + * @see Eval.prototype.isMathOn in less.js 3.0.0 https://github.com/less/less.js/blob/v3.0.0/dist/less.js#L1007 + */ public static function isMathOn() { + if ( !self::$mathOn ) { + return false; + } return !Less_Parser::$options['strictMath'] || self::$parensStack; } diff --git a/lib/Less/Tree/Call.php b/lib/Less/Tree/Call.php index 28a3a192..7dc8861a 100644 --- a/lib/Less/Tree/Call.php +++ b/lib/Less/Tree/Call.php @@ -1,12 +1,15 @@ name = $name; $this->args = $args; + $this->mathOn = ( $name !== 'calc' ); $this->index = $index; $this->currentFileInfo = $currentFileInfo; } @@ -24,22 +28,27 @@ public function accept( $visitor ) { // // When evaluating a function call, - // we either find the function in `tree.functions` [1], - // in which case we call it, passing the evaluated arguments, - // or we simply print it out as it appeared originally [2]. + // we either find the function in Less_Functions, + // in which case we call it, passing the evaluated arguments, + // or we simply print it out as it literal CSS. // - // The *functions.js* file contains the built-in functions. - // - // The reason why we evaluate the arguments, is in the case where - // we try to pass a variable to a function, like: `saturate(@color)`. + // The reason why we compile the arguments, is in the case one + // of them is a LESS variable that only PHP knows the value of, + // like: `saturate(@mycolor)`. // The function should receive the value, not the variable. // public function compile( $env = null ) { + // Turn off math for calc(). https://phabricator.wikimedia.org/T331688 + $currentMathContext = Less_Environment::$mathOn; + Less_Environment::$mathOn = $this->mathOn; + $args = []; foreach ( $this->args as $a ) { $args[] = $a->compile( $env ); } + Less_Environment::$mathOn = $currentMathContext; + $nameLC = strtolower( $this->name ); switch ( $nameLC ) { case '%': diff --git a/lib/Less/Tree/Color.php b/lib/Less/Tree/Color.php index 427f47dc..a30475c6 100644 --- a/lib/Less/Tree/Color.php +++ b/lib/Less/Tree/Color.php @@ -98,6 +98,7 @@ public function toCSS( $doNotCompress = false ) { /** * @param string $op + * @param Less_Tree_Color $other */ public function operate( $op, $other ) { $rgb = []; diff --git a/lib/Less/Tree/Dimension.php b/lib/Less/Tree/Dimension.php index 4dd71983..425f3d5c 100644 --- a/lib/Less/Tree/Dimension.php +++ b/lib/Less/Tree/Dimension.php @@ -72,6 +72,7 @@ public function __toString() { /** * @param string $op + * @param Less_Tree_Dimension $other */ public function operate( $op, $other ) { $value = Less_Functions::operate( $op, $this->value, $other->value ); diff --git a/lib/Less/Tree/Operation.php b/lib/Less/Tree/Operation.php index 4d79dc0b..37d4aabd 100644 --- a/lib/Less/Tree/Operation.php +++ b/lib/Less/Tree/Operation.php @@ -26,6 +26,10 @@ public function compile( $env ) { $a = $this->operands[0]->compile( $env ); $b = $this->operands[1]->compile( $env ); + // Skip operation if argument was not compiled down to a non-operable value. + // For example, if one argument is a Less_Tree_Call like 'var(--foo)' then we + // preserve it as literal for native CSS. + // https://phabricator.wikimedia.org/T331688 if ( Less_Environment::isMathOn() ) { if ( $a instanceof Less_Tree_Dimension && $b instanceof Less_Tree_Color ) { @@ -33,14 +37,15 @@ public function compile( $env ) { } elseif ( $b instanceof Less_Tree_Dimension && $a instanceof Less_Tree_Color ) { $b = $b->toColor(); - } - if ( !method_exists( $a, 'operate' ) ) { + if ( !( $a instanceof Less_Tree_Dimension || $a instanceof Less_Tree_Color ) ) { throw new Less_Exception_Compiler( "Operation on an invalid type" ); } - return $a->operate( $this->op, $b ); + if ( $b instanceof Less_Tree_Dimension || $b instanceof Less_Tree_Color ) { + return $a->operate( $this->op, $b ); + } } return new Less_Tree_Operation( $this->op, [ $a, $b ], $this->isSpaced ); diff --git a/test/Fixtures/less.php/css/T331688-calc.css b/test/Fixtures/less.php/css/T331688-calc.css new file mode 100644 index 00000000..70d2ddf1 --- /dev/null +++ b/test/Fixtures/less.php/css/T331688-calc.css @@ -0,0 +1,6 @@ +.highlight { + width: calc(100% - 2px); + height: calc(50% - 2*1px); + padding-left: 9px; + padding-right: calc(8px + 1px); +} diff --git a/test/Fixtures/less.php/css/T331688-op-var.css b/test/Fixtures/less.php/css/T331688-op-var.css new file mode 100644 index 00000000..f0f01af1 --- /dev/null +++ b/test/Fixtures/less.php/css/T331688-op-var.css @@ -0,0 +1,3 @@ +.highlight { + height: calc(100% - var(--margin-top) - var(--margin-bottom)); +} diff --git a/test/Fixtures/less.php/less/T331688-calc.less b/test/Fixtures/less.php/less/T331688-calc.less new file mode 100644 index 00000000..6f28fb12 --- /dev/null +++ b/test/Fixtures/less.php/less/T331688-calc.less @@ -0,0 +1,10 @@ +@border-width-base: 1px; +@padding-highlight: 8px; +@half: 0.5; + +.highlight { + width: calc( 100% - 2px ); + height: calc( percentage( @half ) - 2*@border-width-base ); + padding-left: @padding-highlight + @border-width-base; + padding-right: calc( @padding-highlight + @border-width-base ); +} diff --git a/test/Fixtures/less.php/less/T331688-op-var.less b/test/Fixtures/less.php/less/T331688-op-var.less new file mode 100644 index 00000000..7236ccbd --- /dev/null +++ b/test/Fixtures/less.php/less/T331688-op-var.less @@ -0,0 +1,3 @@ +.highlight { + height: calc(100% - var(--margin-top) - var(--margin-bottom)); +}