From fe66a9a48e4c9fde354e9195ec64a688d9572a92 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Mon, 19 Feb 2024 23:48:04 +0000 Subject: [PATCH] Parser,Environment: Reduce various internal use of static state This is not a port of any specific upstream Less.js changes. For many years, whenever we ported upstream changes in Parser or Envirnment, we mapped the logic onto static equivalents, to keep the changes simpler. However, upstream is now for the most part based on instance state and by-reference objects. This hasn't yet caused any obvious bugs, but it does decrease confidence in how things work, and it makes some big changes part of T353133 difficult to port. This patchs should help prevent future bugs as result from upcoming porting commits by re-synchronising the code to match upstream not just in how the code looks, but also in the behaviour (instance vs static). Sync with Less.js: * Manage math state in Less_Environment object, instead of modifying Less_Parser::$options or Less_Environment::$mathOn. * Manage list of imported files in Less_Environment object. In Less.js, this is kept in the ImportVisitor, but we haven't implemented that yet. I'm leaving that for a future change by Hannah, and now only moving it away from static state as first step. Also: * Move SetOptions() in Less_Parser::__construct to Reset(). This was causing a problem because $this->env is not yet defined during that call. Without this, SetOption() can't pass strictMath to Less_Environment. Bug: T353133 Change-Id: If4a0a7ce052fb378f078e767fdb7449bd7e78de2 --- API.md | 2 +- lessc.inc.php | 6 ++---- lib/Less/Cache.php | 2 +- lib/Less/Environment.php | 28 +++++++++++++++++++++------ lib/Less/Parser.php | 37 ++++++++++++++++++------------------ lib/Less/Tree/Call.php | 12 ++++-------- lib/Less/Tree/Expression.php | 2 +- lib/Less/Tree/Import.php | 8 ++++++-- lib/Less/Tree/Media.php | 6 +++--- lib/Less/Tree/Negative.php | 6 +----- lib/Less/Tree/Operation.php | 2 +- lib/Less/Tree/Rule.php | 12 ++++++++---- lib/Less/Visitor/import.php | 2 +- test/phpunit/ParserTest.php | 14 ++++++++++---- 14 files changed, 80 insertions(+), 59 deletions(-) diff --git a/API.md b/API.md index 010c90f8..83a918ce 100644 --- a/API.md +++ b/API.md @@ -59,7 +59,7 @@ file(s) and any direct and indirect imports. $parser = new Less_Parser(); $parser->parseFile( '/var/www/mysite/bootstrap.less', '/mysite/' ); $css = $parser->getCss(); -$files = $parser->AllParsedFiles(); +$files = $parser->getParsedFiles(); ``` #### Compress output diff --git a/lessc.inc.php b/lessc.inc.php index 419398ee..f942042a 100644 --- a/lessc.inc.php +++ b/lessc.inc.php @@ -129,8 +129,7 @@ public function compile( $string, $name = null ) { $parser->parse( $string ); $out = $parser->getCss(); - $parsed = Less_Parser::AllParsedFiles(); - foreach ( $parsed as $file ) { + foreach ( $parser->getParsedFiles() as $file ) { $this->addParsedFile( $file ); } @@ -165,8 +164,7 @@ public function compileFile( $fname, $outFname = null ) { $parser->parseFile( $fname ); $out = $parser->getCss(); - $parsed = Less_Parser::AllParsedFiles(); - foreach ( $parsed as $file ) { + foreach ( $parser->getParsedFiles() as $file ) { $this->addParsedFile( $file ); } diff --git a/lib/Less/Cache.php b/lib/Less/Cache.php index 204f4b8d..6d89d84b 100644 --- a/lib/Less/Cache.php +++ b/lib/Less/Cache.php @@ -149,7 +149,7 @@ public static function Cache( &$less_files, $parser_options = [] ) { $compiled = $parser->getCss(); - $less_files = $parser->AllParsedFiles(); + $less_files = $parser->getParsedFiles(); return $compiled; } diff --git a/lib/Less/Environment.php b/lib/Less/Environment.php index 1ad5b127..9225d937 100644 --- a/lib/Less/Environment.php +++ b/lib/Less/Environment.php @@ -26,6 +26,9 @@ class Less_Environment { /** @var Less_Tree_Media[] */ public $mediaPath = []; + /** @var string[] */ + public $imports = []; + public static $parensStack = 0; public static $tabLevel = 0; @@ -36,7 +39,7 @@ class Less_Environment { public static $mixin_stack = 0; - public static $mathOn = true; + public $strictMath = false; /** * @var array @@ -56,6 +59,22 @@ public function Init() { ]; } + /** + * @param string $file + * @return void + */ + public function addParsedFile( $file ) { + $this->imports[] = $file; + } + + /** + * @param string $file + * @return bool + */ + public function isFileParsed( $file ) { + return in_array( $file, $this->imports ); + } + public function copyEvalEnv( $frames = [] ) { $new_env = new self(); $new_env->frames = $frames; @@ -66,11 +85,8 @@ public function copyEvalEnv( $frames = [] ) { * @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; + public function isMathOn() { + return $this->strictMath ? (bool)self::$parensStack : true; } /** diff --git a/lib/Less/Parser.php b/lib/Less/Parser.php index fcda9832..d9060063 100644 --- a/lib/Less/Parser.php +++ b/lib/Less/Parser.php @@ -36,9 +36,12 @@ class Less_Parser { ]; - /** @var array{compress:bool,strictUnits:bool,strictMath:bool,numPrecision:int,import_dirs:array,import_callback:null|callable,indentation:string} */ + /** @var array{compress:bool,strictUnits:bool,strictMath:bool,relativeUrls:bool,urlArgs:string,numPrecision:int,import_dirs:array,import_callback:null|callable,indentation:string} */ public static $options = []; + /** @var Less_Environment */ + private static $envCompat; + private $input; // Less input string private $input_len; // input string length private $pos; // current index in `input` @@ -53,8 +56,6 @@ class Less_Parser { protected $rules = []; - private static $imports = []; - public static $has_extends = false; public static $next_id = 0; @@ -74,8 +75,8 @@ public function __construct( $env = null ) { // which will then be passed around by reference. if ( $env instanceof Less_Environment ) { $this->env = $env; + self::$envCompat = $this->env; } else { - $this->SetOptions( self::$default_options ); $this->Reset( $env ); } @@ -93,16 +94,15 @@ public function __construct( $env = null ) { */ public function Reset( $options = null ) { $this->rules = []; - self::$imports = []; self::$has_extends = false; - self::$imports = []; self::$contentsMap = []; $this->env = new Less_Environment(); + self::$envCompat = $this->env; // set new options + $this->SetOptions( self::$default_options ); if ( is_array( $options ) ) { - $this->SetOptions( self::$default_options ); $this->SetOptions( $options ); } @@ -124,6 +124,10 @@ public function SetOptions( $options ) { */ public function SetOption( $option, $value ) { switch ( $option ) { + case 'strictMath': + $this->env->strictMath = (bool)$value; + self::$options[$option] = $value; + return; case 'import_dirs': $this->SetImportDirs( $value ); @@ -444,7 +448,7 @@ public function parseFile( $filename, $uri_root = '', $returnRoot = false ) { $this->SetFileInfo( $filename, $uri_root ); - self::AddParsedFile( $filename ); + $this->env->addParsedFile( $filename ); if ( $returnRoot ) { $rules = $this->GetRules( $filename ); @@ -706,22 +710,19 @@ public function CacheFile( $file_path ) { } /** - * @internal For use by Less_Tree_Import + * @deprecated since 4.3.0 Use $parser->getParsedFiles() instead. + * @return string[] */ - public static function AddParsedFile( $file ) { - self::$imports[] = $file; - } - public static function AllParsedFiles() { - return self::$imports; + return self::$envCompat->imports; } /** - * @internal For use by Less_Tree_Import - * @param string $file + * @since 4.3.0 + * @return string[] */ - public static function FileParsed( $file ) { - return in_array( $file, self::$imports ); + public function getParsedFiles() { + return $this->env->imports; } /** diff --git a/lib/Less/Tree/Call.php b/lib/Less/Tree/Call.php index d9529075..0825887e 100644 --- a/lib/Less/Tree/Call.php +++ b/lib/Less/Tree/Call.php @@ -36,10 +36,10 @@ public function accept( $visitor ) { // like: `saturate(@mycolor)`. // The function should receive the value, not the variable. // - public function compile( $env = null ) { + public function compile( $env ) { // Turn off math for calc(). https://phabricator.wikimedia.org/T331688 - $currentMathContext = Less_Environment::$mathOn; - Less_Environment::$mathOn = $this->mathOn; + $currentMathContext = $env->strictMath; + $env->strictMath = !$this->mathOn; $args = []; foreach ( $this->args as $a ) { @@ -57,7 +57,7 @@ public function compile( $env = null ) { } } - Less_Environment::$mathOn = $currentMathContext; + $env->strictMath = $currentMathContext; $nameLC = strtolower( $this->name ); switch ( $nameLC ) { @@ -128,8 +128,4 @@ public function genCSS( $output ) { $output->add( ')' ); } - // public function toCSS(){ - // return $this->compile()->toCSS(); - //} - } diff --git a/lib/Less/Tree/Expression.php b/lib/Less/Tree/Expression.php index 192764e5..c75fda29 100644 --- a/lib/Less/Tree/Expression.php +++ b/lib/Less/Tree/Expression.php @@ -54,7 +54,7 @@ public function compile( $env ) { if ( !$this->parensInOp ) { Less_Environment::$parensStack--; - } elseif ( !Less_Environment::isMathOn() && !$doubleParen ) { + } elseif ( !$env->isMathOn() && !$doubleParen ) { $returnValue = new Less_Tree_Paren( $returnValue ); } diff --git a/lib/Less/Tree/Import.php b/lib/Less/Tree/Import.php index 0d29f07e..1c29fe2b 100644 --- a/lib/Less/Tree/Import.php +++ b/lib/Less/Tree/Import.php @@ -170,7 +170,7 @@ public function compile( $env ) { '@phan-var string $full_path'; if ( $this->options['inline'] ) { - Less_Parser::AddParsedFile( $full_path ); + $env->addParsedFile( $full_path ); $contents = new Less_Tree_Anonymous( file_get_contents( $full_path ), 0, [], true, true ); if ( $this->features ) { @@ -260,7 +260,11 @@ public function PathAndUri() { * @return Less_Tree_Media|array */ public function ParseImport( $full_path, $uri, $env ) { + // Most of the env changes during importing temporary, so make a copy. + // Except the imports list, which needs to be merged into the main env. + // Set it by-ref, to match JavaScript behaviour. $import_env = clone $env; + $import_env->imports =& $env->imports; if ( ( isset( $this->options['reference'] ) && $this->options['reference'] ) || isset( $this->currentFileInfo['reference'] ) ) { $import_env->currentFileInfo['reference'] = true; } @@ -288,7 +292,7 @@ public function ParseImport( $full_path, $uri, $env ) { private function skip( $path, $env ) { $path = Less_Parser::AbsPath( $path, true ); - if ( $path && Less_Parser::FileParsed( $path ) ) { + if ( $path && $env->isFileParsed( $path ) ) { if ( isset( $this->currentFileInfo['reference'] ) ) { return true; diff --git a/lib/Less/Tree/Media.php b/lib/Less/Tree/Media.php index f13cc552..0c890d0f 100644 --- a/lib/Less/Tree/Media.php +++ b/lib/Less/Tree/Media.php @@ -45,15 +45,15 @@ public function compile( $env ) { $media = new self( [], [], $this->index, $this->currentFileInfo ); $strictMathBypass = false; - if ( Less_Parser::$options['strictMath'] === false ) { + if ( !$env->strictMath ) { $strictMathBypass = true; - Less_Parser::$options['strictMath'] = true; + $env->strictMath = true; } $media->features = $this->features->compile( $env ); if ( $strictMathBypass ) { - Less_Parser::$options['strictMath'] = false; + $env->strictMath = false; } $env->mediaPath[] = $media; diff --git a/lib/Less/Tree/Negative.php b/lib/Less/Tree/Negative.php index 0dbc1284..7bade410 100644 --- a/lib/Less/Tree/Negative.php +++ b/lib/Less/Tree/Negative.php @@ -10,10 +10,6 @@ public function __construct( $node ) { $this->value = $node; } - // function accept($visitor) { - // $this->value = $visitor->visit($this->value); - //} - /** * @see Less_Tree::genCSS */ @@ -23,7 +19,7 @@ public function genCSS( $output ) { } public function compile( $env ) { - if ( Less_Environment::isMathOn() ) { + if ( $env->isMathOn() ) { $ret = new Less_Tree_Operation( '*', [ new Less_Tree_Dimension( -1 ), $this->value ] ); return $ret->compile( $env ); } diff --git a/lib/Less/Tree/Operation.php b/lib/Less/Tree/Operation.php index 38f2facf..d5df55a9 100644 --- a/lib/Less/Tree/Operation.php +++ b/lib/Less/Tree/Operation.php @@ -29,7 +29,7 @@ public function compile( $env ) { // 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 ( $env->isMathOn() ) { if ( $a instanceof Less_Tree_Dimension && $b instanceof Less_Tree_Color ) { $a = $a->toColor(); diff --git a/lib/Less/Tree/Rule.php b/lib/Less/Tree/Rule.php index 8d46a291..e099e1b7 100644 --- a/lib/Less/Tree/Rule.php +++ b/lib/Less/Tree/Rule.php @@ -58,6 +58,7 @@ public function genCSS( $output ) { } /** + * @see less-2.5.3.js#Rule.prototype.eval * @param Less_Environment $env * @return self */ @@ -73,9 +74,10 @@ public function compile( $env ) { } } - $strictMathBypass = Less_Parser::$options['strictMath']; - if ( $name === "font" && !Less_Parser::$options['strictMath'] ) { - Less_Parser::$options['strictMath'] = true; + $strictMathBypass = false; + if ( $name === "font" && !$env->strictMath ) { + $strictMathBypass = true; + $env->strictMath = true; } try { @@ -102,7 +104,9 @@ public function compile( $env ) { throw $e; } - Less_Parser::$options['strictMath'] = $strictMathBypass; + if ( $strictMathBypass ) { + $env->strictMath = false; + } return $return; } diff --git a/lib/Less/Visitor/import.php b/lib/Less/Visitor/import.php index 7af96ebd..59581bff 100644 --- a/lib/Less/Visitor/import.php +++ b/lib/Less/Visitor/import.php @@ -65,7 +65,7 @@ function visitImport($importNode, &$visitDeeper ){ //todo needs to reference css file not import //$contents = new Less_Tree_Anonymous($importNode->root, 0, array('filename'=>$importNode->importedFilename), true ); - Less_Parser::AddParsedFile($full_path); + $this->env->addParsedFile($full_path); $contents = new Less_Tree_Anonymous( file_get_contents($full_path), 0, array(), true ); if( $importNode->features ){ diff --git a/test/phpunit/ParserTest.php b/test/phpunit/ParserTest.php index 98fb9c33..e07594c4 100644 --- a/test/phpunit/ParserTest.php +++ b/test/phpunit/ParserTest.php @@ -39,22 +39,28 @@ public function testGetVariables() { ); } - public function testAllParsedFiles() { + public function testGetParsedFiles() { $parser = new Less_Parser(); $baseDir = Less_Parser::WinPath( realpath( $this->fixtures_dir . '/less.php/less' ) ); $parser->parseFile( $baseDir . '/imports.less' ); $parser->getCss(); - $files = $parser->AllParsedFiles(); - $files = array_map( fn ( $file ) => str_replace( $baseDir, '', $file ), $files ); + $files = $parser->getParsedFiles(); + $normalFiles = array_map( fn ( $file ) => str_replace( $baseDir, '', $file ), $files ); $this->assertEqualsCanonicalizing( [ '/imports.less', '/imports/b.less', '/imports/a.less' ], - $files + $normalFiles + ); + + $this->assertEquals( + $files, + Less_Parser::AllParsedFiles(), + 'AllParsedFiles compatibility' ); }