From 9e047b4cde53f619097680cbbf8c53780b5b83fc Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinkle@fastmail.com>
Date: Fri, 9 Aug 2024 22:14:21 +0100
Subject: [PATCH] Less_Parser: Remove dead code in getVariables() and improve
 test coverage

Test:
* It doesn't actually return native float or int for whole numbers.
  The test was fooling itself because it used arrayEquals() instead
  of arraySame(), which allows '123' to equal 123.0.

  The number is actually returned as a string in this case.
* Add test where the value is really returned as a native number.
* Add test for array.

Code:
* This method is part of our own API, and not ported from Less.js,
  so we have a bit more freedom over its implementation to make
  sense for us.

* Remove "else if instanceof Less_Tree_Comment" branch,
  because it is impossible to reach. The in_array check already
  excludes where get_class() === Less_Tree_Comment.

  This would also explain the lack of code coverage
  https://doc.wikimedia.org/cover/mediawiki-libs-less.php/Parser.php.html#285

* Remove redundant "not_variable_type" check because the loop
  already does `if instanceof Less_Tree_Declaration" which thus
  excludes all others. None of the entries in not_variable_type
  where subclasses of Less_Tree_Declaration, so it's not there for
  that reason, either.

Change-Id: I42fc406e64157432e99f822ca15c8d9eb8986e3f
---
 lib/Less/Parser.php         | 23 +++--------------------
 lib/Less/Tree/NameValue.php |  2 +-
 test/phpunit/ParserTest.php | 15 ++++++++++++++-
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/lib/Less/Parser.php b/lib/Less/Parser.php
index 1432a479..d4503b39 100644
--- a/lib/Less/Parser.php
+++ b/lib/Less/Parser.php
@@ -276,36 +276,18 @@ public function findValueOf( $varName ) {
 	}
 
 	/**
-	 * Gets the private rules variable and returns an array of the found variables
-	 * it uses a helper method getVariableValue() that contains the logic ot fetch the value
-	 * from the rule object
+	 * Get an array of the found variables in the parsed input.
 	 *
 	 * @return array
+	 * @phan-return array<string,string|float|array>
 	 */
 	public function getVariables() {
 		$variables = [];
 
-		$not_variable_type = [
-			Less_Tree_Comment::class, // this include less comments ( // ) and css comments (/* */)
-			Less_Tree_Import::class, // do not search variables in included files @import
-			Less_Tree_Ruleset::class, // selectors (.someclass, #someid, …)
-			Less_Tree_Operation::class,
-		];
-
 		$rules = $this->cachedEvaldRules ?? $this->rules;
-
 		foreach ( $rules as $key => $rule ) {
-			if ( in_array( get_class( $rule ), $not_variable_type ) ) {
-				continue;
-			}
-
-			// Note: it seems $rule is always Less_Tree_Rule when variable = true
 			if ( $rule instanceof Less_Tree_Declaration && $rule->variable ) {
 				$variables[$rule->name] = $this->getVariableValue( $rule );
-			} else {
-				if ( $rule instanceof Less_Tree_Comment ) {
-					$variables[] = $this->getVariableValue( $rule );
-				}
 			}
 		}
 		return $variables;
@@ -330,6 +312,7 @@ public function findVarByName( $var_name ) {
 	 *
 	 * @param Less_Tree $var
 	 * @return mixed
+	 * @phan-return string|float|array<string|float>
 	 */
 	private function getVariableValue( Less_Tree $var ) {
 		switch ( get_class( $var ) ) {
diff --git a/lib/Less/Tree/NameValue.php b/lib/Less/Tree/NameValue.php
index f35d819a..6b58938d 100644
--- a/lib/Less/Tree/NameValue.php
+++ b/lib/Less/Tree/NameValue.php
@@ -3,7 +3,7 @@
  * A simple CSS name-value pair, e.g. `width: 100px;`
  *
  * In bootstrap, there are about 600-1000 simple name-value pairs (depending on
- * how forgiving the match is) -vs- 6,020 dynamic rules (Less_Tree_Rule).
+ * how forgiving the match is) -vs- 6,020 dynamic rules (Less_Tree_Declaration).
  *
  * Using the name-value object can speed up bootstrap compilation slightly, but
  * it breaks color keyword interpretation: `color: red` -> `color: #FF0000`.
diff --git a/test/phpunit/ParserTest.php b/test/phpunit/ParserTest.php
index 146aa27c..82ba1570 100644
--- a/test/phpunit/ParserTest.php
+++ b/test/phpunit/ParserTest.php
@@ -86,6 +86,9 @@ public function testGetVariables() {
 			// Rule > Dimension
 			@some_number: 123;
 
+			// Rule > Dimension
+			@some_number_dot: 123.1;
+
 			// Rule > Dimension
 			@some_unit: 12px;
 
@@ -97,6 +100,12 @@ public function testGetVariables() {
 
 			// Rule > Url > Quoted
 			@some_url: url("just/a/test.jpg");
+
+			@some_list_comma: 1, 2, foo, bar baz, 42;
+
+			@some_list_comma_dot: 1, 2.1, foo, bar baz, 42;
+
+			@some_list_space: foo bar baz;
 		';
 
 		$parser = new Less_Parser();
@@ -106,11 +115,15 @@ public function testGetVariables() {
 		$this->assertEquals(
 			[
 				'@some_string' => '"foo"',
-				'@some_number' => 123.0,
+				'@some_number' => '123',
+				'@some_number_dot' => 123.1,
 				'@some_unit' => '12px',
 				'@some_unit_op' => '5px',
 				'@some_color' => '#f9f9f9',
 				'@some_url' => 'url("just/a/test.jpg")',
+				'@some_list_comma' => [ '1', '2', 'foo', 'bar baz', '42' ],
+				'@some_list_comma_dot' => [ 1.0, 2.1, 'foo', 'bar baz', 42.0 ],
+				'@some_list_space' => 'foo bar baz',
 			],
 			$parser->getVariables()
 		);