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

is_required validation rule is broken with boolean values (regression) #36540

Closed
dvlpp opened this issue Mar 10, 2021 · 8 comments
Closed

is_required validation rule is broken with boolean values (regression) #36540

dvlpp opened this issue Mar 10, 2021 · 8 comments

Comments

@dvlpp
Copy link

dvlpp commented Mar 10, 2021

  • Laravel Version: 8.32.0
  • PHP Version: 7.4.0
  • Database Driver & Version: irrelevant

Description:

It seems that this PR broke required_if validation rule with boolean values. Here's a validation rule which worked until 8.31:

[
   "is_organization" => ["required", "boolean"],
   "personal_occupation" => ["required_if:is_organization,0"]
]

But since 8.32.0 this will pass without ValidationException with is_organization send to false and personal_occupation to null (or empty).

@jessarcher
Copy link
Member

Hi Philippe,

First off, I'm sorry that my PR has caused this bug for you.

The underlying issue is when and how '0' and '1' should be interpreted as a boolean vs. an integer. For example, in the following scenario you would expect foo not to be required, because the validation rule is clearly specifying numeric values rather than booleans.

Validator::validate(
    ['age' => false],
    ['foo' => 'required_if,age,0,1,2']
);

Prior to my bugfix (which addressed a different scenario) foo would have been mistakenly required because of the loose comparison.

I can't think of any way to make both of our examples work as we each expect. However, because your example was previously working, I feel my bugfix should probably be reverted for 6.x and 8.x.

There is probably a bigger discussion to be had about how this should work in future versions though, because ultimately I feel the stricter validation is preferable, even if it means you may need to specify required_if:is_organization,false,0

@taylorotwell
Copy link
Member

@dvlpp why do you have required_if:is_organization,0 and not required_if:is_organization,false if is_organization is a boolean field?

@dvlpp
Copy link
Author

dvlpp commented Mar 11, 2021

@jessarcher you're right, the solution is required_if:is_organization,0,false which works in all cases (I didn't thought I could set multiple values here).
I close this since my syntax was incorrect.

@dvlpp dvlpp closed this as completed Mar 11, 2021
@timacdonald
Copy link
Member

timacdonald commented Mar 11, 2021

Although I agree with the easy suggest fix, just wanna bring to light that the validator, as per the docs, considers [true, false, 1, 0, "1", and "0"] as a boolean - or at least validates them as a boolean, so I can see that others may also run into this in the future.

From the docs:

boolean

The field under validation must be able to be cast as a boolean. Accepted input are true, false, 1, 0, "1", and "0".

And to possibly add to the confusion, the Illuminate\Http\Request accepts "on" and "yes" as true, so there is a lot of inconsistency in what Laravel accepts as a boolean depending on the context which isn't Laravel's fault. This is symptom of PHP's type juggling / HTML checkbox on value / form inputs always being strings while also handling JSON payloads etc.

@taylorotwell
Copy link
Member

@timacdonald yeah I did consider that actually and that made me think a bit about this. My thinking was "required_if" is not really boolean related - to me it seems like it should be a fairly literal check.

@SjorsO
Copy link
Contributor

SjorsO commented Apr 3, 2021

Just wanted to report that this same issue caused problems in my applications.

The validation rule below worked fine before v8.31, but broke unexpectedly after a composer update.

'subject' => 'exclude_unless:with_action,1|required|string|max:255',

Is is too late to revert this breaking change?

@driesvints
Copy link
Member

Yeah this ship has sailed I'm afraid.

@SjorsO
Copy link
Contributor

SjorsO commented Apr 3, 2021

Yeah this ship has sailed I'm afraid.

Too bad, I probably won't be the last one to get bitten by this breaking change

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

No branches or pull requests

6 participants