FIX: Append any fields that don’t match name in insertBefore/insertAfter

Previous behaviour was to return false, which has been described as
a confusing bug on #1397
where the issue was identified.

src/Forms/CompositeField.php

@@ -347,9 +347,9 @@ public function unshift(FormField $field)
      * @param FormField $field
      * @return false|FormField
      */
-    public function insertBefore($insertBefore, $field)
+    public function insertBefore($insertBefore, $field, $appendIfMissing = true)
     {
-        return $this->children->insertBefore($insertBefore, $field);
+        return $this->children->insertBefore($insertBefore, $field, $appendIfMissing);
     }
 
     /**
@@ -358,9 +358,9 @@ public function insertBefore($insertBefore, $field)
      * @param FormField $field
      * @return false|FormField
      */
-    public function insertAfter($insertAfter, $field)
+    public function insertAfter($insertAfter, $field, $appendIfMissing = true)
     {
-        return $this->children->insertAfter($insertAfter, $field);
+        return $this->children->insertAfter($insertAfter, $field, $appendIfMissing);
     }
 
     /**

src/Forms/FieldList.php

@@ -563,12 +563,14 @@ public function dataFieldByName($name)
 
     /**
      * Inserts a field before a particular field in a FieldList.
+     * Will traverse CompositeFields depth-first to find the maching $name, and insert before the first match
      *
      * @param string $name Name of the field to insert before
      * @param FormField $item The form field to insert
-     * @return FormField|false
+     * @param bool $appendIfMissing Append to the end of the list if $name isn't found
+     * @return FormField|false Field if it was successfully inserted, false if not inserted
      */
-    public function insertBefore($name, $item)
+    public function insertBefore($name, $item, $appendIfMissing = true)
     {
         // Backwards compatibility for order of arguments
         if ($name instanceof FormField) {
@@ -584,25 +586,33 @@ public function insertBefore($name, $item)
                 array_splice($this->items, $i, 0, array($item));
                 return $item;
             } elseif ($child instanceof CompositeField) {
-                $ret = $child->insertBefore($name, $item);
+                $ret = $child->insertBefore($name, $item, false);
                 if ($ret) {
                     return $ret;
                 }
             }
             $i++;
         }
 
+        // $name not found, append if needed
+        if ($appendIfMissing) {
+            $this->push($item);
+            return $item;
+        }
+
         return false;
     }
 
     /**
      * Inserts a field after a particular field in a FieldList.
+     * Will traverse CompositeFields depth-first to find the maching $name, and insert after the first match
      *
      * @param string $name Name of the field to insert after
      * @param FormField $item The form field to insert
-     * @return FormField|false
+     * @param bool $appendIfMissing Append to the end of the list if $name isn't found
+     * @return FormField|false Field if it was successfully inserted, false if not inserted
      */
-    public function insertAfter($name, $item)
+    public function insertAfter($name, $item, $appendIfMissing = true)
     {
         // Backwards compatibility for order of arguments
         if ($name instanceof FormField) {
@@ -618,14 +628,20 @@ public function insertAfter($name, $item)
                 array_splice($this->items, $i+1, 0, array($item));
                 return $item;
             } elseif ($child instanceof CompositeField) {
-                $ret = $child->insertAfter($name, $item);
+                $ret = $child->insertAfter($name, $item, false);
                 if ($ret) {
                     return $ret;
                 }
             }
             $i++;
         }
 
+        // $name not found, append if needed
+        if ($appendIfMissing) {
+            $this->push($item);
+            return $item;
+        }
+
         return false;
     }
 

src/Forms/TabSet.php

@@ -221,12 +221,12 @@ public function unshift(FormField $field)
      * @param FormField $field The form field to insert
      * @return FormField|null
      */
-    public function insertBefore($insertBefore, $field)
+    public function insertBefore($insertBefore, $field, $appendIfMissing = true)
     {
         if ($field instanceof Tab || $field instanceof TabSet) {
             $field->setTabSet($this);
         }
-        return parent::insertBefore($insertBefore, $field);
+        return parent::insertBefore($insertBefore, $field, $appendIfMissing);
     }
 
     /**
@@ -236,12 +236,12 @@ public function insertBefore($insertBefore, $field)
      * @param FormField $field The form field to insert
      * @return FormField|null
      */
-    public function insertAfter($insertAfter, $field)
+    public function insertAfter($insertAfter, $field, $appendIfMissing = true)
     {
         if ($field instanceof Tab || $field instanceof TabSet) {
             $field->setTabSet($this);
         }
-        return parent::insertAfter($insertAfter, $field);
+        return parent::insertAfter($insertAfter, $field, $appendIfMissing);
     }
 
     /**

tests/php/Forms/FieldListTest.php

@@ -615,6 +615,12 @@ public function testInsertBeforeFieldToSet()
 
         /* The position of the Surname field is at number 4 */
         $this->assertEquals('Surname', $fields[3]->getName());
+
+        /* Test that inserting before a field that doesn't exist simply appends
+         * Confirm that a composite field doesn't break this */
+        $fields->push(new CompositeField([ new TextField('Nested1'), new TextField('Nested2')]));
+        $this->assertTrue((bool)$fields->insertBefore('DoesNotExist', new TextField('MyName')));
+        $this->assertEquals('MyName', $fields->Last()->Name);
     }
 
     public function testInsertBeforeMultipleFields()
@@ -688,6 +694,12 @@ public function testInsertAfterFieldToSet()
 
         /* The position of the Surname field is at number 5 */
         $this->assertEquals('Surname', $fields[4]->getName());
+
+        /* Test that inserting before a field that doesn't exist simply appends
+         * Confirm that a composite field doesn't break this */
+        $fields->push(new CompositeField([ new TextField('Nested1'), new TextField('Nested2')]));
+        $this->assertTrue((bool)$fields->insertAfter('DoesNotExist', new TextField('MyName')));
+        $this->assertEquals('MyName', $fields->Last()->Name);
     }
 
     public function testrootFieldList()