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 [email protected] 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.
Originally posted by @GeeH at zendframework/zend-validator#95