Implemented PR feedback. Added some initial test cov

src/Forms/DefaultFormFactory.php

@@ -101,7 +101,12 @@ class DefaultFormFactory implements FormFactory
             return null;
         }
 
-        return $context['Record']->getCMSValidators();
+        $validator = $context['Record']->getValidatorList();
+
+        // Extend validator
+        $this->invokeWithExtensions('updateFormValidator', $validator, $controller, $name, $context);
+
+        return $validator;
     }
 
     /**

src/Forms/Form.php

@@ -1866,11 +1866,17 @@ class Form extends ViewableData implements HasRequestHandler
         if ($this->getSecurityToken()->isEnabled()) {
             return false;
         }
+
         if ($this->FormMethod() !== 'GET') {
             return false;
         }
 
-        // Don't cache if there are required fields, or some other complex validator
-        return $this->getValidator()->canBeCached();
+        $validator = $this->getValidator();
+
+        if (!$validator) {
+            return true;
+        }
+
+        return $validator->canBeCached();
     }
 }

src/Forms/GridField/GridFieldDetailForm.php

@@ -123,7 +123,7 @@ class GridFieldDetailForm implements GridField_URLHandler
 
         // if no validator has been set on the GridField then use the Validators from the record.
         if (!$this->getValidator()) {
-            $this->setValidator($record->getCMSValidators());
+            $this->setValidator($record->getValidatorList());
         }
 
         return $handler->handleRequest($request);

src/Forms/Validator.php

@@ -169,8 +169,7 @@ abstract class Validator
     }
 
     /**
-     * When Validators are set on the form, it can affect whether or not the form cannot be cached. The base
-     * implementation always returns false (in order to match the current paradigm).
+     * When Validators are set on the form, it can affect whether or not the form cannot be cached.
      *
      * @see RequiredFields for an example of when you might be able to cache your form.
      *

src/Forms/ValidatorList.php

@@ -37,6 +37,17 @@ class ValidatorList extends Validator
         return parent::setForm($form);
     }
 
+    /**
+     * @param Validator $validator
+     * @return ValidatorList
+     */
+    public function addValidator(Validator $validator): ValidatorList
+    {
+        $this->getValidators()->add($validator);
+
+        return $this;
+    }
+
     /**
      * Returns any errors there may be. This method considers the enabled status of the ValidatorList as a whole
      * (exiting early if the List is disabled), as well as the enabled status of each individual Validator.
@@ -75,27 +86,6 @@ class ValidatorList extends Validator
         return $this->result;
     }
 
-    /**
-     * Returns whether the field in question is required. This will usually display '*' next to the
-     * field.
-     *
-     * @param string $fieldName
-     *
-     * @return bool
-     */
-    public function fieldIsRequired($fieldName)
-    {
-        foreach ($this->getValidators() as $validator) {
-            if (!$validator->fieldIsRequired($fieldName)) {
-                continue;
-            }
-
-            return true;
-        }
-
-        return false;
-    }
-
     /**
      * Note: The existing implementations for the php() method (@see RequiredFields) does not check whether the
      * Validator is enabled or not, and it also does not reset the validation result - so, neither does this.
@@ -120,14 +110,22 @@ class ValidatorList extends Validator
     }
 
     /**
-     * @param Validator $validator
-     * @return ValidatorList
+     * Returns whether the field in question is required. This will usually display '*' next to the
+     * field.
+     *
+     * @param string $fieldName
+     *
+     * @return bool
      */
-    public function addValidator(Validator $validator): ValidatorList
+    public function fieldIsRequired($fieldName)
     {
-        $this->getValidators()->add($validator);
+        foreach ($this->getValidators() as $validator) {
+            if ($validator->fieldIsRequired($fieldName)) {
+                return true;
+            }
+        }
 
-        return $this;
+        return false;
     }
 
     /**
@@ -161,7 +159,7 @@ class ValidatorList extends Validator
      * @param string $className
      * @return ValidatorList
      */
-    public function removeByType(string $className): ValidatorList
+    public function removeValidatorsByType(string $className): ValidatorList
     {
         foreach ($this->getValidators() as $validator) {
             if (!$validator instanceof $className) {
@@ -180,12 +178,10 @@ class ValidatorList extends Validator
     public function canBeCached(): bool
     {
         foreach ($this->getValidators() as $validator) {
-            if ($validator->canBeCached()) {
-                continue;
-            }
-
+            if (!$validator->canBeCached()) {
                 return false;
             }
+        }
 
         return true;
     }

src/ORM/DataObject.php

@@ -2448,7 +2448,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
         return $actions;
     }
 
-    public function getCMSValidators(): ValidatorList
+    /**
+     * @return ValidatorList
+     */
+    public function getValidatorList(): ValidatorList
     {
         $validatorList = new ValidatorList();
 
@@ -2457,7 +2460,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
             $validatorList->addValidator($this->getCMSValidator());
         }
 
-        $this->invokeWithExtensions('updateCMSValidators', $validatorList);
+        $this->invokeWithExtensions('updateValidatorList', $validatorList);
 
         return $validatorList;
     }

tests/php/Forms/ValidatorListTest.php

@@ -0,0 +1,193 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use SilverStripe\Control\Controller;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\FieldList;
+use SilverStripe\Forms\Form;
+use SilverStripe\Forms\RequiredFields;
+use SilverStripe\Forms\Tests\ValidatorTest\TestValidator;
+use SilverStripe\Forms\Tests\ValidatorTest\TestValidatorList;
+use SilverStripe\Forms\TextField;
+use SilverStripe\Forms\ValidatorList;
+
+/**
+ * @package framework
+ * @subpackage tests
+ */
+class ValidatorListTest extends SapphireTest
+{
+    /**
+     * Common method for setting up form, since that will always be a dependency for the validator.
+     *
+     * @param array $fieldNames
+     * @return Form
+     */
+    protected function getForm(array $fieldNames = array()): Form
+    {
+        // Setup field list now. We're only worried about names right now
+        $fieldList = new FieldList();
+
+        foreach ($fieldNames as $name) {
+            $fieldList->add(new TextField($name));
+        }
+
+        return new Form(Controller::curr(), "testForm", $fieldList, new FieldList([/* no actions */]));
+    }
+
+    public function testAddValidator(): void
+    {
+        $validatorList = new ValidatorList();
+        $validatorList->addValidator(new RequiredFields());
+        $validatorList->addValidator(new RequiredFields());
+
+        $this->assertCount(2, $validatorList->getValidators());
+    }
+
+    public function testSetForm(): void
+    {
+        $form = $this->getForm();
+
+        $validatorList = new TestValidatorList();
+        $validator = new TestValidator();
+
+        $validatorList->addValidator($validator);
+
+        $validatorList->setForm($form);
+
+        $this->assertNotNull($validatorList->getForm());
+        $this->assertCount(1, $validatorList->getValidators());
+
+        foreach ($validatorList->getValidators() as $validator) {
+            /** @var TestValidator $validator */
+            $this->assertNotNull($validator->getForm());
+        }
+    }
+
+    public function testGetValidatorsByType(): void
+    {
+        $validatorList = new ValidatorList();
+        $validatorList->addValidator(new RequiredFields());
+        $validatorList->addValidator(new TestValidator());
+        $validatorList->addValidator(new RequiredFields());
+        $validatorList->addValidator(new TestValidator());
+
+        $this->assertCount(4, $validatorList->getValidators());
+        $this->assertCount(2, $validatorList->getValidatorsByType(RequiredFields::class));
+    }
+
+    public function testRemoveValidatorsByType(): void
+    {
+        $validatorList = new ValidatorList();
+        $validatorList->addValidator(new RequiredFields());
+        $validatorList->addValidator(new TestValidator());
+        $validatorList->addValidator(new RequiredFields());
+        $validatorList->addValidator(new TestValidator());
+
+        $this->assertCount(4, $validatorList->getValidators());
+
+        $validatorList->removeValidatorsByType(RequiredFields::class);
+        $this->assertCount(2, $validatorList->getValidators());
+    }
+
+    public function testCanBeCached(): void
+    {
+        $validatorList = new ValidatorList();
+        $validatorList->addValidator(new RequiredFields());
+
+        $this->assertTrue($validatorList->canBeCached());
+
+        $validatorList = new ValidatorList();
+        $validatorList->addValidator(new RequiredFields(['Foor']));
+
+        $this->assertFalse($validatorList->canBeCached());
+    }
+
+    public function testFieldIsRequired(): void
+    {
+        $validatorList = new ValidatorList();
+
+        $fieldNames = [
+            'Title',
+            'Content',
+        ];
+
+        $requiredFieldsFirst = new RequiredFields(
+            [
+                $fieldNames[0],
+            ]
+        );
+        $requiredFieldsSecond = new RequiredFields(
+            [
+                $fieldNames[1],
+            ]
+        );
+
+        $validatorList->addValidator($requiredFieldsFirst);
+        $validatorList->addValidator($requiredFieldsSecond);
+
+        foreach ($fieldNames as $field) {
+            $this->assertTrue(
+                $validatorList->fieldIsRequired($field),
+                sprintf('Failed to find "%s" field in required list', $field)
+            );
+        }
+    }
+
+    public function testValidate(): void
+    {
+        $validatorList = new ValidatorList();
+        // Add two separate validators, each with one required field
+        $validatorList->addValidator(new RequiredFields(['Foo']));
+        $validatorList->addValidator(new RequiredFields(['Bar']));
+
+        // Setup a form with the fields/data we're testing (a form is a dependency for validation right now)
+        // We'll add three empty fields, but only two of them should be required
+        $data = [
+            'Foo' => '',
+            'Bar' => '',
+            'FooBar' => '',
+        ];
+        // We only care right now about the fields we've got setup in this array
+        $form = $this->getForm(array_keys($data));
+        $form->disableSecurityToken();
+        // Setup validator now that we've got our form
+        $form->setValidator($validatorList);
+        // Put data into the form so the validator can pull it back out again
+        $form->loadDataFrom($data);
+
+        $result = $form->validationResult();
+        $this->assertFalse($result->isValid());
+        $this->assertCount(2, $result->getMessages());
+    }
+
+    public function testRemoveValidation(): void
+    {
+        $validatorList = new ValidatorList();
+        $validatorList->addValidator(new TestValidator());
+
+        // Setup a form with the fields/data we're testing (a form is a dependency for validation right now)
+        $data = [
+            'Foo' => '',
+        ];
+        // We only care right now about the fields we've got setup in this array
+        $form = $this->getForm(array_keys($data));
+        $form->disableSecurityToken();
+        // Setup validator now that we've got our form
+        $form->setValidator($validatorList);
+        // Put data into the form so the validator can pull it back out again
+        $form->loadDataFrom($data);
+
+        $result = $form->validationResult();
+        $this->assertFalse($result->isValid());
+        $this->assertCount(1, $result->getMessages());
+
+        // Make sure it doesn't fail after removing validation AND has no errors (since calling validate should reset
+        // errors)
+        $validatorList->removeValidation();
+        $result = $form->validationResult();
+        $this->assertTrue($result->isValid());
+        $this->assertEmpty($result->getMessages());
+    }
+}

tests/php/Forms/ValidatorTest/TestValidator.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\Forms\Tests\ValidatorTest;
 
 use SilverStripe\Dev\TestOnly;
+use SilverStripe\Forms\Form;
 use SilverStripe\Forms\Validator;
 
 class TestValidator extends Validator implements TestOnly
@@ -19,5 +20,17 @@ class TestValidator extends Validator implements TestOnly
         foreach ($data as $field => $data) {
             $this->validationError($field, 'error');
         }
+
+        return null;
+    }
+
+    /**
+     * Allow us to access the form for test purposes.
+     *
+     * @return Form|null
+     */
+    public function getForm(): ?Form
+    {
+        return $this->form;
     }
 }

tests/php/Forms/ValidatorTest/TestValidatorList.php

@@ -0,0 +1,25 @@
+<?php
+
+namespace SilverStripe\Forms\Tests\ValidatorTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\Forms\Form;
+use SilverStripe\Forms\ValidatorList;
+
+/**
+ * Class TestValidatorList
+ *
+ * @package SilverStripe\Forms\Tests\ValidatorTest
+ */
+class TestValidatorList extends ValidatorList implements TestOnly
+{
+    /**
+     * Allow us to access the form for test purposes.
+     *
+     * @return Form|null
+     */
+    public function getForm(): ?Form
+    {
+        return $this->form;
+    }
+}