From 5a2157ba21a488e0cf0d25f226a814b5532335f8 Mon Sep 17 00:00:00 2001 From: odain Date: Thu, 27 Nov 2025 15:47:25 +0100 Subject: [PATCH] =?UTF-8?q?N=C2=B08724=20-=20refactoring=20for=20maintenab?= =?UTF-8?q?ility?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../dependencyexpression.class.inc.php | 43 +++-- setup/moduledependency/module.class.inc.php | 8 +- .../moduledependencysort.class.inc.php | 19 ++- .../DependencyExpressionTest.php | 156 +++++++++++------- 4 files changed, 133 insertions(+), 93 deletions(-) diff --git a/setup/moduledependency/dependencyexpression.class.inc.php b/setup/moduledependency/dependencyexpression.class.inc.php index 07c5b6048..e0d166c95 100644 --- a/setup/moduledependency/dependencyexpression.class.inc.php +++ b/setup/moduledependency/dependencyexpression.class.inc.php @@ -3,6 +3,7 @@ namespace Combodo\iTop\Setup\ModuleDependency; require_once(APPROOT.'/setup/runtimeenv.class.inc.php'); + use Combodo\iTop\PhpParser\Evaluation\PhpExpressionEvaluator; use ModuleFileReaderException; use RunTimeEnvironment; @@ -38,7 +39,7 @@ class DependencyExpression if (preg_match_all('/([^\(\)&| ]+)/', $sDependencyExpression, $aMatches)) { foreach ($aMatches as $aMatch) { foreach ($aMatch as $sModuleId) { - if (! array_key_exists($sModuleId, $this->aParamsPerModuleId)) { + if (!array_key_exists($sModuleId, $this->aParamsPerModuleId)) { // $sModuleId in the dependency string is made of a / // where the operator is < <= = > >= (by default >=) $aModuleMatches = []; @@ -71,6 +72,7 @@ class DependencyExpression /** * Return module names potentially required by current dependency + * * @return array */ public function GetRemainingModuleNamesToResolve(): array @@ -85,22 +87,25 @@ class DependencyExpression /** * Check if dependency is resolved with current list of module versions - * @param array $aModuleVersions: versions by module names dict - * @param array $aSelectedModules: modules names dict + * + * @param array $aResolvedModuleVersions : versions by module names dict + * @param array $aAllModuleNames : modules names dict * * @return void */ - public function UpdateModuleResolutionState(array $aModuleVersions, array $aSelectedModules): void + public function UpdateModuleResolutionState(array $aResolvedModuleVersions, array $aAllModuleNames): void { if (!$this->bValid) { return; } $aReplacements = []; + + $bDelayEvaluation = false; foreach ($this->aParamsPerModuleId as $sModuleId => list($sModuleName, $sOperator, $sExpectedVersion)) { - if (array_key_exists($sModuleName, $aModuleVersions)) { - // module is present, check the version - $sCurrentVersion = $aModuleVersions[$sModuleName]; + if (array_key_exists($sModuleName, $aResolvedModuleVersions)) { + // module is resolved, check the version + $sCurrentVersion = $aResolvedModuleVersions[$sModuleName]; if (version_compare($sCurrentVersion, $sExpectedVersion, $sOperator)) { if (array_key_exists($sModuleName, $this->aRemainingModuleNamesToResolve)) { unset($this->aRemainingModuleNamesToResolve[$sModuleName]); @@ -112,19 +117,23 @@ class DependencyExpression // a function call that results in a runtime fatal error } } else { - // module is not present - $aReplacements[$sModuleId] = '(false)'; // Add parentheses to protect against invalid condition causing - // a function call that results in a runtime fatal error + // module is not resolved yet + + if (array_key_exists($sModuleName, $aAllModuleNames)) { + //Weird piece of code that covers below usecase: + //module B dependency: 'moduleA || true' + // if moduleA not present on disk, whole expression can be evaluated and may be resolved + // if moduleA present on disk, we need to sort moduleB after moduleA. expression cannot be resolved yet + $bDelayEvaluation = true; + } else { + $aReplacements[$sModuleId] = '(false)'; // Add parentheses to protect against invalid condition causing + } + } } - foreach ($this->aRemainingModuleNamesToResolve as $sModuleName => $c) { - if (array_key_exists($sModuleName, $aSelectedModules)) { - // This module is actually a prerequisite - if (!array_key_exists($sModuleName, $aModuleVersions)) { - return; - } - } + if ($bDelayEvaluation) { + return; } $bResult = false; diff --git a/setup/moduledependency/module.class.inc.php b/setup/moduledependency/module.class.inc.php index 76273fd08..734c081e5 100644 --- a/setup/moduledependency/module.class.inc.php +++ b/setup/moduledependency/module.class.inc.php @@ -93,18 +93,18 @@ class Module /** * Check if module dependencies are resolved with current list of module versions - * @param array $aModuleVersions : versions by module names dict - * @param array $aSelectedModules : modules names dict + * @param array $aResolvedModuleVersions : versions by module names dict + * @param array $aAllModuleNames : resolved modules names * * @return void */ - public function UpdateModuleResolutionState(array $aModuleVersions, array $aSelectedModules): void + public function UpdateModuleResolutionState(array $aResolvedModuleVersions, array $aAllModuleNames): void { $aNextDependencies = []; foreach ($this->aRemainingDependenciesToResolve as $sDependencyExpression => $oModuleDependency) { /** @var DependencyExpression $oModuleDependency*/ - $oModuleDependency->UpdateModuleResolutionState($aModuleVersions, $aSelectedModules); + $oModuleDependency->UpdateModuleResolutionState($aResolvedModuleVersions, $aAllModuleNames); if (!$oModuleDependency->IsResolved()) { $aNextDependencies[$sDependencyExpression] = $oModuleDependency; } diff --git a/setup/moduledependency/moduledependencysort.class.inc.php b/setup/moduledependency/moduledependencysort.class.inc.php index c8484507d..099167c8a 100644 --- a/setup/moduledependency/moduledependencysort.class.inc.php +++ b/setup/moduledependency/moduledependencysort.class.inc.php @@ -33,8 +33,10 @@ class ModuleDependencySort /** * Sort a list of modules, based on their (inter) dependencies + * * @param array $aModules The list of modules to process: 'id' => $aModuleInfo * @param bool $bAbortOnMissingDependency ... + * * @return array * @throws \MissingDependencyException */ @@ -42,13 +44,13 @@ class ModuleDependencySort { // Filter modules to compute $aUnresolvedDependencyModules = []; - $aModuleNames = []; + $aAllModuleNames = []; foreach ($aModules as $sModuleId => $aModule) { $oModule = new Module($sModuleId); $sModuleName = $oModule->GetModuleName(); $oModule->SetDependencies($aModule['dependencies']); $aUnresolvedDependencyModules[$sModuleId] = $oModule; - $aModuleNames[$sModuleName] = true; + $aAllModuleNames[$sModuleName] = true; } // Make sure order is deterministic (alphabtical order) @@ -56,7 +58,7 @@ class ModuleDependencySort //Attempt to resolve module dependencies $aOrderedModules = []; - $aModuleVersions = []; + $aResolvedModuleVersions = []; $iPreviousUnresolvedCount = -1; //loop until no dependency is resolved while ($iPreviousUnresolvedCount !== count($aUnresolvedDependencyModules)) { @@ -67,10 +69,10 @@ class ModuleDependencySort foreach ($aUnresolvedDependencyModules as $sModuleId => $oModule) { /** @var Module $oModule */ - $oModule->UpdateModuleResolutionState($aModuleVersions, $aModuleNames); + $oModule->UpdateModuleResolutionState($aResolvedModuleVersions, $aAllModuleNames); if ($oModule->IsResolved()) { $aOrderedModules[] = $sModuleId; - $aModuleVersions[$oModule->GetModuleName()] = $oModule->GetVersion(); + $aResolvedModuleVersions[$oModule->GetModuleName()] = $oModule->GetVersion(); unset($aUnresolvedDependencyModules[$sModuleId]); } } @@ -100,6 +102,7 @@ class ModuleDependencySort foreach ($aOrderedModules as $sId) { $aResult[$sId] = $aModules[$sId]; } + return $aResult; } @@ -111,7 +114,7 @@ class ModuleDependencySort * - cyclic dependencies * - further versions of same module (name) * - * @param array $aUnresolvedDependencyModules: dict of Module objects by moduleId key + * @param array $aUnresolvedDependencyModules : dict of Module objects by moduleId key * * @return void */ @@ -146,7 +149,7 @@ class ModuleDependencySort uasort($aCountDepsByModuleId, function (array $aDeps1, array $aDeps2) { //compare $iInDegreeCounter - $res = $aDeps1[0] - $aDeps2[0]; + $res = $aDeps1[0] - $aDeps2[0]; if ($res != 0) { return $res; } @@ -177,7 +180,7 @@ class ModuleDependencySort //when 2 versions of the same module (name) below array has been removed already if (array_key_exists($oModule->GetModuleName(), $aDependsOnModuleName)) { foreach ($aDependsOnModuleName[$oModule->GetModuleName()] as $sModuleId2) { - if (! array_key_exists($sModuleId2, $aCountDepsByModuleId)) { + if (!array_key_exists($sModuleId2, $aCountDepsByModuleId)) { continue; } $aDepCount = $aCountDepsByModuleId[$sModuleId2]; diff --git a/tests/php-unit-tests/unitary-tests/setup/moduledependency/DependencyExpressionTest.php b/tests/php-unit-tests/unitary-tests/setup/moduledependency/DependencyExpressionTest.php index bb790ca6e..c55dda442 100644 --- a/tests/php-unit-tests/unitary-tests/setup/moduledependency/DependencyExpressionTest.php +++ b/tests/php-unit-tests/unitary-tests/setup/moduledependency/DependencyExpressionTest.php @@ -24,23 +24,23 @@ class DependencyExpressionTest extends ItopTestCase { return [ "nominal case" => [ - "dep" => "itop-config-mgmt/2.4.0", + "dep" => "itop-config-mgmt/2.4.0", 'expected_operator' => '>=', ], - ">" => [ - "dep" => "itop-config-mgmt/>2.4.0", + ">" => [ + "dep" => "itop-config-mgmt/>2.4.0", 'expected_operator' => '>', ], - ">=" => [ - "dep" => "itop-config-mgmt/>=2.4.0", + ">=" => [ + "dep" => "itop-config-mgmt/>=2.4.0", 'expected_operator' => '>=', ], - "<" => [ - "dep" => "itop-config-mgmt/<2.4.0", + "<" => [ + "dep" => "itop-config-mgmt/<2.4.0", 'expected_operator' => '<', ], - "<=" => [ - "dep" => "itop-config-mgmt/<=2.4.0", + "<=" => [ + "dep" => "itop-config-mgmt/<=2.4.0", 'expected_operator' => '<=', ], ]; @@ -59,31 +59,32 @@ class DependencyExpressionTest extends ItopTestCase $this->assertEquals(['itop-config-mgmt'], $oModuleDependency->GetRemainingModuleNamesToResolve()); } - public static function WithOperatorOperandProvider() + public static function WithVariousOperatorProvider() { - $aInternalStructure = ['itop-structure/3.0.0' => [ 'itop-structure', ">=", '3.0.0'], 'itop-portal/<3.2.1' => [ 'itop-portal', "<", '3.2.1']]; + $aInternalStructure = ['itop-structure/3.0.0' => ['itop-structure', ">=", '3.0.0'], 'itop-portal/<3.2.1' => ['itop-portal', "<", '3.2.1']]; + return [ - '&&' => [ - 'sDepId' => 'itop-structure/3.0.0 && itop-portal/<3.2.1', + '&&' => [ + 'sDepId' => 'itop-structure/3.0.0 && itop-portal/<3.2.1', 'expected_structure' => $aInternalStructure, ], '&& with parenthesis' => [ - 'sDepId' => '(itop-structure/3.0.0) && (itop-portal/<3.2.1)', + 'sDepId' => '(itop-structure/3.0.0) && (itop-portal/<3.2.1)', 'expected_structure' => $aInternalStructure, ], - '||' => [ - 'sDepId' => 'itop-structure/3.0.0 || itop-portal/<3.2.1', + '||' => [ + 'sDepId' => 'itop-structure/3.0.0 || itop-portal/<3.2.1', 'expected_structure' => $aInternalStructure, ], '|| with parenthesis' => [ - 'sDepId' => '(itop-structure/3.0.0) || (itop-portal/<3.2.1)', + 'sDepId' => '(itop-structure/3.0.0) || (itop-portal/<3.2.1)', 'expected_structure' => $aInternalStructure, ], ]; } /** - * @dataProvider WithOperatorOperandProvider + * @dataProvider WithVariousOperatorProvider */ public function testModuleDependencyInit_WithOperand($sDepId, $sExpected) { @@ -97,30 +98,30 @@ class DependencyExpressionTest extends ItopTestCase public static function SimpleDependencyExpressionIsResolvedProvider() { return [ - 'unresolved with major version' => [ - 'expr' => 'itop-config-mgmt/2.4.0', - 'module_versions' => ['itop-config-mgmt' => '1.2.3'], - 'expected_is_resolved' => false, + 'unresolved with major version' => [ + 'expr' => 'itop-config-mgmt/2.4.0', + 'resolved_module_versions' => ['itop-config-mgmt' => '1.2.3'], + 'expected_is_resolved' => false, ], - 'unresolved with minor version' => [ - 'expr' => 'itop-config-mgmt/2.4.1', - 'module_versions' => ['itop-config-mgmt' => '2.4.0-1'], - 'expected_is_resolved' => false, + 'unresolved with minor version' => [ + 'expr' => 'itop-config-mgmt/2.4.1', + 'resolved_module_versions' => ['itop-config-mgmt' => '2.4.0-1'], + 'expected_is_resolved' => false, ], 'resolution OK with major version' => [ - 'expr' => 'itop-config-mgmt/2.4.0', - 'module_versions' => ['itop-config-mgmt' => '2.4.2'], - 'expected_is_resolved' => true, + 'expr' => 'itop-config-mgmt/2.4.0', + 'resolved_module_versions' => ['itop-config-mgmt' => '2.4.2'], + 'expected_is_resolved' => true, ], 'resolution OK with minor version' => [ - 'expr' => 'itop-config-mgmt/2.4.0', - 'module_versions' => ['itop-config-mgmt' => '2.4.0-1'], - 'expected_is_resolved' => true, + 'expr' => 'itop-config-mgmt/2.4.0', + 'resolved_module_versions' => ['itop-config-mgmt' => '2.4.0-1'], + 'expected_is_resolved' => true, ], - 'unproper use of api' => [ - 'expr' => 'itop-config-mgmt/2.4.0', - 'module_versions' => [], - 'expected_is_resolved' => false, + 'unproper use of api' => [ + 'expr' => 'itop-config-mgmt/2.4.0', + 'resolved_module_versions' => [], + 'expected_is_resolved' => false, ], ]; } @@ -140,37 +141,64 @@ class DependencyExpressionTest extends ItopTestCase public static function ComplexDependencyExpressionIsResolvedProvider() { + $aAllModules = ['itop-structure' => true, 'itop-portal' => true]; + return [ - 'and + unresolved due to missing itop-portal' => [ - 'expr' => 'itop-structure/3.0.0 && itop-portal/3.2.1', - 'module_versions' => ['itop-structure' => '3.0.0'], - 'expected_is_resolved' => false, - 'remaining_module_names' => ['itop-portal'], + 'and + unresolved due to missing itop-portal' => [ + 'expr' => 'itop-structure/3.0.0 && itop-portal/3.2.1', + 'resolved_module_versions' => ['itop-structure' => '3.0.0'], + 'all_modules' => $aAllModules, + 'expected_is_resolved' => false, + 'remaining_module_names' => ['itop-portal'], ], - 'and + unresolved due to unsifficient itop-portal version' => [ - 'expr' => 'itop-structure/3.0.0 && itop-portal/3.2.1', - 'module_versions' => ['itop-structure' => '3.0.0', 'itop-portal' => '1.0.0'], - 'expected_is_resolved' => false, - 'remaining_module_names' => ['itop-portal'], + 'and + unresolved due to unsifficient itop-portal version' => [ + 'expr' => 'itop-structure/3.0.0 && itop-portal/3.2.1', + 'resolved_module_versions' => ['itop-structure' => '3.0.0', 'itop-portal' => '1.0.0'], + 'all_modules' => $aAllModules, + 'expected_is_resolved' => false, + 'remaining_module_names' => ['itop-portal'], ], - 'and + resolved' => [ - 'expr' => 'itop-structure/3.0.0 && itop-portal/3.2.1', - 'module_versions' => ['itop-structure' => '3.0.0', 'itop-portal' => '3.3.3'], - 'expected_is_resolved' => true, - 'remaining_module_names' => [], + 'and + resolved' => [ + 'expr' => 'itop-structure/3.0.0 && itop-portal/3.2.1', + 'resolved_module_versions' => ['itop-structure' => '3.0.0', 'itop-portal' => '3.3.3'], + 'all_modules' => $aAllModules, + 'expected_is_resolved' => true, + 'remaining_module_names' => [], ], - 'or + resolved' => [ - 'expr' => 'itop-structure/3.0.0 || itop-portal/3.2.1', - 'module_versions' => ['itop-structure' => '3.0.0'], - 'expected_is_resolved' => false, - 'remaining_module_names' => ['itop-portal'], + 'or||true (step1) + dependency expression evaluation is delayed for sorting purpose' => [ + 'expr' => 'itop-structure/3.0.0||true', + 'resolved_module_versions' => [], + 'all_modules' => $aAllModules, + 'expected_is_resolved' => false, + 'remaining_module_names' => ['itop-structure'], ], - 'or + resolved with less prerequisites' => [ - 'expr' => 'itop-structure/3.0.0 || itop-portal/3.2.1', - 'module_versions' => ['itop-structure' => '3.0.0'], - 'expected_is_resolved' => true, - 'remaining_module_names' => ['itop-portal'], - 'prerequisites' => ['itop-structure' => true], + 'or||true (step2) + expression is evaluated because itop-structure has been resolved' => [ + 'expr' => 'itop-structure/3.0.0||true', + 'resolved_module_versions' => ['itop-structure' => '3.0.0'], + 'all_modules' => $aAllModules, + 'expected_is_resolved' => true, + 'remaining_module_names' => [], + ], + 'or||true + resolved DIRECTLY as itop-structure is not on disk (all_modules)' => [ + 'expr' => 'itop-structure/3.0.0||true', + 'resolved_module_versions' => [], + 'all_modules' => [], + 'expected_is_resolved' => true, + 'remaining_module_names' => ['itop-structure'], + ], + 'or + unresolved because dependency trick used to sort as well' => [ + 'expr' => 'itop-structure/3.0.0 || itop-portal/3.2.1', + 'resolved_module_versions' => ['itop-structure' => '3.0.0'], + 'all_modules' => $aAllModules, + 'expected_is_resolved' => false, + 'remaining_module_names' => ['itop-portal'], + ], + '1 can be used as a boolean' => [ + 'expr' => 'true||1', + 'resolved_module_versions' => [], + 'all_modules' => [], + 'expected_is_resolved' => true, + 'remaining_module_names' => [], ], ]; } @@ -178,11 +206,11 @@ class DependencyExpressionTest extends ItopTestCase /** * @dataProvider ComplexDependencyExpressionIsResolvedProvider */ - public function testComplexDependencyExpressionIsResolved($sExpression, $aModuleVersions, $bExpectedResolved, $aRemainingModuleNames, $aPrerequisites = ['itop-structure' => true, 'itop-portal' => true]) + public function testComplexDependencyExpressionIsResolved($sExpression, $aModuleVersions, $aAllModules, $bExpectedResolved, $aRemainingModuleNames) { $oModuleDependency = new DependencyExpression($sExpression); - $oModuleDependency->UpdateModuleResolutionState($aModuleVersions, $aPrerequisites); + $oModuleDependency->UpdateModuleResolutionState($aModuleVersions, $aAllModules); $this->assertEquals($aRemainingModuleNames, $oModuleDependency->GetRemainingModuleNamesToResolve()); $this->assertEquals($bExpectedResolved, $oModuleDependency->IsResolved()); }