Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Refactor to stateless validators #15

Open
weierophinney opened this issue Dec 31, 2019 · 4 comments
Open

[WIP] Refactor to stateless validators #15

weierophinney opened this issue Dec 31, 2019 · 4 comments

Comments

@weierophinney
Copy link
Member

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:

  • Define a Result interface modeling the results of validation; it would compose the value validated, validation status, and, if present, any validation error messages.
  • Define validators would define a single validate() method, accepting both a value and optional context, and return a Result instance.
  • Define a ResultAggregate interface for aggregating several results, as is necessary in a ValidatorChain; Result instances would be pushed upon an aggregate.

Translation, value obscuration, and message truncation then become presentation issues, and can be achieved by decorating Result instances.

Additionally, we'd remove the concept of $options for 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, and ResultAggregate interfaces, and various Result implementations. Validator would define validate instead of isValid(), allowing devs to implement both in order to forward-proof their validators. We could also provide a wrapper for Validator implementations to make them work as ValidatorInterface instances; 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.


Originally posted by @weierophinney at zendframework/zend-validator#181

@weierophinney
Copy link
Member Author

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.

Every single custom validator and every extension to a supplied validator will need to be rewritten?


Originally posted by @akrabat at zendframework/zend-validator#181 (comment)

@weierophinney
Copy link
Member Author

Every single custom validator and every extension to a supplied validator will need to be rewritten?

Yes - as isValid() goes away, and is replaced by validate(), and the return values would differ. This is, of course, why it's targeted for a new major version.

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 Validator interface:

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.


Originally posted by @weierophinney at zendframework/zend-validator#181 (comment)

@weierophinney
Copy link
Member Author

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.


Originally posted by @akrabat at zendframework/zend-validator#181 (comment)

@weierophinney
Copy link
Member Author

I think there's going to have to be a compelling advantage to v3

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(); // $value2

The other issue I've been seeing is maintenance of the AbstractValidator, as it contains a ton of logic around gathering options, formatting messages, etc:

  • Due to the options based approach, we have to do a lot of internal testing to see if the validator is in a valid state before we validate — does it have the necessary options? are they of the correct type? etc.
  • Moving presentation logic (value obscuration, translation, message truncation, etc.) into results, result decorators, or DTOs greatly reduces the logic, and thus maintenance, of that base class.

One nice benefit of the approach is that we can finally use callables for validation more cleanly. If they return a Result instance, this is no different than using a Validator for a consumer. The Callback validator can be used when you want to return a boolean result instead; you would compose your message templates and variables in that, so it can return a Result. This simplifies the creation and usage of one-off validators; you no longer need to have classes. (That said, PHP 7's anonymous classes also makes this easier.)


Originally posted by @weierophinney at zendframework/zend-validator#181 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants