From 71dad5f68518b9052b657c8dc70d4581fb771e98 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Mon, 1 Oct 2018 18:20:04 +1300 Subject: [PATCH] =?UTF-8?q?FIX:=20Append=20any=20fields=20that=20don?= =?UTF-8?q?=E2=80=99t=20match=20name=20in=20insertBefore/insertAfter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous behaviour was to return false, which has been described as a confusing bug on https://github.com/silverstripe/silverstripe-framework/issues/1397 where the issue was identified. --- src/Forms/CompositeField.php | 8 ++++---- src/Forms/FieldList.php | 28 ++++++++++++++++++++++------ src/Forms/TabSet.php | 8 ++++---- tests/php/Forms/FieldListTest.php | 12 ++++++++++++ 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/src/Forms/CompositeField.php b/src/Forms/CompositeField.php index 4860a7e3a..f4c336eef 100644 --- a/src/Forms/CompositeField.php +++ b/src/Forms/CompositeField.php @@ -347,9 +347,9 @@ class CompositeField extends FormField * @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 @@ class CompositeField extends FormField * @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); } /** diff --git a/src/Forms/FieldList.php b/src/Forms/FieldList.php index 99b634637..6c262db40 100644 --- a/src/Forms/FieldList.php +++ b/src/Forms/FieldList.php @@ -563,12 +563,14 @@ class FieldList extends ArrayList /** * 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,7 +586,7 @@ class FieldList extends ArrayList 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; } @@ -592,17 +594,25 @@ class FieldList extends ArrayList $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,7 +628,7 @@ class FieldList extends ArrayList 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; } @@ -626,6 +636,12 @@ class FieldList extends ArrayList $i++; } + // $name not found, append if needed + if ($appendIfMissing) { + $this->push($item); + return $item; + } + return false; } diff --git a/src/Forms/TabSet.php b/src/Forms/TabSet.php index 8ce06fc0b..da18e9d48 100644 --- a/src/Forms/TabSet.php +++ b/src/Forms/TabSet.php @@ -221,12 +221,12 @@ class TabSet extends CompositeField * @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 @@ class TabSet extends CompositeField * @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); } /** diff --git a/tests/php/Forms/FieldListTest.php b/tests/php/Forms/FieldListTest.php index 9d097a577..a11264972 100644 --- a/tests/php/Forms/FieldListTest.php +++ b/tests/php/Forms/FieldListTest.php @@ -615,6 +615,12 @@ class FieldListTest extends SapphireTest /* 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 @@ class FieldListTest extends SapphireTest /* 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()