BUG Resolve issue where TreeMultiSelectField would error loading its value (#9604)

* BUG Resolve issue with TreeMultiSelectField not retaining value in some situations
E.g. in an elemental form

src/Forms/TreeMultiselectField.php

@@ -2,16 +2,17 @@
 
 namespace SilverStripe\Forms;
 
-use SilverStripe\Core\Convert;
 use SilverStripe\Control\Controller;
+use SilverStripe\Core\Convert;
 use SilverStripe\ORM\ArrayList;
 use SilverStripe\ORM\DataList;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\DataObjectInterface;
 use SilverStripe\ORM\FieldType\DBHTMLText;
+use SilverStripe\ORM\Relation;
+use SilverStripe\ORM\SS_List;
 use SilverStripe\Security\Group;
 use SilverStripe\View\ViewableData;
-use stdClass;
 
 /**
  * This formfield represents many-many joins using a tree selector shown in a dropdown styled element
@@ -119,48 +120,72 @@ class TreeMultiselectField extends TreeDropdownField
      */
     public function getItems()
     {
-        $items = new ArrayList();
+        $value = $this->Value();
 
-        // If the value has been set, use that
+        // If unchanged, load from record
-        if ($this->value != 'unchanged') {
+        if ($value === 'unchanged') {
-            $sourceObject = $this->getSourceObject();
+            // Verify a form exists
-            if (is_array($sourceObject)) {
+            $form = $this->getForm();
-                $values = is_array($this->value) ? $this->value : preg_split('/ *, */', trim($this->value));
+            if (!$form) {
-
+                return ArrayList::create();
-                foreach ($values as $value) {
-                    $item = new stdClass;
-                    $item->ID = $value;
-                    $item->Title = $sourceObject[$value];
-                    $items->push($item);
-                }
-                return $items;
             }
 
-            // Otherwise, look data up from the linked relation
+            // Verify this form has an attached record with the necessary relation
-            if (is_string($this->value)) {
+            $fieldName = $this->getName();
-                $ids = explode(',', $this->value);
+            $record = $form->getRecord();
-                foreach ($ids as $id) {
+            if ($record instanceof DataObject && $record->hasMethod($fieldName)) {
-                    if (!is_numeric($id)) {
-                        continue;
-                    }
-                    $item = DataObject::get_by_id($sourceObject, $id);
-                    if ($item) {
-                        $items->push($item);
-                    }
-                }
-                return $items;
-            }
-        }
-
-        if ($this->form) {
-            $fieldName = $this->name;
-            $record = $this->form->getRecord();
-            if (is_object($record) && $record->hasMethod($fieldName)) {
                 return $record->$fieldName();
             }
+
+            // No relation on parent record found
+            return ArrayList::create();
         }
 
-        return $items;
+        // Value is a list
+        if ($value instanceof SS_List) {
+            return $value;
+        }
+
+        // Parse ids from value string / array
+        $ids = [];
+        if (is_string($value)) {
+            $ids = preg_split("#\s*,\s*#", trim($value));
+        } elseif (is_array($value)) {
+            $ids = array_values($value);
+        }
+
+        // No value
+        if (empty($ids)) {
+            return ArrayList::create();
+        }
+
+        // Query source records by value field
+        return DataObject::get($this->getSourceObject())
+            ->filter($this->getKeyField(), $ids);
+    }
+
+    public function setValue($value, $source = null)
+    {
+        // If loading from a dataobject, get items by relation
+        if ($source instanceof DataObject) {
+            $name = $this->getName();
+            if ($source->hasMethod($name)) {
+                $value = $source->$name();
+            }
+        }
+
+        // Handle legacy value; form-submitted `unchanged` implies empty set.
+        // See TreeDropdownField.js
+        if ($value === 'unchanged') {
+            $value = [];
+        }
+
+        return parent::setValue($value);
+    }
+
+    public function dataValue()
+    {
+        return $this->getItems()->column($this->getKeyField());
     }
 
     /**
@@ -221,10 +246,10 @@ class TreeMultiselectField extends TreeDropdownField
      */
     public function saveInto(DataObjectInterface $record)
     {
-        $items = [];
+        $fieldName = $this->getName();
-        $fieldName = $this->name;
-        $saveDest = $record->$fieldName();
 
+        /** @var Relation $saveDest */
+        $saveDest = $record->$fieldName();
         if (!$saveDest) {
             $recordClass = get_class($record);
             user_error(
@@ -234,24 +259,17 @@ class TreeMultiselectField extends TreeDropdownField
             );
         }
 
-        // Detect whether this field has actually been updated
+        $itemIDs = $this->getItems()->column('ID');
-        if ($this->value !== 'unchanged') {
-            if (is_array($this->value)) {
-                $items = $this->value;
-            } elseif ($this->value) {
-                $items = preg_split("/ *, */", trim($this->value));
-            }
-        }
 
-        // Allows you to modify the items on your object before save
+        // Allows you to modify the itemIDs on your object before save
         $funcName = "onChange$fieldName";
         if ($record->hasMethod($funcName)) {
-            $result = $record->$funcName($items);
+            $result = $record->$funcName($itemIDs);
             if (!$result) {
                 return;
             }
         }
-        $saveDest->setByIDList($items);
+        $saveDest->setByIDList($itemIDs);
     }
 
     /**

tests/php/Forms/TreeMultiselectFieldTest.php

@@ -301,4 +301,44 @@ class TreeMultiselectFieldTest extends SapphireTest
         $this->assertContains($field->ID(), $html);
         $this->assertContains($this->fieldValue, $html);
     }
+
+    public function testGetItems()
+    {
+        // Default items scaffolded from 'unchanged' value (empty)
+        $field = $this->field;
+        $this->assertListEquals(
+            [],
+            $field->getItems()
+        );
+
+        $expectedItem = array_map(
+            function ($folder) {
+                return [
+                    'Filename' => $folder->Filename,
+                ];
+            },
+            $this->loadFolders()
+        );
+
+        // Set list of items by array of ids
+        $field->setValue($this->folderIds);
+        $this->assertListEquals(
+            $expectedItem,
+            $field->getItems()
+        );
+
+        // Set list of items by comma separated ids
+        $field->setValue($this->fieldValue);
+        $this->assertListEquals(
+            $expectedItem,
+            $field->getItems()
+        );
+
+        // Handle legacy empty value (form submits 'unchanged')
+        $field->setValue('unchanged');
+        $this->assertListEquals(
+            [],
+            $field->getItems()
+        );
+    }
 }