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 nullable values for required_if #37128

Merged
merged 2 commits into from Apr 27, 2021
Merged

[6.x] Fix nullable values for required_if #37128

merged 2 commits into from Apr 27, 2021

Conversation

driesvints
Copy link
Member

required_if, required_unless, exclude_if, exclude_unless (referred to below as "these rules").

This PR fixes several things:

  1. A regression introduced in [6.x] Fix required_if boolean validation #36969 (comment): we now will take into account nullable values the same way as we do forboolean values. This will make sure that if someone adds null to these rules, it'll get properly handled for the targetted key's value.
  2. These rules will now automatically pass when the targeted key isn't present in the data. That's the if (! Arr::has($this->data, $parameters[0])) { part that was added to each of them. This was already expected I believe but should now fail early.
  3. I've added an if (is_null($value)) { to FormatsMessages so that validation messages can display things like The baz field is required when foo is null.. This might be a bigger breaking change so if you want that part reverted I'll do so. Keep in mind that that will cause the validation message to display The baz field is required when foo is . instead.

I'd very much like a some second pair of eyes on this. @timacdonald @jessarcher if you're willing?

@taylorotwell taylorotwell merged commit 5b604e1 into 6.x Apr 27, 2021
@taylorotwell taylorotwell deleted the fix-null-values branch April 27, 2021 13:40
@jessarcher
Copy link
Member

Looks good @driesvints, just thinking this should also be applied to prohibited_if and prohibited_unless for consistency and symmetry?

@driesvints
Copy link
Member Author

@jessarcher Yep indeed. Did that on 8.x. 6.x doesn't have those rules. Thanks Jess!

@JackWH
Copy link
Contributor

JackWH commented May 4, 2021

@driesvints Does this fix also apply to nullable input array values? I just updated to v8.40.0 and found the second validation rule below failing unexpectedly:

[
    'start_dates'   => 'array|exclude_if:schedule_type,daily|required_if:create_schedule,true',
    'start_dates.*' => 'nullable|exclude_if:schedule_type,daily|required_if:create_schedule,true|date_format:D M jS Y',
]

Validation would report that "The start_dates.123 field is required when create schedule is 1". However, start_dates.* should also be nullable, as it can be excluded based on another input field.

@driesvints
Copy link
Member Author

@JackWH can you please boil down the issue to the most simple way to reproduce it?

s4muel added a commit to s4muel/framework that referenced this pull request Nov 16, 2021
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 driesvints mentioned this pull request Dec 2, 2022
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

4 participants