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; + } +}