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

Fix validator treating null as true for (required|exclude)_(if|unless) due to loose in_array() check #36498

Closed
wants to merge 4 commits into from
Closed

Fix validator treating null as true for (required|exclude)_(if|unless) due to loose in_array() check #36498

wants to merge 4 commits into from

Conversation

jessarcher
Copy link
Member

@jessarcher jessarcher commented Mar 8, 2021

There's currently some unusual behavior with a number of validation rules that are using in_array() in the default loose mode.

For example, this currently fails:

Validator::validate(['foo' => true], ['bar' => 'required_if:foo,null']);

// "bar": [
//   "The bar field is required when foo is true."
// ]

This is because the null parameter is converted to a string 'null' and placed in an array, which is passed as the haystack to in_array(), with the dependent field value as the needle. e.g.

in_array(true, ['null']); // true

We can pass true as the third parameter to use strict mode, however many people no doubt rely on it loosely comparing numeric strings and integers, so this PR instead only enables strict mode when the value is a boolean, which fixes the bug hopefully without breaking any expected behavior.

I'm not sure if this is considered a breaking change - I guess that depends on whether anyone is depending on the existing behavior. It does feel risky to tweak the behavior of such fundamental validation rules though.

@driesvints
Copy link
Member

@jessarcher is this also broken for v6 LTS? Otherwise it's best that you send this in to 6.x

@jessarcher
Copy link
Member Author

@jessarcher is this also broken for v6 LTS? Otherwise it's best that you send this in to 6.x

Good call @driesvints! I've cherry-picked the commits into a new branch and PR at #36504

@jessarcher jessarcher closed this Mar 8, 2021
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.

2 participants