Merge pull request #10569 from kinglozzer/formfield-validation-extensions

NEW: Add extension hook for field-specific validation

src/Forms/CompositeField.php

@@ -545,6 +545,6 @@ class CompositeField extends FormField
             /** @var FormField $child */
             $valid = ($child && $child->validate($validator) && $valid);
         }
-        return $valid;
+        return $this->extendValidationResult($valid, $validator);
     }
 }

src/Forms/ConfirmedPasswordField.php

@@ -416,7 +416,7 @@ class ConfirmedPasswordField extends FormField
 
         // if field isn't visible, don't validate
         if (!$this->isSaveable()) {
-            return true;
+            return $this->extendValidationResult(true, $validator);
         }
 
         $this->getPasswordField()->setValue($this->value);
@@ -431,7 +431,7 @@ class ConfirmedPasswordField extends FormField
                 "validation"
             );
 
-            return false;
+            return $this->extendValidationResult(false, $validator);
         }
 
         if (!$this->canBeEmpty) {
@@ -443,7 +443,7 @@ class ConfirmedPasswordField extends FormField
                     "validation"
                 );
 
-                return false;
+                return $this->extendValidationResult(false, $validator);
             }
         }
 
@@ -483,7 +483,7 @@ class ConfirmedPasswordField extends FormField
                     "validation"
                 );
 
-                return false;
+                return $this->extendValidationResult(false, $validator);
             }
         }
 
@@ -498,7 +498,7 @@ class ConfirmedPasswordField extends FormField
                     "validation"
                 );
 
-                return false;
+                return $this->extendValidationResult(false, $validator);
             }
         }
 
@@ -513,7 +513,7 @@ class ConfirmedPasswordField extends FormField
                     ),
                     "validation"
                 );
-                return false;
+                return $this->extendValidationResult(false, $validator);
             }
 
             // Check this password is valid for the current user
@@ -527,7 +527,7 @@ class ConfirmedPasswordField extends FormField
                     ),
                     "validation"
                 );
-                return false;
+                return $this->extendValidationResult(false, $validator);
             }
 
             // With a valid user and password, check the password is correct
@@ -543,12 +543,12 @@ class ConfirmedPasswordField extends FormField
                         ),
                         "validation"
                     );
-                    return false;
+                    return $this->extendValidationResult(false, $validator);
                 }
             }
         }
 
-        return true;
+        return $this->extendValidationResult(true, $validator);
     }
 
     /**

src/Forms/CurrencyField.php

@@ -58,6 +58,7 @@ class CurrencyField extends TextField
 
     public function validate($validator)
     {
+        $result = true;
         $currencySymbol = preg_quote(DBCurrency::config()->uninherited('currency_symbol') ?? '');
         $regex = '/^\s*(\-?' . $currencySymbol . '?|' . $currencySymbol . '\-?)?(\d{1,3}(\,\d{3})*|(\d+))(\.\d{2})?\s*$/';
         if (!empty($this->value) && !preg_match($regex ?? '', $this->value ?? '')) {
@@ -66,9 +67,10 @@ class CurrencyField extends TextField
                 _t('SilverStripe\\Forms\\Form.VALIDCURRENCY', "Please enter a valid currency"),
                 "validation"
             );
-            return false;
+            $result = false;
         }
-        return true;
+
+        return $this->extendValidationResult($result, $validator);
     }
 
     public function getSchemaValidation()

src/Forms/DateField.php

@@ -369,7 +369,7 @@ class DateField extends TextField
     {
         // Don't validate empty fields
         if (empty($this->rawValue)) {
-            return true;
+            return $this->extendValidationResult(true, $validator);
         }
 
         // We submitted a value, but it couldn't be parsed
@@ -382,7 +382,7 @@ class DateField extends TextField
                     ['format' => $this->getDateFormat()]
                 )
             );
-            return false;
+            return $this->extendValidationResult(false, $validator);
         }
 
         // Check min date
@@ -406,7 +406,7 @@ class DateField extends TextField
                     ValidationResult::TYPE_ERROR,
                     ValidationResult::CAST_HTML
                 );
-                return false;
+                return $this->extendValidationResult(false, $validator);
             }
         }
 
@@ -431,11 +431,11 @@ class DateField extends TextField
                     ValidationResult::TYPE_ERROR,
                     ValidationResult::CAST_HTML
                 );
-                return false;
+                return $this->extendValidationResult(false, $validator);
             }
         }
 
-        return true;
+        return $this->extendValidationResult(true, $validator);
     }
 
     /**

src/Forms/DatetimeField.php

@@ -565,7 +565,7 @@ class DatetimeField extends TextField
     {
         // Don't validate empty fields
         if (empty($this->rawValue)) {
-            return true;
+            return $this->extendValidationResult(true, $validator);
         }
 
         // We submitted a value, but it couldn't be parsed
@@ -578,7 +578,7 @@ class DatetimeField extends TextField
                     ['format' => $this->getDatetimeFormat()]
                 )
             );
-            return false;
+            return $this->extendValidationResult(false, $validator);
         }
 
         // Check min date (in server timezone)
@@ -602,7 +602,7 @@ class DatetimeField extends TextField
                     ValidationResult::TYPE_ERROR,
                     ValidationResult::CAST_HTML
                 );
-                return false;
+                return $this->extendValidationResult(false, $validator);
             }
         }
 
@@ -627,11 +627,11 @@ class DatetimeField extends TextField
                     ValidationResult::TYPE_ERROR,
                     ValidationResult::CAST_HTML
                 );
-                return false;
+                return $this->extendValidationResult(false, $validator);
             }
         }
 
-        return true;
+        return $this->extendValidationResult(true, $validator);
     }
 
     public function performReadonlyTransformation()

src/Forms/EmailField.php

@@ -29,6 +29,7 @@ class EmailField extends TextField
      */
     public function validate($validator)
     {
+        $result = true;
         $this->value = trim($this->value ?? '');
 
         $pattern = '^[a-z0-9!#$%&\'*+/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&\'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$';
@@ -43,10 +44,10 @@ class EmailField extends TextField
                 'validation'
             );
 
-            return false;
+            $result = false;
         }
 
-        return true;
+        return $this->extendValidationResult($result, $validator);
     }
 
     public function getSchemaValidation()

src/Forms/FileField.php

@@ -187,7 +187,7 @@ class FileField extends FormField implements FileHandleField
         $fieldName = preg_replace('#\[(.*?)\]$#', '', $this->name ?? '');
 
         if (!isset($_FILES[$fieldName])) {
-            return true;
+            return $this->extendValidationResult(true, $validator);
         }
 
         if ($isMultiFileUpload) {
@@ -204,11 +204,12 @@ class FileField extends FormField implements FileHandleField
                     $isValid = false;
                 }
             }
-            return $isValid;
+            return $this->extendValidationResult($isValid, $validator);
         }
 
         // regular single-file upload
-        return $this->validateFileData($validator, $_FILES[$this->name]);
+        $result = $this->validateFileData($validator, $_FILES[$this->name]);
+        return $this->extendValidationResult($result, $validator);
     }
 
     /**

src/Forms/FormField.php

@@ -1219,18 +1219,29 @@ class FormField extends RequestHandler
         return strtolower(preg_replace('/Field$/', '', $type->getShortName() ?? '') ?? '');
     }
 
+    /**
+     * Utility method to call an extension hook which allows the result of validate() calls to be adjusted
+     *
+     * @param bool $result
+     * @param Validator $validator
+     * @return bool
+     */
+    protected function extendValidationResult(bool $result, Validator $validator): bool
+    {
+        $this->extend('updateValidationResult', $result, $validator);
+        return $result;
+    }
+
     /**
      * Abstract method each {@link FormField} subclass must implement, determines whether the field
      * is valid or not based on the value.
      *
-     * @todo Make this abstract.
-     *
      * @param Validator $validator
      * @return bool
      */
     public function validate($validator)
     {
-        return true;
+        return $this->extendValidationResult(true, $validator);
     }
 
     /**

src/Forms/LookupField.php

@@ -73,7 +73,7 @@ class LookupField extends MultiSelectField
      */
     public function validate($validator)
     {
-        return true;
+        return $this->extendValidationResult(true, $validator);
     }
 
     /**

src/Forms/MoneyField.php

@@ -334,11 +334,12 @@ class MoneyField extends FormField
                     ['currency' => $currency]
                 )
             );
-            return false;
+            return $this->extendValidationResult(false, $validator);
         }
 
         // Field-specific validation
-        return $this->fieldAmount->validate($validator) && $this->fieldCurrency->validate($validator);
+        $result = $this->fieldAmount->validate($validator) && $this->fieldCurrency->validate($validator);
+        return $this->extendValidationResult($result, $validator);
     }
 
     public function setForm($form)

src/Forms/MultiSelectField.php

@@ -247,21 +247,23 @@ abstract class MultiSelectField extends SelectField
                 return true;
             }
         );
-        if (empty($invalidValues)) {
+
-            return true;
+        $result = true;
+        if (!empty($invalidValues)) {
+            $result = false;
+            // List invalid items
+            $validator->validationError(
+                $this->getName(),
+                _t(
+                    'SilverStripe\\Forms\\MultiSelectField.SOURCE_VALIDATION',
+                    "Please select values within the list provided. Invalid option(s) {value} given",
+                    ['value' => implode(',', $invalidValues)]
+                ),
+                "validation"
+            );
         }
 
-        // List invalid items
+        return $this->extendValidationResult($result, $validator);
-        $validator->validationError(
-            $this->getName(),
-            _t(
-                'SilverStripe\\Forms\\MultiSelectField.SOURCE_VALIDATION',
-                "Please select values within the list provided. Invalid option(s) {value} given",
-                ['value' => implode(',', $invalidValues)]
-            ),
-            "validation"
-        );
-        return false;
     }
 
     /**

src/Forms/NumericField.php

@@ -191,20 +191,21 @@ class NumericField extends TextField
      */
     public function validate($validator)
     {
+        $result = true;
         // false signifies invalid value due to failed parse()
-        if ($this->value !== false) {
+        if ($this->value === false) {
-            return true;
+            $validator->validationError(
+                $this->name,
+                _t(
+                    'SilverStripe\\Forms\\NumericField.VALIDATION',
+                    "'{value}' is not a number, only numbers can be accepted for this field",
+                    ['value' => $this->originalValue]
+                )
+            );
+            $result = false;
         }
 
-        $validator->validationError(
+        return $this->extendValidationResult($result, $validator);
-            $this->name,
-            _t(
-                'SilverStripe\\Forms\\NumericField.VALIDATION',
-                "'{value}' is not a number, only numbers can be accepted for this field",
-                ['value' => $this->originalValue]
-            )
-        );
-        return false;
     }
 
     public function getSchemaValidation()

src/Forms/OptionsetField.php

@@ -140,7 +140,7 @@ class OptionsetField extends SingleSelectField
     public function validate($validator)
     {
         if (!$this->Value()) {
-            return true;
+            return $this->extendValidationResult(true, $validator);
         }
 
         return parent::validate($validator);

src/Forms/SingleLookupField.php

@@ -44,7 +44,7 @@ class SingleLookupField extends SingleSelectField
      */
     public function validate($validator)
     {
-        return true;
+        return $this->extendValidationResult(true, $validator);
     }
 
     /**

src/Forms/SingleSelectField.php

@@ -137,13 +137,13 @@ abstract class SingleSelectField extends SelectField
             // Use selection rules to check which are valid
             foreach ($validValues as $formValue) {
                 if ($this->isSelectedValue($formValue, $selected)) {
-                    return true;
+                    return $this->extendValidationResult(true, $validator);
                 }
             }
         } else {
             if ($this->getHasEmptyDefault() || !$validValues || in_array('', $validValues ?? [])) {
                 // Check empty value
-                return true;
+                return $this->extendValidationResult(true, $validator);
             }
             $selected = '(none)';
         }
@@ -158,7 +158,7 @@ abstract class SingleSelectField extends SelectField
             ),
             "validation"
         );
-        return false;
+        return $this->extendValidationResult(false, $validator);
     }
 
     public function castedCopy($classOrCopy)

src/Forms/TextField.php

@@ -125,6 +125,7 @@ class TextField extends FormField implements TippableFieldInterface
      */
     public function validate($validator)
     {
+        $result = true;
         if (!is_null($this->maxLength) && mb_strlen($this->value ?? '') > $this->maxLength) {
             $name = strip_tags($this->Title() ? $this->Title() : $this->getName());
             $validator->validationError(
@@ -136,9 +137,9 @@ class TextField extends FormField implements TippableFieldInterface
                 ),
                 "validation"
             );
-            return false;
+            $result = false;
         }
-        return true;
+        return $this->extendValidationResult($result, $validator);
     }
 
     public function getSchemaValidation()

src/Forms/TimeField.php

@@ -324,9 +324,10 @@ class TimeField extends TextField
     {
         // Don't validate empty fields
         if (empty($this->rawValue)) {
-            return true;
+            return $this->extendValidationResult(true, $validator);
         }
 
+        $result = true;
         // We submitted a value, but it couldn't be parsed
         if (empty($this->value)) {
             $validator->validationError(
@@ -337,9 +338,11 @@ class TimeField extends TextField
                     ['format' => $this->getTimeFormat()]
                 )
             );
-            return false;
+            $result = false;
         }
-        return true;
+
+        $this->extendValidationResult($result, $validator);
+        return $result;
     }
 
     /**

tests/php/Forms/FormFieldTest.php

@@ -2,21 +2,40 @@
 
 namespace SilverStripe\Forms\Tests;
 
+use Exception;
 use LogicException;
 use ReflectionClass;
 use SilverStripe\Core\ClassInfo;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Dev\TestOnly;
 use SilverStripe\Forms\CompositeField;
+use SilverStripe\Forms\FieldGroup;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\Form;
 use SilverStripe\Forms\FormField;
+use SilverStripe\Forms\GridField\GridField;
+use SilverStripe\Forms\GridField\GridField_FormAction;
+use SilverStripe\Forms\GridField\GridState;
 use SilverStripe\Forms\NullableField;
+use SilverStripe\Forms\PopoverField;
+use SilverStripe\Forms\PrintableTransformation_TabSet;
 use SilverStripe\Forms\RequiredFields;
+use SilverStripe\Forms\SelectionGroup;
+use SilverStripe\Forms\SelectionGroup_Item;
+use SilverStripe\Forms\Tab;
+use SilverStripe\Forms\Tests\FormFieldTest\FieldValidationExtension;
 use SilverStripe\Forms\Tests\FormFieldTest\TestExtension;
 use SilverStripe\Forms\TextField;
 use SilverStripe\Forms\Tip;
+use SilverStripe\Forms\ToggleCompositeField;
+use SilverStripe\Forms\TreeDropdownField;
+use SilverStripe\Forms\TreeDropdownField_Readonly;
 use SilverStripe\ORM\ValidationResult;
+use SilverStripe\Security\Group;
+use SilverStripe\Security\Permission;
+use SilverStripe\Security\PermissionCheckboxSetField;
+use SilverStripe\Security\PermissionCheckboxSetField_Readonly;
 
 class FormFieldTest extends SapphireTest
 {
@@ -24,6 +43,7 @@ class FormFieldTest extends SapphireTest
     protected static $required_extensions = [
         FormField::class => [
             TestExtension::class,
+            FieldValidationExtension::class,
         ],
     ];
 
@@ -455,6 +475,146 @@ class FormFieldTest extends SapphireTest
         );
     }
 
+    public function testValidationExtensionHooks()
+    {
+        /** @var TextField|FieldValidationExtension $field */
+        $field = new TextField('Test');
+        $field->setMaxLength(5);
+        $field->setValue('IAmLongerThan5Characters');
+        $result = $field->validate(new RequiredFields('Test'));
+        $this->assertFalse($result);
+
+        // Call extension method in FieldValidationExtension
+        $field->setExcludeFromValidation(true);
+        $result = $field->validate(new RequiredFields('Test'));
+        $this->assertTrue($result);
+
+        // Call extension methods in FieldValidationExtension
+        $field->setValue('1234');
+        $field->setExcludeFromValidation(false);
+        $field->setTriggerTestValidationError(true);
+
+        // Ensure messages set via updateValidationResult() propagate through to form fields after validation
+        $form = new Form(null, 'TestForm', new FieldList($field), new FieldList(), new RequiredFields());
+        $form->validationResult();
+        $schema = $field->getSchemaState();
+        $this->assertEquals(
+            'A test error message',
+            $schema['message']['value']
+        );
+    }
+
+    public function testValidationExtensionHooksAreCalledOnFormFieldSubclasses()
+    {
+        // Can't use a dataProvider for this as dataProviders are fetched very early by phpunit,
+        // and the ClassManifest isn't ready then
+        $formFieldClasses = ClassInfo::subclassesFor(FormField::class, false);
+        foreach ($formFieldClasses as $formFieldClass) {
+            $reflection = new ReflectionClass($formFieldClass);
+            // Skip abstract classes, like MultiSelectField, and fields that only exist for unit tests
+            if ($reflection->isAbstract() || is_a($formFieldClass, TestOnly::class, true)) {
+                continue;
+            }
+
+            // Create appropriate constructor arguments for the form field class. These don't have to be offer realistic
+            // data, they just need to ensure we can construct the field and call ->validate() on it
+            switch ($formFieldClass) {
+                //
+                // Fields in framework with specific argument requirements
+                //
+                case NullableField::class:
+                case CompositeField::class:
+                case FieldGroup::class:
+                case PopoverField::class:
+                    $args = [TextField::create('Test2')];
+                    break;
+                case SelectionGroup_Item::class:
+                    $args = ['Test', [TextField::create('Test2')]];
+                    break;
+                case ToggleCompositeField::class:
+                    $args = ['Test', 'Test', TextField::create('Test2')];
+                    break;
+                case PrintableTransformation_TabSet::class:
+                    $args = [Tab::create('TestTab', 'Testtab', TextField::create('Test2'))];
+                    break;
+                case TreeDropdownField::class:
+                case TreeDropdownField_Readonly::class:
+                    $args = ['Test', 'Test', Group::class];
+                    break;
+                case PermissionCheckboxSetField::class:
+                case PermissionCheckboxSetField_Readonly::class:
+                    $args = ['Test', 'Test', Permission::class, 'Test'];
+                    break;
+                case SelectionGroup::class:
+                    $args = ['Test', []];
+                    break;
+                case GridField_FormAction::class:
+                    $args = [GridField::create('GF'), 'Test', 'Test label', 'Test action name', []];
+                    break;
+                case GridState::class:
+                    $args = [GridField::create('GF')];
+                    break;
+                //
+                // Fields from other modules included in the kitchensink recipe
+                //
+                case \SilverStripe\Blog\Admin\GridFieldFormAction::class:
+                    $args = [GridField::create('GF'), 'Test', 'Test label', 'Test action name', []];
+                    break;
+                case \SilverStripe\Blog\Forms\BlogAdminSidebar::class:
+                    $args = [TextField::create('Test2')];
+                    break;
+                case \SilverStripe\CKANRegistry\Forms\PresentedOptionsField::class:
+                    $args = ['Test', \SilverStripe\CKANRegistry\Model\Resource::create()];
+                    break;
+                case \SilverStripe\DocumentConverter\SettingsField::class:
+                    $args = [];
+                    break;
+                case \DNADesign\Elemental\Forms\ElementalAreaField::class:
+                    $args = ['Test', \DNADesign\Elemental\Models\ElementalArea::create(), []];
+                    break;
+                case \SilverStripe\MFA\FormField\RegisteredMFAMethodListField::class:
+                    $args = ['Test', 'Test label', 1];
+                    break;
+                case \SilverStripe\Subsites\Forms\SubsitesTreeDropdownField::class:
+                    $args = ['Test', 'Test', Group::class];
+                    break;
+                case \SilverStripe\UserForms\FormField\UserFormsCompositeField::class:
+                case \SilverStripe\UserForms\FormField\UserFormsGroupField::class:
+                case \SilverStripe\UserForms\FormField\UserFormsStepField::class:
+                    $args = [TextField::create('Test2')];
+                    break;
+                case \Symbiote\AdvancedWorkflow\FormFields\WorkflowField::class:
+                    $args = ['Test', 'Test label', \Symbiote\AdvancedWorkflow\DataObjects\WorkflowDefinition::create()];
+                    break;
+                //
+                // Default arguments, this covers most simple form fields
+                //
+                default:
+                    $args = ['Test', 'Test label'];
+            }
+
+            // Assert that extendValidationResult is called once each time ->validate() is called
+            $mock = $this->getMockBuilder($formFieldClass)
+                ->setConstructorArgs($args)
+                ->onlyMethods(['extendValidationResult'])
+                ->getMock();
+            $mock->expects($invocationRule = $this->once())
+                ->method('extendValidationResult')
+                ->will($this->returnValue(true));
+
+            $isValid = $mock->validate(new RequiredFields());
+            $this->assertTrue($isValid, "$formFieldClass should be valid");
+
+            // This block is not essential and only exists to make test debugging easier - without this,
+            // the error message on failure is generic and doesn't include the class name that failed
+            try {
+                $invocationRule->verify();
+            } catch (Exception $e) {
+                $this->fail("Expectation failed for '$formFieldClass' class: {$e->getMessage()}");
+            }
+        }
+    }
+
     public function testHasClass()
     {
         $field = new FormField('Test');

tests/php/Forms/FormFieldTest/FieldValidationExtension.php

@@ -0,0 +1,38 @@
+<?php
+
+namespace SilverStripe\Forms\Tests\FormFieldTest;
+
+use SilverStripe\Core\Extension;
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\Forms\Validator;
+
+class FieldValidationExtension extends Extension implements TestOnly
+{
+    protected bool $excludeFromValidation = false;
+
+    protected bool $triggerTestValidationError = false;
+
+    public function updateValidationResult(bool &$result, Validator $validator)
+    {
+        if ($this->excludeFromValidation) {
+            $result = true;
+            return;
+        }
+
+        if ($this->triggerTestValidationError) {
+            $result = false;
+            $validator->validationError($this->owner->getName(), 'A test error message');
+            return;
+        }
+    }
+
+    public function setExcludeFromValidation(bool $exclude)
+    {
+        $this->excludeFromValidation = $exclude;
+    }
+
+    public function setTriggerTestValidationError(bool $triggerTestValidationError)
+    {
+        $this->triggerTestValidationError = $triggerTestValidationError;
+    }
+}