N°9831 - Fix setup error when trying to remove class with a DEL_MANUAL ext. key

[Molkobain]

Base information

Question	Answer
Related to a SourceForge thread / Another PR / A GitHub Issue / Combodo ticket?	N°9831
Type of change?	Bug fix

Please be advised that I tried to make a fix for both bugs but that I don't master these parts of the code, so I may not be aware of the big picture.

Symptom (bug) / Objective (enhancement)

When uninstalling a module through the setup, the "check compatibility" step detects the objects that must be cleaned up to keep iTop consistent and redirects to the "extension management" page, which displays a summary of what will be deleted/updated. When some of these objects can't be deleted automatically (e.g. an external key with DEL_MANUAL option), the page is supposed to show a blocking message telling the user that it must remove these data manually first, and the "Proceed with deletion" button must not lead to a deletion.

But in this case, for some modules the blocking message isn't shown even though objects cannot be deleted automatically. The "Proceed with deletion" button stays enabled, and clicking it crashes the application with:

Combodo\iTop\DataFeatureRemoval\Helper\DataFeatureRemovalException : Deletion Plan cannot be executed due to issues

Concrete cases found while troubleshooting (standard datamodel):

Removed module	External key involved	is_null_allowed	on_target_delete	Behavior
Data center devices	Enclosure.rack_id => Rack	false	DEL_MANUAL	✅ Blocking message shown
Virtualization	Hypervisor.farm_id => Farm	true	DEL_MANUAL	❌ No blocking message, crash on "Proceed with deletion" (Bug #1)
Virtualization	VirtualMachine.virtualhost_id => VirtualHost (abstract class)	false	DEL_MANUAL	❌ No blocking message, crash on "Proceed with deletion" (Bug #2)

Reproduction procedure (bug)

1. On iTop 3.3.0-dev
2. Install iTop with the sample data (so that virtualization objects exist)
3. Run setup and select all modules
4. In the backoffice, create an Enclosure linked to a Rack
5. Run the setup again but this time unselect Virtualization
6. Continue to the "check compatibility" step
7. Click on "Cleanup my data"
8. See that there is no blocking message below the data table
9. Click on "Proceed with deletion" and see that the application crashes

Cause (bug)

The "summary" (whether to block) and the "actual execution" rely on two different deletion-plan services that do not do the same processing:

• Summary is done by StaticDeletionPlan::GetCleanupSummary() (called by DataFeatureRemovalController::GetDeletionPlanSummaryTable())
• Execution is done by DataCleanupService::RecursiveDeletion(), which throws the exception through ObjectService::SetIssue().

Two different bugs made them diverge:

Cause of bug #1 - Wrong order between IsNullAllowed() and DEL_MANUAL (DataCleanupService::RecursiveDeletion())
The "execution" service tested DEL_MANUAL before IsNullAllowed(), so a nullable external key with DEL_MANUAL was treated as an unresolvable issue and threw an exception.

But the reference engine DBObject::MakeDeletionPlan() (and StaticDeletionPlan) test IsNullAllowed() first.

A nullable key is always a simple "reset to null" update, and DEL_MANUAL only matters for mandatory ext. keys. That caused for Hypervisor.farm_id (nullable + DEL_MANUAL) the "summary" reported an update (no issue, no blocking message) while "execution" threw an exception.

Cause of bug #2 - Deletion-plan entity overwritten (StaticDeletionPlan::GetStaticDeletionPlan())
While iterating the classes to remove, the method created a new DeletionPlanEntity for each classes and assigned it, loosing any entity already populated for that class by a previously processed class (issues/updates are accumulated on the referencing class while its target is processed).

The setup audit feeds the classes sorted alphabetically and skips abstract classes (AbstractSetupAudit::RunDataAudit()). For VirtualMachine.virtualhost_id => VirtualHost:

• VirtualHost is abstract, so only the concrete subclasses Cloud/Farm/Hypervisor are listed;
• VirtualMachine (referencing, mandatory DEL_MANUAL) is sorted after them, so the issue recorded on the VirtualMachine entity while processing Farm/Hypervisor/Cloud was lost when VirtualMachine was later processed as a root class => iIssueCount = 0 => no blocking message, but execution still detected it and threw an exception.

This also explains why the Data center devices module works as expected: Enclosure (referencing) is sorted before Rack (its target), so the issue recorded on Enclosure is never overwritten afterwards.

Proposed solution (bug and enhancement)

Fix for bug #1
In the "summary" service, test IsNullAllowed() first like in the "execution" service (StaticDeletionPlan) and core DBObject::MakeDeletionPlan().

Fix for bug #2
Do not overwrite an existing plan entity, create it only when missing, then merge the initial IDs.
Issues/updates already accumulated are preserved, making issue detection independent of the processing order.

Tests
I've tried to add test cases in StaticDeletionPlanTest but I really not sure how it's supposed to work, so reviews are welcome 😅

Checklist before requesting a review

• I have performed a self-review of my code
• I have tested all changes I made on an iTop instance
• I have added a unit test, otherwise I have explained why I couldn't
• Is the PR clear and detailed enough so anyone can understand without digging in the code?

[Copilot]

Pull request overview

Fixes setup cleanup planning for nullable and mandatory DEL_MANUAL external keys.

Changes:

• Handles nullable external keys before DEL_MANUAL.
• Preserves accumulated deletion-plan entities.
• Adds order-independence regression tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
DataCleanupService.php	Corrects nullable-key cleanup behavior.
StaticDeletionPlan.php	Prevents accumulated issues from being overwritten.
StaticDeletionPlanTest.php	Tests both class-processing orders.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[greptile-apps]

Greptile Summary

This PR makes data cleanup planning and execution agree for DEL_MANUAL external keys. The main changes are:

• Reset nullable external keys before checking manual deletion rules.
• Preserve issues and updates already collected for a deletion-plan entity.
• Test mandatory manual-deletion references in both class-processing orders.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.

Important Files Changed

Filename	Overview
datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php	Handles nullable external keys before applying DEL_MANUAL blocking behavior.
datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php	Reuses existing plan entities so earlier issues and updates are not overwritten.
tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php	Adds tests for both processing orders and removes an unused import.

_{Reviews (1): Last reviewed commit: "N°9831 - Fix ext. management deletion pl..." | Re-trigger Greptile}

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[odain-cbd]

nice work

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

• This now preserves the plan across separate calls on the same service instance, not only across classes in one call. For example, after GetStaticDeletionPlan(['DFRManual']), a later call after those rows were removed still returns the old delete IDs/issues because Merge() never removes them. Reset the per-computation state before the loop, while retaining the conditional creation here so entities are still preserved within one computation.

			if (false === array_key_exists($sClass, $this->aDeletionPlan)) {
				$this->aDeletionPlan[$sClass] = new DeletionPlanEntity();
			}
			$this->aDeletionPlan[$sClass]->oDelete->Merge($oDeletionPlanItem);