Merge pull request #9406 from chrispenny/feature/standardise-get-cms-validator

v4 improvement: Standardise getCMSValidator for DataObjects/Forms

docs/en/02_Developer_Guides/03_Forms/01_Validation.md

@@ -274,9 +274,10 @@ class MyController extends Controller
 ### Validation in the CMS
 
 In the CMS, we're not creating the forms for editing CMS records. The `Form` instance is generated for us so we cannot
-call `setValidator` easily. However, a `DataObject` can provide its' own `Validator` instance through the 
-`getCMSValidator()` method. The CMS interfaces such as [LeftAndMain](api:SilverStripe\Admin\LeftAndMain), [ModelAdmin](api:SilverStripe\Admin\ModelAdmin) and [GridField](api:SilverStripe\Forms\GridField\GridField) will 
-respect the provided `Validator` and handle displaying error and success responses to the user. 
+call `setValidator` easily. However, a `DataObject` can provide its' own `Validator` instance/s through the 
+`getCMSCompositeValidator()` method. The CMS interfaces such as [LeftAndMain](api:SilverStripe\Admin\LeftAndMain),
+[ModelAdmin](api:SilverStripe\Admin\ModelAdmin) and [GridField](api:SilverStripe\Forms\GridField\GridField) will 
+respect the provided `Validator`/s and handle displaying error and success responses to the user. 
 
 [info]
 Again, custom error messages can be provided through the `FormField`

src/Forms/CompositeValidator.php

@@ -0,0 +1,241 @@
+<?php
+
+namespace SilverStripe\Forms;
+
+use InvalidArgumentException;
+use SilverStripe\ORM\ValidationResult;
+
+/**
+ * CompositeValidator can contain between 0 and many different types of Validators. Each Validator is itself still
+ * responsible for Validating its form and generating its ValidationResult.
+ *
+ * Once each Validator has generated its ValidationResult, the CompositeValidator will combine these results into a
+ * single ValidationResult. This single ValidationResult is what will be returned by the CompositeValidator.
+ *
+ * You can add Validators to the CompositeValidator in any DataObject by extending the getCMSCompositeValidator()
+ * method:
+ *
+ * public function getCMSCompositeValidator(): CompositeValidator
+ * {
+ *   $compositeValidator = parent::getCMSCompositeValidator();
+ *
+ *   $compositeValidator->addValidator(RequiredFields::create(['MyRequiredField']));
+ *
+ *   return $compositeValidator
+ * }
+ *
+ * Or by implementing the updateCMSCompositeValidator() method in a DataExtension:
+ *
+ * public function updateCMSCompositeValidator(CompositeValidator $compositeValidator): void
+ * {
+ *   $compositeValidator->addValidator(RequiredFields::create(['AdditionalContent']));
+ * }
+ *
+ * Class CompositeValidator
+ *
+ * @package SilverStripe\Forms
+ */
+class CompositeValidator extends Validator
+{
+    /**
+     * @var array|Validator[]
+     */
+    private $validators;
+
+    /**
+     * CompositeValidator constructor.
+     *
+     * @param array|Validator[] $validators
+     */
+    public function __construct(array $validators = [])
+    {
+        $this->validators = array_values($validators);
+
+        parent::__construct();
+    }
+
+    /**
+     * Set the provided Form to the CompositeValidator and each Validator that has been added.
+     *
+     * @param Form $form
+     * @return Validator
+     */
+    public function setForm($form)
+    {
+        foreach ($this->getValidators() as $validator) {
+            $validator->setForm($form);
+        }
+
+        return parent::setForm($form);
+    }
+
+    /**
+     * @param Validator $validator
+     * @return CompositeValidator
+     */
+    public function addValidator(Validator $validator): CompositeValidator
+    {
+        $this->validators[] = $validator;
+
+        return $this;
+    }
+
+    /**
+     * Returns any errors there may be. This method considers the enabled status of the CompositeValidator as a whole
+     * (exiting early if the Composite is disabled), as well as the enabled status of each individual Validator.
+     *
+     * @return ValidationResult
+     */
+    public function validate()
+    {
+        $this->resetResult();
+
+        // This CompositeValidator has been disabled in full
+        if (!$this->getEnabled()) {
+            return $this->result;
+        }
+
+        $data = $this->form->getData();
+
+        foreach ($this->getValidators() as $validator) {
+            // Reset the validation results for this Validator
+            $validator->resetResult();
+
+            // This Validator has been disabled, so skip it
+            if (!$validator->getEnabled()) {
+                continue;
+            }
+
+            // Run validation, and exit early if it's valid
+            if ($validator->php($data)) {
+                continue;
+            }
+
+            // Validation result was invalid. Combine our ValidationResult messages
+            $this->getResult()->combineAnd($validator->getResult());
+        }
+
+        return $this->result;
+    }
+
+    /**
+     * 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.
+     *
+     * @param array $data
+     * @return bool
+     */
+    public function php($data)
+    {
+        foreach ($this->getValidators() as $validator) {
+            // Run validation, and exit early if it's valid
+            if ($validator->php($data)) {
+                continue;
+            }
+
+            // Validation result was invalid. Combine our ValidationResult messages
+            $this->getResult()->combineAnd($validator->getResult());
+        }
+
+        // After collating results, return whether or not everything was valid
+        return $this->getResult()->isValid();
+    }
+
+    /**
+     * 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)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * @return array|Validator[]
+     */
+    public function getValidators(): array
+    {
+        return $this->validators;
+    }
+
+    /**
+     * Return all Validators that match a certain class name. EG: RequiredFields::class
+     *
+     * @param string $className
+     * @return array|Validator[]
+     */
+    public function getValidatorsByType(string $className): array
+    {
+        $validators = [];
+
+        foreach ($this->getValidators() as $key => $validator) {
+            if (!$validator instanceof $className) {
+                continue;
+            }
+
+            $validators[$key] = $validator;
+        }
+
+        return $validators;
+    }
+
+    /**
+     * Remove all Validators that match a certain class name. EG: RequiredFields::class
+     *
+     * @param string $className
+     * @return CompositeValidator
+     */
+    public function removeValidatorsByType(string $className): CompositeValidator
+    {
+        foreach ($this->getValidatorsByType($className) as $key => $validator) {
+            $this->removeValidatorByKey($key);
+        }
+
+        return $this;
+    }
+
+    /**
+     * Each Validator is aware of whether or not it can be cached. If even one Validator cannot be cached, then the
+     * CompositeValidator as a whole also cannot be cached.
+     *
+     * @return bool
+     */
+    public function canBeCached(): bool
+    {
+        foreach ($this->getValidators() as $validator) {
+            if (!$validator->canBeCached()) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
+    /**
+     * @internal This method may be updated to public in the future. Let us know if you feel there's a use case for it
+     * @param int $key
+     * @return CompositeValidator
+     */
+    protected function removeValidatorByKey(int $key): CompositeValidator
+    {
+        if (!array_key_exists($key, $this->validators)) {
+            throw new InvalidArgumentException(
+                sprintf('Key "%s" does not exist in $validators array', $key)
+            );
+        }
+
+        unset($this->validators[$key]);
+
+        return $this;
+    }
+}

src/Forms/DefaultFormFactory.php

@@ -5,6 +5,7 @@ namespace SilverStripe\Forms;
 use InvalidArgumentException;
 use SilverStripe\Control\RequestHandler;
 use SilverStripe\Core\Extensible;
+use SilverStripe\ORM\DataObject;
 
 /**
  * Default form builder class.
@@ -96,15 +97,21 @@ class DefaultFormFactory implements FormFactory
      */
     protected function getFormValidator(RequestHandler $controller = null, $name, $context = [])
     {
-        $validator = null;
-        if ($context['Record']->hasMethod('getCMSValidator')) {
-            // @todo Deprecate or formalise support for getCMSValidator()
-            $validator = $context['Record']->getCMSValidator();
+        if (!$context['Record'] instanceof DataObject) {
+            return null;
         }
 
-        // Extend validator
-        $this->invokeWithExtensions('updateFormValidator', $validator, $controller, $name, $context);
-        return $validator;
+        $compositeValidator = $context['Record']->getCMSCompositeValidator();
+
+        // Extend validator - legacy support, will be removed in 5.0.0
+        foreach ($compositeValidator->getValidators() as $validator) {
+            $this->invokeWithExtensions('updateFormValidator', $validator, $controller, $name, $context);
+        }
+
+        // Extend validator - forward support, will be supported beyond 5.0.0
+        $this->invokeWithExtensions('updateCMSCompositeValidator', $compositeValidator);
+
+        return $compositeValidator;
     }
 
     /**

src/Forms/Form.php

@@ -1866,19 +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
         $validator = $this->getValidator();
-        if ($validator instanceof RequiredFields) {
-            if (count($this->validator->getRequired())) {
-                return false;
-            }
-        } else {
-            return false;
+
+        if (!$validator) {
+            return true;
         }
-        return true;
+
+        return $validator->canBeCached();
     }
 }

src/Forms/GridField/GridFieldDetailForm.php

@@ -121,10 +121,9 @@ class GridFieldDetailForm implements GridField_URLHandler
             $gridField->getState(false)->setValue($gridStateStr);
         }
 
-        // if no validator has been set on the GridField and the record has a
-        // CMS validator, use that.
-        if (!$this->getValidator() && ClassInfo::hasMethod($record, 'getCMSValidator')) {
-            $this->setValidator($record->getCMSValidator());
+        // if no validator has been set on the GridField then use the Validators from the record.
+        if (!$this->getValidator()) {
+            $this->setValidator($record->getCMSCompositeValidator());
         }
 
         return $handler->handleRequest($request);

src/Forms/RequiredFields.php

@@ -215,4 +215,12 @@ class RequiredFields extends Validator
     {
         return array_values($this->required);
     }
+
+    /**
+     * @return bool
+     */
+    public function canBeCached(): bool
+    {
+        return count($this->getRequired()) === 0;
+    }
 }

src/Forms/Validator.php

@@ -168,6 +168,18 @@ abstract class Validator
         return $this;
     }
 
+    /**
+     * 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.
+     *
+     * @return bool
+     */
+    public function canBeCached(): bool
+    {
+        return false;
+    }
+
     /**
      * Clear current result
      *

src/ORM/DataExtension.php

@@ -5,6 +5,7 @@ namespace SilverStripe\ORM;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Extension;
 use SilverStripe\Forms\FieldList;
+use SilverStripe\Forms\CompositeValidator;
 use SilverStripe\ORM\Queries\SQLSelect;
 use Exception;
 
@@ -219,6 +220,17 @@ abstract class DataExtension extends Extension
     {
     }
 
+    /**
+     * This function is used to provide modifications to the Validators used on a DataObject.
+     *
+     * Caution: Use {@link CompositeValidator->addValidator()} to add Validators.
+     *
+     * @param CompositeValidator $compositeValidator
+     */
+    public function updateCMSCompositeValidator(CompositeValidator $compositeValidator): void
+    {
+    }
+
     /**
      * This function is used to provide modifications to the form used
      * for front end forms. {@link DataObject->getFrontEndFields()}

src/ORM/DataObject.php

@@ -15,6 +15,7 @@ use SilverStripe\Dev\Deprecation;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\FormField;
 use SilverStripe\Forms\FormScaffolder;
+use SilverStripe\Forms\CompositeValidator;
 use SilverStripe\i18n\i18n;
 use SilverStripe\i18n\i18nEntityProvider;
 use SilverStripe\ORM\Connect\MySQLSchemaManager;
@@ -2447,6 +2448,36 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
         return $actions;
     }
 
+    /**
+     * When extending this class and overriding this method, you will need to instantiate the CompositeValidator by
+     * calling parent::getCMSCompositeValidator(). This will ensure that the appropriate extension point is also
+     * invoked.
+     *
+     * You can also update the CompositeValidator by creating an Extension and implementing the
+     * updateCMSCompositeValidator(CompositeValidator $compositeValidator) method.
+     *
+     * @see CompositeValidator for examples of implementation
+     * @return CompositeValidator
+     */
+    public function getCMSCompositeValidator(): CompositeValidator
+    {
+        $compositeValidator = new CompositeValidator();
+
+        // Support for the old method during the deprecation period
+        if ($this->hasMethod('getCMSValidator')) {
+            Deprecation::notice(
+                '4.6',
+                'getCMSValidator() is removed in 5.0 in favour of getCMSCompositeValidator()'
+            );
+
+            $compositeValidator->addValidator($this->getCMSValidator());
+        }
+
+        // Extend validator - forward support, will be supported beyond 5.0.0
+        $this->invokeWithExtensions('updateCMSCompositeValidator', $compositeValidator);
+
+        return $compositeValidator;
+    }
 
     /**
      * Used for simple frontend forms without relation editing

tests/php/Forms/CompositeValidatorTest.php

@@ -0,0 +1,201 @@
+<?php
+
+namespace SilverStripe\Forms\Tests;
+
+use ReflectionClass;
+use ReflectionException;
+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\TextField;
+use SilverStripe\Forms\CompositeValidator;
+
+/**
+ * @package framework
+ * @subpackage tests
+ */
+class CompositeValidatorTest 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 = []): 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
+    {
+        $compositeValidator = new CompositeValidator();
+        $compositeValidator->addValidator(new RequiredFields());
+        $compositeValidator->addValidator(new RequiredFields());
+
+        $this->assertCount(2, $compositeValidator->getValidators());
+    }
+
+    /**
+     * @throws ReflectionException
+     */
+    public function testSetForm(): void
+    {
+        $form = $this->getForm();
+
+        $reflectionClass = new ReflectionClass(CompositeValidator::class);
+        $property = $reflectionClass->getProperty('form');
+        $property->setAccessible(true);
+
+        $compositeValidator = new CompositeValidator();
+        $validator = new TestValidator();
+
+        $compositeValidator->addValidator($validator);
+
+        $compositeValidator->setForm($form);
+
+        $this->assertNotNull($property->getValue($compositeValidator));
+        $this->assertCount(1, $compositeValidator->getValidators());
+
+        foreach ($compositeValidator->getValidators() as $validator) {
+            /** @var TestValidator $validator */
+            $this->assertNotNull($property->getValue($validator));
+        }
+    }
+
+    public function testGetValidatorsByType(): void
+    {
+        $compositeValidator = new CompositeValidator();
+        $compositeValidator->addValidator(new RequiredFields());
+        $compositeValidator->addValidator(new TestValidator());
+        $compositeValidator->addValidator(new RequiredFields());
+        $compositeValidator->addValidator(new TestValidator());
+
+        $this->assertCount(4, $compositeValidator->getValidators());
+        $this->assertCount(2, $compositeValidator->getValidatorsByType(RequiredFields::class));
+    }
+
+    public function testRemoveValidatorsByType(): void
+    {
+        $compositeValidator = new CompositeValidator();
+        $compositeValidator->addValidator(new RequiredFields());
+        $compositeValidator->addValidator(new TestValidator());
+        $compositeValidator->addValidator(new RequiredFields());
+        $compositeValidator->addValidator(new TestValidator());
+
+        $this->assertCount(4, $compositeValidator->getValidators());
+
+        $compositeValidator->removeValidatorsByType(RequiredFields::class);
+        $this->assertCount(2, $compositeValidator->getValidators());
+    }
+
+    public function testCanBeCached(): void
+    {
+        $compositeValidator = new CompositeValidator();
+        $compositeValidator->addValidator(new RequiredFields());
+
+        $this->assertTrue($compositeValidator->canBeCached());
+
+        $compositeValidator = new CompositeValidator();
+        $compositeValidator->addValidator(new RequiredFields(['Foor']));
+
+        $this->assertFalse($compositeValidator->canBeCached());
+    }
+
+    public function testFieldIsRequired(): void
+    {
+        $compositeValidator = new CompositeValidator();
+
+        $fieldNames = [
+            'Title',
+            'Content',
+        ];
+
+        $requiredFieldsFirst = new RequiredFields(
+            [
+                $fieldNames[0],
+            ]
+        );
+        $requiredFieldsSecond = new RequiredFields(
+            [
+                $fieldNames[1],
+            ]
+        );
+
+        $compositeValidator->addValidator($requiredFieldsFirst);
+        $compositeValidator->addValidator($requiredFieldsSecond);
+
+        foreach ($fieldNames as $field) {
+            $this->assertTrue(
+                $compositeValidator->fieldIsRequired($field),
+                sprintf('Failed to find "%s" field in required list', $field)
+            );
+        }
+    }
+
+    public function testValidate(): void
+    {
+        $compositeValidator = new CompositeValidator();
+        // Add two separate validators, each with one required field
+        $compositeValidator->addValidator(new RequiredFields(['Foo']));
+        $compositeValidator->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($compositeValidator);
+        // 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
+    {
+        $compositeValidator = new CompositeValidator();
+        $compositeValidator->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($compositeValidator);
+        // 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)
+        $compositeValidator->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;
     }
 }