N°9722 - Setup - extension dependencies must be computed with installation choice change#965
Merged
Conversation
…d add logging for selected extensions and modules
…g for extension management
…d session management and cleanup processes
…eforeFinalDeltaToCompile + fix test
Contributor
There was a problem hiding this comment.
Pull request overview
This PR (N°9722) refactors iTop's setup/compilation pipeline so that extension dependencies (selected extensions and modules) are computed from the current installation choices before the data model is compiled, rather than after. It consolidates the compilation logic around DoCompile()/GetModulesToLoadFromChoices(), removes the now-obsolete DryRemovalRuntimeEnvironment "dry run" environment, and drops the older GetMFModulesToCompile()/GetDirsToCompile()/GetModulesToLoad()/GetExtraDirsToScan() machinery in favor of a single GetAdditionalMFModulesBeforeFinalDeltaToCompile() hook.
Changes:
- Reworked
RunTimeEnvironment::CompileFrom()/DoCompile()(now returns the selected modules) and simplifiedGetModulesToLoadFromChoices()to derive search dirs and choices internally. - Updated
DataFeatureRemovalControllerto compute selected extensions/modules up front with a plainRunTimeEnvironment, and deletedDryRemovalRuntimeEnvironmentplus its references. - Adjusted callers/tests (
HubController,DataAuditSequencer,UnitTestRunTimeEnvironment) and commented out several tests inSetupAuditTest.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| setup/runtimeenv.class.inc.php | Core refactor of compile pipeline; removes legacy helpers, adds GetAdditionalMFModulesBeforeFinalDeltaToCompile, changes signatures/returns. Docblock typo flagged. |
| setup/extensionsmap.class.inc.php | Adds an empty GetSelectedModules() stub (flagged as dead code). |
| datamodels/2.x/combodo-data-feature-removal/src/Controller/DataFeatureRemovalController.php | Computes selected extensions/modules before compile; adds unused IssueLog import (flagged). |
| datamodels/2.x/itop-hub-connector/src/Controller/HubController.php | Drops now-unused return capture from CompileFrom. |
| setup/sequencers/DataAuditSequencer.php | Removes unused source_dir/extensions_dir params. |
| setup/feature_removal/DryRemovalRuntimeEnvironment.php | Deleted; no remaining references. |
| tests/php-unit-tests/src/Service/UnitTestRunTimeEnvironment.php | Overrides new parent hook instead of old GetMFModulesToCompile. |
| tests/.../setup/feature_removal/SetupAuditTest.php | Removes teardown and comments out three tests (flagged: maintainability/coverage). |
| tests/.../combodo-data-feature-removal/*.php | Adds require_once for AbstractCleanup.php. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
| Filename | Overview |
|---|---|
| setup/runtimeenv.class.inc.php | Major refactoring of CompileFrom/DoCompile/GetModulesToLoadFromChoices; introduces a P1 bug where array_keys() on an indexed list of extension codes passes integer indices to DoCompile, causing all extensions to be treated as not chosen during compilation. |
| setup/extensionsmap.class.inc.php | Adds CheckExtensionsValidity() method (extracted from DoCompile) and GetSelectedExtensions(); GetSelectedExtensions still crashes when GetChoicesFromDatabase returns false (already flagged externally). |
| datamodels/2.x/combodo-data-feature-removal/src/Controller/DataFeatureRemovalController.php | Removes DryRemovalRuntimeEnvironment dependency; reorders compile/module selection logic; extension selection now computed before CopySetupFiles when first-run (ordering concern flagged externally). |
| tests/php-unit-tests/src/Service/UnitTestRunTimeEnvironment.php | Adapts CompileFrom to new API; contains the same array_keys() P1 bug as RunTimeEnvironment::CompileFrom, causing test compilations to treat all extensions as not chosen. |
| tests/php-unit-tests/unitary-tests/setup/feature_removal/SetupAuditTest.php | Replaces DryRemovalRuntimeEnvironment-based tests with direct RunTimeEnvironment calls; testComputeDryRemoval renamed and rewritten, but some previously covered paths remain untested. |
| tests/php-unit-tests/src/BaseTestCase/ItopTestCase.php | Minor: adds a debug echo statement in tearDown that pollutes test output on every file-cleanup run. |
| setup/feature_removal/DryRemovalRuntimeEnvironment.php | Deleted — its responsibilities have been absorbed into RunTimeEnvironment and the calling controller. |
| setup/itopextension.class.inc.php | Adds HasCode() and HasLabel() helpers to iTopExtension; straightforward additions with no issues. |
| setup/sequencers/DataAuditSequencer.php | Removes unused source_dir and extensions_dir parameters from the compile step; no functional impact. |
| tests/php-unit-tests/unitary-tests/setup/ExtensionsMapTest.php | Adds four tests for CheckExtensionsValidity(); tests are well-structured and cover both the warning and exception paths. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant C as DataFeatureRemovalController
participant RTE as RunTimeEnvironment
participant EM as iTopExtensionsMap
participant DC as DoCompile
C->>EM: GetSelectedExtensions(config, added, removed)
EM-->>C: aSelectedExtensions (indexed list of codes)
C->>RTE: CopySetupFiles()
C->>RTE: GetModulesToLoadFromChoices(config, aSelectedExtensions)
RTE->>EM: GetScannedModulesRootDirs()
RTE-->>C: aSelectedModules
C->>DC: DoCompile(aSelectedExtensions, aRemovedExtensions, aSelectedModules)
DC->>EM: GetExtensionsMap(buildEnv)
DC->>EM: DeclareExtensionAsRemoved(aRemovedExtensionCodes)
DC->>EM: CheckExtensionsValidity()
DC-->>C: aSelectedModules
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant C as DataFeatureRemovalController
participant RTE as RunTimeEnvironment
participant EM as iTopExtensionsMap
participant DC as DoCompile
C->>EM: GetSelectedExtensions(config, added, removed)
EM-->>C: aSelectedExtensions (indexed list of codes)
C->>RTE: CopySetupFiles()
C->>RTE: GetModulesToLoadFromChoices(config, aSelectedExtensions)
RTE->>EM: GetScannedModulesRootDirs()
RTE-->>C: aSelectedModules
C->>DC: DoCompile(aSelectedExtensions, aRemovedExtensions, aSelectedModules)
DC->>EM: GetExtensionsMap(buildEnv)
DC->>EM: DeclareExtensionAsRemoved(aRemovedExtensionCodes)
DC->>EM: CheckExtensionsValidity()
DC-->>C: aSelectedModules
Reviews (4): Last reviewed commit: "N°9722 - ✅" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Comment on lines
193
to
+194
| foreach ($this->aFileToClean as $sPath) { | ||
| echo "aFileToClean: $sPath \n"; |
| $this->InitExtensionMap($oConfig); | ||
| $aSelectedExtensions = $this->GetExtensionMap()->GetSelectedExtensions($oConfig, $aAddedExtensions, []); | ||
| $aSelectedModules = $this->GetModulesToLoadFromChoices($oConfig, $aSelectedExtensions); | ||
| return $this->DoCompile(array_keys($aSelectedExtensions), [], $aSelectedModules, $bUseSymLinks ?? false); |
Comment on lines
+199
to
+202
| //echo json_encode(array_keys($oExtensionMap->GetAllExtensions()), JSON_PRETTY_PRINT); | ||
|
|
||
| // $this->expectException(CoreException::class); | ||
| // $this->expectExceptionMessage("Selected extension(s) cannot be installed: Missing extension code ($sExtensionsDirAbsolute)"); |
Comment on lines
+37
to
+40
| protected function tearDown(): void | ||
| { | ||
| parent::tearDown(); | ||
| } |
| } | ||
| } | ||
|
|
||
| // For unit tests |
Comment on lines
+73
to
+77
| $oConfig = new Config(utils::GetConfigFilePath($sSourceEnv)); | ||
| $this->InitExtensionMap($oConfig); | ||
| $aSelectedExtensions = $this->GetExtensionMap()->GetSelectedExtensions($oConfig, $aAddedExtensions, []); | ||
| $aSelectedModules = $this->GetModulesToLoadFromChoices($oConfig, $aSelectedExtensions); | ||
| return $this->DoCompile(array_keys($aSelectedExtensions), [], $aSelectedModules, $bUseSymLinks ?? false); |
Comment on lines
193
to
195
| foreach ($this->aFileToClean as $sPath) { | ||
| echo "aFileToClean: $sPath \n"; | ||
| if (is_file($sPath)) { |
Comment on lines
1510
to
1514
| $aExtensionDirs = []; | ||
| $aFromSelectedExtensionModules = []; | ||
| foreach ($this->GetExtensionMap()->GetAllExtensions() as $oExtension) { | ||
| if ($oExtension->IsMarkedAsChosen() && is_dir($oExtension->sSourceDir)) { | ||
| if (in_array($oExtension->sCode, $aChoices) && is_dir($oExtension->sSourceDir)) { | ||
| $aExtensionDirs [] = $oExtension->sSourceDir; |
Comment on lines
+5
to
+13
| use Combodo\iTop\DataFeatureRemoval\Service\DataFeatureRemoverExtensionService; | ||
| use Combodo\iTop\Test\UnitTest\ItopTestCase; | ||
| use Config; | ||
| use CoreException; | ||
| use DOMFormatException; | ||
| use Exception; | ||
| use iTopExtensionsMap; | ||
| use RunTimeEnvironment; | ||
| use utils; |
Comment on lines
+197
to
+205
| $oExtensionMap = ItopExtensionsMap::GetExtensionsMap($sEnvironment); | ||
|
|
||
| //echo json_encode(array_keys($oExtensionMap->GetAllExtensions()), JSON_PRETTY_PRINT); | ||
|
|
||
| // $this->expectException(CoreException::class); | ||
| // $this->expectExceptionMessage("Selected extension(s) cannot be installed: Missing extension code ($sExtensionsDirAbsolute)"); | ||
|
|
||
| self::assertFalse($oExtensionMap->CheckExtensionsValidity(), 'Check extensions validity should have detected an extension without code.'); | ||
| } |
Comment on lines
+224
to
+232
| $oExtensionMap = ItopExtensionsMap::GetExtensionsMap($sEnvironment); | ||
|
|
||
| //echo json_encode(array_keys($oExtensionMap->GetAllExtensions()), JSON_PRETTY_PRINT); | ||
|
|
||
| // $this->expectException(CoreException::class); | ||
| // $this->expectExceptionMessage("Selected extension(s) cannot be installed: Missing extension code ($sExtensionsDirAbsolute)"); | ||
|
|
||
| self::assertFalse($oExtensionMap->CheckExtensionsValidity(), 'Check extensions validity should have detected an extension without code.'); | ||
| } |
Comment on lines
+250
to
+257
| $oExtensionMap = ItopExtensionsMap::GetExtensionsMap($sEnvironment); | ||
| $oExtensionMap->MarkAsChosen(''); | ||
|
|
||
| //echo json_encode(array_keys($oExtensionMap->GetAllExtensions()), JSON_PRETTY_PRINT); | ||
|
|
||
| $this->expectException(CoreException::class); | ||
| $this->expectExceptionMessage("Selected extension(s) cannot be installed: Missing extension code ($sExtensionsDirAbsolute)"); | ||
|
|
Comment on lines
+278
to
+285
| $oExtensionMap = ItopExtensionsMap::GetExtensionsMap($sEnvironment); | ||
| $oExtensionMap->MarkAsChosen(''); | ||
|
|
||
| //echo json_encode(array_keys($oExtensionMap->GetAllExtensions()), JSON_PRETTY_PRINT); | ||
|
|
||
| $this->expectException(CoreException::class); | ||
| $this->expectExceptionMessage("Selected extension(s) cannot be installed: Missing extension code (Broken extension)"); | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
N°9722 - Setup - extension dependencies must be computed with installation choice change