[8.x] Make $validator->sometimes() item aware to be able to work with nested arrays

[NickSdot]

#38385 got closed by @taylorotwell with the comment below.

I don't think I would want an entirely different method here.

So here I am with another try. Hope it's better now and has a chance to get merged. :-)

Problem

It is not possible to use $validator->sometimes() with nested arrays (e.g. inwithValidator($validator) of a FormRequest class). Given a request like this:

public function rules(): array
    {
        return [
            "channels"    => "required|array",
            "channels.*.type"  => "required|string|min:3",
            "channels.*.value" => "required|string|distinct|min:3",
            "channels.*.description"  => "nullable|string|min:3|max:80",
        ];
    }

The value can be an url, email or nullable – depending on an other field. So it must be possible to validate it as such, without knowing the key. $validator->sometimes() is not aware of the current item/index. Also the new Rule::when() does not support it. Hence, currently there is no flexible way to to apply specifc rules for nested arrays, to archieve someting like this without tons of foreach or a custom validator:

$validator->sometimes('channels.*.value', 'email:rfc,dns', function($input, $item) {
  return $item->type === 'email';
});

$validator->sometimes('channels.*.value', 'url', function($input, $item) {
  return $item->type !== 'email';
});

$validator->sometimes('channels.*.value', 'nullable', function($input, $item) {
  return $item->type === null;
});

Benefits for users

No need to create custom validators, traits or to use tons if foreach. Because sometimes() is aware of the item data, users can directly access it in beside of the already existing$input data.

Non breaking

This PR adds new functionality without affecting the previous behavior. All tests are still passing and new tests got added. Since there is an (optional) second parameter passed to the callback and the item data is accessable from a Fluent object, it's worth to add this to the documentation (keen to do it if the PR gets accepted)

[taylorotwell]

I'm not sure I totally understand the behavior in this situation:

I end up with the following rules:

I would have expected it to validate that every item in the users array is an array. However, it only verifies that users.0 is an array.

[NickSdot]

You are right @taylorotwell – sorry for that.

Obiviously my tests are not good enough and it seems the existing tests also not cover all cases.

Marked the PR as draft. I will rework it and will mark it for review when done.

[NickSdot]

So, I had a fundamental logic glitch in my first attempt which was not covered by the current tests. My bad that I missed it. Sorry again. Revised the whole thing and added better tests. Commit is ready for review.

For better readability I moved the part to prepare the item data to a private method. In case it's prefered, I can move it back to sometimes().

Here the asserts I've added. If anyone has other cases in mind, I am keen to add them.

// ['users'] -> if users is not empty it must be validated as array
// ['users'] -> if users is null no rules will be applied
// ['company.users'] -> if users is not empty it must be validated as array
// ['company.users'] -> if users is null no rules will be applied
// ['company.*'] -> if users is not empty it must be validated as array
// ['company.*'] -> if users is null no rules will be applied
// ['users.*'] -> all nested array items in users must be validated as array
// ['company.users.*'] -> all nested array items in users must be validated as array
// ['company.*.*'] -> all nested array items in users must be validated as array
// ['user.profile.value'] -> multiple true cases, the item based condition does match and the optional validation is added
// ['user.profile.value'] -> multiple true cases with middle wildcard, the item based condition does match and the optional validation is added
// ['profiles.*.value'] -> true and false cases for the same field with middle wildcard, the item based condition does match and the optional validation is added
// ['profiles.*.value'] -> true case with middle wildcard, the item based condition does match and the optional validation is added
// ['profiles.*.value'] -> false case with middle wildcard, the item based condition does not match and the optional validation is not added
// ['users.profiles.*.value'] -> true case nested and with middle wildcard, the item based condition does match and the optional validation is added
// ['users.*.*.value'] -> true case nested and with double middle wildcard, the item based condition does match and the optional validation is added
// 'user.value' -> true case nested with string, the item based condition does match and the optional validation is added
// 'user.value' -> standard true case with string, the INPUT based condition does match and the optional validation is added
// ['value'] -> standard true case with array, the INPUT based condition does match and the optional validation is added
// ['email'] -> if value is set, it will be validated as string

cc @taylorotwell

[NickSdot]

One thing is bugging me and I have an improvement for it in my mind. It actually would be cool to add another parameter, to be able to define more specific which data we want to use for the condition. Right now we can only use sibling data (same deepth as the validated value) to validate – no data from a higher level.

If we add an optional parameter:

sometimes($attribute, $rules, callable $callback, $conditionDataGetTarget = null)

Then we could use any kind of data from the input without knowing the index/key like:

// assumed something like 'task.project.company.users.*.profile.value' we can do
$v->sometimes(['value'], 'foo', function ($i, $item) {
    return $item->company->type === 'bar';
}, 'task.project');

In alternate we could pass an integer to define how many level we want to go up from the given attribute. Or we directly could pass any array/fluent data we want, which also would be nice.

I assume this would be a b/c break. The interface would need to be changed to allow another parameter. But if this is something you would consider a good idea, I could at least prepare it with a dummy var in the current PR to avoid to touch this for 9.x again to not introduce another b/c break.

Just let me know. :-)

[taylorotwell]

@NickSdot doesn't the callback already receive the entire input array? If you have another target in mind you could just access it off of that array?

[NickSdot]

@NickSdot doesn't the callback already receive the entire input array? If you have another target in mind you could just access it off of that array?

That's correct. But then we would be back to a similiar situation as why I've created this PR. We would need to know about each levels keys. My example above maybe was not the best one. Imagine a case like:

teams.*.users.*.profile.value

What I had in mind was trying to introduce some magic by matching both, the string for the validation in foreach ($response->rules as $ruleKey => $ruleValue) and the 'data target', and then using the data of the given level.

But yeah, fair enough, this is probably worth a whole new PR. Since we anyway talk about the private method makeSometimesItemData() it should not really matter to see this as fully separated. I'll wrap my head a bit more around ity if you don't mind.

---

Any other test cases for the current PR you would like to see?

[taylorotwell]

I found this behavior somewhat surprising, but you can tell me what you think. I would have expected $item (within the sometimes callback) to be the user array, not the entire array of users.

[NickSdot]

It's intentional. We always go one level up, if the given attribute has not only one level. Which must be documented of course.

See: https://github.com/laravel/framework/pull/38443/files#diff-208537657c08f4e2e903a8d449e70741ba161c8f004b0b2c315e317576f1bf2bR1132

Otherwise we would not be able to do stuff like this, which the PR is made for:

$validator->sometimes('channels.*.value', 'email:rfc,dns', function($input, $item) {
  return $item->type === 'email';
});

Given the attribute channels.*.value, which we want to validate conditionally against channels.*.type, means we need to go one level up to channels.*. to be able to return the data like $item->type. Otherwise $item would return the value in *.value itself.

I indeed was trying everything to make this more flexible, but I came to the conclusion that this is the situation where the mentioned extra parameter would need to come into play.

We sadly can not just assume that if the given attribute ends with * that we don't need to go one level up.
Neither we can check whether the returned data e.g. is an array. This test case (just added and comitted) should show why. The given attribute also ends with *, but it's not a nested array as with users.*, means it would get treaten differently.

// ['attendee.*'] -> if attendee name is set, all other fields will be required as well
$trans = $this->getIlluminateArrayTranslator();
$v = new Validator($trans, ['attendee' => ['name' => 'Taylor', 'title' => 'Creator of Laravel', 'type' => 'Developer']], ['attendee.*'=> 'string']);
$v->sometimes(['attendee.*'], 'required', function ($i, $item) {
     return (bool) $item->name;
});
$this->assertEquals(['attendee.name' => ['string', 'required'], 'attendee.title' => ['string', 'required'], 'attendee.type' => ['string', 'required']], $v->getRules());

Imho it's a good solution to consistently go one level up and don't try to introduce to much magic cases here. For everything else you have the current behavior (with some disadvantages), and we could add the extra parameter as mentioned in the other comments.

If you have a better idea, feel free to hint me in the right direction and I'll look into it.

[taylorotwell]

Your example with the attendee doesn't make that much sense to me. If name is set then every other field the user passes in the request is required? That sounds like a meaningless rule. "Everything they pass is required"?

Not trying to make things more complicated but just trying to determine if that type of rule setup where attendee.* actually refers to an attendee struct that is kind of more like an object than an array even makes sense or would be common in Laravel applications.

[NickSdot]

Your example with the attendee doesn't make that much sense to me. If name is set then every other field the user passes in the request is required? That sounds like a meaningless rule. "Everything they pass is required"?

Correct me if I misunderstood you, but that would not be "Everything they pass is required". It's "If name is passed everything else is required too".

Rules based on the test:

 "attendee.name" => array:2 [
    0 => "string"
    1 => "required"
  ]
  "attendee.title" => array:2 [
    0 => "string"
    1 => "required"
  ]
  "attendee.type" => array:2 [
    0 => "string"
    1 => "required"
  ]

[NickSdot]

Not trying to make things more complicated but just trying to determine if that type of rule setup where attendee.* actually refers to an attendee struct that is kind of more like an object than an array even makes sense or would be common in Laravel applications.

Tbh, not sure why this would not make sense (based on the given 'required if' example). But let's assume for a moment that it is not common. What do you think it would change or should change about the behavior?

[taylorotwell]

So, in my original example with the users, what if I needed to know exactly what user I'm applying a rule to? How can I determine that? I don't seem to have any index to be able to access the correct user.

[NickSdot]

Correct, atm you don't. This is where the extra parameter would come into play. I – currently – see no way to achieve both things in a clean way without extra parameter. There always would be a trade offs for one or the other.

Could you please give me a real world example for the case you are talking about? Keen to mess around with it.

[NickSdot]

Do I assume right, that you not really want to have an extra parameter to add the extra flexibilty?

If so, what about we attach :n to the given attribute? For instance: channels.*.value:2 would validate channels.*.value with data available from 2 levels up = channels.

To use : as an separator would be pretty similar to the concept in the validation rules itself. Like: email:rfc,dns.

[taylorotwell]

I think that's pretty confusing honestly and will be hard to document.

I just personally found it "surprising" that users.* didn't give me each individual user array as the $item. I think others may find that surprising as well. I know you are trying to avoid "magic" but it does seem to me like we would the framework to behave as most people might expect in these situations.

[NickSdot]

Yes, I got you. Did you see my comment I've posted a bit before your last one? What do you think about it?

Otherwise, honestly, I not even yet have an idea how this magic could look like. Given that we then must not go one level up, if the given attribute ends with .* – how would we deal with this case here?

// ['company.*'] -> if users is not empty it must be validated as array
$trans = $this->getIlluminateArrayTranslator();
$v = new Validator($trans, ['company' => ['users' => [['name' => 'Taylor'], ['name' => 'Abigail']]]], ['company.users.*.name'=> 'required|string']);
$v->sometimes(['company.*'], 'array', function ($i, $item) {
    return $item->users !== null;
});
$this->assertEquals(['company.users' => ['array'], 'company.users.0.name' => ['required', 'string'], 'company.users.1.name' => ['required', 'string']], $v->getRules());

Maybe I think too complicated and then the given attribute must be explicit company.users instead of company.*? Is that where you are coming from?

Thanks for taking the time btw. Much appreciated! ❤️

[taylorotwell]

I just think if the attribute ends in * you should pass each element of that array as the $item. So, the "one level up" thing would not apply in that case.

In your example you gave earlier (copying here):

// ['attendee.*'] -> if attendee name is set, all other fields will be required as well
$trans = $this->getIlluminateArrayTranslator();
$v = new Validator($trans, ['attendee' => ['name' => 'Taylor', 'title' => 'Creator of Laravel', 'type' => 'Developer']], ['attendee.*'=> 'string']);
$v->sometimes(['attendee.*'], 'required', function ($i, $item) {
     return (bool) $item->name;
});
$this->assertEquals(['attendee.name' => ['string', 'required'], 'attendee.title' => ['string', 'required'], 'attendee.type' => ['string', 'required']], $v->getRules());

It's not really a problem because attendee is not a two-dimensional array. It's one-dimensional. So, you can just access whatever you need from the entire input array ($input['attendee']['title']) instead of using the "item" that is passed as the second argument.

[NickSdot]

Ok. I've changed it as requested (not commited yet): don't go one level up, if the attribute passed to sometimes() ends with a wildcard.

Before I commit it I have one more question. Given your example from above:

$trans = $this->getIlluminateArrayTranslator();
$v = new Validator($trans, ['users' => [['name' => 'Taylor'], ['name' => 'Abigail']]], ['users.*.name'=> 'required|string']);
$v->sometimes(['users.*'], 'array', function ($i, $item) {
    return true;
});
$this->assertEquals(['users.0' => ['array'], 'users.1' => ['array'], 'users.0.name' => ['required', 'string'], 'users.1.name' => ['required', 'string']], $v->getRules());

dump($item) before the change:

  #attributes: array:2 [
    0 => array:1 [
      "name" => "Taylor"
    ]
    1 => array:1 [
      "name" => "Abigail"
    ]
  ]

dump($item) after the change:

  #attributes: array:1 [
    "name" => "Taylor"
  ]
}
  #attributes: array:1 [
    "name" => "Abigail"
  ]

Is this really what you want? Maybe I am missing something, but what is the benefit of validating users.* as an array based on a value ($item->name) from this exact array? Thanks.

[taylorotwell]

You are given the entire user array, not just one attribute - that array just happens to have one attribute. Again, I am just working off principle of least surprise here. Getting the entire array of users is never helpful IMO. I can already access that using a named key. I would rather get the particular user array I am actually adding rules to.

Will review this as it stands now.

[NickSdot]

Done.

[NickSdot]

Thanks for merging! ❤️ I'll start working on the docs on Saturday.

[jasonlewis]

@NickSdot @taylorotwell

This appears to have broken sometimes conditions that add rules for sibling values within an array of data.

Example:

$data = [
    'users' => [
        ['name' => 'Bob', 'starts' => '2021-09-01', 'ends' => '2021-09-20'],
    ],
];

$validator = Validator::make($data, []);
$validator->sometimes('users.*.starts', ['before:users.*.ends'], function () {
    return true;
});
$validator->passes();

The above fails as of 8.57.0.

I haven't had a chance to dig into this yet but from a cursory glance it seems the addRules method no longer expands the asterisks in the conditionally added rules. If I get a chance later today or this week I'll take a bit more of a gander.

[taylorotwell]

@NickSdot able to fix this?

[NickSdot]

@taylorotwell I'll have a look and come back to it ASAP.

[taylorotwell]

@NickSdot any ideas?

[NickSdot]

@taylorotwell was digging yesterday evening, but not yet there why this happens. Could also be an issue with the validation rule parser.

6am here, just woke up. Will post a follow up after breakfast and will at least provide a hot fix within the next few hours.

[NickSdot]

@taylorotwell

I did sent PR #38899 as a hotifx to avoid revert.

There might be a better way. I would need more time to dig into this. Is this ok for now?

[bakerkretzmar]

@NickSdot it sounds like you may already be planning a follow up PR to address this, but I just wanted to add another example for you to consider. I've been working on some complex validation stuff the last couple days and re-read this thread several times to figure out what's going on, in the end I'm doing something custom but your work here came pretty close to what we needed!

We have a request with an array of fields in the input, and each field has a type and several parameters. The parameters need to be validated individually, but they're different depending on the type. The input looks something like this:

$input = [
    'fields' => [
        [
            'name' => 'first_field',
            'type' => 'integer',
            'parameters' => [
                'default' => 10,
            ],
        ],
        [
            'name' => 'second_field',
            'type' => 'string',
            'parameters' => [
                'default' => 'Hello world',
            ],
        ],
    ],
];

And the validation rules I want to generate for that input look something like this:

$rules = [
    'fields' => ['array'],
    'fields.*' => ['array'],
    'fields.*.name' => ['required'],
    'fields.*.type' => ['required'],
    'fields.*.parameters' => ['array'],
    'fields.0.parameters.default' => ['integer'],
    'fields.1.parameters.default' => ['string'],
];

Notice that the default parameter is validated differently depending on the type of the field.

$validator->sometimes() almost works for this except that I'm not adding rules at the same level that I want to loop over. I need to add rules to fields.*.parameters.default but I want to loop over fields.*. So this:

$validator->sometimes('fields.*.parameters.default', 'string', fn ($input, $item) => $item->type === 'string');

doesn't work, because $item is the field parameters (fields.*.parameters) instead of the whole field (fields.*). Being able to customize the nesting level that the $item is retrieved from would solve this really nicely. Based on the example you showed above I assume that could look something like this:

$validator->sometimes(
    'fields.*.parameters.default',
    'string',
    fn ($input, $item) => $item->type === 'string',
    'fields.*'
);

[NickSdot]

Yeah, how you want it is how my initial PR worked. But Taylor was not happy with it, so I modified the behavior as requested and eventually merged.

Maybe he changes his mind after seeing this real life example? @taylorotwell

If so, I would be keen to look into it on how we can make it flexible to get it working with both scenarios (while keeping the current behavior as default).

[bakerkretzmar]

Yeah I wonder if it would be possible to pass the index into the sometimes callback, that seems like it would be fully backwards compatible and it would make my use case straightforward:

$validator->sometimes(
    'fields.*.parameters.default',
    'string',
    fn ($input, $item, $index) => $input->fields[$index]->type === 'string',
);

[NickSdot]

Seems like Taylor is not following here. Maybe you just PR it by yourself.