Merge pull request #11506 from creative-commoners/pulls/6.0/remove-fi…

…eldsvalidator ENH Call validate on form fields within form
silverstripe · Dec 12, 2024 · 45d2b64 · 45d2b64
2 parents d3be004 + 283d69a
commit 45d2b64
Show file tree

Hide file tree

Showing 10 changed files with 84 additions and 140 deletions.
diff --git a/src/Forms/FieldsValidator.php b/src/Forms/FieldsValidator.php
diff --git a/src/Forms/Form.php b/src/Forms/Form.php
@@ -299,8 +299,10 @@ public function __construct(
         $this->setName($name);
 
         // Form validation
-        $this->validator = ($validator) ? $validator : new RequiredFieldsValidator();
-        $this->validator->setForm($this);
+        if ($validator) {
+            $this->validator = $validator;
+            $this->validator->setForm($this);
+        }
 
         // Form error controls
         $this->restoreFormState();
@@ -1264,17 +1266,23 @@ public function getLegend()
      */
     public function validationResult()
     {
-        // Automatically pass if there is no validator, or the clicked button is exempt
+        $result = ValidationResult::create();
+        // Automatically pass if the clicked button is exempt
         // Note: Soft support here for validation with absent request handler
         $handler = $this->getRequestHandler();
         $action = $handler ? $handler->buttonClicked() : null;
-        $validator = $this->getValidator();
-        if (!$validator || $this->actionIsValidationExempt($action)) {
-            return ValidationResult::create();
+        if ($this->actionIsValidationExempt($action)) {
+            return $result;
+        }
+        // Invoke FormField validation
+        foreach ($this->Fields() as $field) {
+            $result->combineAnd($field->validate());
         }
-
         // Invoke validator
-        $result = $validator->validate();
+        $validator = $this->getValidator();
+        if ($validator) {
+            $result->combineAnd($validator->validate());
+        }
         $this->loadMessagesFrom($result);
         return $result;
     }

diff --git a/src/Forms/GridField/GridFieldDetailForm.php b/src/Forms/GridField/GridFieldDetailForm.php
@@ -14,7 +14,6 @@
 use SilverStripe\Core\Extensible;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Forms\FieldList;
-use SilverStripe\Forms\FieldsValidator;
 use SilverStripe\Forms\Validation\Validator;
 use SilverStripe\ORM\DataList;
 use SilverStripe\ORM\DataObject;
@@ -147,10 +146,8 @@ public function handleItem($gridField, $request)
         if (!$this->getValidator()) {
             if ($record->hasMethod('getCMSCompositeValidator')) {
                 $validator = $record->getCMSCompositeValidator();
-            } else {
-                $validator = FieldsValidator::create();
+                $this->setValidator($validator);
             }
-            $this->setValidator($validator);
         }
 
         return $handler->handleRequest($request);

diff --git a/src/Forms/Validation/RequiredFieldsValidator.php b/src/Forms/Validation/RequiredFieldsValidator.php
@@ -116,12 +116,6 @@ public function php($data)
         $valid = true;
         $fields = $this->form->Fields();
 
-        foreach ($fields as $field) {
-            $result = $field->validate();
-            $valid = $result->isValid() && $valid;
-            $this->result->combineAnd($result);
-        }
-
         if (!$this->required) {
             return $valid;
         }

diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php
@@ -19,7 +19,6 @@
 use SilverStripe\Forms\FormField;
 use SilverStripe\Forms\FormScaffolder;
 use SilverStripe\Forms\Validation\CompositeValidator;
-use SilverStripe\Forms\FieldsValidator;
 use SilverStripe\Forms\GridField\GridField;
 use SilverStripe\Forms\GridField\GridFieldConfig_RelationEditor;
 use SilverStripe\Forms\HiddenField;
@@ -2686,7 +2685,7 @@ public function getCMSActions()
      */
     public function getCMSCompositeValidator(): CompositeValidator
     {
-        $compositeValidator = CompositeValidator::create([FieldsValidator::create()]);
+        $compositeValidator = CompositeValidator::create();
 
         // Support for the old method during the deprecation period
         if ($this->hasMethod('getCMSValidator')) {

diff --git a/tests/php/Forms/EmailFieldTest.php b/tests/php/Forms/EmailFieldTest.php
@@ -4,7 +4,6 @@
 
 use SilverStripe\Dev\FunctionalTest;
 use SilverStripe\Forms\EmailField;
-use SilverStripe\Forms\FieldsValidator;
 
 class EmailFieldTest extends FunctionalTest
 {

diff --git a/tests/php/Forms/FieldsValidatorTest.php b/tests/php/Forms/FieldsValidatorTest.php
diff --git a/tests/php/Forms/FormFieldTest.php b/tests/php/Forms/FormFieldTest.php
@@ -41,7 +41,6 @@
 use PHPUnit\Framework\Attributes\DataProvider;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Forms\ConfirmedPasswordField;
-use SilverStripe\Forms\FieldsValidator;
 use SilverStripe\Forms\NumericField;
 use SilverStripe\Forms\CheckboxField_Readonly;
 use SilverStripe\VersionedAdmin\Forms\DiffField;
@@ -548,7 +547,7 @@ public function testValidationExtensionHooks()
         /** @var TextField|FieldValidationExtension $field */
         $field = new TextField('Test');
         $field->setMaxLength(5);
-        $form = new Form(null, 'test', new FieldList($field), new FieldList(), new FieldsValidator());
+        $form = new Form(null, 'test', new FieldList($field), new FieldList());
         $form->disableSecurityToken();
 
         $field->setValue('IAmLongerThan5Characters');

diff --git a/tests/php/Forms/FormTest.php b/tests/php/Forms/FormTest.php
@@ -34,6 +34,7 @@
 use SilverStripe\Model\ArrayData;
 use SilverStripe\View\SSViewer;
 use PHPUnit\Framework\Attributes\DataProvider;
+use SilverStripe\Forms\EmailField;
 
 class FormTest extends FunctionalTest
 {
@@ -1309,4 +1310,67 @@ protected function clean($input)
             trim($input ?? '')
         );
     }
+
+    public static function provideValidateFormFields()
+    {
+        return [
+            'missing values arent invalid' => [
+                'values' => [],
+                'isValid' => true,
+            ],
+            'empty values arent invalid' => [
+                'values' => [
+                    'EmailField1' => '',
+                    'EmailField2' => null,
+                ],
+                'isValid' => true,
+            ],
+            'any invalid is invalid' => [
+                'values' => [
+                    'EmailField1' => '[email protected]',
+                    'EmailField2' => 'not email',
+                ],
+                'isValid' => false,
+            ],
+            'all invalid is invalid' => [
+                'values' => [
+                    'EmailField1' => 'not email',
+                    'EmailField2' => 'not email',
+                ],
+                'isValid' => false,
+            ],
+            'all valid is valid' => [
+                'values' => [
+                    'EmailField1' => '[email protected]',
+                    'EmailField2' => '[email protected]',
+                ],
+                'isValid' => true,
+            ],
+        ];
+    }
+
+    #[DataProvider('provideValidateFormFields')]
+    public function testValidateFormFields(array $values, bool $isValid)
+    {
+        $fieldList = new FieldList([
+            $field1 = new EmailField('EmailField1'),
+            $field2 = new EmailField('EmailField2'),
+        ]);
+        if (array_key_exists('EmailField1', $values)) {
+            $field1->setValue($values['EmailField1']);
+        }
+        if (array_key_exists('EmailField2', $values)) {
+            $field2->setValue($values['EmailField2']);
+        }
+        $form = new Form(null, 'testForm', $fieldList, new FieldList([/* no actions */]));
+
+        $result = $form->validationResult();
+        $this->assertSame($isValid, $result->isValid());
+        $messages = $result->getMessages();
+        if ($isValid) {
+            $this->assertEmpty($messages);
+        } else {
+            $this->assertNotEmpty($messages);
+        }
+    }
 }
diff --git a/tests/php/Forms/GridField/GridFieldTest.php b/tests/php/Forms/GridField/GridFieldTest.php
@@ -551,7 +551,7 @@ public function testValidationMessageInOutput()
         $gridField->setMessage(null);
 
         // A form that passes validation should not display a validation error in the FieldHolder output.
-        $form->setValidator(new RequiredFieldsValidator());
+        $form->setValidator(null);
         $form->validationResult();
         $gridfieldOutput = $gridField->FieldHolder();
         $this->assertStringNotContainsString('<p class="message ' . ValidationResult::TYPE_ERROR . '">', $gridfieldOutput);