[RFC] Stateless Validators (Again) #153
Conversation
By using a value object to represent the validation result, error message iteration and translation can be extracted from all the validators. Signed-off-by: George Steel <[email protected]>
Signed-off-by: George Steel <[email protected]>
Signed-off-by: George Steel <[email protected]>
|
Obviously, I've made absolutely no attempt to make CI pass here! |
src/ValidationResult.php
Outdated
| /** @implements IteratorAggregate<string, non-empty-string> */ | ||
| final class ValidationResult implements IteratorAggregate, Countable |
There was a problem hiding this comment.
I don't think ValidationResult should be iterable nor countable: from a consumer PoV, it should only be iterable+countable if it failed validation
There was a problem hiding this comment.
So do you think it would be better to define different types for success and failure states?
There was a problem hiding this comment.
Yes, it would help:
- while debugging
- to get better types for us internally
- to make it crash in case somebody tries to read error messages out of a success scenario
src/ValidationResult.php
Outdated
| public static function new( | ||
| array $templates, | ||
| array $variables, | ||
| mixed $value, | ||
| bool $valueObscured = false, | ||
| ?TranslatorInterface $translator = null, | ||
| string $textDomain = 'default', | ||
| ): self { | ||
| return new self($templates, $variables, $value, $valueObscured, $translator, $textDomain); | ||
| } |
There was a problem hiding this comment.
We should probably have two separate named constructors:
- failure
- success
See http://marcosh.github.io/post/2017/06/16/maybe-in-php.html and http://marcosh.github.io/post/2017/10/27/maybe-in-php-2.html
There was a problem hiding this comment.
The initial idea here is that it has a kind of builder interface like (inside a validator):
$result = ValidationResult::new($myTemplates, $substitutions, $value, false, $translator);
if (! is_string($value)) {
return $result->withError($someKey);
}
if (! $this->constraintA($value)) {
$result = $result->withError($otherKey);
}
if (! $this->constraintB($value)) {
$result = $result->withError($anotherKey);
}
return $result;
src/ValidationResult.php
Outdated
| use function str_replace; | ||
| use function strlen; | ||
|
|
||
| /** @implements IteratorAggregate<string, non-empty-string> */ |
src/ValidationResult.php
Outdated
| /** @psalm-pure */ | ||
| private static function stringify(mixed $value): string | ||
| { | ||
| if (is_float($value) || is_int($value) || is_string($value)) { | ||
| return (string) $value; | ||
| } | ||
|
|
||
| if (is_bool($value)) { | ||
| return $value ? 'true' : 'false'; | ||
| } | ||
|
|
||
| if ($value === null) { | ||
| return 'null'; | ||
| } | ||
|
|
||
| if (is_array($value)) { | ||
| return 'array'; | ||
| } | ||
|
|
||
| if (is_resource($value)) { | ||
| return 'resource'; | ||
| } | ||
|
|
||
| assert(is_object($value)); | ||
|
|
||
| return self::stringifyObject($value); | ||
| } |
There was a problem hiding this comment.
Not sure if it's the validator's responsibility, or the view helper's 🤔
There was a problem hiding this comment.
Not a major issue for me though.
There was a problem hiding this comment.
Mostly, this was about being absolutely rigid about the expected value of the list of error messages, i.e. array<string, non-empty-string> and because I put translation inside the result, I had to implement some kind of casting for the possible substitutions. It would be cleaner to make the validators provide strings to the constructor, but that would bloat all the validators wouldn't it?
If translation was external to both validators and the result, i.e. $validatorTranslator->translate($validationResult) it would be even cleaner, but more responsibility is shifted to the view, i.e. where do they get the validator translator from? Also, the result would need to expose everything the translator needs, the message templates, keys, value, substitution keys and values.
Signed-off-by: George Steel <[email protected]>
| class StringLength extends AbstractValidator | ||
| /** | ||
| * @psalm-type OptionsArray array{ | ||
| * min: int, |
There was a problem hiding this comment.
int<0, max> to prevent negative numbers (although that would asserting)
| /** | ||
| * @psalm-type OptionsArray array{ | ||
| * min: int, | ||
| * max: int|null, |
There was a problem hiding this comment.
int<0, max>|null to prevent negative numbers (although that would asserting)
Description
Is there any appetite for refactoring the validator component to return a validation result, roughly as described in this PR?
The theory here is that error messages/validation failures are carried around by the immutable validation result and translation of the messages would be performed on/by the result using a composed translator (if any).
This would allow validators to behave more like stateless services.
A refactoring like this has been discussed in the past: #48, #38, #49