Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kenjis opened this issue Apr 30, 2022 · 26 comments
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@kenjis
Copy link
Member

kenjis commented Apr 30, 2022

See #5922 (comment)

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')
        );
    }
@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Apr 30, 2022
@kenjis kenjis changed the title Bug: [Validation] required_without will not work if the field is specified with a wildcard (asterisk) Bug: [Validation] required_without will not work if the field is specified with dot-notation Apr 30, 2022
@kenjis kenjis changed the title Bug: [Validation] required_without will not work if the field is specified with dot-notation Bug: [Validation] required_without will not work if the field is specified with asterisk May 1, 2022
@ping-yee
Copy link
Contributor

ping-yee commented Sep 6, 2022

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
Copy link
Contributor

ping-yee commented Sep 9, 2022

@kenjis What kind of design should I use?

@kenjis
Copy link
Member Author

kenjis commented Sep 14, 2022

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:

* The field is required when all of the other fields are present
* in the data but not required.
*
* Example (field is required when the id or email field is missing):
*
* required_without[id,email]

@ping-yee
Copy link
Contributor

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
Copy link
Contributor

if ((strpos($field, '.') === false && (! array_key_exists($field, $data) || empty($data[$field]))) || (strpos($field, '.') !== false && empty(dot_array_search($field, $data)))) {
return false;
}

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
Copy link
Member Author

kenjis commented Sep 14, 2022

@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
Copy link
Contributor

ping-yee commented Sep 15, 2022

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
Copy link
Member Author

kenjis commented Sep 15, 2022

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
Copy link
Member

MGatner commented Sep 15, 2022

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
Copy link
Member Author

kenjis commented Sep 15, 2022

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
Copy link
Contributor

ping-yee commented Sep 16, 2022

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 :

if (strpos($field, '*') !== false) {
// Process multiple fields
foreach ($values as $dotField => $value) {
$this->processRules($dotField, $setup['label'] ?? $field, $value, $rules, $data, $field);
}
} else {
// Process single field
$this->processRules($field, $setup['label'] ?? $field, $values, $rules, $data);
}

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 :

$passed = $param === false
? $set->{$rule}($value, $error)
: $set->{$rule}($value, $param, $data, $error);

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. 😓

public function required_without($str = null, ?string $fields = null, array $data = []): bool
{
if ($fields === null || empty($data)) {
throw new InvalidArgumentException('You must supply the parameters: fields, data.');
}
// If the field is present we can safely assume that
// the field is here, no matter whether the corresponding
// search field is present or not.
$fields = explode(',', $fields);
$present = $this->required($str ?? '');
if ($present) {
return true;
}
// Still here? Then we fail this test if
// any of the fields are not present in $data
foreach ($fields as $field) {
if ((strpos($field, '.') === false && (! array_key_exists($field, $data) || empty($data[$field]))) || (strpos($field, '.') !== false && empty(dot_array_search($field, $data)))) {
return false;
}
}
return true;
}

public function required($str = null): bool
{
if ($str === null) {
return false;
}
if (is_object($str)) {
return true;
}
if (is_array($str)) {
return $str !== [];
}
return trim((string) $str) !== '';
}

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.

$this->errors[$field] = $error ?? $this->getErrorMessage(
$rule,
$field,
$label,
$param,
(string) $value,
$originalField
);

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
Copy link
Contributor

ping-yee commented Sep 16, 2022

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
Copy link
Contributor

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

@kenjis
Copy link
Member Author

kenjis commented Sep 21, 2022

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
Copy link
Member Author

kenjis commented Sep 21, 2022

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

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

@kenjis
Copy link
Member Author

kenjis commented Sep 21, 2022

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
Copy link
Contributor

ping-yee commented Sep 21, 2022

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

public function required($str = null): bool
{
if ($str === null) {
return false;
}
if (is_object($str)) {
return true;
}
if (is_array($str)) {
return $str !== [];
}
return trim((string) $str) !== '';
}

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
Copy link
Contributor

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
Copy link
Member Author

kenjis commented Sep 21, 2022

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
Copy link
Contributor

ping-yee commented Sep 21, 2022

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 #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
Copy link
Contributor

ping-yee commented Sep 21, 2022

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
Copy link
Member Author

kenjis commented Sep 21, 2022

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
Copy link
Member Author

kenjis commented Sep 21, 2022

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

Yes, and this issue is the bug report.

@kenjis
Copy link
Member Author

kenjis commented Sep 21, 2022

@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.

@kenjis kenjis mentioned this issue Sep 22, 2022
2 tasks
@ping-yee
Copy link
Contributor

ping-yee commented Sep 22, 2022

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

@kenjis
Copy link
Member Author

kenjis commented Oct 1, 2022

Closed by #6589

@kenjis kenjis closed this as completed Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants