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(rollback) required_if and exclude_if validation #39625

Closed
wants to merge 1 commit into from

Conversation

s4muel
Copy link
Contributor

@s4muel s4muel commented Nov 16, 2021

i think you do not want to return early in these cases as @driesvints changed in this PR: #37128
that PR broke using this validation for null values (e.g. checkboxes that are not checked) so you cannot use it like exclude_if,otherfield,null

i think you do not want to return early in these cases as @driesvints changed in this PR: laravel#37128
that PR broke using this validation for `null` values (e.g. checkboxes that are not checked) so you cannot use it like `exclude_if,otherfield,null`
@driesvints
Copy link
Member

Yes you do. This was a vital part of the fix. The problem is that you're not using nullable in the validation rules.

@s4muel
Copy link
Contributor Author

s4muel commented Nov 16, 2021

hm, the checkbox (otherfield in my example), even if nullable, is not present in $this->data, so this always passes: https://github.com/laravel/framework/blob/8.x/src/Illuminate/Validation/Concerns/ValidatesAttributes.php#L1615

@driesvints
Copy link
Member

Please post the full validation rules and data that's being passed so we can reproduce this.

@s4muel
Copy link
Contributor Author

s4muel commented Nov 16, 2021

minimum to reproduce:
validation rules:

[
    'email' => 'exclude_if:owner,null|email',
    'owner' => 'nullable',
]

form contains two inputs:
email: text input field
owner: checkbox

submit form with empty email and unchecked checkbox
dump of $request->input():

[
  "_token" => "rjGnNZQ2OP9oqRQtn7y6yKFK8izvlh4wFXiRtSGF"
  "email" => null
  "send" => "Submit"
]

the email is not excluded and it fails on 'email' rule.

note: if i switch the logic around and use exclude_unless

[
    'email' => 'exclude_unless:owner,on|email',
    'owner' => 'nullable',
]

it works as expected

@driesvints
Copy link
Member

Screen Shot 2021-11-16 at 11 41 13

This is with your example. Like I said: you need to add nullable to your email rule:

Screen Shot 2021-11-16 at 11 42 01

@s4muel
Copy link
Contributor Author

s4muel commented Nov 16, 2021

oh, i see. kind of makes sense now. and since i want that email not to be null if the checkbox is true, i need a sometimes rule instead of nullable ('email' => 'exclude_if:owner,null|sometimes|email'). at least it seems to be working.

it wasnt clear though and when i updated laravel recently (from 8.38.0 to 8.68.1) my tests just broke (phew, at least i got that covered) because of that PR mentioned earlier. it previously worked like that so i got false understanding how it should work.

thank you!

@s4muel s4muel closed this Nov 16, 2021
@driesvints
Copy link
Member

@s4muel no worries. Glad you got that fixed 👍

@s4muel s4muel deleted the patch-1 branch November 16, 2021 12:12
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

2 participants