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

[9.x] Add ability to generate unique validation rules per nested array element (repush) #40498

Merged
merged 6 commits into from
Jan 22, 2022
Merged

[9.x] Add ability to generate unique validation rules per nested array element (repush) #40498

merged 6 commits into from
Jan 22, 2022

Conversation

stevebauman
Copy link
Contributor

@stevebauman stevebauman commented Jan 19, 2022

Description

This PR introduces the ability to generate unique validation rules per nested array element, granting you direct access to each value supplied the request, the attribute name, and the whole data set in case other attribute values are needed.

This will be done using a new Rule static method: Rule::nested($callback).

Purpose

Generation of unique validation rules in nested arrays is currently pretty difficult, especially if you use any sort of custom validation rules. There is no built in way to generate validation rules taking into account the current iteration's value, and you'll have to set the indexes and such manually inside of a $rules array prior to sending it to the validator.

This also becomes quite problematic because we need to trust the data structure that was sent to us in the request and then iterate over it to dynamically generate our rules prior to actual validation taking place.

Example

Before:

$rules = [
    'companies.*.id' => [Rule::exists(Company::class, 'id')],
];

foreach ($request->companies as $index => $company) {
    $rules["companies.{$index}.id"] = [new HasPermission('manage-company', $company['id'])];
}

Validator::validate($request->all(), $rules);

After:

$rules = [
    'companies.*.id' => Rule::nested(function ($attribute, $value) {
        return [
            Rule::exists(Company::class, 'id'),
            new HasPermission('manage-company', $value)
        ];
    }),
];

Validator::validate($request->all(), $rules);

Another example where this is extremely handy is the validation of complex nested relationships:

$rules = [
    'companies.*.id' => Rule::nested(function ($attribute, $value) {
        return [
            Rule::exists(Company::class, 'id'),
            new HasPermission('manage-company', $value)
        ];
    })
    'companies.*.locations.*.id' => Rule::nested(function ($attribute, $value, $data) {
        // $attribute = 'companies.0.locations.0.id'
        $company = Str::before($attribute, '.locations');

        return Rule::exists(Locations::class, 'id')->where('company_id', $data["$company.id"]);
    }),
];

You may also supply an array of rules as you would normally with nested instances, as well as supply multiple nested instances:

$rules = [
    'companies.*.id' => [
        Rule::exists(Company::class, 'id'),
        Rule::nested(function ($attribute, $value) {
            return [new HasPermission('manage-company', $value)];
        }),
        Rule::nested(function ($attribute, $value) {
            // ...
        }),
    ],
];

Validation of arrays is super tricky stuff. I think this greatly simplifies the developer experience, allowing us not to think too much about its complexities.


Please let me know if you have any questions/comments/concerns. Thanks so much for your time! ❤️


Apologies for having to re-push this -- my git skills are not the greatest 😅

@taylorotwell
Copy link
Member

In this example:

    'items.*' => Rule::nested(function () {
        return ['discounts.*.id' => 'distinct'];
    }),

What if you wanted to do something like 'discounts.*.id' => ['distinct', 'another-rule-here']?

@stevebauman
Copy link
Contributor Author

stevebauman commented Jan 19, 2022

What if you wanted to do something like 'discounts.*.id' => ['distinct', 'another-rule-here']?

We could do:

'items.*' => Rule::nested(function () {
    return ['discounts.*.id' => ['distinct', 'another-rule']];
}),
'items.*' => Rule::nested(function () {
    return ['discounts.*.id' => 'distinct|another-rule'];
}),

Test:

return ['discounts.*.id' => ['distinct', 'numeric']];

$compiled->implicitAttributes,
$this->implicitAttributes,
[$attribute => [$key]]
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this statement and why it is needed?

Copy link
Contributor Author

@stevebauman stevebauman Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes absolutely:

I'm using array_merge_recursive here so the implicitAttributes arrays (along with their sub arrays) are merged together properly per iteration. Any additional implicit attribute absolute dot paths will be appended properly to each asterisk dot path.

I also make sure to merge the current implicit attribute dot path using: [$attribute => [$key]] which is done in the else statement below. This "segments" the implicit attributes into their own sections -- and from what I understand, the validator will perform validation on these independently, allowing us to use distinct for example, to validate across the current array and not the entire array:

Example:

$rules = [
    'items.*' => Rule::nested(function () {
        return [
            'discounts.*.id' => Rule::nested(function () {
                return 'distinct';
            }),
        ];
    }),
];

$result = (new ValidationRuleParser(['...']))->explode($rules);

// {#65 ▼
//   +"rules": array:5 [▼
//     "items.0.discounts.0.id" => array:1 [▼
//       0 => "distinct"
//     ]
//     "items.0.discounts.1.id" => array:1 [▼
//       0 => "distinct"
//     ]
//     "items.0.discounts.2.id" => array:1 [▼
//       0 => "distinct"
//     ]
//     "items.1.discounts.0.id" => array:1 [▼
//       0 => "distinct"
//     ]
//     "items.1.discounts.1.id" => array:1 [▼
//       0 => "distinct"
//     ]
//   ]
//   +"implicitAttributes": array:3 [▼
//     "items.1.discounts.*.id" => array:2 [▼
//       0 => "items.1.discounts.0.id"
//       1 => "items.1.discounts.1.id"
//     ]
//     "items.0.discounts.*.id" => array:3 [▼
//       0 => "items.0.discounts.0.id"
//       1 => "items.0.discounts.1.id"
//       2 => "items.0.discounts.2.id"
//     ]
//     "items.*" => array:2 [▼
//       0 => "items.0"
//       1 => "items.1"
//     ]
//   ]
// }

@taylorotwell
Copy link
Member

taylorotwell commented Jan 20, 2022

@stevebauman just thinking out loud - what do you think about Rule::forEach instead of Rule::nested?

@stevebauman
Copy link
Contributor Author

I like Rule::forEach(), that definitely sounds better than nested(). Much clearer to what is taking place in the validator 👍

@stevebauman
Copy link
Contributor Author

Also I forgot to mention @taylorotwell, should we be converting the $value argument to a Fluent instance, similarly to the $validator->sometimes() method?

Sometimes:

$validator->sometimes('...', ['...'], function (Fluent $input) {
     // ...
});

For Each:

Rule::forEach(function ($attribute, Fluent $value, $data = null) {
    // ...
});

@taylorotwell
Copy link
Member

taylorotwell commented Jan 20, 2022

@stevebauman doesn't Fluent kinda seem weird in situations like this where the $value is a single element?

    $validator = Validator::make([
        'users' => [
            ['id' => 1, 'email' => 'taylor@laravel.com'],
            ['id' => 2, 'email' => 'foo bar'],
        ],
    ], [
        'users.*.email' => Rule::forEach(function ($attribute, $value, $data) {
            ray($value);

            return ['email'];
        }),
    ]);

I think passing Fluent would maybe make more sense if the last part of the element being nested was a *, like here:

    $validator = Validator::make([
        'users' => [
            ['id' => 1, 'email' => 'taylor@laravel.com'],
            ['id' => 2, 'email' => 'foo bar'],
        ],
    ], [
        'users.*' => Rule::nested(function ($attribute, $value, $data) {
            ray($value);

            return ['email' => 'email'];
        }),
    ]);

@stevebauman
Copy link
Contributor Author

Ah yea you're right. Guess we can't really convert it to anything since the data type will change

@taylorotwell
Copy link
Member

Do you have any way to detect if the attribute Rule::foreEach is being applied to ends with a *?

@stevebauman
Copy link
Contributor Author

We could detect it via the $attribute parameter in explodeWildcardRules():

$rules = [
    'items.*' => Rule::nested(function () {
        return ['discounts.*.id' => 'distinct'];
    }),
];

$results = (new ValidationRuleParser)->explode($rules);

// ...

// explodeWildcardRules()...

if ($rule instanceof NestedRules) {
    $arrayable = Str::afterLast($attribute, '.') === '*';

    $compiled = $rule->compile($key, $arrayable ? new Fluent($value) : $value, $data);

    // ...
} else {

@taylorotwell
Copy link
Member

What do you think about that? Good idea?

@stevebauman
Copy link
Contributor Author

stevebauman commented Jan 21, 2022

Ah, not too sure after some thought. This wouldn't make sense to use a Fluent instance if items is a list of numbers/ID's:

$data = ['items' => [1,2,3]];

$rules = [
    'items.*' => Rule::nested(function ($attribute, Fluent $value) {
        return 'distinct';
    }),
];

Though we could allow developers to provide a secondary "transformation" function to the Rule::nested() method:

$data = ['items' => [1,2,3]];

Rule::nested(function ($attribute, Collection $value) {
    // ...
}, function ($value) {
    return collect($value);
});

If that's too much, then I think we should play it safe for now and return the raw input's value and let the developer handle it 👍

We can alter the behaviour in a next major version if developers really see it as a need.

@stevebauman
Copy link
Contributor Author

I think we should swap the argument order though since the $value is probably going to be needed more than the $attribute. What do you think?

Rule::forEach(function ($value, $attribute) {
    // ...
});

// vs

Rule::forEach(function ($attribute, $value) {
    // ...
});

@taylorotwell
Copy link
Member

Yeah, I'm fine with swapping the argument order.

@stevebauman
Copy link
Contributor Author

stevebauman commented Jan 21, 2022

Ok, argument order has been swapped! 👍

@taylorotwell taylorotwell merged commit 911593b into laravel:9.x Jan 22, 2022
@siarheipashkevich
Copy link
Contributor

siarheipashkevich commented Jan 24, 2022

@stevebauman why did not you rename this rule from nested to forEach?

@stevebauman
Copy link
Contributor Author

@siarheipashkevich #40586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants