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

[6.x] Fix validator treating null as true for (required|exclude)_(if|unless) due to loose in_array() check #36504

Merged
merged 4 commits into from
Mar 8, 2021
Merged

[6.x] Fix validator treating null as true for (required|exclude)_(if|unless) due to loose in_array() check #36504

merged 4 commits into from
Mar 8, 2021

Conversation

jessarcher
Copy link
Member

Copied from #36498 to target 6.x

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.

@Shkeats
Copy link

Shkeats commented Mar 13, 2021

@jessarcher We happened to have some overzealous unit tests covering part of our validation around required_if between two boolean fields and found this change caused them to fail. On the framework 8.x branch. The fix was to change our validation rule configuration from using 0 and 1 to true and false on the required_if rules. Screenshots of the original test and validation rules below.

It seemed to have no impact on the actual form behaviour from the frontend but hopefully might help someone else understand what's going on if they see a similar issue.

image
image

This was referenced Mar 15, 2021
dshoreman added a commit to dshoreman/servidor that referenced this pull request Apr 20, 2021
In v8.32.0, laravel/framework#36504 was merged which fixes null being
treated as true by the validator, but also enforces strict checking for
boolean values given to the required_unless rule. That's not necessarily
a bad thing, it just means we can't be passing `1` when we want `true`.

When we use `1`, user tests fail because while we pass `'user_group' =>
true` in the request data, the validator checks explicitly for `1` which
it doesn't see, so it says "gid is required unless user group is in 1".
We don't want that.
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