From d7dd93f7a7c4835393aa94563c3fa4e63fca25b9 Mon Sep 17 00:00:00 2001 From: cpenny Date: Thu, 13 Feb 2020 12:51:56 +1300 Subject: [PATCH 01/10] Standardise getCMSValidator for DataObjects/Forms --- src/Forms/DefaultFormFactory.php | 11 +- src/Forms/Form.php | 10 +- src/Forms/GridField/GridFieldDetailForm.php | 7 +- src/Forms/RequiredFields.php | 8 + src/Forms/Validator.php | 13 ++ src/Forms/ValidatorList.php | 192 ++++++++++++++++++++ src/ORM/DataObject.php | 14 ++ 7 files changed, 235 insertions(+), 20 deletions(-) create mode 100644 src/Forms/ValidatorList.php diff --git a/src/Forms/DefaultFormFactory.php b/src/Forms/DefaultFormFactory.php index 24c6e3feb..f179803cc 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,11 @@ 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; + return $context['Record']->getCMSValidators(); } /** diff --git a/src/Forms/Form.php b/src/Forms/Form.php index 1a59bb3ff..5c8309782 100644 --- a/src/Forms/Form.php +++ b/src/Forms/Form.php @@ -1871,14 +1871,6 @@ class Form extends ViewableData implements HasRequestHandler } // 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; - } - return true; + return $this->getValidator()->canBeCached(); } } diff --git a/src/Forms/GridField/GridFieldDetailForm.php b/src/Forms/GridField/GridFieldDetailForm.php index 2bf2ba610..6f909e582 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->getCMSValidators()); } 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..aac77551e 100644 --- a/src/Forms/Validator.php +++ b/src/Forms/Validator.php @@ -168,6 +168,19 @@ abstract class Validator return $this; } + /** + * 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). + * + * @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/Forms/ValidatorList.php b/src/Forms/ValidatorList.php new file mode 100644 index 000000000..57e123e94 --- /dev/null +++ b/src/Forms/ValidatorList.php @@ -0,0 +1,192 @@ +validators = ArrayList::create(); + + parent::__construct(); + } + + /** + * @param Form $form + * @return Validator + */ + public function setForm($form) + { + foreach ($this->getValidators() as $validator) { + $validator->setForm($form); + } + + return parent::setForm($form); + } + + /** + * 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. + * + * @return ValidationResult + */ + public function validate() + { + $this->resetResult(); + + // This ValidatorList 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; + } + + /** + * 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. + * + * @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(); + } + + /** + * @param Validator $validator + * @return ValidatorList + */ + public function addValidator(Validator $validator): ValidatorList + { + $this->getValidators()->add($validator); + + return $this; + } + + /** + * @return ArrayList|Validator[] + */ + public function getValidators(): ArrayList + { + return $this->validators; + } + + /** + * @param string $className + * @return ArrayList|Validator[] + */ + public function getValidatorsByType(string $className): ArrayList + { + $validators = ArrayList::create(); + + foreach ($this->getValidators() as $validator) { + if (!$validator instanceof $className) { + continue; + } + + $validators->add($validator); + } + + return $validators; + } + + /** + * @param string $className + * @return ValidatorList + */ + public function removeByType(string $className): ValidatorList + { + foreach ($this->getValidators() as $validator) { + if (!$validator instanceof $className) { + continue; + } + + $this->getValidators()->remove($validator); + } + + return $this; + } + + /** + * @return bool + */ + public function canBeCached(): bool + { + foreach ($this->getValidators() as $validator) { + if ($validator->canBeCached()) { + continue; + } + + return false; + } + + return true; + } +} diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index b34cb9a86..028e1de32 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\ValidatorList; use SilverStripe\i18n\i18n; use SilverStripe\i18n\i18nEntityProvider; use SilverStripe\ORM\Connect\MySQLSchemaManager; @@ -2447,6 +2448,19 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity return $actions; } + public function getCMSValidators(): ValidatorList + { + $validatorList = new ValidatorList(); + + // Support for the old method during the deprecation period + if ($this->hasMethod('getCMSValidator')) { + $validatorList->addValidator($this->getCMSValidator()); + } + + $this->invokeWithExtensions('updateCMSValidators', $validatorList); + + return $validatorList; + } /** * Used for simple frontend forms without relation editing From b45a3561df4c01003ffa784dc7de9a35ea96bcd6 Mon Sep 17 00:00:00 2001 From: cpenny Date: Fri, 14 Feb 2020 08:21:22 +1300 Subject: [PATCH 02/10] Implemented PR feedback. Added some initial test cov --- src/Forms/DefaultFormFactory.php | 7 +- src/Forms/Form.php | 10 +- src/Forms/GridField/GridFieldDetailForm.php | 2 +- src/Forms/Validator.php | 3 +- src/Forms/ValidatorList.php | 58 +++--- src/ORM/DataObject.php | 7 +- tests/php/Forms/ValidatorListTest.php | 193 ++++++++++++++++++ .../php/Forms/ValidatorTest/TestValidator.php | 13 ++ .../Forms/ValidatorTest/TestValidatorList.php | 25 +++ 9 files changed, 279 insertions(+), 39 deletions(-) create mode 100644 tests/php/Forms/ValidatorListTest.php create mode 100644 tests/php/Forms/ValidatorTest/TestValidatorList.php diff --git a/src/Forms/DefaultFormFactory.php b/src/Forms/DefaultFormFactory.php index f179803cc..7c3fb71a0 100644 --- a/src/Forms/DefaultFormFactory.php +++ b/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; } /** diff --git a/src/Forms/Form.php b/src/Forms/Form.php index 5c8309782..3591c830d 100644 --- a/src/Forms/Form.php +++ b/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(); } } diff --git a/src/Forms/GridField/GridFieldDetailForm.php b/src/Forms/GridField/GridFieldDetailForm.php index 6f909e582..47266f233 100644 --- a/src/Forms/GridField/GridFieldDetailForm.php +++ b/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); diff --git a/src/Forms/Validator.php b/src/Forms/Validator.php index aac77551e..60594e011 100644 --- a/src/Forms/Validator.php +++ b/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. * diff --git a/src/Forms/ValidatorList.php b/src/Forms/ValidatorList.php index 57e123e94..43f5a5065 100644 --- a/src/Forms/ValidatorList.php +++ b/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,11 +178,9 @@ class ValidatorList extends Validator public function canBeCached(): bool { foreach ($this->getValidators() as $validator) { - if ($validator->canBeCached()) { - continue; + if (!$validator->canBeCached()) { + return false; } - - return false; } return true; diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 028e1de32..5d7026ab5 100644 --- a/src/ORM/DataObject.php +++ b/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; } diff --git a/tests/php/Forms/ValidatorListTest.php b/tests/php/Forms/ValidatorListTest.php new file mode 100644 index 000000000..839a797b6 --- /dev/null +++ b/tests/php/Forms/ValidatorListTest.php @@ -0,0 +1,193 @@ +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()); + } +} 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; } } diff --git a/tests/php/Forms/ValidatorTest/TestValidatorList.php b/tests/php/Forms/ValidatorTest/TestValidatorList.php new file mode 100644 index 000000000..7cb91e046 --- /dev/null +++ b/tests/php/Forms/ValidatorTest/TestValidatorList.php @@ -0,0 +1,25 @@ +form; + } +} From f977f9734cccd4866d525b7f82cf1f05c0f7e682 Mon Sep 17 00:00:00 2001 From: cpenny Date: Mon, 17 Feb 2020 07:26:40 +1300 Subject: [PATCH 03/10] Add base updateValidatorList method to DataExtension --- src/ORM/DataExtension.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/ORM/DataExtension.php b/src/ORM/DataExtension.php index 869829b0b..85814447d 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\ValidatorList; use SilverStripe\ORM\Queries\SQLSelect; use Exception; @@ -133,6 +134,17 @@ abstract class DataExtension extends Extension { } + /** + * This function is used to provide modifications to the Validators used on a DataObject. + * + * Caution: Use {@link ValidatorList->addValidator()} to add Validators. + * + * @param ValidatorList $validatorList + */ + public function updateValidatorList(ValidatorList $validatorList): void + { + } + /** * This function is used to provide modifications to the form used * for front end forms. {@link DataObject->getFrontEndFields()} From 11e2005b9ba30457b937c566e1c2cabba0112257 Mon Sep 17 00:00:00 2001 From: cpenny Date: Wed, 19 Feb 2020 11:56:10 +1300 Subject: [PATCH 04/10] Add deprecation notice for 4.6 and update docs --- docs/en/02_Developer_Guides/03_Forms/01_Validation.md | 7 ++++--- src/ORM/DataObject.php | 5 +++++ 2 files changed, 9 insertions(+), 3 deletions(-) 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..4a5467c2f 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 +`getValidatorList()` 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/ORM/DataObject.php b/src/ORM/DataObject.php index 5d7026ab5..12c6dc8da 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -2457,6 +2457,11 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity // 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 getValidatorList()' + ); + $validatorList->addValidator($this->getCMSValidator()); } From a2b57f08016d63b9c173517cb69c33d3a7e4b983 Mon Sep 17 00:00:00 2001 From: cpenny Date: Thu, 20 Feb 2020 08:54:30 +1300 Subject: [PATCH 05/10] Update DefaultFormFactory extension point. Use array for validators --- src/Forms/DefaultFormFactory.php | 13 +++++--- src/Forms/ValidatorList.php | 54 +++++++++++++++++++++----------- 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/Forms/DefaultFormFactory.php b/src/Forms/DefaultFormFactory.php index 7c3fb71a0..a43fbb18c 100644 --- a/src/Forms/DefaultFormFactory.php +++ b/src/Forms/DefaultFormFactory.php @@ -101,12 +101,17 @@ class DefaultFormFactory implements FormFactory return null; } - $validator = $context['Record']->getValidatorList(); + $validatorList = $context['Record']->getValidatorList(); - // Extend validator - $this->invokeWithExtensions('updateFormValidator', $validator, $controller, $name, $context); + // Extend validator - legacy support, will be removed in 5.0.0 + foreach ($validatorList->getValidators() as $validator) { + $this->invokeWithExtensions('updateFormValidator', $validator, $controller, $name, $context); + } - return $validator; + // Extend validator - forward support, will be supported beyond 5.0.0 + $this->invokeWithExtensions('updateValidatorList', $validatorList); + + return $validatorList; } /** diff --git a/src/Forms/ValidatorList.php b/src/Forms/ValidatorList.php index 43f5a5065..ae189c12c 100644 --- a/src/Forms/ValidatorList.php +++ b/src/Forms/ValidatorList.php @@ -2,7 +2,7 @@ namespace SilverStripe\Forms; -use SilverStripe\ORM\ArrayList; +use InvalidArgumentException; use SilverStripe\ORM\ValidationResult; /** @@ -13,13 +13,18 @@ use SilverStripe\ORM\ValidationResult; class ValidatorList extends Validator { /** - * @var ArrayList|Validator[] + * @var array|Validator[] */ private $validators; - public function __construct() + /** + * ValidatorList constructor. + * + * @param array|Validator[] $validators + */ + public function __construct(array $validators = []) { - $this->validators = ArrayList::create(); + $this->validators = array_values($validators); parent::__construct(); } @@ -43,7 +48,7 @@ class ValidatorList extends Validator */ public function addValidator(Validator $validator): ValidatorList { - $this->getValidators()->add($validator); + $this->validators[] = $validator; return $this; } @@ -129,27 +134,27 @@ class ValidatorList extends Validator } /** - * @return ArrayList|Validator[] + * @return array|Validator[] */ - public function getValidators(): ArrayList + public function getValidators(): array { return $this->validators; } /** * @param string $className - * @return ArrayList|Validator[] + * @return array|Validator[] */ - public function getValidatorsByType(string $className): ArrayList + public function getValidatorsByType(string $className): array { - $validators = ArrayList::create(); + $validators = []; - foreach ($this->getValidators() as $validator) { + foreach ($this->getValidators() as $key => $validator) { if (!$validator instanceof $className) { continue; } - $validators->add($validator); + $validators[$key] = $validator; } return $validators; @@ -161,17 +166,30 @@ class ValidatorList extends Validator */ public function removeValidatorsByType(string $className): ValidatorList { - foreach ($this->getValidators() as $validator) { - if (!$validator instanceof $className) { - continue; - } - - $this->getValidators()->remove($validator); + foreach ($this->getValidatorsByType($className) as $key => $validator) { + $this->removeValidatorByKey($key); } return $this; } + /** + * @param int $key + * @return ValidatorList + */ + public function removeValidatorByKey(int $key): ValidatorList + { + 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; + } + /** * @return bool */ From bca4be77ed68382b11c61e2e646c08f61c4e8a19 Mon Sep 17 00:00:00 2001 From: cpenny Date: Thu, 28 May 2020 09:35:07 +1200 Subject: [PATCH 06/10] Update name to CompositeValidator. Add docblocks --- .../03_Forms/01_Validation.md | 2 +- ...lidatorList.php => CompositeValidator.php} | 87 ++++++++++++------ src/Forms/DefaultFormFactory.php | 8 +- src/Forms/GridField/GridFieldDetailForm.php | 2 +- src/ORM/DataExtension.php | 8 +- src/ORM/DataObject.php | 17 ++-- ...istTest.php => CompositeValidatorTest.php} | 92 +++++++++---------- ...torList.php => TestCompositeValidator.php} | 6 +- 8 files changed, 128 insertions(+), 94 deletions(-) rename src/Forms/{ValidatorList.php => CompositeValidator.php} (66%) rename tests/php/Forms/{ValidatorListTest.php => CompositeValidatorTest.php} (57%) rename tests/php/Forms/ValidatorTest/{TestValidatorList.php => TestCompositeValidator.php} (70%) 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 4a5467c2f..191ddd2a7 100644 --- a/docs/en/02_Developer_Guides/03_Forms/01_Validation.md +++ b/docs/en/02_Developer_Guides/03_Forms/01_Validation.md @@ -275,7 +275,7 @@ class MyController extends Controller 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/s through the -`getValidatorList()` method. The CMS interfaces such as [LeftAndMain](api:SilverStripe\Admin\LeftAndMain), +`getCompositeValidator()` 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. diff --git a/src/Forms/ValidatorList.php b/src/Forms/CompositeValidator.php similarity index 66% rename from src/Forms/ValidatorList.php rename to src/Forms/CompositeValidator.php index ae189c12c..f08dd9491 100644 --- a/src/Forms/ValidatorList.php +++ b/src/Forms/CompositeValidator.php @@ -6,11 +6,35 @@ use InvalidArgumentException; use SilverStripe\ORM\ValidationResult; /** - * Class ValidatorList + * 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 getCompositeValidator() method: + * + * public function getCompositeValidator(): CompositeValidator + * { + * $compositeValidator = parent::getCompositeValidator(); + * + * $compositeValidator->addValidator(RequiredFields::create(['MyRequiredField'])); + * + * return $compositeValidator + * } + * + * Or by implementing the updateCompositeValidator() method in a DataExtension: + * + * public function updateCompositeValidator(CompositeValidator $compositeValidator): void + * { + * $compositeValidator->addValidator(RequiredFields::create(['AdditionalContent'])); + * } + * + * Class CompositeValidator * * @package SilverStripe\Forms */ -class ValidatorList extends Validator +class CompositeValidator extends Validator { /** * @var array|Validator[] @@ -18,7 +42,7 @@ class ValidatorList extends Validator private $validators; /** - * ValidatorList constructor. + * CompositeValidator constructor. * * @param array|Validator[] $validators */ @@ -30,6 +54,8 @@ class ValidatorList extends Validator } /** + * Set the provided Form to the CompositeValidator and each Validator that has been added. + * * @param Form $form * @return Validator */ @@ -44,9 +70,9 @@ class ValidatorList extends Validator /** * @param Validator $validator - * @return ValidatorList + * @return CompositeValidator */ - public function addValidator(Validator $validator): ValidatorList + public function addValidator(Validator $validator): CompositeValidator { $this->validators[] = $validator; @@ -54,8 +80,8 @@ class ValidatorList extends Validator } /** - * 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. + * 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 */ @@ -63,7 +89,7 @@ class ValidatorList extends Validator { $this->resetResult(); - // This ValidatorList has been disabled in full + // This CompositeValidator has been disabled in full if (!$this->getEnabled()) { return $this->result; } @@ -142,6 +168,8 @@ class ValidatorList extends Validator } /** + * Return all Validators that match a certain class name. EG: RequiredFields::class + * * @param string $className * @return array|Validator[] */ @@ -161,10 +189,12 @@ class ValidatorList extends Validator } /** + * Remove all Validators that match a certain class name. EG: RequiredFields::class + * * @param string $className - * @return ValidatorList + * @return CompositeValidator */ - public function removeValidatorsByType(string $className): ValidatorList + public function removeValidatorsByType(string $className): CompositeValidator { foreach ($this->getValidatorsByType($className) as $key => $validator) { $this->removeValidatorByKey($key); @@ -174,23 +204,9 @@ class ValidatorList extends Validator } /** - * @param int $key - * @return ValidatorList - */ - public function removeValidatorByKey(int $key): ValidatorList - { - 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; - } - - /** + * 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 @@ -203,4 +219,21 @@ class ValidatorList extends Validator return true; } + + /** + * @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 a43fbb18c..9eea865ee 100644 --- a/src/Forms/DefaultFormFactory.php +++ b/src/Forms/DefaultFormFactory.php @@ -101,17 +101,17 @@ class DefaultFormFactory implements FormFactory return null; } - $validatorList = $context['Record']->getValidatorList(); + $compositeValidator = $context['Record']->getCompositeValidator(); // Extend validator - legacy support, will be removed in 5.0.0 - foreach ($validatorList->getValidators() as $validator) { + 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('updateValidatorList', $validatorList); + $this->invokeWithExtensions('updateCompositeValidator', $compositeValidator); - return $validatorList; + return $compositeValidator; } /** diff --git a/src/Forms/GridField/GridFieldDetailForm.php b/src/Forms/GridField/GridFieldDetailForm.php index 47266f233..4e1a401bb 100644 --- a/src/Forms/GridField/GridFieldDetailForm.php +++ b/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->getValidatorList()); + $this->setValidator($record->getCompositeValidator()); } return $handler->handleRequest($request); diff --git a/src/ORM/DataExtension.php b/src/ORM/DataExtension.php index 85814447d..3d29e7ea7 100644 --- a/src/ORM/DataExtension.php +++ b/src/ORM/DataExtension.php @@ -5,7 +5,7 @@ namespace SilverStripe\ORM; use SilverStripe\Core\Config\Config; use SilverStripe\Core\Extension; use SilverStripe\Forms\FieldList; -use SilverStripe\Forms\ValidatorList; +use SilverStripe\Forms\CompositeValidator; use SilverStripe\ORM\Queries\SQLSelect; use Exception; @@ -137,11 +137,11 @@ abstract class DataExtension extends Extension /** * This function is used to provide modifications to the Validators used on a DataObject. * - * Caution: Use {@link ValidatorList->addValidator()} to add Validators. + * Caution: Use {@link CompositeValidator->addValidator()} to add Validators. * - * @param ValidatorList $validatorList + * @param CompositeValidator $compositeValidator */ - public function updateValidatorList(ValidatorList $validatorList): void + public function updateCompositeValidator(CompositeValidator $compositeValidator): void { } diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 12c6dc8da..0e7b25d20 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -15,7 +15,7 @@ use SilverStripe\Dev\Deprecation; use SilverStripe\Forms\FieldList; use SilverStripe\Forms\FormField; use SilverStripe\Forms\FormScaffolder; -use SilverStripe\Forms\ValidatorList; +use SilverStripe\Forms\CompositeValidator; use SilverStripe\i18n\i18n; use SilverStripe\i18n\i18nEntityProvider; use SilverStripe\ORM\Connect\MySQLSchemaManager; @@ -2449,25 +2449,26 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } /** - * @return ValidatorList + * @return CompositeValidator */ - public function getValidatorList(): ValidatorList + public function getCompositeValidator(): CompositeValidator { - $validatorList = new ValidatorList(); + $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 getValidatorList()' + 'getCMSValidator() is removed in 5.0 in favour of getCompositeValidator()' ); - $validatorList->addValidator($this->getCMSValidator()); + $compositeValidator->addValidator($this->getCMSValidator()); } - $this->invokeWithExtensions('updateValidatorList', $validatorList); + // Extend validator - forward support, will be supported beyond 5.0.0 + $this->invokeWithExtensions('updateCompositeValidator', $compositeValidator); - return $validatorList; + return $compositeValidator; } /** diff --git a/tests/php/Forms/ValidatorListTest.php b/tests/php/Forms/CompositeValidatorTest.php similarity index 57% rename from tests/php/Forms/ValidatorListTest.php rename to tests/php/Forms/CompositeValidatorTest.php index 839a797b6..c6a03e643 100644 --- a/tests/php/Forms/ValidatorListTest.php +++ b/tests/php/Forms/CompositeValidatorTest.php @@ -8,15 +8,15 @@ 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\Tests\ValidatorTest\TestCompositeValidator; use SilverStripe\Forms\TextField; -use SilverStripe\Forms\ValidatorList; +use SilverStripe\Forms\CompositeValidator; /** * @package framework * @subpackage tests */ -class ValidatorListTest extends SapphireTest +class CompositeValidatorTest extends SapphireTest { /** * Common method for setting up form, since that will always be a dependency for the validator. @@ -38,28 +38,28 @@ class ValidatorListTest extends SapphireTest public function testAddValidator(): void { - $validatorList = new ValidatorList(); - $validatorList->addValidator(new RequiredFields()); - $validatorList->addValidator(new RequiredFields()); + $compositeValidator = new CompositeValidator(); + $compositeValidator->addValidator(new RequiredFields()); + $compositeValidator->addValidator(new RequiredFields()); - $this->assertCount(2, $validatorList->getValidators()); + $this->assertCount(2, $compositeValidator->getValidators()); } public function testSetForm(): void { $form = $this->getForm(); - $validatorList = new TestValidatorList(); + $compositeValidator = new TestCompositeValidator(); $validator = new TestValidator(); - $validatorList->addValidator($validator); + $compositeValidator->addValidator($validator); - $validatorList->setForm($form); + $compositeValidator->setForm($form); - $this->assertNotNull($validatorList->getForm()); - $this->assertCount(1, $validatorList->getValidators()); + $this->assertNotNull($compositeValidator->getForm()); + $this->assertCount(1, $compositeValidator->getValidators()); - foreach ($validatorList->getValidators() as $validator) { + foreach ($compositeValidator->getValidators() as $validator) { /** @var TestValidator $validator */ $this->assertNotNull($validator->getForm()); } @@ -67,46 +67,46 @@ class ValidatorListTest extends SapphireTest public function testGetValidatorsByType(): void { - $validatorList = new ValidatorList(); - $validatorList->addValidator(new RequiredFields()); - $validatorList->addValidator(new TestValidator()); - $validatorList->addValidator(new RequiredFields()); - $validatorList->addValidator(new TestValidator()); + $compositeValidator = new CompositeValidator(); + $compositeValidator->addValidator(new RequiredFields()); + $compositeValidator->addValidator(new TestValidator()); + $compositeValidator->addValidator(new RequiredFields()); + $compositeValidator->addValidator(new TestValidator()); - $this->assertCount(4, $validatorList->getValidators()); - $this->assertCount(2, $validatorList->getValidatorsByType(RequiredFields::class)); + $this->assertCount(4, $compositeValidator->getValidators()); + $this->assertCount(2, $compositeValidator->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()); + $compositeValidator = new CompositeValidator(); + $compositeValidator->addValidator(new RequiredFields()); + $compositeValidator->addValidator(new TestValidator()); + $compositeValidator->addValidator(new RequiredFields()); + $compositeValidator->addValidator(new TestValidator()); - $this->assertCount(4, $validatorList->getValidators()); + $this->assertCount(4, $compositeValidator->getValidators()); - $validatorList->removeValidatorsByType(RequiredFields::class); - $this->assertCount(2, $validatorList->getValidators()); + $compositeValidator->removeValidatorsByType(RequiredFields::class); + $this->assertCount(2, $compositeValidator->getValidators()); } public function testCanBeCached(): void { - $validatorList = new ValidatorList(); - $validatorList->addValidator(new RequiredFields()); + $compositeValidator = new CompositeValidator(); + $compositeValidator->addValidator(new RequiredFields()); - $this->assertTrue($validatorList->canBeCached()); + $this->assertTrue($compositeValidator->canBeCached()); - $validatorList = new ValidatorList(); - $validatorList->addValidator(new RequiredFields(['Foor'])); + $compositeValidator = new CompositeValidator(); + $compositeValidator->addValidator(new RequiredFields(['Foor'])); - $this->assertFalse($validatorList->canBeCached()); + $this->assertFalse($compositeValidator->canBeCached()); } public function testFieldIsRequired(): void { - $validatorList = new ValidatorList(); + $compositeValidator = new CompositeValidator(); $fieldNames = [ 'Title', @@ -124,12 +124,12 @@ class ValidatorListTest extends SapphireTest ] ); - $validatorList->addValidator($requiredFieldsFirst); - $validatorList->addValidator($requiredFieldsSecond); + $compositeValidator->addValidator($requiredFieldsFirst); + $compositeValidator->addValidator($requiredFieldsSecond); foreach ($fieldNames as $field) { $this->assertTrue( - $validatorList->fieldIsRequired($field), + $compositeValidator->fieldIsRequired($field), sprintf('Failed to find "%s" field in required list', $field) ); } @@ -137,10 +137,10 @@ class ValidatorListTest extends SapphireTest public function testValidate(): void { - $validatorList = new ValidatorList(); + $compositeValidator = new CompositeValidator(); // Add two separate validators, each with one required field - $validatorList->addValidator(new RequiredFields(['Foo'])); - $validatorList->addValidator(new RequiredFields(['Bar'])); + $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 @@ -153,7 +153,7 @@ class ValidatorListTest extends SapphireTest $form = $this->getForm(array_keys($data)); $form->disableSecurityToken(); // Setup validator now that we've got our form - $form->setValidator($validatorList); + $form->setValidator($compositeValidator); // Put data into the form so the validator can pull it back out again $form->loadDataFrom($data); @@ -164,8 +164,8 @@ class ValidatorListTest extends SapphireTest public function testRemoveValidation(): void { - $validatorList = new ValidatorList(); - $validatorList->addValidator(new TestValidator()); + $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 = [ @@ -175,7 +175,7 @@ class ValidatorListTest extends SapphireTest $form = $this->getForm(array_keys($data)); $form->disableSecurityToken(); // Setup validator now that we've got our form - $form->setValidator($validatorList); + $form->setValidator($compositeValidator); // Put data into the form so the validator can pull it back out again $form->loadDataFrom($data); @@ -185,7 +185,7 @@ class ValidatorListTest extends SapphireTest // Make sure it doesn't fail after removing validation AND has no errors (since calling validate should reset // errors) - $validatorList->removeValidation(); + $compositeValidator->removeValidation(); $result = $form->validationResult(); $this->assertTrue($result->isValid()); $this->assertEmpty($result->getMessages()); diff --git a/tests/php/Forms/ValidatorTest/TestValidatorList.php b/tests/php/Forms/ValidatorTest/TestCompositeValidator.php similarity index 70% rename from tests/php/Forms/ValidatorTest/TestValidatorList.php rename to tests/php/Forms/ValidatorTest/TestCompositeValidator.php index 7cb91e046..44a92a0df 100644 --- a/tests/php/Forms/ValidatorTest/TestValidatorList.php +++ b/tests/php/Forms/ValidatorTest/TestCompositeValidator.php @@ -4,14 +4,14 @@ namespace SilverStripe\Forms\Tests\ValidatorTest; use SilverStripe\Dev\TestOnly; use SilverStripe\Forms\Form; -use SilverStripe\Forms\ValidatorList; +use SilverStripe\Forms\CompositeValidator; /** - * Class TestValidatorList + * Class TestCompositeValidator * * @package SilverStripe\Forms\Tests\ValidatorTest */ -class TestValidatorList extends ValidatorList implements TestOnly +class TestCompositeValidator extends CompositeValidator implements TestOnly { /** * Allow us to access the form for test purposes. From 2765b65f42f6e7c4aeaa0183dbf399f41418bad2 Mon Sep 17 00:00:00 2001 From: cpenny Date: Thu, 28 May 2020 09:52:28 +1200 Subject: [PATCH 07/10] Use ReflectionClass for CompositeValidator tests --- tests/php/Forms/CompositeValidatorTest.php | 16 +++++++++--- .../ValidatorTest/TestCompositeValidator.php | 25 ------------------- 2 files changed, 12 insertions(+), 29 deletions(-) delete mode 100644 tests/php/Forms/ValidatorTest/TestCompositeValidator.php diff --git a/tests/php/Forms/CompositeValidatorTest.php b/tests/php/Forms/CompositeValidatorTest.php index c6a03e643..296958ef8 100644 --- a/tests/php/Forms/CompositeValidatorTest.php +++ b/tests/php/Forms/CompositeValidatorTest.php @@ -2,13 +2,14 @@ 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\Tests\ValidatorTest\TestCompositeValidator; use SilverStripe\Forms\TextField; use SilverStripe\Forms\CompositeValidator; @@ -45,23 +46,30 @@ class CompositeValidatorTest extends SapphireTest $this->assertCount(2, $compositeValidator->getValidators()); } + /** + * @throws ReflectionException + */ public function testSetForm(): void { $form = $this->getForm(); - $compositeValidator = new TestCompositeValidator(); + $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($compositeValidator->getForm()); + $this->assertNotNull($property->getValue($compositeValidator)); $this->assertCount(1, $compositeValidator->getValidators()); foreach ($compositeValidator->getValidators() as $validator) { /** @var TestValidator $validator */ - $this->assertNotNull($validator->getForm()); + $this->assertNotNull($property->getValue($validator)); } } diff --git a/tests/php/Forms/ValidatorTest/TestCompositeValidator.php b/tests/php/Forms/ValidatorTest/TestCompositeValidator.php deleted file mode 100644 index 44a92a0df..000000000 --- a/tests/php/Forms/ValidatorTest/TestCompositeValidator.php +++ /dev/null @@ -1,25 +0,0 @@ -form; - } -} From 8ba65313e9296ce53237808c6fa9c54734210cd7 Mon Sep 17 00:00:00 2001 From: cpenny Date: Thu, 28 May 2020 10:50:56 +1200 Subject: [PATCH 08/10] Add internal note for protected method --- src/Forms/CompositeValidator.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Forms/CompositeValidator.php b/src/Forms/CompositeValidator.php index f08dd9491..89f16e699 100644 --- a/src/Forms/CompositeValidator.php +++ b/src/Forms/CompositeValidator.php @@ -221,6 +221,7 @@ class CompositeValidator extends Validator } /** + * @internal This method may be updated to public in the future. Let us know if you feel there is a use case for it. * @param int $key * @return CompositeValidator */ From d4165db690ddb3270478f7930efc33653b364baa Mon Sep 17 00:00:00 2001 From: cpenny Date: Thu, 28 May 2020 12:23:35 +1200 Subject: [PATCH 09/10] Update getter name to getCMSCompositeValidator --- .../02_Developer_Guides/03_Forms/01_Validation.md | 2 +- src/Forms/CompositeValidator.php | 13 +++++++------ src/Forms/DefaultFormFactory.php | 4 ++-- src/Forms/GridField/GridFieldDetailForm.php | 2 +- src/ORM/DataExtension.php | 2 +- src/ORM/DataObject.php | 14 +++++++++++--- tests/php/Forms/CompositeValidatorTest.php | 4 ++-- 7 files changed, 25 insertions(+), 16 deletions(-) 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 191ddd2a7..f9e25e939 100644 --- a/docs/en/02_Developer_Guides/03_Forms/01_Validation.md +++ b/docs/en/02_Developer_Guides/03_Forms/01_Validation.md @@ -275,7 +275,7 @@ class MyController extends Controller 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/s through the -`getCompositeValidator()` method. The CMS interfaces such as [LeftAndMain](api:SilverStripe\Admin\LeftAndMain), +`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. diff --git a/src/Forms/CompositeValidator.php b/src/Forms/CompositeValidator.php index 89f16e699..61ead41c9 100644 --- a/src/Forms/CompositeValidator.php +++ b/src/Forms/CompositeValidator.php @@ -12,20 +12,21 @@ use SilverStripe\ORM\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 getCompositeValidator() method: + * You can add Validators to the CompositeValidator in any DataObject by extending the getCMSCompositeValidator() + * method: * - * public function getCompositeValidator(): CompositeValidator + * public function getCMSCompositeValidator(): CompositeValidator * { - * $compositeValidator = parent::getCompositeValidator(); + * $compositeValidator = parent::getCMSCompositeValidator(); * * $compositeValidator->addValidator(RequiredFields::create(['MyRequiredField'])); * * return $compositeValidator * } * - * Or by implementing the updateCompositeValidator() method in a DataExtension: + * Or by implementing the updateCMSCompositeValidator() method in a DataExtension: * - * public function updateCompositeValidator(CompositeValidator $compositeValidator): void + * public function updateCMSCompositeValidator(CompositeValidator $compositeValidator): void * { * $compositeValidator->addValidator(RequiredFields::create(['AdditionalContent'])); * } @@ -221,7 +222,7 @@ class CompositeValidator extends Validator } /** - * @internal This method may be updated to public in the future. Let us know if you feel there is a use case for it. + * @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 */ diff --git a/src/Forms/DefaultFormFactory.php b/src/Forms/DefaultFormFactory.php index 9eea865ee..669f1cdeb 100644 --- a/src/Forms/DefaultFormFactory.php +++ b/src/Forms/DefaultFormFactory.php @@ -101,7 +101,7 @@ class DefaultFormFactory implements FormFactory return null; } - $compositeValidator = $context['Record']->getCompositeValidator(); + $compositeValidator = $context['Record']->getCMSCompositeValidator(); // Extend validator - legacy support, will be removed in 5.0.0 foreach ($compositeValidator->getValidators() as $validator) { @@ -109,7 +109,7 @@ class DefaultFormFactory implements FormFactory } // Extend validator - forward support, will be supported beyond 5.0.0 - $this->invokeWithExtensions('updateCompositeValidator', $compositeValidator); + $this->invokeWithExtensions('updateCMSCompositeValidator', $compositeValidator); return $compositeValidator; } diff --git a/src/Forms/GridField/GridFieldDetailForm.php b/src/Forms/GridField/GridFieldDetailForm.php index 4e1a401bb..f9a76d209 100644 --- a/src/Forms/GridField/GridFieldDetailForm.php +++ b/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->getCompositeValidator()); + $this->setValidator($record->getCMSCompositeValidator()); } return $handler->handleRequest($request); diff --git a/src/ORM/DataExtension.php b/src/ORM/DataExtension.php index 3d29e7ea7..9617dced2 100644 --- a/src/ORM/DataExtension.php +++ b/src/ORM/DataExtension.php @@ -141,7 +141,7 @@ abstract class DataExtension extends Extension * * @param CompositeValidator $compositeValidator */ - public function updateCompositeValidator(CompositeValidator $compositeValidator): void + public function updateCMSCompositeValidator(CompositeValidator $compositeValidator): void { } diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 0e7b25d20..3f80a3164 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -2449,9 +2449,17 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } /** + * 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 getCompositeValidator(): CompositeValidator + public function getCMSCompositeValidator(): CompositeValidator { $compositeValidator = new CompositeValidator(); @@ -2459,14 +2467,14 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity if ($this->hasMethod('getCMSValidator')) { Deprecation::notice( '4.6', - 'getCMSValidator() is removed in 5.0 in favour of getCompositeValidator()' + '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('updateCompositeValidator', $compositeValidator); + $this->invokeWithExtensions('updateCMSCompositeValidator', $compositeValidator); return $compositeValidator; } diff --git a/tests/php/Forms/CompositeValidatorTest.php b/tests/php/Forms/CompositeValidatorTest.php index 296958ef8..306bd4b78 100644 --- a/tests/php/Forms/CompositeValidatorTest.php +++ b/tests/php/Forms/CompositeValidatorTest.php @@ -191,8 +191,8 @@ class CompositeValidatorTest extends SapphireTest $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) + // 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()); From f72491f7f4806998fb790dde5712633ef62b85f8 Mon Sep 17 00:00:00 2001 From: cpenny Date: Mon, 8 Jun 2020 09:35:00 +1200 Subject: [PATCH 10/10] Linting fix --- tests/php/Forms/CompositeValidatorTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/Forms/CompositeValidatorTest.php b/tests/php/Forms/CompositeValidatorTest.php index 306bd4b78..43bba0098 100644 --- a/tests/php/Forms/CompositeValidatorTest.php +++ b/tests/php/Forms/CompositeValidatorTest.php @@ -25,7 +25,7 @@ class CompositeValidatorTest extends SapphireTest * @param array $fieldNames * @return Form */ - protected function getForm(array $fieldNames = array()): Form + protected function getForm(array $fieldNames = []): Form { // Setup field list now. We're only worried about names right now $fieldList = new FieldList();