N°9010 fix flags when extension missing#800
N°9010 fix flags when extension missing#800Timmy38 wants to merge 2 commits intofeature/uninstallationfrom
Conversation
| public function ComputeChoiceFlags(array $aChoice, string $sChoiceId, array $aSelectedComponents, bool $bAllDisabled, bool $bDisableUninstallCheck, bool $bUpgradeMode) | ||
| { | ||
| $oITopExtension = $this->oExtensionsMap->GetFromExtensionCode($aChoice['extension_code']); | ||
| //If the extension is missing from disk, it won't exist in the ExtensionsMap, thus returning null |
There was a problem hiding this comment.
whole method has become uggly and hard to understand functionally... even if most cases are covered by tests (which is nice).
please refactor it to have sth we can understand. for ex. use [if else] structure to spit things apart. on one side treat the exceptional case (extension missing on disk). then others...
There was a problem hiding this comment.
refactor should not be so long as you have the cover :)
|
|
||
| $bMissingFromDisk = isset($aChoice['missing']) && $aChoice['missing'] === true; | ||
|
|
||
| $bMandatory = (isset($aChoice['mandatory']) && $aChoice['mandatory']) || $bUpgradeMode && !$bMissingFromDisk && $oITopExtension->bInstalled && !$bCanBeUninstalled && !$bDisableUninstallCheck; |
There was a problem hiding this comment.
variable $bMandatory has become sth much more complex than his name. maybe keep its logic simple (mandatory is mandatory). and the addional stuff can come after to treat each flag.
There was a problem hiding this comment.
Pull request overview
This PR fixes the handling of extension flags when an extension is installed in the database but missing from disk. The fix ensures such extensions are properly disabled and unchecked in the wizard UI, preventing users from selecting or modifying them.
Changes:
- Fixed boolean logic to check
$bMissingFromDiskbefore accessing potentially null extension object properties - Ensured missing extensions are always disabled and unchecked, regardless of mandatory or uninstallable flags
- Added comprehensive test coverage for missing extension scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| setup/wizardsteps.class.inc.php | Fixed ComputeChoiceFlags method to properly handle missing extensions by reordering variable initialization and adding guards to prevent null pointer access |
| tests/php-unit-tests/unitary-tests/setup/WizStepModulesChoiceTest.php | Added three test cases covering missing extensions in different scenarios (non-mandatory, mandatory, and non-uninstallable) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'A missing extension should be disabled and unchecked' => [ | ||
| 'aExtensionsOnDiskOrDb' => [ | ||
| 'itop-ext1' => [ | ||
| 'installed' => true, | ||
| ], | ||
| ], | ||
| 'aWizardStepDefinition' => [ | ||
| 'extension_code' => 'itop-ext1', | ||
| 'mandatory' => false, | ||
| 'missing' => true, | ||
| 'uninstallable' => true, | ||
| ], | ||
| 'bCurrentSelected' => false, | ||
| 'aExpectedFlags' => [ | ||
| 'uninstallable' => true, | ||
| 'missing' => true, | ||
| 'installed' => true, | ||
| 'disabled' => true, | ||
| 'checked' => false, | ||
| ], | ||
| ], | ||
| 'A missing extension should always be disabled and unchecked, even when mandatory' => [ | ||
| 'aExtensionsOnDiskOrDb' => [ | ||
| 'itop-ext1' => [ | ||
| 'installed' => true, | ||
| ], | ||
| ], | ||
| 'aWizardStepDefinition' => [ | ||
| 'extension_code' => 'itop-ext1', | ||
| 'mandatory' => true, | ||
| 'missing' => true, | ||
| 'uninstallable' => true, | ||
| ], | ||
| 'bCurrentSelected' => false, | ||
| 'aExpectedFlags' => [ | ||
| 'uninstallable' => true, | ||
| 'missing' => true, | ||
| 'installed' => true, | ||
| 'disabled' => true, | ||
| 'checked' => false, | ||
| ], | ||
| ], | ||
| 'A missing extension should always be disabled and unchecked, even when non-uninstallable' => [ | ||
| 'aExtensionsOnDiskOrDb' => [ | ||
| 'itop-ext1' => [ | ||
| 'installed' => true, | ||
| ], | ||
| ], | ||
| 'aWizardStepDefinition' => [ | ||
| 'extension_code' => 'itop-ext1', | ||
| 'mandatory' => true, | ||
| 'missing' => true, | ||
| 'uninstallable' => false, | ||
| ], | ||
| 'bCurrentSelected' => false, | ||
| 'aExpectedFlags' => [ | ||
| 'uninstallable' => false, | ||
| 'missing' => true, | ||
| 'installed' => true, | ||
| 'disabled' => true, | ||
| 'checked' => false, | ||
| ], | ||
| ], |
There was a problem hiding this comment.
The test cases for missing extensions don't accurately simulate the production scenario. In production, when an extension is missing from disk, it won't exist in the ExtensionsMap's aExtensionsByCode array, so GetFromExtensionCode() returns null. However, in these tests, the extension is included in aExtensionsOnDiskOrDb, which means it WILL be present in the fake extension map and GetFromExtensionCode() will NOT return null. Consider updating the test setup to more accurately reflect the production scenario, perhaps by not including missing extensions in aExtensionsOnDiskOrDb, or by having a separate mechanism in the fake map to simulate extensions that are in the database but removed from disk.
3 participants
No description provided.