laravel-arcanist / arcanist

🧙‍♀️ Arcanist takes the pain out of building multi-step form wizards in Laravel.
https://laravel-arcanist.com
MIT License
403 stars 32 forks source link

Make it possible to validate nested array input #33

Open erikwittek opened 2 years ago

erikwittek commented 2 years ago

This PR lets you define validation rules for nested array input:

Field::make('checkboxes')
    ->rules(['checkboxes.*' => 'in:optionA,optionB')
ksassnowski commented 2 years ago

Thanks for the PR, I didn't even realize that this wasn't currently possible. That being said, this implementation has a couple of issues:

  1. It forces you to specify the name of the field again as soon as you want to define a nested rule. This is redundant and a possible source of bugs (like typos).
  2. With the changes made in this PR, the field name (i.e. the parameter you pass to make) gets completely ignored as soon as you specify the rules as an associative array. This is completely opaque to the user, however, and could lead to hard to track down bugs. Which leads to the next point.
  3. Since the field name gets ignored if the rules are defined as an associative array, that means that as soon as you specify any rule in this way, you have to specify them all like this. Consider the case where you might want to define rules for both the field itself as well as some nested rules.
Field::make('checkbox')
    ->rules([
        'checkbox' => 'required_if:some_other_field',
        'checkbox.*' => 'in:optionA,optionB',
    ]);

Now the field name has to be mentioned three times.


Here's what I would propose instead.

Field::make('checkboxes')
    // resolves to `['checkboxes' => ['required_if:some_other_field']]`
    ->rules(['required_if:some_other_field'])
    // resolves to `['checkboxes.*' => ['in:optionA,optionB']]`
    ->nestedRules(['in:optionA,optionB'])

The nestedRules method could work similar to how your current PR works. If the provided array is not an associated array, we could implicitly treat it as the * rule. But you could also provide an associative array if you need to nest your rules even further like checkboxes.*.id.

Field::make('checkboxes')
    ->nestedRules(['in:optionA,optionB']);
// => ['checkboxes.*' => ['in:optionA,optionB']]

Field::make('options')
    ->nestedRules([
        'id' => 'required',
        'name' => ['string', 'max:255']
    ]);
// => [
//           'options.*.id' => 'required',
//           'options.*.name' => ['string', 'max:255']
//        ]

Notice how the names checkboxes and options are only mentioned once.

erikwittek commented 2 years ago

Thank you very much for the Feedback. We've discussed the solution you proposed already but decided against a separate method, because we preferred to have one single place to set all the validation rules.

Nevertheless, your points are valid, so what do you think about a second optional parameter on the ->rules() method:

function rules($rules, $nested_rules = [])

This would keep everything in one place but at the same time prevent the bugs you mentioned.

Also, I wouldn't set the * selector implicitly, because I'd prefer the visibility of what I have configured. Additionally there are situations where you need to define rules for the properties of an object, rather than an array of objects:

Field::make('photos')
    ->rules([
        'photos.profile' => 'required|image',
    ]);

(Example from: https://laravel.com/docs/8.x/validation#validating-nested-array-input)