PR review + code cleanup + added usecases and test cover

setup/extensionsmap.class.inc.php

@@ -304,7 +304,7 @@ class iTopExtensionsMap
 					{
 						// Found a module
 						try {
-							$aModuleInfo = ModuleFileReader::GetInstance()->ReadModuleFileConfiguration($sSearchDir.'/'.$sFile);
+							$aModuleInfo = ModuleFileReader::GetInstance()->ReadModuleFileInformation($sSearchDir.'/'.$sFile);
 						} catch(ModuleFileReaderException $e){
 							continue;
 						}

setup/modulediscovery.class.inc.php

@@ -503,7 +503,7 @@ class ModuleDiscovery
 					self::SetModulePath($sRelDir);
 					$sModuleFilePath = $sDirectory.'/'.$sFile;
 					try {
-						$aModuleInfo = ModuleFileReader::GetInstance()->ReadModuleFileConfiguration($sDirectory.'/'.$sFile);
+						$aModuleInfo = ModuleFileReader::GetInstance()->ReadModuleFileInformation($sDirectory.'/'.$sFile);
 						SetupWebPage::AddModule($sModuleFilePath, $aModuleInfo[1], $aModuleInfo[2]);
 					} catch(ModuleFileReaderException $e){
 						continue;

setup/modulediscovery/ModuleFileParser.php

@@ -72,9 +72,7 @@ class ModuleFileParser {
 			throw new ModuleFileReaderException("2nd parameter to SetupWebPage::AddModule not a string: " . get_class($oModuleId->value), 0, null, $sModuleFilePath);
 		}
 
-		/** @var PhpParser\Node\Scalar\String_ $sModuleIdStringObj */
-		$sModuleIdStringObj = $oModuleId->value;
-		$sModuleId = $sModuleIdStringObj->value;
+		$sModuleId = $oModuleId->value->value;
 
 		$oModuleConfigInfo = $aArgs[2];
 		if (false === ($oModuleConfigInfo instanceof PhpParser\Node\Arg)) {
@@ -92,6 +90,7 @@ class ModuleFileParser {
 		if (! is_array($aModuleConfig)){
 			throw new ModuleFileReaderException("3rd parameter to SetupWebPage::AddModule not an array: " . get_class($oModuleConfigInfo->value), 0, null, $sModuleFilePath);
 		}
+
 		return [
 			$sModuleFilePath,
 			$sModuleId,
@@ -102,37 +101,40 @@ class ModuleFileParser {
 	public function FillModuleInformationFromArray(PhpParser\Node\Expr\Array_ $oArray, array &$aModuleInformation) : void
 	{
 		$iIndex=0;
+
 		/** @var \PhpParser\Node\Expr\ArrayItem $oValue */
 		foreach ($oArray->items as $oArrayItem){
 			if ($oArrayItem->key instanceof PhpParser\Node\Scalar\String_) {
 				//dictionnary
 				$sKey = $oArrayItem->key->value;
 			} else if ($oArrayItem->key instanceof \PhpParser\Node\Expr\ConstFetch) {
+				//dictionnary
 				$sKey = $this->EvaluateConstantExpression($oArrayItem->key);
 				if (is_null($sKey)){
 					continue;
 				}
-			}else {
+			} else {
+				//array
 				$sKey = $iIndex++;
 			}
 
 			$oValue = $oArrayItem->value;
-
 			if ($oValue instanceof PhpParser\Node\Expr\Array_) {
 				$aSubConfig=[];
 				$this->FillModuleInformationFromArray($oValue, $aSubConfig);
 				$aModuleInformation[$sKey]=$aSubConfig;
-			}
-
-			if ($oValue instanceof PhpParser\Node\Scalar\String_||$oValue instanceof PhpParser\Node\Scalar\Int_) {
-				$aModuleInformation[$sKey]=$oValue->value;
 				continue;
 			}
 
-			if ($oValue instanceof \PhpParser\Node\Expr\ConstFetch) {
-				$oEvaluatedConstant = $this->EvaluateConstantExpression($oValue);
-				$aModuleInformation[$sKey]= $oEvaluatedConstant;
+			try {
+				$oEvaluatuedValue = $this->EvaluateExpression($oValue);
+			} catch(ModuleFileReaderException $e){
+				//required to support legacy below dump dependency
+				//'dependencies' => ['itop-config-mgmt/2.0.0'||'itop-structure/3.0.0']
+				continue;
 			}
+
+			$aModuleInformation[$sKey]=$oEvaluatuedValue;
 		}
 	}
 
@@ -155,6 +157,7 @@ class ModuleFileParser {
 					}
 				}
 			}
+
 			return null;
 		}
 
@@ -163,19 +166,13 @@ class ModuleFileParser {
 				/** @var \PhpParser\Node\Stmt\ElseIf_ $oElseIfSubNode */
 				$bCondition = $this->EvaluateExpression($oElseIfSubNode->cond);
 				if ($bCondition) {
-					$aModuleConfig = $this->GetModuleConfigurationFromStatement($sModuleFilePath, $oElseIfSubNode->stmts);
-					if (!is_null($aModuleConfig)) {
-						return $aModuleConfig;
-					}
-					break;
+					return $this->GetModuleConfigurationFromStatement($sModuleFilePath, $oElseIfSubNode->stmts);
 				}
 			}
 		}
 
 		if (! is_null($oNode->else)) {
-			$aModuleConfig = $this->GetModuleConfigurationFromStatement($sModuleFilePath, $oNode->else->stmts);
-
-			return $aModuleConfig;
+			return $this->GetModuleConfigurationFromStatement($sModuleFilePath, $oNode->else->stmts);
 		}
 
 		return null;
@@ -195,59 +192,74 @@ class ModuleFileParser {
 		return null;
 	}
 
-	//TODO replace eval
 	public function EvaluateConstantExpression(\PhpParser\Node\Expr\ArrayItem|\PhpParser\Node\Expr\ConstFetch $oValue) : mixed
 	{
-		$bResult = false;
-		try{
-			@eval('$bResult = '.$oValue->name.';');
-		} catch (Throwable $t) {
-			throw new ModuleFileReaderException("Eval of ' . $oValue->name . ' caused an error: ".$t->getMessage());
-		}
-
-		return $bResult;
+		return $this->UnprotectedComputeBooleanExpression($oValue->name);
 	}
 
-	private function GetMixedValueForBooleanOperatorEvaluation(\PhpParser\Node\Expr $oExpr) : string
+	public function EvaluateClassConstantExpression(\PhpParser\Node\Expr\ClassConstFetch $oValue) : mixed
 	{
-		if ($oExpr instanceof \PhpParser\Node\Scalar\Int_ || $oExpr instanceof \PhpParser\Node\Scalar\Float_){
-			return "" . $oExpr->value;
+		$sClassName = $oValue->class->name;
+		$sProperty = $oValue->name->name;
+		if (class_exists($sClassName)){
+			$class = new \ReflectionClass($sClassName);
+			if (array_key_exists($sProperty, $class->getConstants())) {
+				$oReflectionConstant = $class->getReflectionConstant($sProperty);
+				if ($oReflectionConstant->isPublic()){
+					return $class->getConstant($sProperty);
+				}
+			}
 		}
 
-		return $this->EvaluateExpression($oExpr) ? "true" : "false";
+		if ('class' === $sProperty){
+			return $sClassName;
+		}
+
+		return null;
+	}
+
+	public function EvaluateStaticPropertyExpression(\PhpParser\Node\Expr\StaticPropertyFetch $oValue) : mixed
+	{
+		$sClassName = $oValue->class->name;
+		$sProperty = $oValue->name->name;
+		if (class_exists($sClassName)){
+			$class = new \ReflectionClass($sClassName);
+			if (array_key_exists($sProperty, $class->getStaticProperties())) {
+				$oReflectionProperty = $class->getProperty($sProperty);
+				if ($oReflectionProperty->isPublic()){
+					return $class->getStaticPropertyValue($sProperty);
+				}
+			}
+		}
+
+		return null;
 	}
 
 	/**
 	 * @param string $sBooleanExpr
 	 *
-	 * @return bool
+	 * @return mixed
 	 * @throws ModuleFileReaderException
 	 */
-	private function UnprotectedComputeBooleanExpression(string $sBooleanExpr) : bool
+	private function UnprotectedComputeBooleanExpression(string $sBooleanExpr) : mixed
 	{
-		$bResult = false;
 		try{
+			$bResult = null;
 			@eval('$bResult = '.$sBooleanExpr.';');
+			return $bResult;
 		} catch (Throwable $t) {
 			throw new ModuleFileReaderException("Eval of '$sBooleanExpr' caused an error: ".$t->getMessage());
 		}
-
-		return $bResult;
 	}
 
 	/**
 	 * @param string $sBooleanExpr
-	 * @param bool $bSafe: when true, evaluation relies on unsafe eval() call
 	 *
 	 * @return bool
 	 * @throws ModuleFileReaderException
 	 */
-	public function EvaluateBooleanExpression(string $sBooleanExpr, $bSafe=true) : bool
+	public function EvaluateBooleanExpression(string $sBooleanExpr) : bool
 	{
-		if (! $bSafe){
-			return 	$this->UnprotectedComputeBooleanExpression($sBooleanExpr);
-		}
-
 		$sPhpContent = <<<PHP
 <?php
 $sBooleanExpr;
@@ -255,43 +267,68 @@ PHP;
 		try{
 			$aNodes = $this->ParsePhpCode($sPhpContent);
 			$oExpr = $aNodes[0];
-			return $this->EvaluateExpression($oExpr->expr);
+			$oRes = $this->EvaluateExpression($oExpr->expr);
+
+			return (bool) $oRes;
+
 		} catch (Throwable $t) {
 			throw new ModuleFileReaderException("Eval of '$sBooleanExpr' caused an error:".$t->getMessage());
 		}
 	}
 
-	private function EvaluateExpression(\PhpParser\Node\Expr $oCondExpression) : bool
+	private function GetMixedValueToString(mixed $oExpr) : string
 	{
-		if ($oCondExpression instanceof \PhpParser\Node\Expr\BinaryOp){
-			$sExpr = $this->GetMixedValueForBooleanOperatorEvaluation($oCondExpression->left)
-				. " "
-				. $oCondExpression->getOperatorSigil()
-				. " "
-				. $this->GetMixedValueForBooleanOperatorEvaluation($oCondExpression->right);
-			return $this->EvaluateBooleanExpression($sExpr, false);
+		if (false === $oExpr){
+			return "false";
 		}
 
-		if ($oCondExpression instanceof \PhpParser\Node\Expr\BooleanNot){
-			return ! $this->EvaluateExpression($oCondExpression->expr);
+		if (true === $oExpr){
+			return "true";
 		}
 
-		if ($oCondExpression instanceof \PhpParser\Node\Expr\FuncCall){
-			return $this->EvaluateCallFunction($oCondExpression);
-		}
-
-		if ($oCondExpression instanceof \PhpParser\Node\Expr\StaticCall){
-			return $this->EvaluateStaticCallFunction($oCondExpression);
-		}
-
-		if ($oCondExpression instanceof \PhpParser\Node\Expr\ConstFetch){
-			return $this->EvaluateConstantExpression($oCondExpression);
-		}
-
-		return true;
+		return $oExpr;
 	}
 
-	private function EvaluateCallFunction(\PhpParser\Node\Expr\FuncCall $oFunct) : bool
+	private function EvaluateExpression(\PhpParser\Node\Expr $oExpression) : mixed
+	{
+		if ($oExpression instanceof \PhpParser\Node\Expr\BinaryOp){
+			$sExpr = sprintf("%s %s %s",
+				$this->GetMixedValueToString($this->EvaluateExpression($oExpression->left)),
+				$oExpression->getOperatorSigil(),
+				$this->GetMixedValueToString($this->EvaluateExpression($oExpression->right))
+			);
+			//return $this->UnprotectedComputeBooleanExpression($sBooleanExpr);;
+			return $this->UnprotectedComputeBooleanExpression($sExpr);
+		}
+
+		if ($oExpression instanceof \PhpParser\Node\Expr\BooleanNot){
+			return ! $this->EvaluateExpression($oExpression->expr);
+		}
+
+		if ($oExpression instanceof \PhpParser\Node\Expr\FuncCall){
+			return $this->EvaluateCallFunction($oExpression);
+		}
+
+		if ($oExpression instanceof \PhpParser\Node\Expr\StaticCall){
+			return $this->EvaluateStaticCallFunction($oExpression);
+		}
+
+		if ($oExpression instanceof \PhpParser\Node\Expr\ConstFetch){
+			return $this->EvaluateConstantExpression($oExpression);
+		}
+
+		if ($oExpression instanceof \PhpParser\Node\Expr\ClassConstFetch) {
+			return $this->EvaluateClassConstantExpression($oExpression);
+		}
+
+		if ($oExpression instanceof \PhpParser\Node\Expr\StaticPropertyFetch) {
+			return $this->EvaluateStaticPropertyExpression($oExpression);
+		}
+
+		return $oExpression->value;
+	}
+
+	private function EvaluateCallFunction(\PhpParser\Node\Expr\FuncCall $oFunct) : mixed
 	{
 		$sFunction = $oFunct->name->name;
 		$aWhiteList = ["function_exists", "class_exists", "method_exists"];
@@ -306,17 +343,17 @@ PHP;
 		}
 
 		$oReflectionFunction = new ReflectionFunction($sFunction);
-		return (bool)$oReflectionFunction->invoke(...$aArgs);
+		return $oReflectionFunction->invoke(...$aArgs);
 	}
 
 	/**
 	 * @param \PhpParser\Node\Expr\StaticCall $oStaticCall
 	 *
-	 * @return bool
+	 * @return mixed
 	 * @throws \ModuleFileReaderException
 	 * @throws \ReflectionException
 	 */
-	private function EvaluateStaticCallFunction(\PhpParser\Node\Expr\StaticCall $oStaticCall) : bool
+	private function EvaluateStaticCallFunction(\PhpParser\Node\Expr\StaticCall $oStaticCall) : mixed
 	{
 		$sClassName = $oStaticCall->class->name;
 		$sMethodName = $oStaticCall->name->name;
@@ -338,6 +375,6 @@ PHP;
 			throw new ModuleFileReaderException("StaticCall $sStaticCallDescription not public");
 		}
 
-		return (bool) $method->invokeArgs(null, $aArgs);
+		return $method->invokeArgs(null, $aArgs);
 	}
 }

setup/modulediscovery/ModuleFileReader.php

@@ -27,12 +27,58 @@ class ModuleFileReader {
 
 	/**
 	 * Read the information from a module file (module.xxx.php)
-	 * Use this method to load the ModuleInstallerAPI
 	 * @param string $sModuleFile
 	 * @return array
 	 * @throws ModuleFileReaderException
 	 */
-	public function ReadModuleFileConfigurationUnsafe(string $sModuleFilePath) : array
+	public function ReadModuleFileInformation(string $sModuleFilePath) : array
+	{
+		try
+		{
+			$aNodes = ModuleFileParser::GetInstance()->ParsePhpCode(file_get_contents($sModuleFilePath));
+		}
+		catch (PhpParser\Error $e) {
+			throw new \ModuleFileReaderException($e->getMessage(), 0, $e, $sModuleFilePath);
+		}
+
+		try {
+			foreach ($aNodes as $sKey => $oNode) {
+				if ($oNode instanceof \PhpParser\Node\Stmt\Expression) {
+					$aModuleInfo = ModuleFileParser::GetInstance()->GetModuleInformationFromAddModuleCall($sModuleFilePath, $oNode);
+					if (! is_null($aModuleInfo)){
+						$this->CompleteModuleInfoWithFilePath($aModuleInfo);
+						return $aModuleInfo;
+					}
+				}
+
+				if ($oNode instanceof PhpParser\Node\Stmt\If_) {
+					$aModuleInfo = ModuleFileParser::GetInstance()->GetModuleInformationFromIf($sModuleFilePath, $oNode);
+					if (! is_null($aModuleInfo)){
+						$this->CompleteModuleInfoWithFilePath($aModuleInfo);
+						return $aModuleInfo;
+					}
+				}
+			}
+		} catch(ModuleFileReaderException $e) {
+			// Continue...
+			throw $e;
+		} catch(Exception $e) {
+			// Continue...
+			throw new ModuleFileReaderException("Eval of $sModuleFilePath caused an exception: ".$e->getMessage(), 0, $e, $sModuleFilePath);
+		}
+
+		throw new ModuleFileReaderException("No proper call to SetupWebPage::AddModule found in module file", 0, null, $sModuleFilePath);
+	}
+
+	/**
+	 * Read the information from a module file (module.xxx.php)
+	 * Warning: this method is using eval() function to load the ModuleInstallerAPI classes.
+	 * Current method is never called at design/runtime. It is acceptable to use it during setup only.
+	 * @param string $sModuleFile
+	 * @return array
+	 * @throws ModuleFileReaderException
+	 */
+	public function ReadModuleFileInformationUnsafe(string $sModuleFilePath) : array
 	{
 		$aModuleInfo = []; // will be filled by the "eval" line below...
 		try
@@ -84,52 +130,6 @@ class ModuleFileReader {
 		return $aModuleInfo;
 	}
 
-
-	/**
-	 * Read the information from a module file (module.xxx.php)
-	 * @param string $sModuleFile
-	 * @return array
-	 * @throws ModuleFileReaderException
-	 */
-	public function ReadModuleFileConfiguration(string $sModuleFilePath) : array
-	{
-		try
-		{
-			$aNodes = ModuleFileParser::GetInstance()->ParsePhpCode(file_get_contents($sModuleFilePath));
-		}
-		catch (PhpParser\Error $e) {
-			throw new \ModuleFileReaderException($e->getMessage(), 0, $e, $sModuleFilePath);
-		}
-
-		try {
-			foreach ($aNodes as $sKey => $oNode) {
-				if ($oNode instanceof \PhpParser\Node\Stmt\Expression) {
-					$aModuleInfo = ModuleFileParser::GetInstance()->GetModuleInformationFromAddModuleCall($sModuleFilePath, $oNode);
-					if (! is_null($aModuleInfo)){
-						$this->CompleteModuleInfoWithFilePath($aModuleInfo);
-						return $aModuleInfo;
-					}
-				}
-
-				if ($oNode instanceof PhpParser\Node\Stmt\If_) {
-					$aModuleInfo = ModuleFileParser::GetInstance()->GetModuleInformationFromIf($sModuleFilePath, $oNode);
-					if (! is_null($aModuleInfo)){
-						$this->CompleteModuleInfoWithFilePath($aModuleInfo);
-						return $aModuleInfo;
-					}
-				}
-			}
-		} catch(ModuleFileReaderException $e) {
-			// Continue...
-			throw $e;
-		} catch(Exception $e) {
-			// Continue...
-			throw new ModuleFileReaderException("Eval of $sModuleFilePath caused an exception: ".$e->getMessage(), 0, $e, $sModuleFilePath);
-		}
-
-		throw new ModuleFileReaderException("No proper call to SetupWebPage::AddModule found in module file", 0, null, $sModuleFilePath);
-	}
-
 	/**
 	 *
 	 * Internal trick: additional path is added into the module info structure to handle ModuleInstallerAPI execution during setup
@@ -153,7 +153,7 @@ class ModuleFileReader {
 		$sModuleInstallerClass = $aModuleInfo['installer'];
 		if (!class_exists($sModuleInstallerClass)) {
 			$sModuleFilePath = $aModuleInfo['module_file_path'];
-			$this->ReadModuleFileConfigurationUnsafe($sModuleFilePath);
+			$this->ReadModuleFileInformationUnsafe($sModuleFilePath);
 		}
 
 		if (!class_exists($sModuleInstallerClass))

tests/php-unit-tests/unitary-tests/setup/modulediscovery/ModuleFileParserTest.php

@@ -6,9 +6,14 @@ use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
 use ModuleFileParser;
 use ModuleFileReader;
 use PhpParser\ParserFactory;
+use SetupUtils;
 
 class ModuleFileParserTest extends ItopDataTestCase
 {
+	public static $STATIC_PROPERTY = 123;
+	private static $PRIVATE_STATIC_PROPERTY = 123;
+	private const PRIVATE_CONSTANT = 123;
+
 	protected function setUp(): void
 	{
 		parent::setUp();
@@ -20,17 +25,20 @@ class ModuleFileParserTest extends ItopDataTestCase
 		return [
 			"true" => [ "expr" => "true", "expected" => true],
 			"(true)" => [ "expr" => "(true)", "expected" => true],
+			"(false|true)" => [ "expr" => "(false|true)", "expected" => true],
 			"(false||true)" => [ "expr" => "(false||true)", "expected" => true],
 			"false" => [ "expr" => "false", "expected" => false],
 			"(false)" => [ "expr" => "(false)", "expected" => false],
 			"(false&&true)" => [ "expr" => "(false&&true)", "expected" => false],
+			"(false&true)" => [ "expr" => "(false&true)", "expected" => false],
+			"10 * 10" => [ "expr" => "10 * 10", "expected" => 100],
 		];
 	}
 
 	/**
 	 * @dataProvider EvaluateBooleanExpressionProvider
 	 */
-	public function testEvaluateBooleanExpression(string $sBooleanExpression, bool $expected){
+	public function testEvaluateBooleanExpression(string $sBooleanExpression, $expected){
 		$this->assertEquals($expected, ModuleFileParser::GetInstance()->EvaluateBooleanExpression($sBooleanExpression), $sBooleanExpression);
 	}
 
@@ -90,6 +98,67 @@ PHP;
 		$this->assertEquals(APPROOT, $val);
 	}
 
+	public function testEvaluateClassConstantExpression_PublicConstant()
+	{
+		$this->validateEvaluateClassConstantExpression('SetupUtils::PHP_MIN_VERSION', SetupUtils::PHP_MIN_VERSION);
+	}
+
+	public function testEvaluateClassConstantExpression_PrivateConstantShouldNotBeFound()
+	{
+		$this->validateEvaluateClassConstantExpression('Combodo\iTop\Test\UnitTest\Setup\ModuleDiscovery\ModuleFileParserTest::PRIVATE_CONSTANT', null);
+	}
+
+	public function testEvaluateClassConstant_UnknownConstant()
+	{
+		$this->validateEvaluateClassConstantExpression('SetupUtils::UNKOWN_CONSTANT', null);
+	}
+
+	public function testEvaluateClassConstant_UnknownClass()
+	{
+		$this->validateEvaluateClassConstantExpression('UnknownGaBuZoMeuClass::PHP_MIN_VERSION', null);
+	}
+
+	public function testEvaluateClassConstant_UnknownClassGetClass()
+	{
+		$this->validateEvaluateClassConstantExpression('UnknownGaBuZoMeuClass::class', 'UnknownGaBuZoMeuClass');
+	}
+
+	public function validateEvaluateClassConstantExpression($sExpression, $expected)
+	{
+		$sPHP = <<<PHP
+<?php
+$sExpression;
+PHP;
+		$aNodes = ModuleFileParser::GetInstance()->ParsePhpCode($sPHP);
+		/** @var \PhpParser\Node\Expr $oExpr */
+		$oExpr = $aNodes[0];
+		$val = $this->InvokeNonPublicMethod(ModuleFileParser::class, "EvaluateClassConstantExpression", ModuleFileParser::GetInstance(), [$oExpr->expr]);
+		$this->assertEquals($expected, $val, "$sExpression");
+	}
+
+	public function testEvaluateClassConstant_PublicGetStaticProperty()
+	{
+		$this->validateEvaluateStaticPropertyExpression('Combodo\iTop\Test\UnitTest\Setup\ModuleDiscovery\ModuleFileParserTest::$STATIC_PROPERTY', ModuleFileParserTest::$STATIC_PROPERTY);
+	}
+
+	public function testEvaluateClassConstant_PrivateGetStaticPropertyShouldNotBeFound()
+	{
+		$this->validateEvaluateStaticPropertyExpression('Combodo\iTop\Test\UnitTest\Setup\ModuleDiscovery\ModuleFileParserTest::$PRIVATE_STATIC_PROPERTY', null);
+	}
+
+	public function validateEvaluateStaticPropertyExpression($sExpression, $expected)
+	{
+		$sPHP = <<<PHP
+<?php
+$sExpression;
+PHP;
+		$aNodes = ModuleFileParser::GetInstance()->ParsePhpCode($sPHP);
+		/** @var \PhpParser\Node\Expr $oExpr */
+		$oExpr = $aNodes[0];
+		$val = $this->InvokeNonPublicMethod(ModuleFileParser::class, "EvaluateStaticPropertyExpression", ModuleFileParser::GetInstance(), [$oExpr->expr]);
+		$this->assertEquals($expected, $val, "$sExpression");
+	}
+
 	public static function EvaluateExpressionBooleanProvider() {
 		$sTruePHP = <<<PHP
 <?php
@@ -99,6 +168,11 @@ if (COND){
 PHP;
 
 		return [
+			'"true"' => [
+				"code" => str_replace("COND", '"true"', $sTruePHP),
+				"bool_expected" => "true",
+
+			],
 			"true" => [
 				"code" => str_replace("COND", "true", $sTruePHP),
 				"bool_expected" => true,
@@ -108,6 +182,11 @@ PHP;
 				"code" => str_replace("COND", "false", $sTruePHP),
 				"bool_expected" => false,
 
+			],
+			'"false"' => [
+				"code" => str_replace("COND", '"false"', $sTruePHP),
+				"bool_expected" => "false",
+
 			],
 			"not ok" => [
 				"code" => str_replace("COND", "! false", $sTruePHP),

tests/php-unit-tests/unitary-tests/setup/modulediscovery/ModuleFileReaderTest.php

@@ -15,10 +15,10 @@ class ModuleFileReaderTest extends ItopDataTestCase
 		$this->RequireOnceItopFile('setup/modulediscovery/ModuleFileReader.php');
 	}
 
-	public function testReadModuleFileConfigurationLegacy()
+	public function testReadModuleFileInformationUnsafe()
 	{
 		$sModuleFilePath = __DIR__.'/resources/module.itop-full-itil.php';
-		$aRes = ModuleFileReader::GetInstance()->ReadModuleFileConfiguration($sModuleFilePath);
+		$aRes = ModuleFileReader::GetInstance()->ReadModuleFileInformationUnsafe($sModuleFilePath);
 
 		$this->assertCount(3, $aRes);
 		$this->assertEquals($sModuleFilePath, $aRes[0]);
@@ -30,34 +30,66 @@ class ModuleFileReaderTest extends ItopDataTestCase
 
 	/*public function testAllReadModuleFileConfiguration()
 	{
-		foreach (glob(__DIR__.'/resources/all/module.*.php') as $sModuleFilePath){
-			$aRes = ModuleFileReader::GetInstance()->ReadModuleFileConfiguration($sModuleFilePath);
-			$aExpected = ModuleFileReader::GetInstance()->ReadModuleFileConfigurationLegacy($sModuleFilePath);
+		$aErrors=[];
+		foreach (glob(__DIR__.'/resources/all_factory/module.*.php') as $sModuleFilePath){
+			$aRes = ModuleFileReader::GetInstance()->ReadModuleFileInformation($sModuleFilePath);
+			$aExpected = ModuleFileReader::GetInstance()->ReadModuleFileInformationUnsafe($sModuleFilePath);
 
-			$this->assertEquals($aExpected, $aRes);
-
-			$aAutoselect = $aRes[2]['auto_select'] ?? "";
-			if (strlen($aAutoselect) >0){
-				var_dump($aAutoselect);
+			if ($aExpected !== $aRes){
+				$aErrors[]=basename($sModuleFilePath);
+				continue;
 			}
+			//$this->assertEquals($aExpected, $aRes);
 		}
+
+		$this->assertEquals([], $aErrors);
 	}*/
 
-	public function testReadModuleFileConfiguration()
+	public static function ReadModuleFileConfigurationFileNameProvider()
 	{
-		$sModuleFilePath = __DIR__.'/resources/module.itop-full-itil.php';
-		$aRes = ModuleFileReader::GetInstance()->ReadModuleFileConfiguration($sModuleFilePath);
-		$aExpected = ModuleFileReader::GetInstance()->ReadModuleFileConfigurationUnsafe($sModuleFilePath);
+		return [
+			'nominal case : module.itop-full-itil.php' => ['module.itop-full-itil.php'],
+			'constant as value of a dict entry: module.authent-ldap.php' => ['module.authent-ldap.php'],
+			'int operation evaluation required: email-synchro' => ['module.combodo-email-synchro.php'],
+			'module.itop-admin-delegation-profiles-bridge-for-combodo-email-synchro.php' => ['module.itop-admin-delegation-profiles-bridge-for-combodo-email-synchro.php'],
+			'unknown class name to evaluation as installer: module.itop-global-requests-mgmt.php' => ['module.itop-global-requests-mgmt.php'],
+		];
+	}
+
+	/**
+	 * @dataProvider ReadModuleFileConfigurationFileNameProvider
+	 */
+	public function testReadModuleFileConfigurationVsLegacyMethod(string $sModuleBasename)
+	{
+		$sModuleFilePath = __DIR__."/resources/$sModuleBasename";
+		$aRes = ModuleFileReader::GetInstance()->ReadModuleFileInformation($sModuleFilePath);
+		$aExpected = ModuleFileReader::GetInstance()->ReadModuleFileInformationUnsafe($sModuleFilePath);
 
 		$this->assertEquals($aExpected, $aRes);
 	}
 
-	public function testReadModuleFileConfigurationWithConstants()
-	{
-		$sModuleFilePath = __DIR__.'/resources/module.authent-ldap.php';
-		$aRes = ModuleFileReader::GetInstance()->ReadModuleFileConfiguration($sModuleFilePath);
-		$aExpected = ModuleFileReader::GetInstance()->ReadModuleFileConfigurationUnsafe($sModuleFilePath);
+	/**
+	 * Covers below legacy usecase
+	 * 'dependencies' => array(
+	 * 'itop-config-mgmt/2.0.0'||'itop-structure/3.0.0',
+	 * 'itop-request-mgmt/2.0.0||itop-request-mgmt-itil/2.0.0||itop-incident-mgmt-itil/2.0.0',
+	 * ),
+	 *
+	 * @param string $sModuleBasename
+	 *
+	 * @return void
+	 * @throws \ModuleFileReaderException
+	 */
+	public function testReadModuleFileConfiguration_BadlyWrittenDependencies(){
+		//$sModuleFilePath = __DIR__."/resources/module.combodo-make-it-vip.php";
+		$sModuleFilePath = __DIR__."/resources/module.itop-admin-delegation-profiles.php";
+		$aRes = ModuleFileReader::GetInstance()->ReadModuleFileInformation($sModuleFilePath);
+		$aExpected = ModuleFileReader::GetInstance()->ReadModuleFileInformationUnsafe($sModuleFilePath);
 
+		//do not check dumb conf on dependencies
+		$aDependencies=$aRes[2]['dependencies'];
+		$aDependencies= array_merge([true], $aDependencies);
+		$aRes[2]['dependencies']=$aDependencies;
 		$this->assertEquals($aExpected, $aRes);
 	}
 
@@ -68,10 +100,9 @@ class ModuleFileReaderTest extends ItopDataTestCase
 		$this->expectException(\ModuleFileReaderException::class);
 		$this->expectExceptionMessage("Syntax error, unexpected T_CONSTANT_ENCAPSED_STRING, expecting ',' or ']' or ')' on line 31");
 
-		ModuleFileReader::GetInstance()->ReadModuleFileConfiguration($sModuleFilePath);
+		ModuleFileReader::GetInstance()->ReadModuleFileInformation($sModuleFilePath);
 	}
 
-
 	/**
 	 * local tool function
 	 */
@@ -80,7 +111,7 @@ class ModuleFileReaderTest extends ItopDataTestCase
 		$this->sTempModuleFilePath = tempnam(__DIR__, "test");
 		file_put_contents($this->sTempModuleFilePath, $sPHpCode);
 		try {
-			return ModuleFileReader::GetInstance()->ReadModuleFileConfiguration($this->sTempModuleFilePath);
+			return ModuleFileReader::GetInstance()->ReadModuleFileInformation($this->sTempModuleFilePath);
 		}
 		finally {
 			@unlink($this->sTempModuleFilePath);
@@ -213,7 +244,7 @@ PHP;
 
 		try {
 			$this->assertFalse(class_exists($sModuleInstallerClass));
-			$aModuleInfo = ModuleFileReader::GetInstance()->ReadModuleFileConfiguration($this->sTempModuleFilePath);
+			$aModuleInfo = ModuleFileReader::GetInstance()->ReadModuleFileInformation($this->sTempModuleFilePath);
 			$this->assertFalse(class_exists($sModuleInstallerClass));
 
 			$this->assertEquals($sModuleInstallerClass, ModuleFileReader::GetInstance()->GetAndCheckModuleInstallerClass($aModuleInfo[2]));

tests/php-unit-tests/unitary-tests/setup/modulediscovery/resources/module.combodo-email-synchro.php

@@ -0,0 +1,123 @@
+<?php
+
+SetupWebPage::AddModule(
+	__FILE__, // Path to the current file, all other file names are relative to the directory containing this file
+	'combodo-email-synchro/3.8.2',
+	array(
+		// Identification
+		'label'        => 'Tickets synchronization via e-mail',
+		'category'     => 'business',
+		// Setup
+		'dependencies' => array(
+            'itop-profiles-itil/3.0.0',
+		),
+		'mandatory'    => false,
+		'visible'      => true,
+		'installer'    => 'EmailSynchroInstaller',
+		// Components
+		'datamodel' => array(
+			'classes/autoload.php',
+			'model.combodo-email-synchro.php',
+		),
+		'dictionary' => array(),
+		'data.struct' => array(
+	),
+	'data.sample' => array(
+	),
+	// Documentation
+	'doc.manual_setup' => '', // No manual installation required
+	'doc.more_information' => '', // None
+	// Default settings
+	'settings' => array(
+		'notify_errors_to' => '', // mandatory to track errors not handled by the email processing module
+		'notify_errors_from' => '', // mandatory as well (can be set at the same value as notify_errors_to)
+		'debug' => false, // Set to true to turn on debugging
+		'periodicity' => 30, // interval at which to check for incoming emails (in s)
+		'retention_period' => 1, // number of hour we keep the replica
+		'body_parts_order' => 'text/html,text/plain', // Order in which to read the parts of the incoming emails
+		'pop3_auth_option' => 'USER',
+		'imap_options' => array('imap'),
+		'imap_open_options' => array(),
+		'maximum_email_size' => '10M', // Maximum allowed size for incoming emails
+		'big_files_dir' => '',
+		'exclude_attachment_types' => array('application/exe'), // Example: 'application/exe', 'application/x-winexe', 'application/msdos-windows'
+		// Lines to be removed just above the 'new part' in a reply-to message... add your own patterns below
+		'introductory-patterns' => array(
+			'/^le .+ a écrit :$/i', // Thunderbird French
+			'/^on .+ wrote:$/i', // Thunderbird English
+			'|^[0-9]{4}/[0-9]{1,2}/[0-9]{1,2} .+:$|', // Gmail style
+		),
+		// Some patterns which delimit the previous message in case of a Reply
+		// The "new" part of the message is the text before the pattern
+		// Add your own multi-line patterns (use \\R for a line break)
+		// These patterns depend on the mail client/server used... feel free to add your own discoveries to the list
+		'multiline-delimiter-patterns' => array(
+			'/\\RFrom: .+\\RSent: .+\\R/m', // Outlook English
+			'/\\R_+\\R/m', // A whole line made only of underscore characters
+			'/\\RDe : .+\\R\\R?Envoyé : /m', // Outlook French, HTML and rich text
+			'/\\RDe : .+\\RDate d\'envoi : .+\\R/m', // Outlook French, plain text
+			'/\\R-----Message d\'origine-----\\R/m',
+		),
+		'use_message_id_as_uid' => false, // Do NOT change this unless you known what you are doing!!
+		'images_minimum_size' => '100x20', // Images smaller that these dimensions will be ignored (signatures...)
+		'images_maximum_size' => '', // Images bigger that these dimensions will be resized before uploading into iTop
+		'recommended_max_allowed_packet' => 10*1024*1024, // MySQL parameter for attachments
+	),
+	)
+);
+
+if (!class_exists('EmailSynchroInstaller'))
+{
+
+	// Module installation handler
+	//
+	class EmailSynchroInstaller extends ModuleInstallerAPI
+	{
+
+		/**
+		 * Handler called after the creation/update of the database schema
+		 *
+		 * @param $oConfiguration Config The new configuration of the application
+		 * @param $sPreviousVersion string Previous version number of the module (empty string in case of first install)
+		 * @param $sCurrentVersion string Current version number of the module
+		 *
+		 * @throws \ArchivedObjectException
+		 * @throws \CoreException
+		 * @throws \CoreUnexpectedValue
+		 * @throws \DictExceptionMissingString
+		 * @throws \MySQLException
+		 * @throws \MySQLHasGoneAwayException
+		 */
+		public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion)
+		{
+			// For each email sources, update email replicas by setting mailbox_path to source.mailbox where mailbox_path is null
+			SetupLog::Info("Updating email replicas to set their mailbox path.");
+
+			// Preparing mailboxes search
+			$oSearch = new DBObjectSearch('MailInboxBase');
+
+			// Retrieving definition of attribute to update
+			$sTableName = MetaModel::DBGetTable('EmailReplica');
+
+			$UidlAttDef = MetaModel::GetAttributeDef('EmailReplica', 'uidl');
+			$sUidlColName = $UidlAttDef->Get('sql');
+
+			$oMailboxAttDef = MetaModel::GetAttributeDef('EmailReplica', 'mailbox_path');
+			$sMailboxColName = $oMailboxAttDef->Get('sql');
+
+			$sFrienlynameAttCode = MetaModel::GetFriendlyNameAttributeCode('EmailReplica');
+
+			// Looping on inboxes to update
+			$oSet = new DBObjectSet($oSearch);
+			while ($oInbox = $oSet->Fetch())
+			{
+				$sUpdateQuery = "UPDATE $sTableName SET $sMailboxColName = " . CMDBSource::Quote($oInbox->Get('mailbox')) . " WHERE $sUidlColName LIKE " . CMDBSource::Quote($oInbox->Get('login') . '_%') . " AND $sMailboxColName IS NULL";
+				SetupLog::Info("Executing query: " . $sUpdateQuery);
+				$iRet = CMDBSource::Query($sUpdateQuery); // Throws an exception in case of error
+				SetupLog::Info("Updated $iRet rows.");
+			}
+		}
+
+	}
+
+}

tests/php-unit-tests/unitary-tests/setup/modulediscovery/resources/module.combodo-make-it-vip.php

@@ -0,0 +1,57 @@
+<?php
+/**
+ *
+ * @copyright   Copyright (C) 2010-2019 Combodo SARL
+ * @license     https://www.combodo.com/documentation/combodo-software-license.html
+ *
+ */
+//
+// iTop module definition file
+//
+
+SetupWebPage::AddModule(
+	__FILE__, // Path to the current file, all other file names are relative to the directory containing this file
+	'combodo-make-it-vip/1.2.0',
+	array(
+		// Identification
+		//
+		'label' => 'Flag important contacts in your database and highlight tickets',
+		'category' => 'business',
+
+		// Setup
+		//
+		'dependencies' => array(
+			'itop-config-mgmt/2.0.0'||'itop-structure/3.0.0',
+			'itop-request-mgmt/2.0.0||itop-request-mgmt-itil/2.0.0||itop-incident-mgmt-itil/2.0.0',
+		),
+		'mandatory' => false,
+		'visible' => true,
+
+		// Components
+		//
+		'datamodel' => array(
+			'model.combodo-make-it-vip.php',
+			'main.combodo-make-it-vip.php',
+		),
+		'webservice' => array(
+			
+		),
+		'data.struct' => array(
+			// add your 'structure' definition XML files here,
+		),
+		'data.sample' => array(
+			// add your sample data XML files here,
+		),
+		
+		// Documentation
+		//
+		'doc.manual_setup' => '', // hyperlink to manual setup documentation, if any
+		'doc.more_information' => '', // hyperlink to more information, if any 
+
+		// Default settings
+		//
+		'settings' => array(
+			// Module specific settings go here, if any
+		),
+	)
+);

tests/php-unit-tests/unitary-tests/setup/modulediscovery/resources/module.itop-admin-delegation-profiles-bridge-for-combodo-email-synchro.php

@@ -0,0 +1,51 @@
+<?php
+//
+// iTop module definition file
+//
+
+SetupWebPage::AddModule(
+	__FILE__, // Path to the current file, all other file names are relative to the directory containing this file
+    'itop-admin-delegation-profiles-bridge-for-combodo-email-synchro/1.0.0',
+    array(
+        // Identification
+        //
+        'label' => 'Profiles per admin fonction: Mail inboxes and messages',
+        'category' => 'Datamodel',
+
+        // Setup
+        //
+        'dependencies' => array(
+            'itop-admin-delegation-profiles/1.0.0',
+            'itop-admin-delegation-profiles/1.0.0 || combodo-email-synchro/3.7.2 || itop-oauth-client/2.7.7', // Optional dependency to silence the setup to not display a warning if the other module is not present
+        ),
+		'mandatory' => false,
+		'visible' => false,
+		'auto_select' => 'SetupInfo::ModuleIsSelected("itop-admin-delegation-profiles") && SetupInfo::ModuleIsSelected("combodo-email-synchro") && SetupInfo::ModuleIsSelected("itop-oauth-client")',
+
+		// Components
+		//
+		'datamodel' => array(
+			'model.itop-admin-delegation-profiles-bridge-for-combodo-email-synchro.php'
+		),
+		'webservice' => array(
+			
+		),
+		'data.struct' => array(
+			// add your 'structure' definition XML files here,
+		),
+		'data.sample' => array(
+			// add your sample data XML files here,
+		),
+		
+		// Documentation
+		//
+		'doc.manual_setup' => '', // hyperlink to manual setup documentation, if any
+		'doc.more_information' => '', // hyperlink to more information, if any 
+
+		// Default settings
+		//
+		'settings' => array(
+			// Module specific settings go here, if any
+		),
+	)
+);

tests/php-unit-tests/unitary-tests/setup/modulediscovery/resources/module.itop-admin-delegation-profiles.php

@@ -0,0 +1,52 @@
+<?php
+//
+// iTop module definition file
+//
+
+SetupWebPage::AddModule(
+	__FILE__, // Path to the current file, all other file names are relative to the directory containing this file
+    'itop-admin-delegation-profiles/1.2.1',
+    array(
+        // Identification
+        //
+        'label' => 'Profiles per admin fonction',
+        'category' => 'Datamodel',
+
+        // Setup
+        //
+        'dependencies' => array(
+	        'itop-config-mgmt/2.7.0' || 'itop-structure/3.0.0',
+	        // itop-profiles-itil is here to ensure that the /itop_design/groups/group[@id="History"] alteration comes after those from that module.
+	        // This allows to define the missing "History" group in iTop 2.7 / 3.0, while merging smoothly with iTop 3.1+
+	        'itop-profiles-itil/2.7.0',
+        ),
+		'mandatory' => false,
+		'visible' => true,
+
+		// Components
+		//
+		'datamodel' => array(
+			'model.itop-admin-delegation-profiles.php'
+		),
+		'webservice' => array(
+			
+		),
+		'data.struct' => array(
+			// add your 'structure' definition XML files here,
+		),
+		'data.sample' => array(
+			// add your sample data XML files here,
+		),
+		
+		// Documentation
+		//
+		'doc.manual_setup' => '', // hyperlink to manual setup documentation, if any
+		'doc.more_information' => '', // hyperlink to more information, if any 
+
+		// Default settings
+		//
+		'settings' => array(
+			// Module specific settings go here, if any
+		),
+	)
+);

tests/php-unit-tests/unitary-tests/setup/modulediscovery/resources/module.itop-global-requests-mgmt.php

@@ -0,0 +1,114 @@
+<?php
+
+/**
+ * Module itop-global-requests
+ *
+ * @copyright   Copyright (C) 2012-2019 Combodo SARL
+ * @license     https://www.combodo.com/documentation/combodo-software-license.html
+ */
+
+/** @noinspection PhpUnhandledExceptionInspection */
+SetupWebPage::AddModule(
+	__FILE__, // Path to the current file, all other file names are relative to the directory containing this file
+	'itop-global-requests-mgmt/1.6.3',
+	array(
+		// Identification
+		//
+		'label'        => 'iTop Global Requests Management',
+		'category'     => 'business',
+
+		// Setup
+		//
+		'dependencies' => array(
+			'itop-portal-base/3.2.0',
+			'approval-base/2.5.1',
+			'combodo-approval-extended/1.2.3',
+			'itop-config-mgmt/3.2.0',
+			'itop-tickets/3.2.0',
+			'combodo-dispatch-userrequest/1.1.4',
+			'itop-request-mgmt-itil/3.2.0||itop-request-mgmt/3.2.0',
+			'itop-service-mgmt/3.2.0||itop-service-mgmt-provider/3.2.0',
+			'itop-request-template/2.0.1',
+			'itop-request-template-portal/1.0.0',
+		),
+		'mandatory' => false,
+		'visible' => true,
+		'installer' => GlobalRequestInstaller::class,
+
+		// Components
+		//
+		'datamodel' => array(
+			'vendor/autoload.php',
+			// Explicitly load hooks classes
+			'src/Hook/GRPopupMenuExtension.php',
+			// Explicitly load DM classes
+			'model.itop-global-requests-mgmt.php',
+			//Needed for symfony dependency injection
+			'src/Portal/Router/GlobalRequestBrickRouter.php',
+		),
+		'webservice' => array(),
+		'data.struct' => array(// add your 'structure' definition XML files here,
+		),
+		'data.sample' => array(// add your sample data XML files here,
+		),
+
+		// Documentation
+		//
+		'doc.manual_setup' => '', // hyperlink to manual setup documentation, if any
+		'doc.more_information' => '', // hyperlink to more information, if any
+
+		// Default settings
+		//
+		'settings' => array(
+			'target_state' => 'new',
+			'bypass_profiles' => 'Administrator, Service Manager',
+			'reuse_previous_answers' => true,
+		),
+	)
+);
+
+
+class GlobalRequestInstaller extends ModuleInstallerAPI
+{
+	/**
+	 * Handler called before creating or upgrading the database schema
+	 *
+	 * @param $oConfiguration Config The new configuration of the application
+	 * @param $sPreviousVersion string Previous version number of the module (empty string in case of first install)
+	 * @param $sCurrentVersion string Current version number of the module
+	 *
+	 * @throws \CoreException
+	 * @throws \MySQLException
+	 * @throws \MySQLHasGoneAwayException
+	 */
+	public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion)
+	{
+		if (strlen($sPreviousVersion) > 0 && version_compare($sPreviousVersion, '1.6.0', '<')) {
+			$slnkGRTypeToServiceSubcategory = MetaModel::DBGetTable('lnkGRTypeToServiceSubcategory','parent_servicesubcategory_id');
+			$oAttDefToUpdate = MetaModel::GetAttributeDef('lnkGRTypeToServiceSubcategory', 'parent_servicesubcategory_id');
+			$aColumnsToUpdate = array_keys($oAttDefToUpdate->GetSQLColumns());
+			$sColumnToUpdate = $aColumnsToUpdate[0]; // We know that a string has only one column*/
+			$oAttDefLink = MetaModel::GetAttributeDef('lnkGRTypeToServiceSubcategory', 'servicesubcategory_id');
+			$aColumnsLink = array_keys($oAttDefLink->GetSQLColumns());
+			$sColumnLink = $aColumnsLink[0]; // We know that a string has only one column*/
+
+			$sTableToRead = MetaModel::DBGetTable('ServiceSubcategory', 'parent_servicesubcategory_id');
+			$oAttDefToRead = MetaModel::GetAttributeDef('ServiceSubcategory', 'parent_servicesubcategory_id');
+			$aColumnsToReads = array_keys($oAttDefToRead->GetSQLColumns());
+			$sColumnToRead = $aColumnsToReads[0]; // We know that a string has only one column
+			$sTableToReadPrimaryKey = MetaModel::DBGetKey('ServiceSubcategory');
+
+			$sQueryUpdate = "
+				UPDATE `$slnkGRTypeToServiceSubcategory` 
+				JOIN `$sTableToRead` 
+				ON `$slnkGRTypeToServiceSubcategory`.`$sColumnLink` = `$sTableToRead`.`$sTableToReadPrimaryKey` 
+				SET `$slnkGRTypeToServiceSubcategory`.`$sColumnToUpdate` = `$sTableToRead`.`$sColumnToRead` 
+				WHERE `$slnkGRTypeToServiceSubcategory`.`$sColumnToUpdate` = 0 OR `$slnkGRTypeToServiceSubcategory`.`$sColumnToUpdate` IS NULL
+			";
+            SetupLog::Info(" GlobalRequestInstaller Query: " . $sQueryUpdate);
+			CMDBSource::Query($sQueryUpdate);
+		}
+	}
+}
+
+