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

[8.x] Add Validator::excludeArrays() to exclude array keys that aren't included in the validation rules #37943

Merged
merged 5 commits into from
Jul 10, 2021

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Jul 7, 2021

Given the following rules:

return request()->validate([
    'users' => 'required|array',
    'users.*.name' => 'required',
]);

The validator will return the users attribute with all its children:

array:1 [▼
  "users" => array:1 [▼
    1 => array:2 [▼
      "name" => "mohamed"
      "job" => "dev"
    ]
  ]
]

You can see that the job key was returned even though it wasn't included in the validator rules.

This PR adds a new Validator::excludeArrays() that when added to a boot method of a service provider, will instruct the validator to exclude any array keys that weren't validated:

array:1 [▼
  "users" => array:1 [▼
    1 => array:1 [▼
      "name" => "mohamed"
    ]
  ]
]

@SjorsO
Copy link
Contributor

SjorsO commented Jul 7, 2021

This behavior of the validator has caused me trouble in the past too. The validate method returning data that wasn't validated should be considered a bug if you ask me. What about making this fix the default in Laravel 9, and removing the need for the Validator::excludeArrays() call?

@themsaid
Copy link
Member Author

themsaid commented Jul 7, 2021

@SjorsO that's the plan for Laravel 9. But since it's a breaking change, people must opt-in.

foreach ($this->getRules() as $key => $rules) {
if ($this->excludeArrays &&
in_array('array', $rules) &&
! empty(preg_grep('/^'.$key.'\.*/', array_keys($this->implicitAttributes)))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Escaping the regex pattern with preg_quote($key, '/') may be required to cover every character that <input name=""> allows, not just restricted to alphanumeric/underscore.

e.g., validating a parsed XML document (external spec) can lead to some weird field name edge cases with hyphens, slashes, etc.

@taylorotwell
Copy link
Member

If anyone has good ideas for the final method name of this method let me know... /cc @derekmd

@SjorsO
Copy link
Contributor

SjorsO commented Jul 7, 2021

@SjorsO that's the plan for Laravel 9. But since it's a breaking change, people must opt-in.

So in Laravel 9 we won't have to call Validator::excludeArrays()?

If anyone has good ideas for the final method name of this method let me know.

It isn't easy to name something that fixes such a specific problem. Maybe something like:

  • Validator::excludeDataWithoutValidationRules()
  • Validator::excludeNestedDataWithoutValidationRules()
  • Validator::excludeUnvalidatedNestedData()
  • Validator::alwaysExcludeDataWithoutValidationRules()

@derekmd
Copy link
Contributor

derekmd commented Jul 7, 2021

excludeArrayValuesNotValidated()

@bonroyage
Copy link
Contributor

I ran into this issue a little over a month ago too, but as I was working on a fix I found the (at the time undocumented) feature that you could add the keys behind the array rule, like array:name,job which validates that no other array keys are included on that array. Given that this rendered my fix unnecessary for the use-case I was trying to fix, I didn't move forward with it.

Of course, validating the absence of invalid keys is not the same as not returning them in the validated() method if they don't have a rule.

One additional feature I made in my fix was allowing array:*, which would ignore the flag and return everything for that array, as I could see a situation where in the validator you have two arrays: (1) should only return the validated keys and nothing more; and (2) where you may want all the keys even if they don't have their own rules.

8.x...bonroyage:validator-arrays

@taylorotwell
Copy link
Member

@bonroyage I forgot you can do that... 👀

@dbakan
Copy link
Contributor

dbakan commented Jul 8, 2021

I've run into this a few times as well. Nice work, Mohamed!

  • Validator::preventAccidentalArrayKeys()
  • Validator::skipUnattendedArrayKeys()

@themsaid
Copy link
Member Author

themsaid commented Jul 9, 2021

@bonroyage @taylorotwell

array:one,two,... has two problems. First, it throws a validation error when different keys are provided. That's not the desired behaviour in most cases. Second, it requires that you define all the keys you accept. So you have to define them once as parameters to the array rule, and another to define validation rules for each of the keys.

@dbakan
Copy link
Contributor

dbakan commented Jul 9, 2021

And it gets even trickier with more levels of nesting, like users.*.jobs.*.title.

I'm not sure if the current approach offers a way to restrict keys for both arrays users and jobs.

@taylorotwell taylorotwell merged commit 45d439e into laravel:8.x Jul 10, 2021
@riesjart
Copy link
Contributor

riesjart commented Jul 14, 2021

Until Laravel 9 you can make this the default using:

app()->resolving(
    \Illuminate\Validation\Factory::class,
    fn($factory) => $factory->excludeUnvalidatedArrayKeys()
);

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.

None yet

7 participants