Merge branch 'fix/setting-null-with-merge'

doc/82-Changelog.md

@@ -32,6 +32,7 @@ This version hasn't been released yet
 * FEATURE: property modifiers can now be applied based on filters (#2756)
 * FEATURE: CIDR notation (network ranges) is supported in such filters (#2757)
 * FIX: synchronizing Service (and -Set) Templates has been fixed (#2745, #2217)
+* FIX: null properties with Sync policy "ignore" are now being ignored (#2657)
 
 ### REST API
 * FIX: Commands give 304 w/o ghost changes for same properties (#2660)

library/Director/Import/Sync.php

@@ -49,6 +49,9 @@ class Sync
     /** @var array<mixed, array<int, string>> key => [property, property]*/
     protected $setNull = [];
 
+    /** @var array<mixed, array<string, mixed>> key => [propertyName, newValue]*/
+    protected $newProperties = [];
+
     /** @var bool Whether we already prepared your sync */
     protected $isPrepared = false;
 
@@ -136,24 +139,6 @@ class Sync
         return $modified;
     }
 
-    /**
-     * Transform the given value to an array
-     *
-     * @param  array|string|null $value
-     *
-     * @return array
-     */
-    protected function wantArray($value)
-    {
-        if (is_array($value)) {
-            return $value;
-        } elseif ($value === null) {
-            return [];
-        } else {
-            return [$value];
-        }
-    }
-
     /**
      * Raise PHP resource limits
      *
@@ -553,6 +538,15 @@ class Sync
      */
     protected function prepareNewObject($row, DbObject $object, $objectKey, $sourceId)
     {
+        if (!isset($this->newProperties[$objectKey])) {
+            $this->newProperties[$objectKey] = [];
+        }
+        // TODO: some more improvements are possible here. First, no need to instantiate
+        //       all new objects, we could stick with the newProperties array. Next, we
+        //       should be more correct when respecting sync property order. Right now,
+        //       a property from another Import Source might win, even if property order
+        //       tells something different. This is a very rare case, but still incorrect.
+        $properties = &$this->newProperties[$objectKey];
         foreach ($this->syncProperties as $propertyKey => $p) {
             if ($p->get('source_id') !== $sourceId) {
                 continue;
@@ -578,36 +572,33 @@ class Sync
                     $varName = substr($prop, 5);
                     if (substr($varName, -2) === '[]') {
                         $varName = substr($varName, 0, -2);
-                        $current = $this->wantArray($object->vars()->$varName);
                         $object->vars()->$varName = array_merge(
-                            $current,
-                            $this->wantArray($val)
+                            (array) ($object->vars()->$varName),
+                            (array) $val
                         );
                     } else {
-                        if ($val === null) {
-                            $this->setNull[$objectKey][$prop] = $prop;
-                        } else {
-                            unset($this->setNull[$objectKey][$prop]);
-                            $object->vars()->$varName = $val;
-                        }
+                        $this->setPropertyWithNullLogic($object, $objectKey, $prop, $val, $properties);
                     }
                 } else {
-                    if ($val === null) {
-                        $this->setNull[$objectKey][$prop] = $prop;
-                    } else {
-                        unset($this->setNull[$objectKey][$prop]);
-                        $object->set($prop, $val);
-                    }
+                    $this->setPropertyWithNullLogic($object, $objectKey, $prop, $val, $properties);
                 }
             } else {
-                if ($val === null) {
-                    $this->setNull[$objectKey][$prop] = $prop;
+                $this->setPropertyWithNullLogic($object, $objectKey, $prop, $val, $properties);
+            }
+        }
+    }
+
+    protected function setPropertyWithNullLogic(DbObject $object, $objectKey, $property, $value, &$allProps)
+    {
+        if ($value === null) {
+            if (! array_key_exists($property, $allProps) || $allProps[$property] === null) {
+                $this->setNull[$objectKey][$property] = $property;
+            }
         } else {
-                    unset($this->setNull[$objectKey][$prop]);
-                    $object->set($prop, $val);
-                }
-            }
+            unset($this->setNull[$objectKey][$property]);
+            $object->set($property, $value);
         }
+        $allProps[$property] = $value;
     }
 
     /**
@@ -812,7 +803,11 @@ class Sync
             }
         }
 
-        if (isset($this->setNull[$key])) {
+        // Hint: in theory, NULL should be set on new objects, but this has no effect
+        //       anyway, and we also do not store vars.something = null, this would
+        //       instead delete the variable. So here we do not need to check for new
+        //       objects, and skip all null values with update policy = 'ignore'
+        if ($policy !== 'ignore' && isset($this->setNull[$key])) {
             foreach ($this->setNull[$key] as $property) {
                 $this->objects[$key]->set($property, null);
             }