Bug: [Validation] `required_without` will not work if the field is specified with asterisk

[kenjis]

See https://github.com/codeigniter4/CodeIgniter4/issues/5922#issuecomment-1113904559

The following test fails. The validation passes.

    public function testRequireWithoutWithWildCard()
    {
        $data = [
            'a' => [
                ['b' => 1, 'c' => 2],
                ['c' => ''],
            ],
        ];

        $this->validation->setRules([
            'a.*.c' => 'required_without[a.*.b]',
        ])->run($data);

        $this->assertSame(
            'The a.*.c field is required when a.*.b is not present.',
            $this->validation->getError('a.1.c')
        );
    }

[ping-yee]

I think the description of required_without is strange.

The field is required when all of the other fields are present in the data but not required.

Is the description different from the value of getError means ?

The a..c field is required when a..b is not present.

[ping-yee]

@kenjis What kind of design should I use?

[kenjis]

Is the description different from the value of getError means ?

Yes. The explanation is the same as the following comment, but it seems different from the Example blow: https://github.com/codeigniter4/CodeIgniter4/blob/ac2ad223531210bf4624a113469cb099c35e09fe/system/Validation/Rules.php#L274-L279

[ping-yee]

The field is required when all of the other fields are present in the data but not required.

I think this explanation and description of official document of required_without[] isn't right , the right explanation should be as below.

The field is required when any of the other required fields are not present in any data.

[ping-yee]

https://github.com/codeigniter4/CodeIgniter4/blob/ac2ad223531210bf4624a113469cb099c35e09fe/system/Validation/Rules.php#L302-L304

empty(dot_array_search($field, $data))

As the conditional shows, it judge whether the param field of required_without[] isn't presented in data.

For example:

public function testRequireWithoutWithWildCard()
    {
        $data = [
            'a' => [
                ['b' => 1, 'c' => 2],
                ['c' => ''],
            ],
        ];

        $this->validation->setRules([
            'a.*.c' => 'required_without[a.*.b]',
        ])->run($data);

        $this->assertSame(
            'The a.*.c field is required when a.*.b is not present.',
            $this->validation->getError('a.1.c')
        );
    }

In this test case, a.*.c field will be required when all of a.*.b field don't present in data. If any data is presented in a.*.b field, a.*.c will not be required, so the dataset of this test case should be chaged as below.

$data = [
    'a' => [
        ['b' => '', 'c' => 2],
        ['c' => ''],
    ],
];

[kenjis]

@ping-yee Do you mean this is not a bug?

I thought that a.1.c field will be required when a.1.b field doesn't present in data. It seems we need a realistic use case.

[ping-yee]

Do you mean this is not a bug?

Yes. I think this is not a bug, it just need to change the command out and the description of official document.

I thought that a.1.c field will be required when a.1.b field doesn't present in data.

Yes. It should be that. So the data set of this case should be as below.

    public function testRequireWithoutWithWildCard()
    {
        $data = [
            'a' => [
                ['b' => '', 'c' => 2],
                ['c' => ''],
            ],
        ];

        $this->validation->setRules([
            'a.*.c' => 'required_without[a.*.b]',
        ])->run($data);

        $this->assertSame(
            'The a.*.c field is required when a.*.b is not present.',
            $this->validation->getError('a.1.c')
        );
    }

When a.0.b field doesn't present in data, the a.0.c and a.1.c field will be required. But a.1.c field doesn't have any data, it will get the error The a.*.c field is required when a.*.b is not present..

[kenjis]

When a.0.b field doesn't present in data, the a.0.c and a.1.c field will be required.

I thought the checking should be done for each array (a.0, a.1). When a.0.b field does not pass required check, the a.0.c field will be required. When a.1.b field does not pass required check, the a.1.c field will be required.

[MGatner]

I read through this whole thread and thought to myself: "has our validation system gotten too abstruse?"

I can't think of an example where I would use this, but also it seems so difficult to understand I think if I did have such a case I would break it into smaller validations.

[kenjis]

To be frank, I am not sure of the correct specifications. I would like to hold off until a use case can be found.

[ping-yee]

Sorry, I think What I express is too abstruse 😢, I think I figure out and find a bug last night.

And I want to check the process logic and design with you all.

In asterisk fields case.

    public function index()
    {
        $data = [
            'a' => [
                ['b' => 1, 'c' => ''],
                ['b' => '', 'c' => 2],
                ['c' => ''],
            ],
        ];

        $this->validation->setRules([
            'a.*.c' => 'required_without[a.*.b]',
        ])->run($data);
        
        dd($this->validation->getError('a.2.c'));
    }

To begin with, we can see these code in last run() function as below : https://github.com/codeigniter4/CodeIgniter4/blob/ac2ad223531210bf4624a113469cb099c35e09fe/system/Validation/Validation.php#L168-L176

In this case, it input the one by one a.*.c field (a.0.c, a.1.c, etc...) by using foreach into the processRules() function to judge this field whether following the rule of setRule() or not.

Now a.0.c field in first round

In processRules() function, it load the correspondence rule instances and put $value, $param, $data, $error these data into correspondence rule function (like required_with(), required_without(), etc...) as below : https://github.com/codeigniter4/CodeIgniter4/blob/ac2ad223531210bf4624a113469cb099c35e09fe/system/Validation/Validation.php#L313-L315

There is value of each required_without() params : $value => "" (This is key of setRule() field data, in this case is a.0.c field data so that is "") $param => "a.*.b" (This is the param field of required_without[] in this case, so it's a.*.b) $data => $data array (This is the data set of present in). $error => This isn't used in this case.

Here we are required_without() function, it call the required() function to judge the $str (I don't think this is a good name, $str correspond $value as shown above) whether the field value isn't empty string or not In line 293.

If it isn't empty string, means there is data in key of setRule() field this time then return true and finish the judgement of this field (because key of setRule() field have data, it will never error occur in this rule) in line 296.

If it is empty string, then we keep go on. 😓

https://github.com/codeigniter4/CodeIgniter4/blob/ac2ad223531210bf4624a113469cb099c35e09fe/system/Validation/Rules.php#L283-L308

https://github.com/codeigniter4/CodeIgniter4/blob/ac2ad223531210bf4624a113469cb099c35e09fe/system/Validation/Rules.php#L214-L229

In line 301 - 308, it have some condition to judge whether the param field(s) of required_without[] have data or not. The param field is a.*.b in this case, the condition is strpos($field, '.') !== false && empty(dot_array_search($field, $data)).

The function of dot_array_search($field, $data) is search all data of incoming field, in this case it return array of all of a.*.b data. But if there is only one field in data set, it will only return the string type without array. (This is a bug but I have fixed in PR because empty() function can't judge whether all of array value is empty or not, it will be always be false even all of array value is empty).

array(2) {
  [0]=>
  string(0) ""
  [1]=>
  string(0) ""
}

The condition judge whether the array return by dot_array_search($field, $data) function is empty or not. if it return false, then this round key of setRule() field will store in the $this->errors[$field] (like this round field a.0.c) as below. https://github.com/codeigniter4/CodeIgniter4/blob/ac2ad223531210bf4624a113469cb099c35e09fe/system/Validation/Validation.php#L340-L347

Continue to a.1.c field judge, until all of a.*.c finish judged.

And $this->validation->getError('a.1.c') get the error message with passin field in $error array.

This is all of process logic and there is a design problem will show at next comment. Thanks for reading.

[ping-yee]

I find a bug is empty(dot_array_search($field, $data)) only can judge whether the field value is empty or not in one field.

If there are more than two fields in data set, dot_array_search($field, $data) will return all of this $field values in array type, then empty() function can't judge whether all of values of this return array are empty or not (it will always return false) as shown above.

So I fixed this issue in PR, Add these to check whether dot_array_search() return data in array type or not.

if (strpos($field, '.') !== false && is_array(dot_array_search($field, $data))) {
    foreach (dot_array_search($field, $data) as $value) {
        if (empty($value)) {
            return false;
        }
    }
}

And this change can all pass in ValidationTest.php and RulesTest.php files unit test.

In asterisk field case.

But this design is when any of required_without[] param field is empty, the key of setRule() field is required (I think this is what the author design).

    public function index()
    {
        $data = [
            'a' => [
                ['b' => 1, 'c' => ''],
                ['b' => '', 'c' => 2],
                ['c' => ''],
            ],
        ];

        $this->validation->setRules([
            'a.*.c' => 'required_without[a.*.b]',
        ])->run($data);
        
        // This will error occur.
        dd($this->validation->getError('a.0.c'));

        // This will not error occur.
        dd($this->validation->getError('a.1.c'));
        
        // This will error occur.
        dd($this->validation->getError('a.2.c'));
    }

In this case, becase a.1.b field doesn't present in data, make all of a.*.c field become required.

And this is why a.0.c and a.2.c fields will get error message, becasue these two fields don't present in data.

After figure out what the author of this class design, I think this is reasonable for this design becasue it conform the word required_without[] means like:

required_without[] param field is main required, if any required_without[] param field don't present in data then the key of setRule() field need to support the main required to become second required.

I think this is make sense.

[ping-yee]

@kenjis Are these thing and use case reasonable? or anything I miss.

[kenjis]

Sorry, I don't get what you say. It is too complicated. Can you show a realistic use case? Not a or b, but something meaningful.

In this case, becase a.1.b field doesn't present in data, make all of a.*.c field become required.

It seems a.1.b field is present and its value is an empty string.

[kenjis]

In your PR: a.0.c: error a.1.c: pass a.2.c: error

But your explanation above is: error, pass, pass.

[kenjis]

In my opinion, When a.0.b field does not pass required check, the a.0.c field will be required. When a.1.b field does not pass required check, the a.1.c field will be required. When a.2.b field does not pass required check, the a.2.c field will be required.

So the results should be: pass, pass, error. But I don't know my opinion is correct or not.

[ping-yee]

It seems a.1.b field is present and its value is an empty string.

https://github.com/codeigniter4/CodeIgniter4/blob/ac2ad223531210bf4624a113469cb099c35e09fe/system/Validation/Rules.php#L214-L229

at last judge, it seems the empty string also means doesn't present the data.

But your explanation above is: error, pass, pass.

Sorry for the mistake comment out, I have been modified.

[ping-yee]

It doesn't run with a pair of field like a.0.b => a.0.c, a.1.b => a.1.c. It run with judge all of a.*.b is required or not, then decide all of a.*.c is required or not. If any of a.*.b doesn't present the data in, then all of a.*.c field need to present in data (I think this is how the field with asterisk run like).

    public function index()
    {
        $data = [
            'a' => [
                ['b' => 1, 'c' => ''],
                ['b' => '', 'c' => 2],
                ['c' => ''],
            ],
        ];

        $this->validation->setRules([
            'a.*.c' => 'required_without[a.*.b]',
        ])->run($data);
        
        // This will error occur.
        dd($this->validation->getError('a.0.c'));

        // This will not error occur.
        dd($this->validation->getError('a.1.c'));
        
        // This will error occur.
        dd($this->validation->getError('a.2.c'));
    }

So this is why a.0.c and a.2.c get fail.

[kenjis]

Thank you for your explanation! Okay, I got your logic. And it seems to be implemented well in your PR.

But I don't know it is correct as the specification. After all, it seems to depend on use cases.

In the case 3 in #5922, my opinion seems appropriate.

[ping-yee]

I got this logic after reading the Validation class as above show, and I think this is what the original developer design.

In the case 3 in https://github.com/codeigniter4/CodeIgniter4/issues/5922, my opinion seems appropriate.

I agree with this, this use case is more appropriate using your opinion because it shouldn't have dependency between accounts.

[ping-yee]

It seems to what required_without rule actually work different from what #5922 expect that validate each array separately.

I think we need:

1. modify the description of required_without[] rule.
2. add one more rule to validate each array separately.

Because now on the required_without[] rule work as original designer expect, probably it may also be someone expect and follow this to develop their own project.

@kenjis what do you think about this?

[kenjis]

I think this is what the original developer design.

I don't believe the original developer design. I think the original developer did not consider about fields with * at all. Because fields with * did not work at all. See #5938.

[kenjis]

It seems to what required_without rule actually work different from what https://github.com/codeigniter4/CodeIgniter4/issues/5922 expect that validate each array separately.

Yes, and this issue is the bug report.

[kenjis]

@ping-yee I think your PR #6541 is logical, but I can't imagine a real use case. But the use case of #5922 is a real use case. If we fix the bug, it is better to support #5922 use case.

[ping-yee]

Ok, I got it. I will close my PR then modify the code logic to support the #5922 use case.

[kenjis]

Closed by #6589