From 97f7be502fa48ad27e27fdd5dc7b34e1e1dbb914 Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Fri, 4 Nov 2022 11:59:52 +0000 Subject: [PATCH] NEW: Add extension hook for field-specific validation --- src/Forms/CompositeField.php | 2 +- src/Forms/ConfirmedPasswordField.php | 18 +-- src/Forms/CurrencyField.php | 6 +- src/Forms/DateField.php | 10 +- src/Forms/DatetimeField.php | 10 +- src/Forms/EmailField.php | 5 +- src/Forms/FileField.php | 7 +- src/Forms/FormField.php | 17 ++- src/Forms/LookupField.php | 2 +- src/Forms/MoneyField.php | 5 +- src/Forms/MultiSelectField.php | 28 ++-- src/Forms/NumericField.php | 23 ++-- src/Forms/OptionsetField.php | 2 +- src/Forms/SingleLookupField.php | 2 +- src/Forms/SingleSelectField.php | 6 +- src/Forms/TextField.php | 5 +- src/Forms/TimeField.php | 9 +- tests/php/Forms/FormFieldTest.php | 120 ++++++++++++++++++ .../FieldValidationExtension.php | 38 ++++++ 19 files changed, 248 insertions(+), 67 deletions(-) create mode 100644 tests/php/Forms/FormFieldTest/FieldValidationExtension.php diff --git a/src/Forms/CompositeField.php b/src/Forms/CompositeField.php index b59a5f11804..0e2c4b35a14 100644 --- a/src/Forms/CompositeField.php +++ b/src/Forms/CompositeField.php @@ -545,6 +545,6 @@ public function validate($validator) /** @var FormField $child */ $valid = ($child && $child->validate($validator) && $valid); } - return $valid; + return $this->extendValidationResult($valid, $validator); } } diff --git a/src/Forms/ConfirmedPasswordField.php b/src/Forms/ConfirmedPasswordField.php index 479c89d597a..ec5fabe22fe 100644 --- a/src/Forms/ConfirmedPasswordField.php +++ b/src/Forms/ConfirmedPasswordField.php @@ -416,7 +416,7 @@ public function validate($validator) // if field isn't visible, don't validate if (!$this->isSaveable()) { - return true; + return $this->extendValidationResult(true, $validator); } $this->getPasswordField()->setValue($this->value); @@ -431,7 +431,7 @@ public function validate($validator) "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } if (!$this->canBeEmpty) { @@ -443,7 +443,7 @@ public function validate($validator) "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -483,7 +483,7 @@ public function validate($validator) "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -498,7 +498,7 @@ public function validate($validator) "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -513,7 +513,7 @@ public function validate($validator) ), "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } // Check this password is valid for the current user @@ -527,7 +527,7 @@ public function validate($validator) ), "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } // With a valid user and password, check the password is correct @@ -543,12 +543,12 @@ public function validate($validator) ), "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } } } - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/CurrencyField.php b/src/Forms/CurrencyField.php index f3358d861c2..c340fce7bf8 100644 --- a/src/Forms/CurrencyField.php +++ b/src/Forms/CurrencyField.php @@ -58,6 +58,7 @@ public function performReadonlyTransformation() public function validate($validator) { + $result = true; $currencySymbol = preg_quote(DBCurrency::config()->uninherited('currency_symbol') ?? ''); $regex = '/^\s*(\-?' . $currencySymbol . '?|' . $currencySymbol . '\-?)?(\d{1,3}(\,\d{3})*|(\d+))(\.\d{2})?\s*$/'; if (!empty($this->value) && !preg_match($regex ?? '', $this->value ?? '')) { @@ -66,9 +67,10 @@ public function validate($validator) _t('SilverStripe\\Forms\\Form.VALIDCURRENCY', "Please enter a valid currency"), "validation" ); - return false; + $result = false; } - return true; + + return $this->extendValidationResult($result, $validator); } public function getSchemaValidation() diff --git a/src/Forms/DateField.php b/src/Forms/DateField.php index acb6d536c89..880fa0827aa 100644 --- a/src/Forms/DateField.php +++ b/src/Forms/DateField.php @@ -369,7 +369,7 @@ public function validate($validator) { // Don't validate empty fields if (empty($this->rawValue)) { - return true; + return $this->extendValidationResult(true, $validator); } // We submitted a value, but it couldn't be parsed @@ -382,7 +382,7 @@ public function validate($validator) ['format' => $this->getDateFormat()] ) ); - return false; + return $this->extendValidationResult(false, $validator); } // Check min date @@ -406,7 +406,7 @@ public function validate($validator) ValidationResult::TYPE_ERROR, ValidationResult::CAST_HTML ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -431,11 +431,11 @@ public function validate($validator) ValidationResult::TYPE_ERROR, ValidationResult::CAST_HTML ); - return false; + return $this->extendValidationResult(false, $validator); } } - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/DatetimeField.php b/src/Forms/DatetimeField.php index a455803d87f..849a0ecb834 100644 --- a/src/Forms/DatetimeField.php +++ b/src/Forms/DatetimeField.php @@ -565,7 +565,7 @@ public function validate($validator) { // Don't validate empty fields if (empty($this->rawValue)) { - return true; + return $this->extendValidationResult(true, $validator); } // We submitted a value, but it couldn't be parsed @@ -578,7 +578,7 @@ public function validate($validator) ['format' => $this->getDatetimeFormat()] ) ); - return false; + return $this->extendValidationResult(false, $validator); } // Check min date (in server timezone) @@ -602,7 +602,7 @@ public function validate($validator) ValidationResult::TYPE_ERROR, ValidationResult::CAST_HTML ); - return false; + return $this->extendValidationResult(false, $validator); } } @@ -627,11 +627,11 @@ public function validate($validator) ValidationResult::TYPE_ERROR, ValidationResult::CAST_HTML ); - return false; + return $this->extendValidationResult(false, $validator); } } - return true; + return $this->extendValidationResult(true, $validator); } public function performReadonlyTransformation() diff --git a/src/Forms/EmailField.php b/src/Forms/EmailField.php index 53517370468..68ea66d4199 100644 --- a/src/Forms/EmailField.php +++ b/src/Forms/EmailField.php @@ -29,6 +29,7 @@ public function Type() */ public function validate($validator) { + $result = true; $this->value = trim($this->value ?? ''); $pattern = '^[a-z0-9!#$%&\'*+/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&\'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$'; @@ -43,10 +44,10 @@ public function validate($validator) 'validation' ); - return false; + $result = false; } - return true; + return $this->extendValidationResult($result, $validator); } public function getSchemaValidation() diff --git a/src/Forms/FileField.php b/src/Forms/FileField.php index 7d087c67f44..66b0fe51f15 100644 --- a/src/Forms/FileField.php +++ b/src/Forms/FileField.php @@ -187,7 +187,7 @@ public function validate($validator) $fieldName = preg_replace('#\[(.*?)\]$#', '', $this->name ?? ''); if (!isset($_FILES[$fieldName])) { - return true; + return $this->extendValidationResult(true, $validator); } if ($isMultiFileUpload) { @@ -204,11 +204,12 @@ public function validate($validator) $isValid = false; } } - return $isValid; + return $this->extendValidationResult($isValid, $validator); } // regular single-file upload - return $this->validateFileData($validator, $_FILES[$this->name]); + $result = $this->validateFileData($validator, $_FILES[$this->name]); + return $this->extendValidationResult($result, $validator); } /** diff --git a/src/Forms/FormField.php b/src/Forms/FormField.php index 8574185ec93..763d388b804 100644 --- a/src/Forms/FormField.php +++ b/src/Forms/FormField.php @@ -1219,18 +1219,29 @@ public function Type() return strtolower(preg_replace('/Field$/', '', $type->getShortName() ?? '') ?? ''); } + /** + * Utility method to call an extension hook which allows the result of validate() calls to be adjusted + * + * @param bool $result + * @param Validator $validator + * @return bool + */ + protected function extendValidationResult(bool $result, Validator $validator): bool + { + $this->extend('updateValidationResult', $result, $validator); + return $result; + } + /** * Abstract method each {@link FormField} subclass must implement, determines whether the field * is valid or not based on the value. * - * @todo Make this abstract. - * * @param Validator $validator * @return bool */ public function validate($validator) { - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/LookupField.php b/src/Forms/LookupField.php index 6a5c31ba583..113ba553814 100644 --- a/src/Forms/LookupField.php +++ b/src/Forms/LookupField.php @@ -73,7 +73,7 @@ public function Field($properties = []) */ public function validate($validator) { - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/MoneyField.php b/src/Forms/MoneyField.php index f6da4d8dc4c..6f1c231a847 100644 --- a/src/Forms/MoneyField.php +++ b/src/Forms/MoneyField.php @@ -334,11 +334,12 @@ public function validate($validator) ['currency' => $currency] ) ); - return false; + return $this->extendValidationResult(false, $validator); } // Field-specific validation - return $this->fieldAmount->validate($validator) && $this->fieldCurrency->validate($validator); + $result = $this->fieldAmount->validate($validator) && $this->fieldCurrency->validate($validator); + return $this->extendValidationResult($result, $validator); } public function setForm($form) diff --git a/src/Forms/MultiSelectField.php b/src/Forms/MultiSelectField.php index 4c3d1e8ddf2..7fe0a9b5999 100644 --- a/src/Forms/MultiSelectField.php +++ b/src/Forms/MultiSelectField.php @@ -247,21 +247,23 @@ function ($userValue) use ($self, $validValues) { return true; } ); - if (empty($invalidValues)) { - return true; + + $result = true; + if (!empty($invalidValues)) { + $result = false; + // List invalid items + $validator->validationError( + $this->getName(), + _t( + 'SilverStripe\\Forms\\MultiSelectField.SOURCE_VALIDATION', + "Please select values within the list provided. Invalid option(s) {value} given", + ['value' => implode(',', $invalidValues)] + ), + "validation" + ); } - // List invalid items - $validator->validationError( - $this->getName(), - _t( - 'SilverStripe\\Forms\\MultiSelectField.SOURCE_VALIDATION', - "Please select values within the list provided. Invalid option(s) {value} given", - ['value' => implode(',', $invalidValues)] - ), - "validation" - ); - return false; + return $this->extendValidationResult($result, $validator); } /** diff --git a/src/Forms/NumericField.php b/src/Forms/NumericField.php index 4c05caa463e..776ea381f0c 100644 --- a/src/Forms/NumericField.php +++ b/src/Forms/NumericField.php @@ -191,20 +191,21 @@ public function getAttributes() */ public function validate($validator) { + $result = true; // false signifies invalid value due to failed parse() - if ($this->value !== false) { - return true; + if ($this->value === false) { + $validator->validationError( + $this->name, + _t( + 'SilverStripe\\Forms\\NumericField.VALIDATION', + "'{value}' is not a number, only numbers can be accepted for this field", + ['value' => $this->originalValue] + ) + ); + $result = false; } - $validator->validationError( - $this->name, - _t( - 'SilverStripe\\Forms\\NumericField.VALIDATION', - "'{value}' is not a number, only numbers can be accepted for this field", - ['value' => $this->originalValue] - ) - ); - return false; + return $this->extendValidationResult($result, $validator); } public function getSchemaValidation() diff --git a/src/Forms/OptionsetField.php b/src/Forms/OptionsetField.php index f07d6b08e4c..f47d6dd576e 100644 --- a/src/Forms/OptionsetField.php +++ b/src/Forms/OptionsetField.php @@ -140,7 +140,7 @@ public function Field($properties = []) public function validate($validator) { if (!$this->Value()) { - return true; + return $this->extendValidationResult(true, $validator); } return parent::validate($validator); diff --git a/src/Forms/SingleLookupField.php b/src/Forms/SingleLookupField.php index f0ce89c3b9a..9f3e8883a1f 100644 --- a/src/Forms/SingleLookupField.php +++ b/src/Forms/SingleLookupField.php @@ -44,7 +44,7 @@ protected function valueToLabel() */ public function validate($validator) { - return true; + return $this->extendValidationResult(true, $validator); } /** diff --git a/src/Forms/SingleSelectField.php b/src/Forms/SingleSelectField.php index 8f847cb59e9..2230cd807c8 100644 --- a/src/Forms/SingleSelectField.php +++ b/src/Forms/SingleSelectField.php @@ -137,13 +137,13 @@ public function validate($validator) // Use selection rules to check which are valid foreach ($validValues as $formValue) { if ($this->isSelectedValue($formValue, $selected)) { - return true; + return $this->extendValidationResult(true, $validator); } } } else { if ($this->getHasEmptyDefault() || !$validValues || in_array('', $validValues ?? [])) { // Check empty value - return true; + return $this->extendValidationResult(true, $validator); } $selected = '(none)'; } @@ -158,7 +158,7 @@ public function validate($validator) ), "validation" ); - return false; + return $this->extendValidationResult(false, $validator); } public function castedCopy($classOrCopy) diff --git a/src/Forms/TextField.php b/src/Forms/TextField.php index 7988f61966c..4247afa6b67 100644 --- a/src/Forms/TextField.php +++ b/src/Forms/TextField.php @@ -125,6 +125,7 @@ public function getSchemaDataDefaults() */ public function validate($validator) { + $result = true; if (!is_null($this->maxLength) && mb_strlen($this->value ?? '') > $this->maxLength) { $name = strip_tags($this->Title() ? $this->Title() : $this->getName()); $validator->validationError( @@ -136,9 +137,9 @@ public function validate($validator) ), "validation" ); - return false; + $result = false; } - return true; + return $this->extendValidationResult($result, $validator); } public function getSchemaValidation() diff --git a/src/Forms/TimeField.php b/src/Forms/TimeField.php index 43a796e1a74..525f6dfc40b 100644 --- a/src/Forms/TimeField.php +++ b/src/Forms/TimeField.php @@ -324,9 +324,10 @@ public function validate($validator) { // Don't validate empty fields if (empty($this->rawValue)) { - return true; + return $this->extendValidationResult(true, $validator); } + $result = true; // We submitted a value, but it couldn't be parsed if (empty($this->value)) { $validator->validationError( @@ -337,9 +338,11 @@ public function validate($validator) ['format' => $this->getTimeFormat()] ) ); - return false; + $result = false; } - return true; + + $this->extendValidationResult($result, $validator); + return $result; } /** diff --git a/tests/php/Forms/FormFieldTest.php b/tests/php/Forms/FormFieldTest.php index 47a3ff94614..026a588377d 100644 --- a/tests/php/Forms/FormFieldTest.php +++ b/tests/php/Forms/FormFieldTest.php @@ -2,21 +2,40 @@ namespace SilverStripe\Forms\Tests; +use Exception; use LogicException; use ReflectionClass; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Config; use SilverStripe\Dev\SapphireTest; +use SilverStripe\Dev\TestOnly; use SilverStripe\Forms\CompositeField; +use SilverStripe\Forms\FieldGroup; use SilverStripe\Forms\FieldList; use SilverStripe\Forms\Form; use SilverStripe\Forms\FormField; +use SilverStripe\Forms\GridField\GridField; +use SilverStripe\Forms\GridField\GridField_FormAction; +use SilverStripe\Forms\GridField\GridState; use SilverStripe\Forms\NullableField; +use SilverStripe\Forms\PopoverField; +use SilverStripe\Forms\PrintableTransformation_TabSet; use SilverStripe\Forms\RequiredFields; +use SilverStripe\Forms\SelectionGroup; +use SilverStripe\Forms\SelectionGroup_Item; +use SilverStripe\Forms\Tab; +use SilverStripe\Forms\Tests\FormFieldTest\FieldValidationExtension; use SilverStripe\Forms\Tests\FormFieldTest\TestExtension; use SilverStripe\Forms\TextField; use SilverStripe\Forms\Tip; +use SilverStripe\Forms\ToggleCompositeField; +use SilverStripe\Forms\TreeDropdownField; +use SilverStripe\Forms\TreeDropdownField_Readonly; use SilverStripe\ORM\ValidationResult; +use SilverStripe\Security\Group; +use SilverStripe\Security\Permission; +use SilverStripe\Security\PermissionCheckboxSetField; +use SilverStripe\Security\PermissionCheckboxSetField_Readonly; class FormFieldTest extends SapphireTest { @@ -24,6 +43,7 @@ class FormFieldTest extends SapphireTest protected static $required_extensions = [ FormField::class => [ TestExtension::class, + FieldValidationExtension::class, ], ]; @@ -455,6 +475,106 @@ public function testGetSchemaStateWithFormValidation() ); } + public function testValidationExtensionHooks() + { + /** @var TextField|FieldValidationExtension $field */ + $field = new TextField('Test'); + $field->setMaxLength(5); + $field->setValue('IAmLongerThan5Characters'); + $result = $field->validate(new RequiredFields('Test')); + $this->assertFalse($result); + + // Call extension method in FieldValidationExtension + $field->setExcludeFromValidation(true); + $result = $field->validate(new RequiredFields('Test')); + $this->assertTrue($result); + + // Call extension methods in FieldValidationExtension + $field->setValue('1234'); + $field->setExcludeFromValidation(false); + $field->setTriggerTestValidationError(true); + + // Ensure messages set via updateValidationResult() propagate through to form fields after validation + $form = new Form(null, 'TestForm', new FieldList($field), new FieldList(), new RequiredFields()); + $form->validationResult(); + $schema = $field->getSchemaState(); + $this->assertEquals( + 'A test error message', + $schema['message']['value'] + ); + } + + public function testValidationExtensionHooksAreCalledOnFormFieldSubclasses() + { + // Can't use a dataProvider for this as dataProviders are fetched very early by phpunit, + // and the ClassManifest isn't ready then + $formFieldClasses = ClassInfo::subclassesFor(FormField::class, false); + foreach ($formFieldClasses as $formFieldClass) { + $reflection = new ReflectionClass($formFieldClass); + // Skip abstract classes, like MultiSelectField, and fields that only exist for unit tests + if ($reflection->isAbstract() || is_a($formFieldClass, TestOnly::class, true)) { + continue; + } + + switch ($formFieldClass) { + case NullableField::class: + case CompositeField::class: + case FieldGroup::class: + case PopoverField::class: + $args = [TextField::create('Test2')]; + break; + case SelectionGroup_Item::class: + $args = ['Test', [TextField::create('Test2')]]; + break; + case ToggleCompositeField::class: + $args = ['Test', 'Test', TextField::create('Test2')]; + break; + case PrintableTransformation_TabSet::class: + $args = [Tab::create('TestTab', 'Testtab', TextField::create('Test2'))]; + break; + case TreeDropdownField::class: + case TreeDropdownField_Readonly::class: + $args = ['Test', 'Test', Group::class]; + break; + case PermissionCheckboxSetField::class: + case PermissionCheckboxSetField_Readonly::class: + $args = ['Test', 'Test', Permission::class, 'Test']; + break; + case SelectionGroup::class: + $args = ['Test', []]; + break; + case GridField_FormAction::class: + $args = [GridField::create('GF'), 'Test', 'Test label', 'Test action name', []]; + break; + case GridState::class: + $args = [GridField::create('GF')]; + break; + default: + $args = ['Test', 'Test label']; + } + + // Assert that extendValidationResult is called once each time ->validate() is called + $mock = $this->getMockBuilder($formFieldClass) + ->setConstructorArgs($args) + ->onlyMethods(['extendValidationResult']) + ->getMock(); + $mock->expects($invocationRule = $this->once()) + ->method('extendValidationResult') + ->will($this->returnValue(true)); + + $isValid = $mock->validate(new RequiredFields()); + $this->assertTrue($isValid, "$formFieldClass should be valid"); + + // This block is not essential and only exists to make test debugging easier - without this, + // the error message on failure is generic and doesn't include the class name that failed + try { + $invocationRule->verify(); + } catch (Exception $e) { + $this->fail("Expectation failed for '$formFieldClass' class: {$e->getMessage()}"); + } + } + } + public function testHasClass() { $field = new FormField('Test'); diff --git a/tests/php/Forms/FormFieldTest/FieldValidationExtension.php b/tests/php/Forms/FormFieldTest/FieldValidationExtension.php new file mode 100644 index 00000000000..8376dc4afcf --- /dev/null +++ b/tests/php/Forms/FormFieldTest/FieldValidationExtension.php @@ -0,0 +1,38 @@ +excludeFromValidation) { + $result = true; + return; + } + + if ($this->triggerTestValidationError) { + $result = false; + $validator->validationError($this->owner->getName(), 'A test error message'); + return; + } + } + + public function setExcludeFromValidation(bool $exclude) + { + $this->excludeFromValidation = $exclude; + } + + public function setTriggerTestValidationError(bool $triggerTestValidationError) + { + $this->triggerTestValidationError = $triggerTestValidationError; + } +}