From dd529563f7ea2385e9c71dbe7dd2bb9777e27061 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Fri, 17 Jan 2025 17:15:14 +1300 Subject: [PATCH] NEW Show more information in ValidationException message --- src/Core/Validation/ConstraintValidator.php | 3 +- src/Core/Validation/ValidationException.php | 31 +++ src/Core/Validation/ValidationResult.php | 212 ++++++++++++------ src/Forms/Form.php | 47 ++-- src/Forms/Validation/Validator.php | 2 +- src/ORM/DataObject.php | 2 +- src/Security/Member.php | 8 +- .../Validation/ConstraintValidatorTest.php | 2 +- .../Validation/ValidationExceptionTest.php | 179 ++++++++++++++- .../ValidationExceptionTest/TestObject.php | 15 ++ .../Core/Validation/ValidationResultTest.php | 86 +++++-- 11 files changed, 459 insertions(+), 128 deletions(-) create mode 100644 tests/php/Core/Validation/ValidationExceptionTest/TestObject.php diff --git a/src/Core/Validation/ConstraintValidator.php b/src/Core/Validation/ConstraintValidator.php index 9a65689986d..f1ec0ac5e57 100644 --- a/src/Core/Validation/ConstraintValidator.php +++ b/src/Core/Validation/ConstraintValidator.php @@ -17,8 +17,9 @@ class ConstraintValidator /** * Validate a value by a constraint * + * @param $value The value to validate * @param Constraint|Constraint[] $constraints a constraint or array of constraints to validate against - * @param string $fieldName The field name the value relates to, if relevant + * @param $fieldName The field name the value relates to, if relevant */ public static function validate(mixed $value, Constraint|array $constraints, string $fieldName = ''): ValidationResult { diff --git a/src/Core/Validation/ValidationException.php b/src/Core/Validation/ValidationException.php index 155da158819..f7f2092331c 100644 --- a/src/Core/Validation/ValidationException.php +++ b/src/Core/Validation/ValidationException.php @@ -5,6 +5,10 @@ use Exception; use InvalidArgumentException; use SilverStripe\Core\Injector\Injectable; +use SilverStripe\Control\Director; +use SilverStripe\Dev\DevelopmentAdmin; +use SilverStripe\ORM\DataObject; +use SilverStripe\Control\Controller; /** * Exception thrown by {@link DataObject}::write if validation fails. By throwing an @@ -48,6 +52,16 @@ public function __construct($result = null, $code = 0) // Pick first message foreach ($result->getMessages() as $message) { $exceptionMessage = $message['message']; + if ($this->doShowAdditionalInfo()) { + if ($message['fieldName']) { + $exceptionMessage .= ' - fieldName: ' . $message['fieldName']; + } + $dataClass = $result->getDataClass(); + if (is_subclass_of($dataClass, DataObject::class, true)) { + $exceptionMessage .= ', recordID: ' . $result->getRecordID(); + $exceptionMessage .= ', dataClass: ' . $dataClass; + } + } break; } } elseif (is_string($result)) { @@ -71,4 +85,21 @@ public function getResult() { return $this->result; } + + /** + * Whether to show additional information in the error message depending on the context + * + * We do not want this to show this in a context where it's easy to know the record and value that + * triggered the error e.g. form submission, API calls, etc + */ + private function doShowAdditionalInfo() + { + if (Director::is_cli()) { + return true; + } + // e.g. dev/build + if (is_a(Controller::curr(), DevelopmentAdmin::class)) { + return true; + } + } } diff --git a/src/Core/Validation/ValidationResult.php b/src/Core/Validation/ValidationResult.php index 552dc391bd4..b06aceb0c46 100644 --- a/src/Core/Validation/ValidationResult.php +++ b/src/Core/Validation/ValidationResult.php @@ -3,6 +3,7 @@ namespace SilverStripe\Core\Validation; use InvalidArgumentException; +use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Injector\Injectable; /** @@ -15,6 +16,7 @@ class ValidationResult { use Injectable; + use Configurable; /** * Standard "error" type @@ -49,130 +51,175 @@ class ValidationResult /** * Is the result valid or not. * Note that there can be non-error messages in the list. - * - * @var bool */ - protected $isValid = true; + protected bool $isValid = true; /** * List of messages - * - * @var array */ - protected $messages = []; + protected array $messages = []; + + /** + * The class of the DataObject being validated. + */ + private string $dataClass = ''; + + /** + * The record ID of the object being validated. + */ + private int $recordID = 0; + + public function __construct(string $dataClass = '', int $recordID = 0) + { + $this->dataClass = $dataClass; + $this->recordID = $recordID; + } + + public function getDataClass(): string + { + return $this->dataClass; + } + + public function setDataClass(string $dataClass): static + { + $this->dataClass = $dataClass; + return $this; + } + + public function getRecordID(): int + { + return $this->recordID; + } + + public function setRecordID(int $recordID): static + { + $this->recordID = $recordID; + return $this; + } /** * Record an error against this validation result, * - * @param string $message The message string. - * @param string $messageType Passed as a CSS class to the form, so other values can be used if desired. - * Standard types are defined by the TYPE_ constant definitions. - * @param string $code A codename for this error. Only one message per codename will be added. - * This can be usedful for ensuring no duplicate messages - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $message The message string. + * @param $messageType The type of message: e.g. "bad", "warning", "good", or "required" + * Passed as a CSS class to the form, so other values can be used if desired + * @param $code A codename for this error. Only one message per codename will be added. + * This can be usedful for ensuring no duplicate messages + * @param $cast Cast type; One of the CAST_ constant definitions. * @return $this */ public function addError( - $message, - $messageType = ValidationResult::TYPE_ERROR, - $code = null, - $cast = ValidationResult::CAST_TEXT, - ) { - return $this->addFieldError(null, $message, $messageType, $code, $cast); + string $message, + string $messageType = ValidationResult::TYPE_ERROR, + string $code = '', + string $cast = ValidationResult::CAST_TEXT, + ): static { + return $this->addFieldError( + '', + $message, + $messageType, + $code, + $cast, + ); } /** * Record an error against this validation result, * - * @param string $fieldName The field to link the message to. If omitted; a form-wide message is assumed. - * @param string $message The message string. - * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS - * class to the form, so other values can be used if desired. - * @param string $code A codename for this error. Only one message per codename will be added. - * This can be usedful for ensuring no duplicate messages - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $fieldName The field to link the message to. If omitted; a form-wide message is assumed. + * @param $message The message string. + * @param $messageType The type of message: e.g. "bad", "warning", "good", or "required" + * Passed as a CSS class to the form, so other values can be used if desired + * @param $code A codename for this error. Only one message per codename will be added. + * This can be usedful for ensuring no duplicate messages + * @param $cast Cast type; One of the CAST_ constant definitions. * @return $this */ public function addFieldError( - $fieldName, - $message, - $messageType = ValidationResult::TYPE_ERROR, - $code = null, - $cast = ValidationResult::CAST_TEXT, - ) { + string $fieldName, + string $message, + string $messageType = ValidationResult::TYPE_ERROR, + string $code = '', + string $cast = ValidationResult::CAST_TEXT, + ): static { $this->isValid = false; - return $this->addFieldMessage($fieldName, $message, $messageType, $code, $cast); + return $this->addFieldMessage( + $fieldName, + $message, + $messageType, + $code, + $cast, + ); } /** * Add a message to this ValidationResult without necessarily marking it as an error * - * @param string $message The message string. - * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS - * class to the form, so other values can be used if desired. - * @param string $code A codename for this error. Only one message per codename will be added. - * This can be usedful for ensuring no duplicate messages - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $message The message string. + * @param $messageType The type of message: e.g. "bad", "warning", "good", or "required" + * Passed as a CSS class to the form, so other values can be used if desired + * @param $code A codename for this error. Only one message per codename will be added. + * This can be usedful for ensuring no duplicate messages + * @param $cast Cast type; One of the CAST_ constant definitions. * @return $this */ public function addMessage( - $message, - $messageType = ValidationResult::TYPE_ERROR, - $code = null, - $cast = ValidationResult::CAST_TEXT, - ) { - return $this->addFieldMessage(null, $message, $messageType, $code, $cast); + string $message, + string $messageType = ValidationResult::TYPE_ERROR, + string $code = '', + string $cast = ValidationResult::CAST_TEXT, + ): static { + return $this->addFieldMessage( + '', + $message, + $messageType, + $code, + $cast, + ); } /** * Add a message to this ValidationResult without necessarily marking it as an error * - * @param string $fieldName The field to link the message to. If omitted; a form-wide message is assumed. - * @param string $message The message string. - * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS - * class to the form, so other values can be used if desired. - * @param string $code A codename for this error. Only one message per codename will be added. - * This can be usedful for ensuring no duplicate messages - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $fieldName The field to link the message to. If omitted; a form-wide message is assumed. + * @param $message The message string. + * @param $messageType The type of message: e.g. "bad", "warning", "good", or "required" + * Passed as a CSS class to the form, so other values can be used if desired + * @param $code A codename for this error. Only one message per codename will be added. + * This can be usedful for ensuring no duplicate messages + * @param $cast Cast type; One of the CAST_ constant definitions. * @return $this */ public function addFieldMessage( - $fieldName, - $message, - $messageType = ValidationResult::TYPE_ERROR, - $code = null, - $cast = ValidationResult::CAST_TEXT, - ) { + string $fieldName, + string $message, + string $messageType = ValidationResult::TYPE_ERROR, + string $code = '', + string $cast = ValidationResult::CAST_TEXT, + ): static { if ($code && is_numeric($code)) { - throw new InvalidArgumentException("Don't use a numeric code '$code'. Use a string."); - } - if (is_bool($cast)) { - $cast = $cast ? ValidationResult::CAST_TEXT : ValidationResult::CAST_HTML; + throw new InvalidArgumentException("Don't use a numeric code '$code'. Use a string."); } $metadata = [ 'message' => $message, 'fieldName' => $fieldName, 'messageType' => $messageType, 'messageCast' => $cast, + 'dataClass' => $this->dataClass, + 'recordID' => $this->recordID, ]; if ($code) { $this->messages[$code] = $metadata; } else { $this->messages[] = $metadata; } - return $this; } /** * Returns true if the result is valid. - * @return boolean */ - public function isValid() + public function isValid(): bool { return $this->isValid; } @@ -180,9 +227,9 @@ public function isValid() /** * Return the full error meta-data, suitable for combining with another ValidationResult. * - * @return array Array of messages, where each item is an array of data for that message. + * @return Array of messages, where each item is an array of data for that message. */ - public function getMessages() + public function getMessages(): array { return $this->messages; } @@ -193,12 +240,12 @@ public function getMessages() * This object will be modified to contain the new validation information. * * @param ValidationResult $other the validation result object to combine - * @return $this */ - public function combineAnd(ValidationResult $other) + public function combineAnd(ValidationResult $other): static { $this->isValid = $this->isValid && $other->isValid(); $this->messages = array_merge($this->messages, $other->getMessages()); + $this->combineDataClassAndRecordID($other); return $this; } @@ -215,4 +262,25 @@ public function __unserialize(array $data): void $this->messages = $data['messages']; $this->isValid = $data['isValid']; } + + /** + * Combine the data class and record ID from another ValidationResult object. + */ + private function combineDataClassAndRecordID(ValidationResult $other): void + { + $thisDataClass = $this->getDataClass(); + $otherDataClass = $other->getDataClass(); + $thisRecordID = $this->getRecordID(); + $otherRecordID = $other->getRecordID(); + if ($thisDataClass === '' && $otherDataClass !== '') { + $this->setDataClass($otherDataClass); + } elseif ($thisDataClass !== '' && $otherDataClass === '') { + $other->setDataClass($thisDataClass); + } + if ($thisRecordID === 0 && $otherRecordID !== 0) { + $this->setRecordID($otherRecordID); + } elseif ($thisRecordID !== 0 && $otherRecordID === 0) { + $other->setRecordID($thisRecordID); + } + } } diff --git a/src/Forms/Form.php b/src/Forms/Form.php index c53d6d51d7a..70c70b23bfd 100644 --- a/src/Forms/Form.php +++ b/src/Forms/Form.php @@ -1179,32 +1179,36 @@ public function FieldMap() /** * Set a message to the session, for display next time this form is shown. * - * @param string $message the text of the message - * @param string $type Should be set to good, bad, or warning. - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. - */ - public function sessionMessage($message, $type = ValidationResult::TYPE_ERROR, $cast = ValidationResult::CAST_TEXT) - { + * @param $message the text of the message + * @param $type Should be set to good, bad, or warning. + * @param $cast Cast type; One of the CAST_ constant definitions. + */ + public function sessionMessage( + string $message, + string $type = ValidationResult::TYPE_ERROR, + string $cast = ValidationResult::CAST_TEXT + ) { $this->setMessage($message, $type, $cast); $result = $this->getSessionValidationResult() ?: ValidationResult::create(); - $result->addMessage($message, $type, null, $cast); + $result->addMessage($message, $type, '', $cast); $this->setSessionValidationResult($result); } /** * Set an error to the session, for display next time this form is shown. * - * @param string $message the text of the message - * @param string $type Should be set to good, bad, or warning. - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param $message the text of the message + * @param $type Should be set to good, bad, or warning. + * @param $cast Cast type; One of the CAST_ constant definitions. */ - public function sessionError($message, $type = ValidationResult::TYPE_ERROR, $cast = ValidationResult::CAST_TEXT) - { + public function sessionError( + string $message, + string $type = ValidationResult::TYPE_ERROR, + string $cast = ValidationResult::CAST_TEXT + ) { $this->setMessage($message, $type, $cast); $result = $this->getSessionValidationResult() ?: ValidationResult::create(); - $result->addError($message, $type, null, $cast); + $result->addError($message, $type, '', $cast); $this->setSessionValidationResult($result); } @@ -1214,14 +1218,17 @@ public function sessionError($message, $type = ValidationResult::TYPE_ERROR, $ca * @param string $message the text of the message * @param string $fieldName Name of the field to set the error message on it. * @param string $type Should be set to good, bad, or warning. - * @param string|bool $cast Cast type; One of the CAST_ constant definitions. - * Bool values will be treated as plain text flag. + * @param string $cast Cast type; One of the CAST_ constant definitions. */ - public function sessionFieldError($message, $fieldName, $type = ValidationResult::TYPE_ERROR, $cast = ValidationResult::CAST_TEXT) - { + public function sessionFieldError( + string $message, + string $fieldName, + string $type = ValidationResult::TYPE_ERROR, + string $cast = ValidationResult::CAST_TEXT + ) { $this->setMessage($message, $type, $cast); $result = $this->getSessionValidationResult() ?: ValidationResult::create(); - $result->addFieldMessage($fieldName, $message, $type, null, $cast); + $result->addFieldMessage($fieldName, $message, $type, '', $cast); $this->setSessionValidationResult($result); } diff --git a/src/Forms/Validation/Validator.php b/src/Forms/Validation/Validator.php index 04df3d263cb..e36a85b80da 100644 --- a/src/Forms/Validation/Validator.php +++ b/src/Forms/Validation/Validator.php @@ -83,7 +83,7 @@ public function validationError( $messageType = ValidationResult::TYPE_ERROR, $cast = ValidationResult::CAST_TEXT ) { - $this->result->addFieldError($fieldName, $message, $messageType, null, $cast); + $this->result->addFieldError($fieldName, $message, $messageType, '', $cast); return $this; } diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 2f0903d4c9b..00d78789cb2 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -1256,7 +1256,7 @@ public function forceChange() */ public function validate(): ValidationResult { - $result = ValidationResult::create(); + $result = ValidationResult::create(get_class($this), $this->ID); // Call DBField::validate() on every DBField $specs = DataObject::getSchema()->fieldSpecs(static::class); foreach (array_keys($specs) as $fieldName) { diff --git a/src/Security/Member.php b/src/Security/Member.php index ef5ef2a9261..54d93b529e1 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -325,7 +325,7 @@ public function validateCanLogin(?ValidationResult &$result = null) _t( __CLASS__ . '.ERRORLOCKEDOUT2', 'Your account has been temporarily disabled because of too many failed attempts at ' . 'logging in. Please try again in {count} minutes.', - null, + '', ['count' => static::config()->get('lock_out_delay_mins')] ) ); @@ -1625,17 +1625,17 @@ public function validate(): ValidationResult return ValidationResult::create(); } - $valid = parent::validate(); + $result = parent::validate(); $validator = static::password_validator(); if ($validator) { if ((!$this->ID && $this->Password) || $this->isChanged('Password')) { $userValid = $validator->validate($this->Password, $this); - $valid->combineAnd($userValid); + $result->combineAnd($userValid); } } - return $valid; + return $result; } /** diff --git a/tests/php/Core/Validation/ConstraintValidatorTest.php b/tests/php/Core/Validation/ConstraintValidatorTest.php index e2dc9aaab6e..96a82a11e28 100644 --- a/tests/php/Core/Validation/ConstraintValidatorTest.php +++ b/tests/php/Core/Validation/ConstraintValidatorTest.php @@ -352,7 +352,7 @@ public function testValidateResults(mixed $value, Constraint|array $constraints, $this->assertCount($countViolations, $violations); foreach ($violations as $violation) { - $this->assertSame($fieldName ?: null, $violation['fieldName']); + $this->assertSame($fieldName ?: '', $violation['fieldName']); } } diff --git a/tests/php/Core/Validation/ValidationExceptionTest.php b/tests/php/Core/Validation/ValidationExceptionTest.php index dbc23b1de33..7702b47c8f6 100644 --- a/tests/php/Core/Validation/ValidationExceptionTest.php +++ b/tests/php/Core/Validation/ValidationExceptionTest.php @@ -2,12 +2,23 @@ namespace SilverStripe\Core\Tests\Validation; +use ReflectionClass; use SilverStripe\Core\Validation\ValidationResult; use SilverStripe\Core\Validation\ValidationException; use SilverStripe\Dev\SapphireTest; +use SilverStripe\Core\Environment; +use SilverStripe\Control\Controller; +use SilverStripe\Dev\DevelopmentAdmin; +use SilverStripe\Core\Tests\Validation\ValidationExceptionTest\TestObject; +use PHPUnit\Framework\Attributes\DataProvider; +use SilverStripe\Forms\EmailField; class ValidationExceptionTest extends SapphireTest { + protected static $extra_dataobjects = [ + TestObject::class + ]; + private function arrayContainsArray($expectedSubArray, $array) { foreach ($array as $subArray) { @@ -18,6 +29,28 @@ private function arrayContainsArray($expectedSubArray, $array) return false; } + private ?bool $initialCliOverride = null; + + private array $initialControllerStack = []; + + protected function setUp(): void + { + parent::setUp(); + $reflectionCli = new ReflectionClass(Environment::class); + $reflectionStack = new ReflectionClass(Controller::class); + $this->initialCliOverride = $reflectionCli->getStaticPropertyValue('isCliOverride'); + $this->initialControllerStack = $reflectionStack->getStaticPropertyValue('controller_stack'); + } + + protected function tearDown(): void + { + parent::tearDown(); + $reflectionCli = new ReflectionClass(Environment::class); + $reflectionStack = new ReflectionClass(Controller::class); + $reflectionCli->setStaticPropertyValue('isCliOverride', $this->initialCliOverride); + $reflectionStack->setStaticPropertyValue('controller_stack', $this->initialControllerStack); + } + /** * Test that ValidationResult object can correctly populate a ValidationException */ @@ -35,7 +68,9 @@ public function testCreateFromValidationResult() 'message' => 'Not a valid result', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', + 'dataClass' => '', + 'recordID' => 0, ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); } @@ -60,7 +95,9 @@ public function testCreateFromComplexValidationResult() 'message' => 'Invalid type', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', + 'dataClass' => '', + 'recordID' => 0, ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); @@ -68,7 +105,9 @@ public function testCreateFromComplexValidationResult() 'message' => 'Out of kiwis', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', + 'dataClass' => '', + 'recordID' => 0, ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); } @@ -90,6 +129,8 @@ public function testCreateFromMessage() 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, 'fieldName' => null, + 'dataClass' => '', + 'recordID' => 0, ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); } @@ -113,7 +154,9 @@ public function testCreateWithComplexValidationResultAndMessage() 'message' => 'A spork is not a knife', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', + 'dataClass' => '', + 'recordID' => 0, ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); @@ -121,7 +164,9 @@ public function testCreateWithComplexValidationResultAndMessage() 'message' => 'A knife is not a back scratcher', 'messageCast' => ValidationResult::CAST_TEXT, 'messageType' => ValidationResult::TYPE_ERROR, - 'fieldName' => null, + 'fieldName' => '', + 'dataClass' => '', + 'recordID' => 0, ], $exception->getResult()->getMessages()); $this->assertTrue($b, 'Messages array should contain expected messaged'); } @@ -135,7 +180,7 @@ public function testCombineResults() $anotherresult = new ValidationResult(); $yetanotherresult = new ValidationResult(); $anotherresult->addError("Eat with your mouth closed", 'bad', "EATING101"); - $yetanotherresult->addError("You didn't wash your hands", 'bad', "BECLEAN", false); + $yetanotherresult->addError("You didn't wash your hands", 'bad', "BECLEAN", ValidationResult::CAST_HTML); $this->assertTrue($result->isValid()); $this->assertFalse($anotherresult->isValid()); @@ -150,13 +195,17 @@ public function testCombineResults() 'message' => 'Eat with your mouth closed', 'messageType' => 'bad', 'messageCast' => ValidationResult::CAST_TEXT, - 'fieldName' => null, + 'fieldName' => '', + 'dataClass' => '', + 'recordID' => 0, ], 'BECLEAN' => [ 'message' => 'You didn\'t wash your hands', 'messageType' => 'bad', 'messageCast' => ValidationResult::CAST_HTML, - 'fieldName' => null, + 'fieldName' => '', + 'dataClass' => '', + 'recordID' => 0, ], ], $result->getMessages() @@ -179,31 +228,141 @@ public function testValidationResultAddMethods() $this->assertEquals( [ [ - 'fieldName' => null, + 'fieldName' => '', 'message' => 'A spork is not a knife', 'messageType' => 'bad', 'messageCast' => ValidationResult::CAST_TEXT, + 'dataClass' => '', + 'recordID' => 0, ], [ - 'fieldName' => null, + 'fieldName' => '', 'message' => 'A knife is not a back scratcher', 'messageType' => 'error', 'messageCast' => ValidationResult::CAST_TEXT, + 'dataClass' => '', + 'recordID' => 0, ], [ 'fieldName' => 'Title', 'message' => 'Title is good', 'messageType' => 'good', 'messageCast' => ValidationResult::CAST_TEXT, + 'dataClass' => '', + 'recordID' => 0, ], [ 'fieldName' => 'Content', 'message' => 'Content is bad', 'messageType' => 'bad', 'messageCast' => ValidationResult::CAST_TEXT, + 'dataClass' => '', + 'recordID' => 0, ] ], $result->getMessages() ); } + + public static function provideDoShowAdditionalInfo(): array + { + return [ + 'is_cli' => [ + 'isCli' => true, + 'isDevAdmin' => false, + 'expected' => true, + ], + 'is_dev_admin' => [ + 'isCli' => false, + 'isDevAdmin' => true, + 'expected' => true, + ], + 'is_both' => [ + 'isCli' => true, + 'isDevAdmin' => true, + 'expected' => true, + ], + 'is_neither' => [ + 'isCli' => false, + 'isDevAdmin' => false, + 'expected' => false, + ], + ]; + } + + #[DataProvider('provideDoShowAdditionalInfo')] + public function testDoShowAdditionalInfo( + bool $isCli, + bool $isDevAdmin, + bool $expected + ) { + // Set cli override + $reflectionCli = new ReflectionClass(Environment::class); + $reflectionCli->setStaticPropertyValue('isCliOverride', $isCli); + if ($isDevAdmin) { + // Ensure that Controller::curr() to return a DevelopmentAdmin + $reflectionStack = new ReflectionClass(Controller::class); + $value = $reflectionStack->getStaticPropertyValue('controller_stack'); + array_unshift($value, new DevelopmentAdmin()); + $reflectionStack->setStaticPropertyValue('controller_stack', $value); + } + $result = new ValidationResult(); + $result->addFieldMessage('Title', 'Invalid'); + $exception = new ValidationException($result); + $showsAdditionalInfo = $exception->getMessage() === 'Invalid - fieldName: Title'; + $this->assertSame($expected, $showsAdditionalInfo); + } + + public static function provideAdditonalInfoDataObject(): array + { + return [ + 'dataObject' => [ + 'type' => 'DataObject', + 'expect' => 'all-info', + ], + 'formField' => [ + 'type' => 'FormField', + 'expect' => 'partial-info', + ], + 'error' => [ + 'type' => 'NoFieldName', + 'expect' => 'no-info', + ], + ]; + } + + #[DataProvider('provideAdditonalInfoDataObject')] + public function testAdditonalInfoDataObject(string $type, string $expect): void + { + // Set cli override to ensure additonal info is shown + $reflectionCli = new ReflectionClass(Environment::class); + $reflectionCli->setStaticPropertyValue('isCliOverride', true); + // Run test + $obj = new TestObject(['Email' => 'valid@example.com']); + $recordID = $obj->write(); + $dataClass = get_class($obj); + $actual = null; + if ($type === 'DataObject') { + try { + $obj->update(['Email' => 'invalid'])->write(); + } catch (ValidationException $e) { + $actual = $e->getMessage(); + } + } elseif ($type === 'FormField') { + $result = (new EmailField('Email', 'Email', 'invalid'))->validate(); + $exception = new ValidationException($result); + $actual = $exception->getMessage(); + } else { + $result = new ValidationResult(); + $result->addError('Invalid email address'); + $exception = new ValidationException($result); + $actual = $exception->getMessage(); + } + $expected = match ($expect) { + 'all-info' => "Invalid email address - fieldName: Email, recordID: $recordID, dataClass: $dataClass", + 'partial-info' => 'Invalid email address - fieldName: Email', + 'no-info' => 'Invalid email address', + }; + $this->assertSame($expected, $actual); + } } diff --git a/tests/php/Core/Validation/ValidationExceptionTest/TestObject.php b/tests/php/Core/Validation/ValidationExceptionTest/TestObject.php new file mode 100644 index 00000000000..bc362e0683c --- /dev/null +++ b/tests/php/Core/Validation/ValidationExceptionTest/TestObject.php @@ -0,0 +1,15 @@ + 'Email', + ]; +} diff --git a/tests/php/Core/Validation/ValidationResultTest.php b/tests/php/Core/Validation/ValidationResultTest.php index ac725919928..b833c822da4 100644 --- a/tests/php/Core/Validation/ValidationResultTest.php +++ b/tests/php/Core/Validation/ValidationResultTest.php @@ -5,37 +5,87 @@ use SilverStripe\Dev\SapphireTest; use SilverStripe\Core\Validation\ValidationResult; +use PHPUnit\Framework\Attributes\DataProvider; class ValidationResultTest extends SapphireTest { public function testSerialise() { $result = new ValidationResult(); - $result->addError("Error", ValidationResult::TYPE_ERROR, null, ValidationResult::CAST_HTML); - $result->addMessage("Message", ValidationResult::TYPE_GOOD); + $result->addError( + 'Error', + ValidationResult::TYPE_ERROR, + 'code-a', + ValidationResult::CAST_HTML, + ); + $result->addMessage( + 'Message', + ValidationResult::TYPE_GOOD, + 'code-b', + ValidationResult::CAST_HTML, + ); $serialised = serialize($result); - - /** - * @var ValidationResult $result2 -*/ + /** @var ValidationResult $result2 */ $result2 = unserialize($serialised ?? ''); $this->assertEquals( [ - [ - 'message' => 'Error', - 'fieldName' => null, - 'messageCast' => ValidationResult::CAST_HTML, - 'messageType' => ValidationResult::TYPE_ERROR, - ], - [ - 'message' => 'Message', - 'fieldName' => null, - 'messageCast' => ValidationResult::CAST_TEXT, - 'messageType' => ValidationResult::TYPE_GOOD, - ] + 'code-a' => [ + 'message' => 'Error', + 'fieldName' => '', + 'messageCast' => ValidationResult::CAST_HTML, + 'messageType' => ValidationResult::TYPE_ERROR, + 'dataClass' => '', + 'recordID' => 0, + ], + 'code-b' => [ + 'message' => 'Message', + 'fieldName' => '', + 'messageCast' => ValidationResult::CAST_HTML, + 'messageType' => ValidationResult::TYPE_GOOD, + 'dataClass' => '', + 'recordID' => 0, + ], ], $result2->getMessages() ); $this->assertFalse($result2->isValid()); } + + public static function provideCombineDataClassAndRecordID(): array + { + return [ + 'first-has-data' => [ + 'firstHasData' => true, + 'secondHasData' => false, + ], + 'second-has-data' => [ + 'firstHasData' => false, + 'secondHasData' => true, + ], + ]; + } + + #[DataProvider('provideCombineDataClassAndRecordID')] + public function testCombineDataClassAndRecordID( + bool $firstHasData, + bool $secondHasData, + ): void { + $dataClass = 'Some\\DataObject'; + $recordID = 123; + $first = new ValidationResult(); + if ($firstHasData) { + $first->setDataClass($dataClass); + $first->setRecordID($recordID); + } + $second = new ValidationResult(); + if ($secondHasData) { + $second->setDataClass($dataClass); + $second->setRecordID($recordID); + } + $first->combineAnd($second); + $this->assertSame($dataClass, $first->getDataClass()); + $this->assertSame($recordID, $first->getRecordID()); + $this->assertSame($dataClass, $second->getDataClass()); + $this->assertSame($recordID, $second->getRecordID()); + } }