N°9165 - secure data cleanup

[eespie]

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

[greptile-apps]

Greptile Summary

This PR replaces the DeletionPlanService (which used iTop's DeletionPlan API) with a new DataCleanupService that performs recursive object deletion directly, respecting execution time and memory limits via the new ExecutionLimits utility. The controller's guard for proceeding with cleanup is updated from an operation-count ceiling to an issue-presence check.

• P1 — wrong namespace in tests: DataCleanupServiceTest imports ExecutionLimits from Combodo\\iTop\\DataFeatureRemoval\\Helper\\ExecutionLimits, but the class lives in Combodo\\iTop\\Service\\Limits\\ExecutionLimits; mock-based stop-in-process tests will fail to load.
• P1 — ExecutionLimits zero-duration bug: Passing iMaxDuration = 0 (the constructor default) stores time() as the deadline. ShouldStopExecution() will return true on its very first call, silently halting cleanup after one operation.

Confidence Score: 3/5

Not safe to merge as-is: two P1 bugs (wrong namespace breaks tests, zero-duration default halts cleanup) need to be resolved first.

Two distinct P1 findings — a namespace mismatch that makes stop-in-process tests unrunnable, and a constructor bug in ExecutionLimits where iMaxDuration=0 immediately triggers the timeout guard — pull the score below the P1 ceiling of 4. The core cleanup logic and the service decomposition are otherwise clean.

sources/Service/Limits/ExecutionLimits.php (zero-duration bug) and tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php (wrong namespace import)

Important Files Changed

Filename	Overview
sources/Service/Limits/ExecutionLimits.php	New execution limits class; constructor bug: iMaxDuration=0 sets deadline to current time, causing ShouldStopExecution() to return true immediately; also has a truncated doc comment.
tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php	New test file; imports ExecutionLimits from wrong namespace (DataFeatureRemoval\Helper vs Service\Limits), causing mock-based tests to fail at load time.
datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php	New service replacing DeletionPlanService; implements recursive deletion with execution limits. Logic appears sound but inherits the iMaxDuration=0 issue from ExecutionLimits.
setup/unattended-install/unattended-install.php	Backup config handling changed from copy-restore to unlink; may be intentional for uninstallation flow but silently discards the backup.
setup/wizardsteps/WizStepModulesChoice.php	Exception for extensions with no associated modules is commented out, silently ignoring a previously fatal validation error.
application/utils.inc.php	New GetMemoryLimit() helper; returns hardcoded 128MB when PHP memory is unlimited (-1), which may over-restrict cleanup on large-memory servers.
datamodels/2.x/combodo-data-feature-removal/src/Controller/DataFeatureRemovalController.php	Updated to use DataCleanupService; bDeletionPossible now blocks on issues rather than operation count; clean refactor.
datamodels/2.x/combodo-data-feature-removal/src/Service/ObjectService.php	New concrete service extending ObjectServiceSummary; performs actual DB deletes/updates within transactions and delegates counters to parent.
datamodels/2.x/combodo-data-feature-removal/src/Service/ObjectServiceSummary.php	New dry-run service that accumulates counts without touching the DB; used by GetCleanupSummary for preview.
datamodels/2.x/combodo-data-feature-removal/src/Service/DeletionPlanService.php	Deleted; replaced entirely by DataCleanupService + ObjectService hierarchy.

Sequence Diagram

sequenceDiagram
    participant Controller as DataFeatureRemovalController
    participant DCS as DataCleanupService
    participant OSS as ObjectServiceSummary
    participant OS as ObjectService
    participant EL as ExecutionLimits

    Note over Controller,EL: Preview / Summary flow
    Controller->>DCS: GetCleanupSummary(aClasses)
    DCS->>OSS: (uses ObjectServiceSummary as dry-run)
    DCS->>DCS: ExecuteCleanup(aClasses, oObjectService=OSS)
    loop For each object
        DCS->>DCS: RecursiveDeletion(oObject)
        DCS->>OSS: Update / Delete / SetIssue
        DCS->>EL: ShouldStopExecution()
        EL-->>DCS: bool
    end
    DCS-->>Controller: array[DataCleanupSummaryEntity]

    Note over Controller,EL: Execute / Real cleanup flow
    Controller->>DCS: ExecuteCleanup(aClasses)
    DCS->>OS: (uses ObjectService for real writes)
    loop For each object
        DCS->>DCS: RecursiveDeletion(oObject)
        DCS->>OS: Update (DBUpdate) / Delete (SQL + ROLLBACK on error)
        DCS->>EL: ShouldStopExecution()
        EL-->>DCS: bool
    end
    DCS-->>Controller: array[DataCleanupSummaryEntity]

Loading

_{Reviews (1): Last reviewed commit: "N°9165 - Moved ExecutionLimits" | Re-trigger Greptile}