N°9830 - Fix setup error when trying to remove class with a TagSet attribute

This reverts commit 52fca750b0.

datamodels/2.x/combodo-data-feature-removal/src/Controller/DataFeatureRemovalController.php

@@ -127,6 +127,9 @@ class DataFeatureRemovalController extends Controller
 
 			$aRemovedExtensions = $this->aExtensionsToCheck['to_be_removed'];
 			$aHiddenInputs['removed_extensions'] = $this->ConvertIntoSetupFormat($aRemovedExtensions);
+
+			$aExtensionsNotUninstallable = $this->aExtensionsToCheck['extensions_not_uninstallable'];
+			$aHiddenInputs['extensions_not_uninstallable'] = $this->ConvertIntoSetupFormat($aExtensionsNotUninstallable);
 		}
 
 		$aParams['aAddedExtensions'] = $aAddedExtensions;
@@ -344,7 +347,7 @@ class DataFeatureRemovalController extends Controller
 					'uninstallable' => $oExtension->CanBeUninstalled(),
 					'remote' => $oExtension->IsRemote(),
 					'missing' => $oExtension->bRemovedFromDisk,
-					'cannot-be-installed' => $oExtension->HasDependencyIssue(),
+					'dependency_issue' => $oExtension->HasDependencyIssue(),
 				],
 
 			];
@@ -425,6 +428,7 @@ class DataFeatureRemovalController extends Controller
 		$this->aExtensionsToCheck = [
 			'to_be_installed' => [],
 			'to_be_removed' => [],
+			'extensions_not_uninstallable' => [],
 		];
 		foreach ($aAvailableExtensions as $sCode => &$aExtensionData) {
 			if (!isset($aSelectedExtensionsFromUI[$sCode])) {
@@ -438,6 +442,9 @@ class DataFeatureRemovalController extends Controller
 				if (! $this->bForcedUninstallation && $aExtensionData['extra_flags']['uninstallable']) {
 					$this->bForcedUninstallation = true;
 				}
+				if (false === $aExtensionData['extra_flags']['uninstallable'] || true === $aExtensionData['extra_flags']['remote']) {
+					$this->aExtensionsToCheck['extensions_not_uninstallable'][] = $sCode;
+				}
 			} elseif (!$aExtensionData['installed'] && $aSelectedExtensionsFromUI[$sCode] === 'on') {
 				$aExtensionData['extra_flags']['selected'] = true;
 				$sLabel = $aAvailableExtensions[$sCode]['label'];

datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php

@@ -134,10 +134,6 @@ class DataCleanupService
 				/** @var DBObject $oDependentObj */
 				while ($oDependentObj = $oSet->Fetch()) {
 					$iDeletePropagationOption = $oExtKeyAttDef->GetDeletionPropagationOption();
-					if ($iDeletePropagationOption == DEL_MANUAL) {
-						$this->oObjectService->SetIssue(get_class($oDependentObj));
-						continue;
-					}
 
 					if ($oExtKeyAttDef->IsNullAllowed()) {
 						// Optional external key, list to reset
@@ -152,6 +148,12 @@ class DataCleanupService
 							return false;
 						}
 					} else {
+						// Mandatory external key
+						if ($iDeletePropagationOption == DEL_MANUAL) {
+							// Cannot be deleted automatically, must be handled manually
+							$this->oObjectService->SetIssue(get_class($oDependentObj));
+							continue;
+						}
 						// Propagate deletion only if not visited
 						if ($this->IsVisited($oDependentObj)) {
 							continue;

datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php

@@ -59,9 +59,11 @@ class StaticDeletionPlan
 	{
 		foreach ($aClasses as $sClass) {
 			$oDeletionPlanItem = $this->GetInitialClassDeletionPlan($sClass);
-			$oDeletionPlanEntity = new DeletionPlanEntity();
-			$oDeletionPlanEntity->oDelete->Merge($oDeletionPlanItem);
-			$this->aDeletionPlan[$sClass] = $oDeletionPlanEntity;
+			// N°9831 Do not overwrite existing entity as it may already exist for this class if a previously processed class references it (issues/updates already accumulated must be kept)
+			if (false === array_key_exists($sClass, $this->aDeletionPlan)) {
+				$this->aDeletionPlan[$sClass] = new DeletionPlanEntity();
+			}
+			$this->aDeletionPlan[$sClass]->oDelete->Merge($oDeletionPlanItem);
 
 			$this->DeletionPlanForReferencingClasses($sClass);
 		}

setup/feature_removal/AbstractSetupAudit.php

@@ -8,6 +8,7 @@ use DBObjectSet;
 use IssueLog;
 use MetaModel;
 use SetupLog;
+use TagSetFieldData;
 
 require_once APPROOT.'setup/feature_removal/ModelReflectionSerializer.php';
 
@@ -68,6 +69,10 @@ abstract class AbstractSetupAudit
 				continue;
 			}
 
+			if (MetaModel::IsSameFamily($sClass, TagSetFieldData::class)) {
+				continue;
+			}
+
 			if (!MetaModel::IsStandaloneClass($sClass)) {
 				$iCount = $this->Count($sClass);
 				$this->aFinalClassesToCleanup[$sClass] = $iCount;

setup/itopextension.class.inc.php

@@ -176,7 +176,7 @@ class iTopExtension
 		};
 	}
 
-	public function IsRemote(): string
+	public function IsRemote(): bool
 	{
 		return $this->sSource === self::SOURCE_REMOTE;
 	}

setup/runtimeenv.class.inc.php

@@ -933,6 +933,7 @@ class RunTimeEnvironment
 			@chmod($sFinalConfig, 0440); // Read-only for owner and group, nothing for others
 
 			SetupUtils::rrmdir(dirname($sBuildConfig)); // Cleanup the temporary build dir if empty
+			MetaModel::ResetAllCaches($this->sBuildEnv);
 			MetaModel::ResetAllCaches($this->sFinalEnv);
 
 			if (! isset($_SESSION)) {

setup/wizardsteps/WizStepInstall.php

@@ -112,6 +112,22 @@ JS);
 			return;
 		}
 
+		// When the setup reach this step, it already checked whether extensions were uninstallable (during WizStepModulesChoice or combodo-data-feature-removal). We only need to log what has been done.
+		if ($this->oWizard->GetParameter('force-uninstall', false)) {
+			SetupLog::Warning("User disabled uninstallation checks");
+		}
+		$aExtensionsRemoved = json_decode($this->oWizard->GetParameter('removed_extensions'), true) ?? [];
+		$aExtensionsNotUninstallable = json_decode($this->oWizard->GetParameter('extensions_not_uninstallable'), true) ?? [];
+		$aExtensionsForceUninstalled = [];
+		foreach ($aExtensionsRemoved as $sExtensionCode => $sLabel) {
+			if (in_array($sExtensionCode, $aExtensionsNotUninstallable)) {
+				$aExtensionsForceUninstalled[] = $sExtensionCode;
+			}
+		}
+		if (count($aExtensionsForceUninstalled)) {
+			SetupLog::Warning("Extensions uninstalled forcefully : ".implode(',', $aExtensionsForceUninstalled));
+		}
+
 		$oPage->add_ready_script(<<<JS
 	$("#wiz_form").data("installation_status", "not started");
 	ExecuteStep("");

setup/wizardsteps/WizStepLandingBeforeAudit.php

@@ -31,8 +31,8 @@ class WizStepLandingBeforeAudit extends WizStepModulesChoice
 		$oWizard->SetParameter('db_tls_enabled', $this->oConfig->Get('db_tls.enabled'));
 		$oWizard->SetParameter('db_tls_ca', $this->oConfig->Get('db_tls.ca') ?? '');
 		$oWizard->SetParameter('display_choices', '');
-		$oWizard->SetParameter('extensions_not_uninstallable', '[]');
 
+		$oWizard->SaveParameter('extensions_not_uninstallable', '[]');
 		$oWizard->SaveParameter('use_symbolic_links', MFCompiler::UseSymbolicLinks());
 		$oWizard->SaveParameter('force-uninstall', false);
 		$oWizard->SaveParameter('skip_wizard', false);

setup/wizardsteps/WizStepModulesChoice.php

@@ -750,8 +750,8 @@ EOF
 		$bSelected = isset($aSelectedComponents[$sChoiceId]) && ($aSelectedComponents[$sChoiceId] === $sChoiceId);
 		$bMissingFromDisk = isset($aChoice['missing']) && $aChoice['missing'] === true;
 		$bMandatory = (isset($aChoice['mandatory']) && $aChoice['mandatory']);
-		$bInstalled = $bMissingFromDisk || $oITopExtension->bInstalled;
-		$bDependencyIssue = $oITopExtension->HasDependencyIssue();
+		$bInstalled = $bMissingFromDisk || $oITopExtension?->bInstalled ?? false;
+		$bDependencyIssue = $oITopExtension?->HasDependencyIssue() ?? false;
 
 		$bChecked = $bSelected;
 		$bDisabled = false;

setup/wizardsteps/WizStepSummary.php

@@ -116,7 +116,7 @@ class WizStepSummary extends AbstractWizStepInstall
 		$oPage->add('<div class="closed"><a class="title ibo-setup-summary-title" href="#" aria-label="Extensions to be uninstalled">Extensions to be uninstalled</a>');
 
 		$aExtensionsRemoved = json_decode($this->oWizard->GetParameter('removed_extensions'), true) ?? [];
-		$aExtensionsNotUninstallable = json_decode($this->oWizard->GetParameter('extensions_not_uninstallable')) ?? [];
+		$aExtensionsNotUninstallable = json_decode($this->oWizard->GetParameter('extensions_not_uninstallable'), true) ?? [];
 		if (count($aExtensionsRemoved) > 0) {
 			$sExtensionsRemoved = '<ul>';
 			foreach ($aExtensionsRemoved as $sExtensionCode => $sLabel) {

tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php

@@ -163,6 +163,58 @@ class DataCleanupServiceTest extends \AbstractCleanup
 		$this->AssertSummaryEquals($aExpected, $aRes);
 	}
 
+	/**
+	 * N°9831
+	 *
+	 * A nullable external key configured as DEL_MANUAL must NOT be treated as an issue: it is simply
+	 * reset (set to null), exactly like core DBObject::MakeDeletionPlan does. DEL_MANUAL only blocks
+	 * for mandatory keys. This mirrors the real "Hypervisor.farm_id -> Farm" case (nullable + DEL_MANUAL).
+	 *
+	 * Regression guard for the ordering of the IsNullAllowed()/DEL_MANUAL checks in RecursiveDeletion:
+	 * reverting that change would make ExecuteCleanup throw here instead of resetting the object.
+	 */
+	public function testExecuteCleanup_NullableManualShouldResetAndNotThrow()
+	{
+		$this->GivenDFRTreeInDB(<<<EOF
+			DFRToRemoveLeaf_1 <- DFRManualNullable_1
+		EOF);
+
+		$aClasses = [ 'DFRToRemoveLeaf' ];
+		$oService = new DataCleanupService();
+		// Must NOT throw: the nullable external key is reset, not reported as an issue
+		$aRes = $oService->ExecuteCleanup($aClasses);
+
+		$aExpected = [
+			['DFRManualNullable', 1, 0 ],
+			['DFRToRemoveLeaf', 0, 1 ],
+		];
+		$this->AssertSummaryEquals($aExpected, $aRes);
+	}
+
+	/**
+	 * N°9831
+	 *
+	 * Summary counterpart of the test above: a nullable DEL_MANUAL external key is reported as an
+	 * update (reset), with an issue count of 0. Reverting the RecursiveDeletion change would report
+	 * it as an issue instead.
+	 */
+	public function testGetCleanupSummary_NullableManualShouldBeUpdateNotIssue()
+	{
+		$this->GivenDFRTreeInDB(<<<EOF
+			DFRToRemoveLeaf_1 <- DFRManualNullable_1
+		EOF);
+
+		$aClasses = [ 'DFRToRemoveLeaf' ];
+		$oService = new DataCleanupService();
+		$aRes = $oService->GetCleanupSummary($aClasses);
+
+		$aExpected = [
+			['DFRManualNullable', 1, 0, 0 ],
+			['DFRToRemoveLeaf', 0, 1, 0 ],
+		];
+		$this->AssertSummaryEquals($aExpected, $aRes);
+	}
+
 	private function AssertSummaryEquals(array $expected, $actual, $sMessage = '')
 	{
 		$aExpected = [];

tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php

@@ -9,7 +9,6 @@ namespace Combodo\iTop\Test\UnitTest\Module\DataFeatureRemoval;
 
 use Combodo\iTop\DataFeatureRemoval\Entity\DataCleanupSummaryEntity;
 use Combodo\iTop\DataFeatureRemoval\Service\StaticDeletionPlan;
-use MetaModel;
 
 require_once __DIR__."/AbstractCleanup.php";
 class StaticDeletionPlanTest extends \AbstractCleanup
@@ -150,6 +149,66 @@ EOF);
 		$this->AssertSummaryEquals($aExpected, $aRes);
 	}
 
+	/**
+	 * N°9831
+	 *
+	 * A DEL_MANUAL issue must still be reported when the referencing class (DFRManual) is itself part
+	 * of the classes to remove AND is processed AFTER the class it points to (DFRToRemoveLeaf).
+	 *
+	 * This mirrors the real "VirtualMachine -> VirtualHost" case: the setup audit sorts the removed
+	 * classes alphabetically and drops abstract classes, so a referencing class such as VirtualMachine
+	 * ends up processed after its concrete targets (Farm/Hypervisor/Cloud). Here DFRToRemove is abstract
+	 * and DFRToRemoveLeaf is its concrete child, while DFRManual points to DFRToRemove with a mandatory
+	 * DEL_MANUAL external key.
+	 *
+	 * Regression: the plan entity accumulating the issue used to be overwritten when the referencing
+	 * class was processed as a top-level class, silently dropping the issue (no blocking message on the
+	 * summary page, then a crash at execution time).
+	 */
+	public function testGetCleanupSummary_IssueKeptWhenReferencingClassListedAfterTarget(): void
+	{
+		$this->GivenDFRObjectsInDB(<<<EOF
+			create DFRToRemoveLeaf (name = DFRToRemoveLeaf_1)
+			create DFRManual       (name = DFRManual_1, extkey_id = DFRToRemoveLeaf_1)
+EOF);
+
+		// Target listed BEFORE the referencing class, as produced by the alphabetically sorted audit
+		$aClasses = ['DFRToRemoveLeaf', 'DFRManual'];
+		$oService = new StaticDeletionPlan();
+		$aRes = $oService->GetCleanupSummary($aClasses);
+
+		$aExpected = [
+			'DFRToRemoveLeaf' => ['iDeleteCount' => 1],
+			'DFRManual'       => ['iDeleteCount' => 1, 'iIssueCount' => 1],
+		];
+		$this->AssertSummaryEquals($aExpected, $aRes);
+	}
+
+	/**
+	 * N°9831
+	 *
+	 * Control case: the same issue must also be reported when the referencing class is listed BEFORE
+	 * its target (this order already worked, e.g. "Enclosure -> Rack"). Together with the test above,
+	 * this locks in that issue detection is independent of the processing order.
+	 */
+	public function testGetCleanupSummary_IssueKeptWhenReferencingClassListedBeforeTarget(): void
+	{
+		$this->GivenDFRObjectsInDB(<<<EOF
+			create DFRToRemoveLeaf (name = DFRToRemoveLeaf_1)
+			create DFRManual       (name = DFRManual_1, extkey_id = DFRToRemoveLeaf_1)
+EOF);
+
+		$aClasses = ['DFRManual', 'DFRToRemoveLeaf'];
+		$oService = new StaticDeletionPlan();
+		$aRes = $oService->GetCleanupSummary($aClasses);
+
+		$aExpected = [
+			'DFRToRemoveLeaf' => ['iDeleteCount' => 1],
+			'DFRManual'       => ['iDeleteCount' => 1, 'iIssueCount' => 1],
+		];
+		$this->AssertSummaryEquals($aExpected, $aRes);
+	}
+
 	public function testCircularRefsShouldNotRunInfinitely()
 	{
 		$this->GivenDFRObjectsInDB(<<<EOF

tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/data_cleanup_delta.xml

@@ -319,6 +319,58 @@
       </presentation>
       <parent>cmdbAbstractObject</parent>
     </class>
+    <class id="DFRManualNullable" _created_in="itop-structure" _delta="define">
+      <properties>
+        <category>bizmodel,searchable</category>
+        <abstract>false</abstract>
+        <db_table>dfrmanualnullable</db_table>
+        <naming>
+          <attributes/>
+        </naming>
+        <reconciliation>
+          <attributes/>
+        </reconciliation>
+      </properties>
+      <fields>
+        <field id="name" xsi:type="AttributeString">
+          <sql>name</sql>
+          <default_value/>
+          <is_null_allowed>true</is_null_allowed>
+          <validation_pattern/>
+          <dependencies/>
+          <tracking_level>all</tracking_level>
+        </field>
+        <field id="extkey_id" xsi:type="AttributeExternalKey">
+          <sql>extkey_id</sql>
+          <filter/>
+          <dependencies/>
+          <is_null_allowed>true</is_null_allowed>
+          <target_class>DFRToRemove</target_class>
+          <on_target_delete>DEL_MANUAL</on_target_delete>
+          <tracking_level>all</tracking_level>
+        </field>
+      </fields>
+      <methods/>
+      <presentation>
+        <list>
+          <items/>
+        </list>
+        <search>
+          <items/>
+        </search>
+        <details>
+          <items>
+            <item id="name">
+              <rank>10</rank>
+            </item>
+            <item id="extkey_id">
+              <rank>20</rank>
+            </item>
+          </items>
+        </details>
+      </presentation>
+      <parent>cmdbAbstractObject</parent>
+    </class>
     <class id="DFRLeafNotToRemove" _created_in="itop-structure" _delta="define">
       <properties>
         <category>bizmodel,searchable</category>
@@ -718,6 +770,14 @@
         <entry id="Class:DFRManual/Attribute:name+" _delta="define"><![CDATA[]]></entry>
         <entry id="Class:DFRManual/Attribute:extkey_id" _delta="define"><![CDATA[Dfrtoremove id]]></entry>
         <entry id="Class:DFRManual/Attribute:extkey_id+" _delta="define"><![CDATA[]]></entry>
+        <entry id="Class:DFRManualNullable/Name" _delta="define"><![CDATA[]]></entry>
+        <entry id="Class:DFRManualNullable/ComplementaryName" _delta="define"><![CDATA[]]></entry>
+        <entry id="Class:DFRManualNullable" _delta="define"><![CDATA[DFRManualNullable]]></entry>
+        <entry id="Class:DFRManualNullable+" _delta="define"><![CDATA[]]></entry>
+        <entry id="Class:DFRManualNullable/Attribute:name" _delta="define"><![CDATA[Name]]></entry>
+        <entry id="Class:DFRManualNullable/Attribute:name+" _delta="define"><![CDATA[]]></entry>
+        <entry id="Class:DFRManualNullable/Attribute:extkey_id" _delta="define"><![CDATA[Dfrtoremove id]]></entry>
+        <entry id="Class:DFRManualNullable/Attribute:extkey_id+" _delta="define"><![CDATA[]]></entry>
       </entries>
     </dictionary>
   </dictionaries>

tests/php-unit-tests/unitary-tests/setup/WizStepModulesChoiceTest.php

@@ -78,9 +78,6 @@ class WizStepModulesChoiceTest extends ItopTestCase
 			],
 			'A missing extension should be disabled and unchecked' => [
 				'aExtensionsOnDiskOrDb' => [
-					'itop-ext1' => [
-						'installed' => false,
-					],
 				],
 				'aWizardStepDefinition' => [
 					'extension_code' => 'itop-ext1',
@@ -102,9 +99,6 @@ class WizStepModulesChoiceTest extends ItopTestCase
 			],
 			'A missing extension should always be disabled and unchecked, even when mandatory' => [
 				'aExtensionsOnDiskOrDb' => [
-					'itop-ext1' => [
-						'installed' => false,
-					],
 				],
 				'aWizardStepDefinition' => [
 					'extension_code' => 'itop-ext1',
@@ -126,9 +120,6 @@ class WizStepModulesChoiceTest extends ItopTestCase
 			],
 			'A missing extension should always be disabled and unchecked, even when non-uninstallable' => [
 				'aExtensionsOnDiskOrDb' => [
-					'itop-ext1' => [
-						'installed' => false,
-					],
 				],
 				'aWizardStepDefinition' => [
 					'extension_code' => 'itop-ext1',

tests/php-unit-tests/unitary-tests/setup/feature_removal/SetupAuditTest.php

@@ -79,6 +79,7 @@ class SetupAuditTest extends ItopCustomDatamodelTestCase
 			"Feature1Module1MyClass",
 			"FinalClassFeature2Module1MyClass",
 			"FinalClassFeature2Module1MyFinalClassFromLocation",
+			"TagSetFieldDataFor_FinalClassFeature2Module1MyFinalClassFromLocation__domains",
 		];
 		$this->assertEqualsCanonicalizing($expected, $oSetupAudit->GetRemovedClasses());
 

tests/php-unit-tests/unitary-tests/setup/feature_removal/additional_features/finalclass_ext1/finalclass_ext1_module1/datamodel.finalclass_ext1_module1.xml

@@ -64,6 +64,12 @@
           <is_null_allowed>false</is_null_allowed>
           <validation_pattern/>
         </field>
+        <field id="domains" xsi:type="AttributeTagSet">
+          <sql>domains</sql>
+          <is_null_allowed>true</is_null_allowed>
+          <tracking_level>all</tracking_level>
+          <max_items>12</max_items>
+        </field>
       </fields>
       <methods/>
       <presentation/>

tests/php-unit-tests/unitary-tests/setup/feature_removal/additional_features/finalclass_ext2/finalclass_ext2_module1/datamodel.finalclass_ext2_module1.xml

@@ -31,6 +31,12 @@
           <default_value/>
           <is_null_allowed>false</is_null_allowed>
         </field>
+        <field id="domains" xsi:type="AttributeTagSet">
+          <sql>domains</sql>
+          <is_null_allowed>true</is_null_allowed>
+          <tracking_level>all</tracking_level>
+          <max_items>12</max_items>
+        </field>
       </fields>
       <methods/>
       <presentation/>