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

Wrong assoc array detection #332

Closed eugene-borovov closed 3 years ago

eugene-borovov commented 3 years ago

In validateIn and validateListContains method threats non-sequential array as associative.

  1. [0 => 'A', 1 => 'B', 2 => 'C'] is an indexed array with sequential indexes
  2. [1 => 'A', 2 => 'B', 3 => 'C'] is an indexed array with non-sequential indexes
  3. ['0' => 'A', '1' => 'B', '2' => 'C'] is an associative array with numeric keys
  4. ['a' => 'A', 'b' => 'B', 'c' => 'C'] is an associative array with string keys

Maybe you should soften the check of the associative array?

return count(array_filter(array_keys($array), 'is_string')) > 0

https://stackoverflow.com/a/4254008

The array_diff function and the array_intersect function preserve keys, so the indexed array suddenly turns into an associative array.

s-titov commented 3 years ago

Any news?

willemwollebrants commented 3 years ago

Sorry I've been drowning in work :/ Can you give me a piece of code to outline the problem you're facing?

eugene-borovov commented 3 years ago

These pieces of code are logically equivalent. But the later checks keys not values. It's weird cause its contract remind the in_array function.

$v = new Valitron\Validator(['color' => 'purple']);
$v->rules([
    'in' => [
        ['color', ['blue', 'green', 'red', 'purple']] // check values
    ]
]);
$v->validate();
$v = new Valitron\Validator(['color' => 'purple']);
$v->rules([
    'in' => [
        ['color', array_diff(['blue', 'green', 'red', 'purple'], ['green', 'blue'])] // check keys
    ]
]);
$v->validate();

I agree that validateListContains should check the array for a sequence of keys, but validateIn should not.

willemwollebrants commented 3 years ago

I've been doing a bit of work on this, and I'm almost there I think. See: https://github.com/vlucas/valitron/commit/500472808c1434c1ca06bc1f8b607932bf953a78

Your suggestion works great for non-sequential numeric keys, but not for your 3th case:

$input = array('3'=>'green', '2'=>'purple');
count(array_filter(array_keys($input), 'is_string')); // returns 0

Reading the php documentation, this is expected behavior:

Strings containing valid decimal ints, unless the number is preceded by a + sign, will be cast to the int type. E.g. the key "8" will actually be stored under 8. On the other hand "08" will not be cast, as it isn't a valid decimal integer.

eugene-borovov commented 3 years ago

@willemwollebrants Thanks for your work. I should have done it. :) What do you think about the whole idea treat arrays with non-sequential numeric keys as indexed? Does it need a third parameter here to control the search mode?

s-titov commented 3 years ago

@willemwollebrants please look at my case

willemwollebrants commented 3 years ago

@Nuboskill this is fixed in the new code which uses a different way to detect associative arrays, mentioned above:

http://sandbox.onlinephpfunctions.com/code/5709c08ff1f557c8664e8061cce8601e67cb9a6e

willemwollebrants commented 3 years ago

@eugene-borovov I made an update here to add a parameter: https://github.com/vlucas/valitron/commit/be8cb9e074ce2dbf1a47d8a61a450f8fb2712420

It seems to work but I'm not sure if I like it 100% :)

s-titov commented 3 years ago

@willemwollebrants thanks for fix. When will a new release available?

willemwollebrants commented 3 years ago

As soon as possible, I'm looking at a few more fixes but should be today or tomorrow I hope

eugene-borovov commented 3 years ago

Your suggestion works great for non-sequential numeric keys, but not for your 3th case:

The third case is impossible in PHP as mentioned above.

@willemwollebrants

I made an update here to add a parameter: be8cb9e

Thanks, but I`d prefer force indexed array because current implementation false positive for associative array, doesn`t it?

willemwollebrants commented 3 years ago

@eugene-borovov

Thanks, but Id prefer force indexed array because current implementation false positive for associative array, doesnt it?

Not anymore, I think:

$v = new Valitron\Validator(array('color' => 'purple'));

$v->rules(array(
     'in' => array(
        array('color', array(3=>'green', 2=>'purple'))
    )
));

$this->assertTrue($v->validate());
eugene-borovov commented 3 years ago

@willemwollebrants

Should you check "$forceAsAssociative " first to avoid a performance penalty?

if ($this->isAssociativeArray($params[0]) || $forceAsAssociative) {
  $params[0] = array_keys($params[0]);
}

Thank you very much I look forward to the release.

willemwollebrants commented 3 years ago

Good call, fixed and merged