Empty input partially invalid #124

Closed
mf opened this Issue May 15, 2013 · 6 comments

5 participants

@mf
mf commented May 15, 2013

Guys,

I am probably losing my mind, but according to all my tests ->validate() WILL fail on all empty input (all rules), but assert passes fine. (Obviously not using notEmpty)

Am I missing something? For example tests for phone validator pass for empty input (refer to tests)
Unless of course the validation rules automatically disallow empty input since they validate certain criteria like length, in which case I think we should update the docs since they state that empty input should be passed through unless a NotEmpty validator is applied.

Here are some details:

Validators:

    $nameValidator      = Validation\Validator::create()->string()->length(5,100);
    $phoneValidator     = Validation\Validator::create()->string()->phone();
    $voteValidator      = Validation\Validator::create()->numeric()->between(1,20,true);
    $ageValidator       = Validation\Validator::create()->numeric()->between(1,120, true);
    $genderValidator    = Validation\Validator::create()->numeric()->between(1,2, true);
    $countryValidator   = Validation\Validator::create()->numeric()->between(1,200, true);

$age = ''; // All other inputs are empty as well
$phone = ''; // etc...

Trying assert does not generate an exception

    try {
        $phoneValidator->assert($phone);
    } catch (Exception $e) {
         echo $e->getFullMessages();
    }

Trying to validate fails

if (!$phoneValidator->validate($phone)) {
    echo 'error';
}

Adding var_dump($v); on line 38 in AllOf.php rule generates:
As you can see for some reason NotEmpty is being applied?

object(Respect\Validation\Rules\NotEmpty)#14 (2) {
["name":protected]=>
NULL
["template":protected]=>
NULL
}
object(Respect\Validation\Rules\NotEmpty)#22 (2) {
["name":protected]=>
NULL
["template":protected]=>
NULL
}
object(Respect\Validation\Rules\Numeric)#30 (2) {
["name":protected]=>
NULL
["template":protected]=>
NULL
}
object(Respect\Validation\Rules\Numeric)#35 (2) {
["name":protected]=>
NULL
["template":protected]=>
NULL
}
object(Respect\Validation\Rules\Numeric)#40 (2) {
["name":protected]=>
NULL
["template":protected]=>
NULL
}

@nickl-
Respect member

No you're not going crazy (says the crazy guy) and its nothing to fear, it is simply us eating our own dog food

/* excerpt from AbstractRule line 19 */
public function __invoke($input)
    {
        return !is_a($this, __NAMESPACE__.'\\NotEmpty')
            && $input === '' || $this->validate($input);
    }

Making sense?

@mf
mf commented May 20, 2013

Ok, in this case which way are we going to do this? Either let's change the code to allow empty input OR update the documentation to reflect the change. (Also, how come it was not throwing the exception?)

@nickl-
Respect member

I am not following, is this not working as expected?

P.S. excuse the late reply...

@shanesmith

I think @mf has a point that there's something counter-intuitive here.

I had assumed, as I'm sure most would, that assert() would always throw an exception when validate() is false.

Personally I'd rather see a fix that would make the above be true, although I understand this would break compatibility. Otherwise, if this is intended, perhaps what would be needed is just an update to the readme under the "Input Option" header, which should somehow specify that input is only treated as optional in the assert() and check() methods, and not validate().

@augustohp augustohp added this to the 0.5.1 milestone Feb 15, 2014
@augustohp
Respect member

I agree with @shanesmith on changing the README for 0.5 versions so we don't break compatibility.

I also agree the Exception is expected, but I think it is a different issue altogether (so we can break compatibility on it and leave this one compatible and solved).

@augustohp augustohp added Discussion and removed Easy Pick labels Feb 16, 2014
@augustohp augustohp modified the milestone: 0.7.0, 0.6.1 Feb 17, 2014
@augustohp augustohp modified the milestone: Backlog, 0.7.0 May 9, 2014
@henriquemoody
Respect member

Will be fixed on version 1.0, see changelog for more info about the version.

To fix this issue now all rules are mandatory, but you can use optional() rule to handle optional values, which now is null and an empty string by default.

@henriquemoody henriquemoody modified the milestone: 1.0, Backlog Oct 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment