From 7b847f8d7e03bbbbf982b165097223d8e7065de6 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Thu, 16 May 2024 18:16:08 +1200 Subject: [PATCH] API Strongly type Fieldlist --- src/Forms/CompositeField.php | 4 +- src/Forms/FieldList.php | 193 ++++++++++++------------------ src/Forms/FormField.php | 6 +- src/Forms/Tab.php | 3 + tests/php/Forms/FieldListTest.php | 36 ++++++ tests/php/Forms/FormFieldTest.php | 7 ++ 6 files changed, 130 insertions(+), 119 deletions(-) diff --git a/src/Forms/CompositeField.php b/src/Forms/CompositeField.php index 15c164c07..15d9abb7a 100644 --- a/src/Forms/CompositeField.php +++ b/src/Forms/CompositeField.php @@ -340,7 +340,7 @@ class CompositeField extends FormField * @param string $insertBefore * @param FormField $field * @param bool $appendIfMissing - * @return false|FormField + * @return FormField|null */ public function insertBefore($insertBefore, $field, $appendIfMissing = true) { @@ -352,7 +352,7 @@ class CompositeField extends FormField * @param string $insertAfter * @param FormField $field * @param bool $appendIfMissing - * @return false|FormField + * @return FormField|null */ public function insertAfter($insertAfter, $field, $appendIfMissing = true) { diff --git a/src/Forms/FieldList.php b/src/Forms/FieldList.php index 7e8a80cd1..210aa98ee 100644 --- a/src/Forms/FieldList.php +++ b/src/Forms/FieldList.php @@ -2,6 +2,10 @@ namespace SilverStripe\Forms; +use InvalidArgumentException; +use RuntimeException; +use SilverStripe\Forms\Tab; +use SilverStripe\Forms\TabSet; use SilverStripe\ORM\ArrayList; /** @@ -16,24 +20,25 @@ class FieldList extends ArrayList * Cached flat representation of all fields in this set, * including fields nested in {@link CompositeFields}. * - * @var FormField[] + * @var array */ - protected $sequentialSet; + protected array $sequentialSet = []; /** - * @var FormField[] + * @var array */ - protected $sequentialSaveableSet; + protected array $sequentialSaveableSet = []; /** * If this fieldlist is owned by a parent field (e.g. CompositeField) * this is the parent field. - * - * @var CompositeField */ - protected $containerField; + protected ?CompositeField $containerField = null; - public function __construct($items = []) + /** + * @param array|FormField $items Fields Can be given as an array or as a list of arguments + */ + public function __construct(array|FormField $items = []) { if (!is_array($items) || func_num_args() > 1) { $items = func_get_args(); @@ -42,13 +47,14 @@ class FieldList extends ArrayList parent::__construct($items); foreach ($items as $item) { - if ($item instanceof FormField) { - $item->setContainerFieldList($this); + if (!is_a($item, FormField::class)) { + throw new InvalidArgumentException('Item is not a FormField, is of class ' . get_class($item)); } + $item->setContainerFieldList($this); } } - public function __clone() + public function __clone(): void { // Clone all fields in this list foreach ($this->items as $key => $field) { @@ -58,10 +64,8 @@ class FieldList extends ArrayList /** * Iterate over each field in the current list recursively - * - * @param callable $callback */ - public function recursiveWalk(callable $callback) + public function recursiveWalk(callable $callback): void { $stack = $this->toArray(); while (!empty($stack)) { @@ -75,10 +79,8 @@ class FieldList extends ArrayList /** * Return a flattened list of all fields - * - * @return static */ - public function flattenFields() + public function flattenFields(): static { $fields = []; $this->recursiveWalk(function (FormField $field) use (&$fields) { @@ -91,9 +93,9 @@ class FieldList extends ArrayList * Return a sequential set of all fields that have data. This excludes wrapper composite fields * as well as heading / help text fields. * - * @return FormField[] + * @return array */ - public function dataFields() + public function dataFields(): array { if (empty($this->sequentialSet)) { $fields = []; @@ -113,9 +115,9 @@ class FieldList extends ArrayList } /** - * @return FormField[] + * @return array */ - public function saveableFields() + public function saveableFields(): array { if (empty($this->sequentialSaveableSet)) { $fields = []; @@ -136,21 +138,16 @@ class FieldList extends ArrayList /** * Return array of all field names - * - * @return array */ - public function dataFieldNames() + public function dataFieldNames(): array { return array_keys($this->dataFields() ?? []); } /** * Trigger an error for duplicate field names - * - * @param FormField $field - * @param $functionName */ - protected function fieldNameError(FormField $field, $functionName) + protected function fieldNameError(FormField $field, string $functionName): void { if ($field->getForm()) { $errorSuffix = sprintf( @@ -170,10 +167,10 @@ class FieldList extends ArrayList )); } - protected function flushFieldsCache() + protected function flushFieldsCache(): void { - $this->sequentialSet = null; - $this->sequentialSaveableSet = null; + $this->sequentialSet = []; + $this->sequentialSaveableSet = []; } /** @@ -183,11 +180,9 @@ class FieldList extends ArrayList * @param string $tabName The name of the tab or tabset. Subtabs can be referred to as TabSet.Tab * or TabSet.Tab.Subtab. This function will create any missing tabs. * @param FormField $field The {@link FormField} object to add to the end of that tab. - * @param string $insertBefore The name of the field to insert before. Optional. - * - * @return $this + * @param string|null $insertBefore The name of the field to insert before. */ - public function addFieldToTab($tabName, $field, $insertBefore = null) + public function addFieldToTab(string $tabName, FormField $field, ?string $insertBefore = null): static { // This is a cache that must be flushed $this->flushFieldsCache(); @@ -211,12 +206,10 @@ class FieldList extends ArrayList * * @param string $tabName The name of the tab or tabset. Subtabs can be referred to as TabSet.Tab * or TabSet.Tab.Subtab. This function will create any missing tabs. - * @param array $fields An array of {@link FormField} objects. - * @param string $insertBefore Name of field to insert before - * - * @return $this + * @param array $fields An array of {@link FormField} objects. + * @param string|null $insertBefore Name of field to insert before */ - public function addFieldsToTab($tabName, $fields, $insertBefore = null) + public function addFieldsToTab(string $tabName, array $fields, ?string $insertBefore = null): static { $this->flushFieldsCache(); @@ -240,14 +233,9 @@ class FieldList extends ArrayList } /** - * Remove the given field from the given tab in the field. - * - * @param string $tabName The name of the tab - * @param string $fieldName The name of the field - * - * @return $this + * Remove the given field from the given tab in the fieldlist. */ - public function removeFieldFromTab($tabName, $fieldName) + public function removeFieldFromTab(string $tabName, string $fieldName): static { $this->flushFieldsCache(); @@ -264,11 +252,9 @@ class FieldList extends ArrayList * Removes a number of fields from a Tab/TabSet within this FieldList. * * @param string $tabName The name of the Tab or TabSet field - * @param array $fields A list of fields, e.g. array('Name', 'Email') - * - * @return $this + * @param array $fields A list of fields, e.g. array('Name', 'Email') */ - public function removeFieldsFromTab($tabName, $fields) + public function removeFieldsFromTab(string $tabName, array $fields): static { $this->flushFieldsCache(); @@ -291,10 +277,8 @@ class FieldList extends ArrayList * @param boolean $dataFieldOnly If this is true, then a field will only * be removed if it's a data field. Dataless fields, such as tabs, will * be left as-is. - * - * @return $this */ - public function removeByName($fieldName, $dataFieldOnly = false) + public function removeByName(string|array $fieldName, bool $dataFieldOnly = false): static { if (!$fieldName) { user_error('FieldList::removeByName() was called with a blank field name.', E_USER_WARNING); @@ -336,7 +320,7 @@ class FieldList extends ArrayList * @return bool TRUE field was successfully replaced * FALSE field wasn't found, nothing changed */ - public function replaceField($fieldName, $newField, $dataFieldOnly = true) + public function replaceField(string $fieldName, FormField $newField, bool $dataFieldOnly = true): bool { $this->flushFieldsCache(); foreach ($this as $i => $field) { @@ -357,9 +341,8 @@ class FieldList extends ArrayList * * @param string $fieldName Name of field to rename title of * @param string $newFieldTitle New title of field - * @return bool */ - public function renameField($fieldName, $newFieldTitle) + public function renameField(string $fieldName, string $newFieldTitle): bool { $field = $this->dataFieldByName($fieldName); if (!$field) { @@ -371,10 +354,7 @@ class FieldList extends ArrayList return $field->Title() == $newFieldTitle; } - /** - * @return bool - */ - public function hasTabSet() + public function hasTabSet(): bool { foreach ($this->items as $i => $field) { if (is_object($field) && $field instanceof TabSet) { @@ -389,9 +369,9 @@ class FieldList extends ArrayList * Returns the specified tab object, if it exists * * @param string $tabName The tab to return, in the form "Tab.Subtab.Subsubtab". - * @return Tab|null The found or null + * @return Tab|TabSet|null The found Tab or TabSet, or null if nothing was found. */ - public function findTab($tabName) + public function findTab(string $tabName): Tab|TabSet|null { $parts = explode('.', $tabName ?? ''); @@ -400,6 +380,7 @@ class FieldList extends ArrayList foreach ($parts as $k => $part) { $currentPointer = $currentPointer->fieldByName($part); } + $this->checkIsTabOrTabSetOrNull($tabName, $currentPointer); return $currentPointer; } @@ -410,12 +391,12 @@ class FieldList extends ArrayList * @param string $tabName The tab to return, in the form "Tab.Subtab.Subsubtab". * Caution: Does not recursively create TabSet instances, you need to make sure everything * up until the last tab in the chain exists. - * @param string $title Natural language title of the tab. If {@link $tabName} is passed in dot notation, + * @param string|null $title Natural language title of the tab. If {@link $tabName} is passed in dot notation, * the title parameter will only apply to the innermost referenced tab. * The title is only changed if the tab doesn't exist already. - * @return Tab The found or newly created Tab instance + * @return Tab|TabSet The found or created Tab, or TabSet if "Root" was passed for $tabName */ - public function findOrMakeTab($tabName, $title = null) + public function findOrMakeTab(string $tabName, ?string $title = null): Tab|TabSet { $parts = explode('.', $tabName ?? ''); $last_idx = count($parts ?? []) - 1; @@ -447,6 +428,7 @@ class FieldList extends ArrayList } } } + $this->checkIsTabOrTabSetOrNull($tabName, $currentPointer); return $currentPointer; } @@ -454,11 +436,8 @@ class FieldList extends ArrayList /** * Returns a named field. * You can use dot syntax to get fields from child composite fields - * - * @param string $name - * @return FormField|null */ - public function fieldByName($name) + public function fieldByName(string $name): ?FormField { $fullName = $name; if (strpos($name ?? '', '.') !== false) { @@ -493,11 +472,8 @@ class FieldList extends ArrayList /** * Returns a named field in a sequential set. * Use this if you're using nested FormFields. - * - * @param string $name The name of the field to return - * @return FormField|null */ - public function dataFieldByName($name) + public function dataFieldByName(string $name): ?FormField { if ($dataFields = $this->dataFields()) { foreach ($dataFields as $child) { @@ -513,7 +489,7 @@ class FieldList extends ArrayList * Inserts a field before a particular field in a FieldList. * Will traverse CompositeFields depth-first to find the matching $name, and insert before the first match */ - public function insertBefore(string $name, FormField $item, bool $appendIfMissing = true): FormField|bool + public function insertBefore(string $name, FormField $item, bool $appendIfMissing = true): ?FormField { $this->onBeforeInsert($item); $item->setContainerFieldList($this); @@ -538,14 +514,14 @@ class FieldList extends ArrayList return $item; } - return false; + return null; } /** * Inserts a field after a particular field in a FieldList. * Will traverse CompositeFields depth-first to find the matching $name, and insert after the first match */ - public function insertAfter(string $name, FormField $item, bool $appendIfMissing = true): FormField|bool + public function insertAfter(string $name, FormField $item, bool $appendIfMissing = true): ?FormField { $this->onBeforeInsert($item); $item->setContainerFieldList($this); @@ -570,7 +546,7 @@ class FieldList extends ArrayList return $item; } - return false; + return null; } /** @@ -616,11 +592,8 @@ class FieldList extends ArrayList /** * Set the Form instance for this FieldList. - * - * @param Form $form The form to set this FieldList to - * @return $this */ - public function setForm($form) + public function setForm(Form $form): static { foreach ($this as $field) { $field->setForm($form); @@ -633,9 +606,8 @@ class FieldList extends ArrayList * Load the given data into this form. * * @param array $data An map of data to load into the FieldList - * @return $this */ - public function setValues($data) + public function setValues(array $data): static { foreach ($this->dataFields() as $field) { $fieldName = $field->getName(); @@ -650,10 +622,8 @@ class FieldList extends ArrayList * Return all fields * in a form - including fields nested in {@link CompositeFields}. * Useful when doing custom field layouts. - * - * @return FieldList */ - public function HiddenFields() + public function HiddenFields(): FieldList { $hiddenFields = new FieldList(); $dataFields = $this->dataFields(); @@ -673,7 +643,7 @@ class FieldList extends ArrayList * Return all fields except for the hidden fields. * Useful when making your own simplified form layouts. */ - public function VisibleFields() + public function VisibleFields(): FieldList { $visibleFields = new FieldList(); @@ -689,11 +659,8 @@ class FieldList extends ArrayList /** * Transform this FieldList with a given transform method, * e.g. $this->transform(new ReadonlyTransformation()) - * - * @param FormTransformation $trans - * @return FieldList */ - public function transform($trans) + public function transform(FormTransformation $trans): FieldList { $this->flushFieldsCache(); $newFields = new FieldList(); @@ -705,10 +672,8 @@ class FieldList extends ArrayList /** * Returns the root field set that this belongs to - * - * @return FieldList|FormField */ - public function rootFieldList() + public function rootFieldList(): FieldList { if ($this->containerField) { return $this->containerField->rootFieldList(); @@ -717,19 +682,12 @@ class FieldList extends ArrayList return $this; } - /** - * @return CompositeField|null - */ - public function getContainerField() + public function getContainerField(): ?CompositeField { return $this->containerField; } - /** - * @param CompositeField|null $field - * @return $this - */ - public function setContainerField($field) + public function setContainerField(?CompositeField $field): static { $this->containerField = $field; return $this; @@ -737,20 +695,16 @@ class FieldList extends ArrayList /** * Transforms this FieldList instance to readonly. - * - * @return FieldList */ - public function makeReadonly() + public function makeReadonly(): FieldList { return $this->transform(new ReadonlyTransformation()); } /** * Transform the named field into a readonly field. - * - * @param string|array|FormField $field */ - public function makeFieldReadonly($field) + public function makeFieldReadonly(string|array|FormField $field): void { if (!is_array($field)) { $field = [$field]; @@ -774,9 +728,9 @@ class FieldList extends ArrayList * * Please note that any tabs or other dataless fields will be clobbered by this operation. * - * @param array $fieldNames Field names can be given as an array, or just as a list of arguments. + * @param array|string $fieldNames Field names can be given as an array, or just as a list of arguments. */ - public function changeFieldOrder($fieldNames) + public function changeFieldOrder(array|string $fieldNames): void { // Field names can be given as an array, or just as a list of arguments. if (!is_array($fieldNames)) { @@ -813,11 +767,10 @@ class FieldList extends ArrayList * Find the numerical position of a field within * the children collection. Doesn't work recursively. * - * @param string|FormField $field - * @return int Position in children collection (first position starts with 0). + * @return int|false Position in children collection (first position starts with 0). * Returns FALSE if the field can't be found. */ - public function fieldPosition($field) + public function fieldPosition(string|FormField $field): int|false { if ($field instanceof FormField) { $field = $field->getName(); @@ -837,7 +790,7 @@ class FieldList extends ArrayList /** * Default template rendering of a FieldList will concatenate all FieldHolder values. */ - public function forTemplate() + public function forTemplate(): string { $output = ""; foreach ($this as $field) { @@ -845,4 +798,12 @@ class FieldList extends ArrayList } return $output; } + + private function checkIsTabOrTabSetOrNull(string $tabName, mixed $currentPointer): void + { + if ($currentPointer && (!is_a($currentPointer, Tab::class) && !is_a($currentPointer, TabSet::class))) { + $className = get_class($currentPointer); + throw new RuntimeException("$tabName is an instance of '$className', not Tab or TabSet"); + } + } } diff --git a/src/Forms/FormField.php b/src/Forms/FormField.php index c6a56ece7..c9cc22eb6 100644 --- a/src/Forms/FormField.php +++ b/src/Forms/FormField.php @@ -1362,7 +1362,11 @@ class FormField extends RequestHandler $field = $classOrCopy; if (!is_object($field)) { - $field = $classOrCopy::create($this->name); + if (is_a($classOrCopy, CompositeField::class, true)) { + $field = $classOrCopy::create([]); + } else { + $field = $classOrCopy::create($this->name); + } } $extraClasses = $this->extraClasses ? array_values($this->extraClasses) : []; diff --git a/src/Forms/Tab.php b/src/Forms/Tab.php index bc267cbb5..3f3fe538d 100644 --- a/src/Forms/Tab.php +++ b/src/Forms/Tab.php @@ -66,6 +66,9 @@ class Tab extends CompositeField $title = static::name_to_label($name); } + // Remove any blank strings from fields + $fields = array_filter($fields); + // Remaining arguments are child fields parent::__construct($fields); diff --git a/tests/php/Forms/FieldListTest.php b/tests/php/Forms/FieldListTest.php index ec235dfee..6394da73a 100644 --- a/tests/php/Forms/FieldListTest.php +++ b/tests/php/Forms/FieldListTest.php @@ -2,6 +2,9 @@ namespace SilverStripe\Forms\Tests; +use stdClass; +use InvalidArgumentException; +use RuntimeException; use SilverStripe\Dev\SapphireTest; use SilverStripe\Forms\ConfirmedPasswordField; use SilverStripe\Forms\FieldList; @@ -23,6 +26,13 @@ use SilverStripe\Forms\HiddenField; */ class FieldListTest extends SapphireTest { + public function testInvalidArrayConstructorArg() + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Item is not a FormField, is of class stdClass'); + new FieldList([new stdClass()]); + } + public function testRecursiveWalk() { $fields = [ @@ -259,6 +269,32 @@ class FieldListTest extends SapphireTest $this->assertNull($fields->findTab('More')); $this->assertEquals($fields->findTab('Root.More'), $more); $this->assertEquals($fields->findTab('Root.More.Tab4'), $tab4); + + $fields->addFieldToTab('Root.Tab1', new TextField('Field1')); + $this->expectException(RuntimeException::class); + $message = "Root.Tab1.Field1 is an instance of 'SilverStripe\Forms\TextField', not Tab or TabSet"; + $this->expectExceptionMessage($message); + $fields->findTab('Root.Tab1.Field1'); + } + + public function testFindOrMakeTab() + { + $fields = new FieldList( + $root = new TabSet( + 'Root', + $tab1 = new Tab('Tab1'), + ) + ); + $this->assertEquals($fields->findTab('Root'), $root); + $this->assertEquals($fields->findOrMakeTab('Root.Tab1'), $tab1); + $tab2 = $fields->findOrMakeTab('Root.Tab2'); + $this->assertEquals(Tab::class, get_class($tab2)); + + $fields->addFieldToTab('Root.Tab1', new TextField('Field1')); + $this->expectException(RuntimeException::class); + $message = "Root.Tab1.Field1 is an instance of 'SilverStripe\Forms\TextField', not Tab or TabSet"; + $this->expectExceptionMessage($message); + $fields->findOrMakeTab('Root.Tab1.Field1'); } /** diff --git a/tests/php/Forms/FormFieldTest.php b/tests/php/Forms/FormFieldTest.php index 07d6c1268..21b4a0128 100644 --- a/tests/php/Forms/FormFieldTest.php +++ b/tests/php/Forms/FormFieldTest.php @@ -683,4 +683,11 @@ class FormFieldTest extends SapphireTest $this->assertInstanceOf(Tip::class, $field->getTitleTip()); $this->assertSame('Test tip', $field->getTitleTip()->getMessage()); } + + public function testCastedCopy() + { + $field = new FormField('MyField'); + $this->assertTrue(is_a($field->castedCopy(TextField::class), TextField::class)); + $this->assertTrue(is_a($field->castedCopy(CompositeField::class), CompositeField::class)); + } }