`delete()` can destroy a whole variant family — the variant-base guard is missing

[Pichinov-Jose]

Repository: SplashSync/Dolibarr
Module version: 2.23.3
Dolibarr: 23.0.0
Severity: data loss

Summary

CRUDTrait::load() refuses to touch a variant base product. CRUDTrait::delete() does not carry the
same guard, and Dolibarr's own Product::delete() will not stop it either: a variant parent has
no order lines of its own — the sales sit on its children — so isObjectUsed() reports it as free.

A delete request that resolves to a variant base therefore deletes it, taking the whole family with
it, including children with sales attached.

The guard that exists, and the one that does not

src/Objects/Product/CRUDTrait.php, in load():

//====================================================================//
// Loading Variant Parent Product is Forbidden!
if (!$force && VariantsManager::hasProductVariants((int) $objectId)) {
    return Splash::log()->errNull(Splash::trans("ProductIsVariantBase"));
}

The same file, in delete() — nothing equivalent:

public function delete(string $objectId): bool
{
    global $db,$user;
    Splash::log()->trace();
    // ... no variant-base check ...
    $object->id = (int) $objectId;
    // ...
    if ($object->delete($user) <= 0) {
        return $this->catchDolibarrErrors($object);
    }

Measured on a real install

693   COURS25-STL    children=14   isObjectUsed=0   -> Dolibarr ALLOWS deletion
1180  SAM-DIRT2-...  children=0    isObjectUsed=8   -> refused (correct)

Product 693 is a course family with 14 variations, several of them sold. Dolibarr considers the
parent unused and would delete it.

How a delete request reaches a variant base

This is not theoretical — we hit the exact setup that produces it.

Five WooCommerce variable products each had one variation carrying the parent's own SKU (price 0,
never sold, malformed attribute value). The Splash link for such a variation resolves to the
variant base on the Dolibarr side. Every write returned ProductIsVariantBase, which is how we
noticed them.

Deleting one of those variations in WooCommerce commits Product / SPL_A_DELETE
(Post\HooksTrait::deleted()), and that link resolves to the Dolibarr parent — i.e. a request to
delete a product with 14 children and sales. Nothing on the Dolibarr side would have refused it.

We removed the deleted_post hook in our own process before cleaning those variations up, which is
the only reason this did not happen. That was luck of timing, not a safety net.

Suggested fix

Apply the same guard load() already uses, at the top of delete():

 public function delete(string $objectId): bool
 {
     global $db,$user;
     //====================================================================//
     // Stack Trace
     Splash::log()->trace();
+    //====================================================================//
+    // Deleting a Variant Parent Product is Forbidden!
+    if (VariantsManager::hasProductVariants((int) $objectId)) {
+        return Splash::log()->err(Splash::trans("ProductIsVariantBase"));
+    }

VariantsManager is already imported in the file, and the translation key already exists.

This does not break the legitimate case handled at the end of delete(), where removing the last
variant also removes its parent: at that point the child is already gone, no combination remains, so
hasProductVariants() is false and the guard does not apply. Verified on the same install —
hasProductVariants(693) is true (parent, refused) while hasProductVariants(1180) is false
(sold child, normal path preserved).

Related

A duplicate SKU between a variable parent and one of its variations is what makes a link point at a
variant base in the first place. Refusing to link — or warning — in that situation would remove the
trigger as well as the consequence.