diff --git a/docs/en/02_Developer_Guides/03_Forms/01_Validation.md b/docs/en/02_Developer_Guides/03_Forms/01_Validation.md index a3a049f3d..f9e25e939 100644 --- a/docs/en/02_Developer_Guides/03_Forms/01_Validation.md +++ b/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` diff --git a/src/Forms/CompositeValidator.php b/src/Forms/CompositeValidator.php new file mode 100644 index 000000000..61ead41c9 --- /dev/null +++ b/src/Forms/CompositeValidator.php @@ -0,0 +1,241 @@ +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; + } +} diff --git a/src/Forms/DefaultFormFactory.php b/src/Forms/DefaultFormFactory.php index 24c6e3feb..669f1cdeb 100644 --- a/src/Forms/DefaultFormFactory.php +++ b/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; } /** diff --git a/src/Forms/Form.php b/src/Forms/Form.php index 29d4987ab..8ab457d40 100644 --- a/src/Forms/Form.php +++ b/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(); } } diff --git a/src/Forms/GridField/GridFieldDetailForm.php b/src/Forms/GridField/GridFieldDetailForm.php index 2bf2ba610..f9a76d209 100644 --- a/src/Forms/GridField/GridFieldDetailForm.php +++ b/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); diff --git a/src/Forms/RequiredFields.php b/src/Forms/RequiredFields.php index 2395b1455..fcfa84775 100644 --- a/src/Forms/RequiredFields.php +++ b/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; + } } diff --git a/src/Forms/Validator.php b/src/Forms/Validator.php index b51e39631..60594e011 100644 --- a/src/Forms/Validator.php +++ b/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 * diff --git a/src/ORM/DataExtension.php b/src/ORM/DataExtension.php index 1c0cb8f88..b446b7211 100644 --- a/src/ORM/DataExtension.php +++ b/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()} diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index dc8e54fb0..eaa3704e0 100644 --- a/src/ORM/DataObject.php +++ b/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 diff --git a/tests/php/Forms/CompositeValidatorTest.php b/tests/php/Forms/CompositeValidatorTest.php new file mode 100644 index 000000000..43bba0098 --- /dev/null +++ b/tests/php/Forms/CompositeValidatorTest.php @@ -0,0 +1,201 @@ +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()); + } +} diff --git a/tests/php/Forms/ValidatorTest/TestValidator.php b/tests/php/Forms/ValidatorTest/TestValidator.php index 5cdc4c419..141c35221 100644 --- a/tests/php/Forms/ValidatorTest/TestValidator.php +++ b/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; } }