From cfb04561ecd719aaf54a5ef193eb0650ca044983 Mon Sep 17 00:00:00 2001 From: Timmy38 <101416770+Timmy38@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:40:36 +0200 Subject: [PATCH] =?UTF-8?q?N=C2=B09998=20fix=20remote=20uninstallable=20fl?= =?UTF-8?q?ag?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/extensionsmap.class.inc.php | 23 ++- .../wizardsteps/WizStepLandingBeforeAudit.php | 2 +- setup/wizardsteps/WizStepModulesChoice.php | 7 +- .../unitary-tests/setup/ExtensionsMapTest.php | 39 ++++- .../setup/WizStepModulesChoiceTest.php | 136 ++++++++++++++++++ .../setup/iTopExtensionsMapFake.php | 7 +- 6 files changed, 196 insertions(+), 18 deletions(-) diff --git a/setup/extensionsmap.class.inc.php b/setup/extensionsmap.class.inc.php index 3cef2e89a7..e53ae57e11 100644 --- a/setup/extensionsmap.class.inc.php +++ b/setup/extensionsmap.class.inc.php @@ -208,18 +208,15 @@ class iTopExtensionsMap */ protected function AddExtension(iTopExtension $oNewExtension) { - foreach ($this->aExtensions as $key => $oExtension) { - if ($oExtension->sCode == $oNewExtension->sCode) { - if (version_compare($oNewExtension->sVersion, $oExtension->sVersion, '>')) { - // This "new" extension is "newer" than the previous one, let's replace the previous one - unset($this->aExtensions[$key]); - $this->aExtensions[$oNewExtension->sCode.'/'.$oNewExtension->sVersion] = $oNewExtension; - $this->aExtensionsByCode[$oNewExtension->sCode] = $oNewExtension; - return; - } else { - // This "new" extension is not "newer" than the previous one, let's ignore it - return; - } + $oExtension = $this->GetFromExtensionCode($oNewExtension->sCode); + if (!is_null($oExtension)) { + if (version_compare($oNewExtension->sVersion, $oExtension->sVersion, '>') || $oExtension->bRemovedFromDisk) { + // This "new" extension is "newer" than the previous one, let's replace the previous one + // We should also replace the previous extension if it has been removed from disk + unset($this->aExtensions[$oExtension->sCode.'/'.$oExtension->sVersion]); + } else { + // This "new" extension is not "newer" than the previous one, let's ignore it + return; } } // Finally it's not a duplicate, let's add it to the list @@ -696,8 +693,10 @@ class iTopExtensionsMap $oChoice = $this->GetFromExtensionCode($oExtension->sCode); if ($oChoice) { $oChoice->bInstalled = true; + $oExtension->bRemovedFromDisk = $oChoice->bRemovedFromDisk; } else { $oExtension->bRemovedFromDisk = true; + $this->aExtensionsByCode[$oExtension->sCode] = $oExtension; } $this->aInstalledExtensions[$oExtension->sCode.'/'.$oExtension->sVersion] = $oExtension; diff --git a/setup/wizardsteps/WizStepLandingBeforeAudit.php b/setup/wizardsteps/WizStepLandingBeforeAudit.php index f4310ca625..a00aeebd16 100644 --- a/setup/wizardsteps/WizStepLandingBeforeAudit.php +++ b/setup/wizardsteps/WizStepLandingBeforeAudit.php @@ -88,7 +88,7 @@ class WizStepLandingBeforeAudit extends WizStepModulesChoice $sChoiceId = self::$SEP.$index; $oLatestWizardState = $this->oWizard->GetLatestWizardStateFromStepClass(WizStepModulesChoice::class); $aFlags = $this->ComputeChoiceFlags($aChoice, $sChoiceId, $aSelectedComponents[$oLatestWizardState->GetState()], false, false); - if (!$this->CanMoveForwardFromChoiceFlags($aFlags)) { + if (!static::CanMoveForwardFromChoiceFlags($aFlags)) { // Pop the latest step from the stack, since we are going back to it $this->oWizard->PopStep(); diff --git a/setup/wizardsteps/WizStepModulesChoice.php b/setup/wizardsteps/WizStepModulesChoice.php index 5a927c6254..3f6222db9f 100644 --- a/setup/wizardsteps/WizStepModulesChoice.php +++ b/setup/wizardsteps/WizStepModulesChoice.php @@ -822,12 +822,13 @@ EOF 'dependency_issue' => $bDependencyIssue, 'mandatory' => $bMandatory, 'missing' => $bMissingFromDisk, + 'remote' => $bIsRemoteExtension, 'installed' => $bInstalled, 'disabled' => $bDisabled, 'checked' => $bChecked, ]; - $this->bCanMoveForward = $this->bCanMoveForward && $this->CanMoveForwardFromChoiceFlags($aFlags, $bDisableUninstallCheck); + $this->bCanMoveForward = $this->bCanMoveForward && static::CanMoveForwardFromChoiceFlags($aFlags, $bDisableUninstallCheck); $this->aFlagsByChoiceId[$sChoiceId] = $aFlags; return $aFlags; @@ -889,7 +890,7 @@ EOF } } - protected function CanMoveForwardFromChoiceFlags(array $aFlags, bool $bDisableUninstallCheck = false): bool + public static function CanMoveForwardFromChoiceFlags(array $aFlags, bool $bDisableUninstallCheck = false): bool { // The user can force to move forward with the "force-uninstall" option if ($bDisableUninstallCheck) { @@ -903,7 +904,7 @@ EOF } } elseif ($aFlags['installed']) { // An extension cannot be uninstalled if it is not uninstallable - if (!$aFlags['uninstallable']) { + if (!$aFlags['uninstallable'] || $aFlags['remote']) { return false; } } diff --git a/tests/php-unit-tests/unitary-tests/setup/ExtensionsMapTest.php b/tests/php-unit-tests/unitary-tests/setup/ExtensionsMapTest.php index 49e2872bdf..ff9df153b9 100644 --- a/tests/php-unit-tests/unitary-tests/setup/ExtensionsMapTest.php +++ b/tests/php-unit-tests/unitary-tests/setup/ExtensionsMapTest.php @@ -6,6 +6,7 @@ use Combodo\iTop\Test\UnitTest\ItopTestCase; use CoreException; use iTopExtension; use ItopExtensionsMap; +use iTopExtensionsMapFake; use ModuleDiscovery; class ExtensionsMapTest extends ItopTestCase @@ -117,6 +118,42 @@ class ExtensionsMapTest extends ItopTestCase $this->assertEquals($expected, array_keys($aExtensions)); } + public function testAddExtensionAlwaysKeepsNewestVersion(): void + { + $oExtensionsMap = iTopExtensionsMapFake::createFromArray([]); + + $oExtensionV1 = $this->GivenExtension('my-ext', '1.0.0'); + $oExtensionsMap->AddExtension($oExtensionV1); + $this->assertSame($oExtensionV1, $oExtensionsMap->GetFromExtensionCode('my-ext')); + $this->assertEquals(['my-ext/1.0.0'], array_keys($oExtensionsMap->GetAllExtensions())); + + $oExtensionV3 = $this->GivenExtension('my-ext', '3.0.0'); + $oExtensionsMap->AddExtension($oExtensionV3); + $this->assertSame($oExtensionV3, $oExtensionsMap->GetFromExtensionCode('my-ext')); + $this->assertEquals(['my-ext/3.0.0'], array_keys($oExtensionsMap->GetAllExtensions())); + + $oExtensionV2 = $this->GivenExtension('my-ext', '2.0.0'); + $oExtensionsMap->AddExtension($oExtensionV2); + $this->assertSame($oExtensionV3, $oExtensionsMap->GetFromExtensionCode('my-ext')); + $this->assertEquals(['my-ext/3.0.0'], array_keys($oExtensionsMap->GetAllExtensions())); + } + + public function testAddExtensionReplaceRemovedExtensions(): void + { + $oExtensionsMap = iTopExtensionsMapFake::createFromArray([]); + + $oExtensionV3Removed = $this->GivenExtension('my-ext', '3.0.0'); + $oExtensionV3Removed->bRemovedFromDisk = true; + $oExtensionsMap->AddExtension($oExtensionV3Removed); + $this->assertSame($oExtensionV3Removed, $oExtensionsMap->GetFromExtensionCode('my-ext')); + $this->assertEquals(['my-ext/3.0.0'], array_keys($oExtensionsMap->GetAllExtensions())); + + $oExtensionV2 = $this->GivenExtension('my-ext', '2.0.0'); + $oExtensionsMap->AddExtension($oExtensionV2); + $this->assertSame($oExtensionV2, $oExtensionsMap->GetFromExtensionCode('my-ext')); + $this->assertEquals(['my-ext/2.0.0'], array_keys($oExtensionsMap->GetAllExtensions())); + } + private function GiveExtensionMapWithAllTypeOfExtensions(): iTopExtensionsMap { $oExtensionsMap = iTopExtensionsMap::GetExtensionsMap(); @@ -167,7 +204,7 @@ class ExtensionsMapTest extends ItopTestCase return $oExtensionsMap; } - private function GivenExtension(string $sCode, string $sVersion, bool $bVisible, string $sSource, bool $bMandatory, array $aMissingDependencies = []): iTopExtension + private function GivenExtension(string $sCode, string $sVersion, bool $bVisible = true, string $sSource = iTopExtension::SOURCE_REMOTE, bool $bMandatory = false, array $aMissingDependencies = []): iTopExtension { $oExt = new iTopExtension(); $oExt->sCode = $sCode; diff --git a/tests/php-unit-tests/unitary-tests/setup/WizStepModulesChoiceTest.php b/tests/php-unit-tests/unitary-tests/setup/WizStepModulesChoiceTest.php index 28b617699c..425f152ab3 100644 --- a/tests/php-unit-tests/unitary-tests/setup/WizStepModulesChoiceTest.php +++ b/tests/php-unit-tests/unitary-tests/setup/WizStepModulesChoiceTest.php @@ -54,6 +54,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node1 - A missing extension should always be disabled and unchecked, even a mandatory extension included in package' => [ @@ -79,6 +80,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => false, 'mandatory' => true, + 'remote' => false, ], ], '#node2 - A mandatory extension included in package should be checked and disabled even if the "disable uninstallation check" flag is set' => [ @@ -103,6 +105,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => true, + 'remote' => false, ], ], '#node3 - An installed non uninstallable sub extension should force its parent to be checked and disabled' => [ @@ -136,6 +139,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node3 - An installed remote sub extension should force its parent to be checked and disabled' => [ @@ -170,6 +174,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node4 - An installed uninstallable sub extension should force its parent to be checked but not disabled' => [ @@ -203,6 +208,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node4 - An installed non uninstallable sub extension should force its parent to be checked but not disabled if the "disable uninstallation check" flag is set' => [ @@ -236,6 +242,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node4 - An installed remote sub extension should force its parent to be checked but not disabled if the "disable uninstallation check" flag is set' => [ @@ -270,6 +277,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node5 - A non installed extension with missing dependencies should be not checked and disabled' => [ @@ -296,6 +304,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => true, 'mandatory' => false, + 'remote' => false, ], ], '#node5 - A non installed extension with missing dependencies should be not checked and disabled even with force uninstall' => [ @@ -322,6 +331,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => true, 'mandatory' => false, + 'remote' => false, ], ], '#node6 - An installed extension with missing dependencies and without force uninstall should be checked and disabled' => [ @@ -348,6 +358,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => true, 'mandatory' => false, + 'remote' => false, ], ], '#node7 - An installed extension with missing dependencies and with force uninstall should be checked and enabled' => [ @@ -374,6 +385,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => true, 'mandatory' => false, + 'remote' => false, ], ], '#node8 - An installed but not selected extension with missing dependencies and with force uninstall should be unchecked and enabled' => [ @@ -400,6 +412,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => true, 'mandatory' => false, + 'remote' => false, ], ], '#node9 - A not selected, not installed extension should not be checked and be enabled' => [ @@ -422,6 +435,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node10 - A selected but not installed extension should be checked and enabled' => [ @@ -444,6 +458,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node11 - An installed but not selected extension should not be checked and be enabled' => [ @@ -466,6 +481,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node12 - An installed and selected extension should be checked and enabled' => [ @@ -489,6 +505,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node13 - An installed non uninstallable extension should be checked and disabled' => [ @@ -511,6 +528,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node13 - An installed remote extension should be checked and disabled' => [ @@ -534,6 +552,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => true, ], ], '#node14 - An installed but not selected non uninstallable extension should be checked and enabled if the "disable uninstallation check" flag is set' => [ @@ -556,6 +575,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node14 - An installed but not selected remote extension should be checked and enabled if the "disable uninstallation check" flag is set' => [ @@ -579,6 +599,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => true, ], ], '#node15 - An installed non uninstallable extension should be checked and enabled if the "disable uninstallation check" flag is set' => [ @@ -601,6 +622,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node15 - An installed remote extension should be checked and enabled if the "disable uninstallation check" flag is set' => [ @@ -624,6 +646,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => true, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => true, ], ], '#node16 - A non installed non uninstallable sub extension should not force its parent flags' => [ @@ -657,6 +680,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], '#node16 - A non installed remote sub extension should not force its parent flags' => [ @@ -691,6 +715,7 @@ class WizStepModulesChoiceTest extends ItopTestCase 'checked' => false, 'dependency_issue' => false, 'mandatory' => false, + 'remote' => false, ], ], ]; @@ -706,6 +731,117 @@ class WizStepModulesChoiceTest extends ItopTestCase $this->assertEquals($aExpectedFlags, $aFlags); } + public function ProviderCanMoveForwardFromChoiceFlags(): array + { + return [ + 'force uninstall enabled should always allow move forward' => [ + 'aChoiceFlags' => [ + 'checked' => true, + 'disabled' => true, + 'dependency_issue' => true, + 'installed' => true, + 'uninstallable' => false, + 'remote' => true, + ], + 'bDisableUninstallChecks' => true, + 'bExpectedCanMoveForward' => true, + ], + 'checked disabled with dependency issue should block move forward' => [ + 'aChoiceFlags' => [ + 'checked' => true, + 'disabled' => true, + 'dependency_issue' => true, + 'installed' => false, + 'uninstallable' => true, + 'remote' => false, + ], + 'bDisableUninstallChecks' => false, + 'bExpectedCanMoveForward' => false, + ], + 'checked enabled with dependency issue should allow move forward' => [ + 'aChoiceFlags' => [ + 'checked' => true, + 'disabled' => false, + 'dependency_issue' => true, + 'installed' => true, + 'uninstallable' => true, + 'remote' => false, + ], + 'bDisableUninstallChecks' => false, + 'bExpectedCanMoveForward' => true, + ], + 'checked disabled without dependency issue should allow move forward' => [ + 'aChoiceFlags' => [ + 'checked' => true, + 'disabled' => true, + 'dependency_issue' => false, + 'installed' => true, + 'uninstallable' => false, + 'remote' => false, + ], + 'bDisableUninstallChecks' => false, + 'bExpectedCanMoveForward' => true, + ], + 'unchecked installed and not uninstallable should block move forward' => [ + 'aChoiceFlags' => [ + 'checked' => false, + 'disabled' => true, + 'dependency_issue' => false, + 'installed' => true, + 'uninstallable' => false, + 'remote' => false, + ], + 'bDisableUninstallChecks' => false, + 'bExpectedCanMoveForward' => false, + ], + 'unchecked installed and uninstallable should allow move forward' => [ + 'aChoiceFlags' => [ + 'checked' => false, + 'disabled' => false, + 'dependency_issue' => false, + 'installed' => true, + 'uninstallable' => true, + 'remote' => false, + ], + 'bDisableUninstallChecks' => false, + 'bExpectedCanMoveForward' => true, + ], + 'unchecked not installed should allow move forward' => [ + 'aChoiceFlags' => [ + 'checked' => false, + 'disabled' => false, + 'dependency_issue' => true, + 'installed' => false, + 'uninstallable' => false, + 'remote' => false, + ], + 'bDisableUninstallChecks' => false, + 'bExpectedCanMoveForward' => true, + ], + 'unchecked installed and remote should block move forward' => [ + 'aChoiceFlags' => [ + 'checked' => false, + 'disabled' => true, + 'dependency_issue' => false, + 'installed' => true, + 'uninstallable' => true, + 'remote' => true, + ], + 'bDisableUninstallChecks' => false, + 'bExpectedCanMoveForward' => false, + ], + ]; + } + + /** + * @dataProvider ProviderCanMoveForwardFromChoiceFlags + */ + public function testCanMoveForwardFromChoiceFlags(array $aChoiceFlags, bool $bDisableUninstallChecks, bool $bExpectedCanMoveForward): void + { + $bCanMoveForward = WizStepModulesChoiceFake::CanMoveForwardFromChoiceFlags($aChoiceFlags, $bDisableUninstallChecks); + $this->assertSame($bExpectedCanMoveForward, $bCanMoveForward); + } + public function ProviderGetAddedAndRemovedExtensions() { return [ diff --git a/tests/php-unit-tests/unitary-tests/setup/iTopExtensionsMapFake.php b/tests/php-unit-tests/unitary-tests/setup/iTopExtensionsMapFake.php index ecc68c5ab5..aed8b3cdaf 100644 --- a/tests/php-unit-tests/unitary-tests/setup/iTopExtensionsMapFake.php +++ b/tests/php-unit-tests/unitary-tests/setup/iTopExtensionsMapFake.php @@ -9,7 +9,7 @@ class iTopExtensionsMapFake extends iTopExtensionsMap $this->aScannedDirs = []; } - public static function createFromArray($aExtensions) + public static function createFromArray($aExtensions): static { $oMap = new static(); @@ -28,4 +28,9 @@ class iTopExtensionsMapFake extends iTopExtensionsMap } return $oMap; } + + public function AddExtension(iTopExtension $oNewExtension) + { + parent::AddExtension($oNewExtension); + } }