PresenceOf and Regex validators

[phalcon]

PresenceOf and Regex validators

[sergeyklay]

Actually I'm not sure that we should do that. It is good practice in the PHP world to throw exceptions.

Usually when writing a function, you should throw exceptions for error management instead of returning a boolean. Because you are relying on the developer using your function to actively check the return value and see if everything went all right. But developers are lazy. They tend to forget to add required checks. Or they don't have time and they skip error handling. So instead of returning a status code, your function should not return anything but it should throw an exception in case something goes wrong.

[Jurigag]

Well will see, maybe it's not so bad idea to just return false if type is wrong.

[TimurFlush]

@scrnjakovic And you handsome)

[TimurFlush]

I would probably call it a bug.

[sergeyklay]

/cc @niden

[TimurFlush]

Excuse me, @sergeyklay, I'm accidentally added a PresenceOf validator to the current Issue.

I totally agree with you, just sometimes some validators need to be insured against passing them an array. I suggest to add an option for validators 'returnFalseIfArrayPassed'/'returnFalseIfPassedNotString' or something like that.

I think you know what I'm getting at.

[sergeyklay]

@TimurFlush Could you please provide a code example which demonstrates your proposal and benefits (if any)?

[niden]

Please correct me if I got this wrong. Something like this?

 $validator->add(
     "created_at",
     new RegexValidator(
         [
             "pattern" => "/^[0-9]{4}[-\/](0[1-9]|1[12])[-\/](0[1-9]|[12][0-9]|3[01])$/",
             "message" => "The creation date is invalid",
             "returnFalseIfArrayPassed" => true,
         ]
     )
 );

The Phalcon\Validation\Validator as well as the ValidatorInterface accept array! as their options constructor. As such, for now if you don't pass an array, you will get the exception because it is a different type.

To allow for different types to be sent to the validator we will need to change the ValidatorInterface and add additional checks for every validator since now we are not going to be receiving an array upon construction (or at least it will not be guaranteed that we will).

Suppose we do the above, how would we change this flag so that the validator does not throw the exception and returns false?

One way I can think of is to create the validator without the constructor array. Then add the options you need (one by one) using setOption and then allow it to run.

I would say that we can go down that route but for more flexibility, we can add an option that is called: callback. That option will receive an anonymous function that returns true/false and if it validates it allows the validator to proceed, otherwise it will return false or whatever we need.

Thoughts?

[TimurFlush]

@sergeyklay, @niden already wrote the code for me. Thanks, Niden.

My knowledge of English is not very great, I can not translate your message accurately.
Just try to do so:

 $validator->add(
     "created_at",
     new RegexValidator(
         [
             "pattern" => "/^[0-9]{4}[-\/](0[1-9]|1[12])[-\/](0[1-9]|[12][0-9]|3[01])$/",
             "message" => "The creation date is invalid",
             //"returnFalseIfArrayPassed" => true, #I mean that.
         ]
     )
 );
$validator->validate(
    [
        'created_at' => ['sample', 'array', 'lol, 'kek', 'cheburek']
    ]
);

I specifically passed an array to display this problem.
And then, instead of the validator returning false, we get an array to string conversion error in \Phalcon\Validation\Validator\Regex on line 87

After all, instead of this array, the validator can have an array $_POST in which there can also be arrays =)

You either need to rewrite the current validators with a strict check for the value belonging to a certain type, or add the flag 'failIfArrayPassed'/ 'neededValueType' for the validator

I hope you understand me.

[Jurigag]

I don't understand this, can't we just throw exception if wrong type is passed and that's it? Not sure why we should handle stuff like this.

[TimurFlush]

@Jurigag Let's remember what a Validator is? Do not complicate it with exceptions, its main task is to check the incoming data for correctness and no more. In this case, the exception is a good idea on the one hand, but on the other hand the validator ceases to be the validator that returns true or false.

This is only my opinion, even if it violates the current development standards. We sacrifice standards for the good)

Still, if my idea led you to the exceptions, please, I don't mind. With this you need to do something and do not miss.

[scrnjakovic]

On one hand, I'm with @sergeyklay and @Jurigag on this one. Phalcon\Validatation\Validator\Regex is meant to perform Regex validation, not type checking and type validations.

But on the other hand, let's say you need me to bring you student ID so I can take the exam, but I bring you my library membership card instead. What will you do? Will you panic and say "Everybody stop! He brought library membership instead of student ID!" or will you inform me I failed the requirement and cannot take the exam?

Actually, the more I think about it, the more I agree with @TimurFlush. The point of validators is to validate user input which is by default assumed to be fishy and by definition out of developers control. Invalid input shouldn't be able to break the validators, but have them fail.

So I side with @TimurFlush on this one.

[niden]

Closing in favor of phalcon/cphalcon#13855. Will revisit if the community votes for it, or in later versions.