[WIP] Refactor to stateless validators #181
Conversation
Instead of `isValid() : bool`, `isValid()` now returns a `ValidatorResult`, which will contain the value validated, results of validation, and, in the case of an invalid result, validation error messages. Additionally, `isValid()` now accepts an additional, optional array argument, `$context`; this allows validators to check against other values in a validation set in order to do their work.
`ValidatorResult` now composes message templates and message variables instead of messages; this will allow the various validators to omit translation features. A new class, `ValidatorResultTranslator`, composes a translator and text domain, and accepts a `ValidatorResult` to a method `translateMessages()`; it will then return an array of translated message strings. `ValidatorResult::getMessages()` will return the verbatim message templates with variable interpolations, with no translation.
Removes all translator awareness from the `AbstractValidator`. Consolidates `error` and `createMessage` to only do a lookup for the given message key; `error` is removed. Removes `__get()` and `__invoke()` as being superfluous. Removes memoization of error messages and the validated value.
Realized that decoration is likely the best path for altering the values returned via `getMessages()`. To enable that, added a new interface, `Result`, describing the capabilities of a validation result, and modified `ValidatorResult` to implement it. Renamed `ValidatorResultTranslator` to `TranslatableValidatorResult`; it now accepts a `Result` to its constructor and implements `Result`. For all methods other than `getMessages()`, it proxies to the underlying `Result` instance (these are implemented by a new trait, `ValidatorResultDecorator`). Also provides a new result type, `ObscuredValueValidatorResult`; it overrides the `getValue()` method to obscure the value, and the `getMessages()` method to ensure that its own `getValue()` is called when performing message template substitutions. A new method was extracted within `ValidatorResultMessageInterpolator`, `castValueToString()`; this was done to allow the `ObscuredValueValidatorResult` to work with the string value when performing obfuscation.
- Removing "Interface", "Trait" suffixes as part of PHP 7.1 initiative.
Adds `AbstractValidator::createInvalidResult()`, for automating creation of the `Result` instance returned by validators; that method retrieves method templates based on the message keys passed to it, and pulls the message variables from the instance, passing them along with the value to the `ValidatorResult::createInvalidResult()` constructor. If the "value obscured" flag is enabled, decorates the result with `ObscuredValueValidatorResult` before returning it. The `Between` validator was refactored to use this approach.
Updates the `Between` validator tests to reflect the refactor to return Result instances. Discovers a bug in the ValidatorResult::getMessageTemplates method, and corrects it.
Done for two reasons. First, the method no longer returns a boolean, so calling it `isValid()` did not indicate that there might be a non-boolean result. Second, having the name different will allow us to add the `Validator` interface to v2 releases, and thus provide a forwards-compat path for developers.
Validator chains need to aggregate results... but still return a `Result`. This commit does the following: - Adds a `ResultAggregate` interface, which extends `Result`, as well as `IteratorAggregate` and `Countable`. It defines one additional method, `push()`, allowing pushing validation results onto the aggregate. - Adds `ValidatorResultAggregate` as the only concrete implementation of a `ResultAggregate`. - `ValidatorChain` now implements `Validator`, and its `validate()` method creates a `ValidatorResultAggregate` and pushes the result of each validator in the chain to it. - `TranslatableValidatorResult` and `ObscuredValueValidatorResult` now check if the underlying result is an aggregate when `getMessages()` is called; if so, they loop through each result in order to aggregate messages to return. This allows them to act strictly as decorators.
Like translation, value obscuration is a presentation issue; leave it to the decorators.
Like translation and value obfuscation, message truncation is a presentation issue.
Actual failure messages are no longer stored in the validator, only message templates and variables.
…act options to instance variables Removes the constructor, `getOptions()`, `setOptions()`, and `setOption()` from the `AbstractValidator`. Additionally, it promotes the two remaining members of `$abstractOptions` to instance variables, and modifies the various methods that reference them to reference the new variables.
Updates the `Between` validator to no longer accept an array of options, but instead three discrete arguments. It also sets the `$messageVariables` property during instantiation. Tests were refactored to use data providers, and to test for both valid arguments and the expected behavior, as well as invalid constructor arguments and expected exceptions.
Every single custom validator and every extension to a supplied validator will need to be rewritten? |
Yes - as My hope is that we can provide some tooling and guidance around this. For instance, something like the following trait would allow adapting an existing validator to implement the new trait LegacyValidator
{
public function validate($value, array $context = null) : Result
{
if ($this->isValid($value, $context)) {
return ValidatorResult::createValidResult($value);
}
return ValidatorResult::createInvalidResult(
$value,
$this->getMessageTemplates(),
$this->abstractOptions['messageVariables']
);
}
}
class ExistingValidator extends AbstractValidator implements Validator
{
use LegacyValidator;
/* ... */
}If we provide the legacy interface in v3, but mark it deprecated, this approach would allow mixing and matching existing v2-capable validators with v3, providing a longer upgrade path. |
|
I think there's going to have to be a compelling advantage to v3 or upgrading is going to have to be quite easy :) A quick check of one of my projects shows that I have 30 or so Validators that would need upgrading and that won't be an easy sell to the client if there's no tangible benefit if it's more than a day's work. I assume that the trait won't work for validators that extend current validators, so they'll have to be re-done. |
One of the issues currently is that stateful validators can lead to hard to debug issues, particularly if you share an instance. As an example: $validator->isValid($value1);
$validator->isValid($value2);
$messages = $validator->getMessages(); // $value1 messages are missing
$value = $validator->getValue(); // $value2The other issue I've been seeing is maintenance of the
One nice benefit of the approach is that we can finally use callables for validation more cleanly. If they return a |
| /** | ||
| * Value object representing results of validation. | ||
| */ | ||
| class ValidatorResult implements Result |
There was a problem hiding this comment.
Should we make it final? We should promote good practics and teach developers to use patterns e.g. decorators
| /** | ||
| * @param mixed $value | ||
| */ | ||
| public static function createValidResult($value) : self |
| /** | ||
| * @return iterable | ||
| */ | ||
| public function getIterator() |
There was a problem hiding this comment.
Why not just getIterator(): iterable ?
| */ | ||
| public function getIterator() | ||
| { | ||
| foreach ($this->results as $result) { |
There was a problem hiding this comment.
What is difference here to use just return $this->results; ?
| public function __construct( | ||
| $value, | ||
| bool $isValid, | ||
| array $messageTemplates = [], |
There was a problem hiding this comment.
What are you think to make a collection value object for message templates and variables? We can keep types strictly.
ghost
mentioned this pull request
|
This repository has been closed and moved to laminas/laminas-validator; a new issue has been opened at laminas/laminas-validator#15. |
|
This repository has been moved to laminas/laminas-validator. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:
|
This work-in-progress is exploring how we might approach stateless validators.
Essentially, instead of an
isValid()method returning a boolean, and a subsequent call on the validator to retrieve validation error messages, we would instead:Resultinterface modeling the results of validation; it would compose the value validated, validation status, and, if present, any validation error messages.validate()method, accepting both a value and optional context, and return aResultinstance.ResultAggregateinterface for aggregating several results, as is necessary in aValidatorChain;Resultinstances would be pushed upon an aggregate.Translation, value obscuration, and message truncation then become presentation issues, and can be achieved by decorating
Resultinstances.Additionally, we'd remove the concept of
$optionsfor creating individual validator instances; they would instead have concrete constructor arguments. This simplifies a ton of logic internally, but means that each validator would require its own factory class. On the flip side, it also means developers can write factories for specific options combinations, and have shared instances without worrying about state.Migration concerns
We could create a new minor release that adds the new
Validator,Result, andResultAggregateinterfaces, and variousResultimplementations.Validatorwould definevalidateinstead ofisValid(), allowing devs to implement both in order to forward-proof their validators. We could also provide a wrapper forValidatorimplementations to make them work asValidatorInterfaceinstances; in such a case, it would pull info from the result in order to populate its members.The bigger issue is existing validators, and extensions to them. Developers extending existing validators may want to copy/paste implementations and begin migration of those to make them forwards-compatible with version 3. Since we would have version 3 released simultaneously to a v2 with the new interface additions, they could even copy those from v3 to aid their migration.
Integration concerns
I have not yet investigated impact on zend-inputfilter; however, that version will require a similar refactor towards stateless inputs/input filters as well.