Attribute validation errors give no context — which product? which attribute?

[Pichinov-Jose]

Repository: SplashSync/Dolibarr
Module version: 2.23.3
Type: enhancement (diagnosability)

Summary

When a variant attribute fails validation, the module logs a message that names neither the product
nor the attribute nor the offending value. Combined with the fact that an invalid attribute is
skipped with continue — so the object write still succeeds and the UI shows green — a data loss
can go unnoticed for months, and is painful to diagnose once noticed.

What you get today

[Splash Client] Product Attribute Value Name is Not Valid.
[Splash Client] Product Attribute Value Name is Not Valid.

That is the entire signal for a product with two broken attributes. Nothing identifies the product,
the attribute, or what was actually received.

Source

src/Objects/Product/Variants/AttributesTrait.php:203 and :233

if (empty($attrData["code"]) || !is_string($attrData["code"])) {
    return Splash::log()->err(" Product Attribute Code is Not Valid.");
}
...
private function isValidScalarData(array $attrData, string $key, string $name): bool
{
    if (empty($attrData[$key]) || !is_scalar($attrData[$key])) {
        return Splash::log()->err("Product Attribute ".$name." is Not Valid.");
    }
    return true;
}

Suggested fix

Build the context once and pass it through:

 private function isValidAttributeDefinition(array $attrData): bool
 {
+    $context = sprintf(
+        " [product %s (id %s) / attribute %s]",
+        $this->object->ref ?? "?",
+        $this->object->id ?? "?",
+        is_scalar($attrData["code"] ?? null) ? (string) $attrData["code"] : "?"
+    );
     if (empty($attrData["code"]) || !is_string($attrData["code"])) {
-        return Splash::log()->err(" Product Attribute Code is Not Valid.");
+        return Splash::log()->err(" Product Attribute Code is Not Valid.".$context);
     }
-    if (!$this->isValidScalarData($attrData, "name", "Public Name")) {
+    if (!$this->isValidScalarData($attrData, "name", "Public Name", $context)) {
         return false;
     }
-    if (!$this->isValidScalarData($attrData, "value", "Value Name")) {
+    if (!$this->isValidScalarData($attrData, "value", "Value Name", $context)) {
         return false;
     }
     return true;
 }

-private function isValidScalarData(array $attrData, string $key, string $name): bool
+private function isValidScalarData(array $attrData, string $key, string $name, string $context = ""): bool
 {
     if (empty($attrData[$key]) || !is_scalar($attrData[$key])) {
-        return Splash::log()->err("Product Attribute ".$name." is Not Valid.");
+        return Splash::log()->err(
+            "Product Attribute ".$name." is Not Valid.".$context
+            ." (received: ".var_export($attrData[$key] ?? null, true).")"
+        );
     }
     return true;
 }

Result on the same failure:

[Splash Client] Product Attribute Value Name is Not Valid.
  [product RF-INS-2023-NON-3M (id 269) / attribute foyer] (received: '')

No extra log volume — the same messages, simply usable.

Wider suggestion

The continue in setVariantsAttributesFields() means an object can be reported as successfully
written while part of its data was dropped. A counter of skipped attributes surfaced as a warning
on the object result would make partial writes visible without changing the tolerant behaviour.