[WIP] [ZF3] Validators refactoring

This issue has been moved from the zendframework repository as part of the bug migration program as outlined here - http://framework.zend.com/blog/2016-04-11-issue-closures.html

Original Issue: https://api.github.com/repos/zendframework/zendframework/issues/5067 User: @bakura10 Created On: 2013-09-03T14:43:59Z Updated At: 2014-09-28T00:34:50Z Body Hi everyone,

Again another PR/issue for ZF3. I promise, this is the last one I will make (ok, maybe Forms too :D...).

This one is another lower-level component, but I need this one to correctly finish the InputFilter one.

As for filter, the main changes will be: some performance optimizations we can make with PHP 5.4, cleaning some code, homogenize options by using underscore_separated... The only real change I've made for now is to make the ValidatorChain exactly the same as FilterChain (I was always bothered that those classes didn't have the same method names or underlying implementation).

HOWEVER, there is a change @ocramius and I discussed on IRC and I'd love to have your opinion on that. The InputFilter refactor made the component completely stateless. It opened interesting optimizations, and the idea came to make validators also stateless.

To stay simple, the following code:

$validator = new Boolean();
if ($validator->isValid(true)) {
   // Do something
} else {
  $error = $validator->getErrorMessages();
}

Will be transformed to:

$validator = new Boolean();
$validationResult = $validator->validate($value);

if ($validationResult->isValid()) {
   // Do something
} else {
   $error = $validationResult->getErrorMessages();
}

As for input filters, validationResult instances are serializable and can be transformed to JSON.

Translation would still be handled by the validators themselves: they will translate the error messages and pass them to the validation result.

What do you think of this idea? Is there any drawback that you may see from it?

EDIT : Credit for the idea to George (https://gist.github.com/texdc/5929229#file-validationresultinterface-php)

Comment

User: @Ocramius Created On: 2013-09-03T16:41:29Z Updated At: 2013-09-03T16:47:17Z Body Note: as discussed on IRC (if you are reading this and are not on IRC, then the question is why you aren't!), translations can be handled in a very different way.

Having the translator within the validation component is quite problematic, so what we can do is having an error translator that works like following:

$validationTranslator = new ValidationTranslator(new Translator(/*...*/));
$validator = new Boolean();
$validationResult = $validator->validate($value);

if ($validationResult->isValid()) {
    // Do something
} else {
    $translatedResult = $validationTranslator->translate($validationResult);
    // pseudo - could also have a different API from the one in ValidationResult
    $error = $translatedResult->getErrorMessages();
}

Look ma! Validators don't know about translations anymore!

Eventually, a ValidationResult may implement logic to ease access to patterns or context variables needed by the translator. That interface doesn't necessarily cause a dependency to the I18n component.

Comment

User: @bakura10 Created On: 2013-09-03T16:48:16Z Updated At: 2013-09-03T16:48:16Z Body That's pure genius.

Comment

User: @bakura10 Created On: 2013-09-05T16:59:04Z Updated At: 2013-09-05T16:59:04Z Body ping @Ocramius and @weierophinney

I've made a proof of concept with the Between validator. There are multiple things I've made that we need to talk about.

First, the AbstractValidator is now very lightweight compared to previously. It has no reference to any Translator, no data to store. I've also removed the static "minimumLength" variable because I think it's the job of a view helper to cut a message.
Validators are completely decoupled from translation layer.
All variables were changed from camelCased to underscore_separated as it is by convention.
Please note the "setOptions" thing. This pattern goes beyond the Validator class. A LOT of classes inside a framework needs that kind of thing (an option array). I think that we can make it really convenient in ZF3 by writing a trait (like my previous PR @weierophinney :)) that could be used in all those classes. Of course, this trait would make some assumptions : that keys are underscore_separated, and that the setter of the corresponding property is called to set the value (this thing should propably take into account isser and has, until we have real property accessors in PHP :(). Of course, custom needs can avoid the use of the trait, but I think that would be a convenient pattern if we really follow the same covnentions accross the framework. We could even write an interface like "OptionsCapableInterface, that a factory could check and inject options.
The validator classes are no longer responsible to create error messages. They just create a ValidationResult object with error messages, data and optional variables that can be injected inside the message.
The Validation error have three main methods: isValid, getRawErrorMessages and getErrorMessages. When getErrorMessages is called, variables are being replaced using a preg_replace. The getRawErrorMessages can be used by for translation. For instance, as @Ocramius said, we could create, in the translation component, a kind of decorator:

class ValidationResultTranslater
{
    public function getTranslatedErrorMessages(ValidationResultInterface $interface)
    {
        // do things
    }
}

Or, if we want to keep it simple, the dependency to i18n could be moved to the ValidationResult (which is probably better, as some people may be angry if we introduce anothr abstraction level).

What do you think of this?

Comment

User: @bakura10 Created On: 2013-09-05T17:03:35Z Updated At: 2013-09-05T17:03:35Z Body Some more additional notes: now, the message variables are defined like this:

protected $messageVariables = array('min');

It is assumed that the validator has a property called "min". Still abut convention, we need to know if, in a message template, we prefer this:

protected $messageTemplates = array(
    self::KEY => 'An error occured "%my_variable%"
)

OR:

protected $messageTemplates = array(
    self::KEY => 'An error occured "%myVariable%"
)

I'd argue that it makes more sense to use camelCased in string.

Comment

User: @Ocramius Created On: 2013-09-05T17:10:38Z Updated At: 2013-09-05T17:10:38Z Body @bakura10 as long as it goes along what is defined in $messageVariables this shouldn't really matter...

Comment

User: @bakura10 Created On: 2013-09-05T17:17:27Z Updated At: 2013-09-05T17:17:27Z Body Well. Yes it does, because we should follow the convention that options are underscore_separated. So in your validator classes, you will have:

protected $messageVariables = array('my_variable');

If we want adopt a differnt convention that may make more sense in the message, we need to inflect the value.

Comment

User: @weierophinney Created On: 2013-09-06T14:24:19Z Updated At: 2013-09-06T14:24:19Z Body @bakura10 This is looking great!

I'd argue we wouldn't need any i18n awareness in Validator after this. It would be up to the developer and/or a view helper to take the ValidationResult and use the data it contains to create translated messages. This approach would offer a cleaner separation of concerns. Since you already define the ability to get the messages as well as the message variables, this should be trivial to accomplish.

Comment

User: @bakura10 Created On: 2013-09-06T14:58:55Z Updated At: 2013-09-06T14:58:55Z Body Thanks.

The only thing that bother me currently is how to handle the specific case of ValidatorChain. How should the ValidationResult be used? Because, by definition, the validator chain may return multiple error messages per validators, and multiple set of variables. If have no idea about how to do it.

Should we have a specific ValidationResult (ValidationResultCollection) or other way using nested arrays?

Comment

User: @weierophinney Created On: 2013-09-06T20:54:03Z Updated At: 2013-09-06T20:54:03Z Body @bakura10 I would argue having a ValidationResult and a ValidationResultCollection, which aggregates named ValidationResult and ValidationResultCollection instances; they should share a common interface that defines things like isValid() so that the developer can determine whether or not anything further needs to be done.

This allows developers to find the correctly named result in order to display individual validation messages, and also allows arbitrary nesting.

Comment

User: @bakura10 Created On: 2013-09-06T20:57:18Z Updated At: 2013-09-06T20:57:18Z Body Ok. I'll do that then. Now, I need to also think about how to use them in other context (I think about input filters here). Remember that now InputFilter also have InputFilterResult classes, we need to know how everything would work together (currently, I extracted the error messages directly from the validators).

Anyway, I think it will be clearer once we start merging some refactored components, it's a bit hard for me to work with different branches like taht.

Comment

User: @texdc Created On: 2013-10-04T00:10:10Z Updated At: 2013-10-04T00:12:52Z Body Hmm, wasn't I the one who originally proposed this change?

https://gist.github.com/texdc/5929229#file-validationresultinterface-php

Comment

User: @bakura10 Created On: 2013-10-04T09:25:19Z Updated At: 2013-10-04T09:25:19Z Body Yes @texdc . Although this one is only for the Validator component. You submitted it for InputFilter component (and I implemented it this way in the Input filter too).

Comment

User: @texdc Created On: 2013-10-04T13:06:40Z Updated At: 2013-10-04T13:06:40Z Body While I'm glad to see this implemented, you ripped the proposal straight from my gist. Credit where credit is due.

On Oct 4, 2013, at 4:25 AM, Michaël Gallego notifications@github.com wrote:

Yes @texdc . Although this one is only for the Validator component. You submitted it for InputFilter component (and I implemented it this way in the Input filter too).

— Reply to this email directly or view it on GitHub.

Comment

User: @bakura10 Created On: 2013-10-04T13:09:10Z Updated At: 2013-10-04T13:09:10Z Body Updated the description. Happy ? =)

Comment

User: @texdc Created On: 2013-10-04T17:42:34Z Updated At: 2013-10-04T17:42:34Z Body @bakura10 merci, c'est bien tout

Comment

User: @Thinkscape Created On: 2013-10-05T18:29:50Z Updated At: 2013-10-05T18:29:50Z Body I'd also add this for BC:

class AbstractValidator {
    public function isValid($value){
        return $this->validate($value)->isValid();
    }

    // [...]
}

Comment

User: @bakura10 Created On: 2013-10-05T18:33:05Z Updated At: 2013-10-05T18:33:05Z Body As it is for ZF3 we don't care about BC @Thinkscape :).

Comment

User: @Thinkscape Created On: 2013-10-05T21:25:24Z Updated At: 2013-10-05T21:25:24Z Body Let me rephrase: . ease . of . use.

It's another one in a long list of "enhancements" that promote code culture but reduce usability, so that's the least we could do (in this particular case).

Comment

User: @bakura10 Created On: 2013-10-05T21:28:20Z Updated At: 2013-10-05T21:28:20Z Body I'm not "against" that that much. The only problem with this approach is as because validators are now completely stateless, I suppose people still using that (just to avoid typing "validate($value)") will try to use this as it was before by fetching messages ($validator->getMessages()), while this obviosuly won't work, as error messages are now wrap in a simple, serializable ValidationResult object.

In fact, most of the time when dealing with validators, I think we also want the error messages, so it makes this shortcut lless useful and somewhat confusing, I think.

Comment

User: @Thinkscape Created On: 2013-10-05T22:19:00Z Updated At: 2013-10-05T22:19:00Z Body Point taken. It's ugly complex though :-\

Maybe we should take some time now and draft the whole process with a gist or two. I'd like to see the whole use case, from an array of data up to a view script displaying the message...

Comment

User: @bakura10 Created On: 2013-10-05T22:24:43Z Updated At: 2013-10-05T22:26:11Z Body Well it's not that more complex and it allows us a lot of code simplifications (specifically regarding translations). I've done the same thing in the Input Filter refactor. Basically, it works like this:

$validator = new BooleanValidator();
$result     = $validator->validate($value); // value is not stored in validator so it can be reused directly again
$result2    = $validator->validate($anotherValue);

if ($result->isValid() && $result2->isValid()) {
    // Do something
} else {
    // as validators are now stateless, messages are stored in validation result, so you can
    // get both although we executed the validator twice
    $messages = array_merge($result->getErrorMessages(), $result2->getErrorMessages());
}

A nice thing of having this simple object is that it is now serializable and json serializable. So in a REST context, you could simply do that:

return json_encode($result); // will return the error messages

The dependency to translation layer is compeltely removed in validators (see discussion above), so we could think about a thin layer in translator:

$translate = $this->translator->translate($validationResult);

Not sure about the syntax yet.

Comment

User: @Thinkscape Created On: 2013-10-06T08:29:03Z Updated At: 2013-10-06T08:29:03Z Body Hmm... ok... how would you imagine specific errors propagating down to the translator helper ? How/when would interpolation be handled ? Would it work without translator out of the box? (as it currently does)

Comment

User: @bakura10 Created On: 2013-10-06T09:56:31Z Updated At: 2013-10-06T09:56:31Z Body @Thinkscape , see this file: https://github.com/bakura10/zf2/blob/95af54febe08a2844f92738a94d0bd70db3af865/library/Zend/Validator/Result/ValidationResult.php

Interpolation is done on the ValidationResult itself, along the variables. This allow to translate error messages in multiple languages too (which is nearly impossible to do right now).

My only concern now is how to handle the specific case of ValidationChain to avoid conflicts in variables names.

Comment

User: @bakura10 Created On: 2013-10-06T12:22:06Z Updated At: 2013-10-06T12:22:06Z Body @texdc : I've made some more work today: https://github.com/bakura10/zf2/blob/14dcdd175329f56f922e377eed7c3fc1c9d29e83/library/Zend/Validator/Result/ValidationResult.php#L124

So now for serialization, I save the data, message variables and raw error messages (to be able to recreate the object). On the other hand, for jsonSerializable, I just encode the error messages (because I think this is the most sane use case as this feature will mostly be used for REST I think).

@Thinkscape : I've written tests for the ValidationResult stuff. I think it should be clear about the usage: https://github.com/bakura10/zf2/blob/14dcdd175329f56f922e377eed7c3fc1c9d29e83/tests/ZendTest/Validator/Result/ValidationResultTest.php

Comment

User: @texdc Created On: 2013-10-06T21:36:32Z Updated At: 2013-10-06T21:36:32Z Body I think you're missing the point of a dedicated result class. It should be extremely light-weight to facilitate messaging. Adding data is out-of-scope and unnecessary because the data is available before validation. Message building should (still) be handled by the validator, not the result. The result should only contain ready-to-be-consumed messages.

public function validate($data)
{
    $result = $this->validator->validate($data);
    $this->eventManager->trigger(__METHOD__, $this, compact('data', 'result'));
    return $result;
}

Comment

User: @Ocramius Created On: 2013-10-06T21:40:08Z Updated At: 2013-10-06T21:40:32Z Body @texdc the result is just a value. Translating it happens somewhere in a Zend\I18n\Validator\Something.

It's an immutable packet of data containing all what is necessary to assemble relevant bits of sentences for validation errors (to me).

The submitted data can be part of the packet in my opinion...

Comment

User: @bakura10 Created On: 2013-10-06T21:40:28Z Updated At: 2013-10-06T21:40:28Z Body Isn't that the case in my implementation? I agree that data may be removed from the ValidationResult (well, the problem is for people that would like to include the value inside the error message during interpolation).

However I don't agree with you on another point: the validator should not create the error messages itself and do the interpolation. Otherwise you miss the whole advantage of being able to separate i18n component from the validator. As the ValidationResult contains message variables AND raw error messages, a ValidationResultTransaltor can now be used to translate all the messages.

Comment

User: @texdc Created On: 2013-10-06T21:42:06Z Updated At: 2013-10-06T21:42:06Z Body @Ocramius right, but the data is not necessary and weighs the result down.

@bakura10 The value can be included in the messages when they're generated by the validator. If the validator doesn't generate messages then it should just return a boolean and let a message generator populate the result.

Comment

User: @bakura10 Created On: 2013-10-06T21:42:49Z Updated At: 2013-10-06T21:42:49Z Body Having said this, I think it even makes sense to remove the data from ValidationResult (so we only stay with raw error message and message variables, the only thing that are needed to create error messages). What do you think @Ocramius ?

Comment

User: @bakura10 Created On: 2013-10-06T21:43:49Z Updated At: 2013-10-06T21:43:49Z Body @texdc : so you're assuming the translation still happen in validator as it is now, while I saw the biggest advantage of this specific result class to allow complete separatin of concenrs.

zendframework / zend-validator