vlucas / valitron

Valitron is a simple, elegant, stand-alone validation library with NO dependencies
BSD 3-Clause "New" or "Revised" License
1.57k stars 250 forks source link

Using array validation on something that isn't an array causes a PHP warning #182

Open thinkjson opened 7 years ago

thinkjson commented 7 years ago

If you do something like this:

<?php

require './vendor/autoload.php';
use Valitron\Validator;
$v = new Validator(['arrayTest' => '300']);
$v->rule('integer', ['arrayTest.*']);
var_dump($v->validate());

Then you get the following PHP warning:

PHP Warning:  Invalid argument supplied for foreach() in vendor/vlucas/valitron/src/Valitron/Validator.php on line 831
PHP Stack trace:
PHP   1. {main}() validation_test.php:0
PHP   2. Valitron\Validator->validate() validation_test.php:7
PHP   3. Valitron\Validator->getPart() vendor/vlucas/valitron/src/Valitron/Validator.php:868
PHP   4. Valitron\Validator->getPart() vendor/vlucas/valitron/src/Valitron/Validator.php:855
willemwollebrants commented 7 years ago

Indeed.

There are a few possible fixes I think:

I'm going to do a bit of thinking on this

thinkjson commented 7 years ago

I mean, ideally it would just fail validation and everything would be happy.

willemwollebrants commented 7 years ago

But why should it fail validation? What error would you expect to receive in this case?

The "valitron way" would probably be not to run the validation for arrayTest.* at all I guess. We're telling the validator "here are some rules for all the elements of an array called arrayTest", but when there is no such array it's perhaps the same as when there are rules for a field that does not exist in the data. Unless such a non-existing field has a 'required' rule, it will not be validated.

This would allow something like this:

$v = new Validator(array('foo'=>'bar'));
//must be an array
$v->rule('array', 'foo'); //this will return an error 
//all elements of the array must be integer
$v->rule('integer', 'foo.*'); //this won't run
thinkjson commented 7 years ago

I like that, ignore it if required it false, and fail validation if required is true. Is that what you're recommending?

willemwollebrants commented 7 years ago

It's not a recommendation, it's what the could should do after the bugfix. For now it's probably easiest if you cast 'arrayTest' to an array before adding it to the validator :(