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

[8.x] Fix fields not required with required_unless #37262

Conversation

dshoreman
Copy link
Contributor

In v8.40, given a form with this data:

['foo' => true]

...and this rule in the FormRequest:

['bar' => 'required_unless:baz,true'],

The short-circuit introduced in #37128 prevents 'validateRequired' from being called on bar when baz is omitted from data - even though the very lack of it might mean "bar" should be required.

Removing the short-circuit from required_unless seems to solve the issue without breaking anything else. I tried false first, but that just broke it differently so I added cases to cover that too.

This fixes an issue where the `required_unless` rule does not add a
"required" error when the 'other' field isn't included in input data.

Take `'field' => 'required_unless:other,true'` as an example. With `[]`,
the absense of `other` implies its value is *not `true`* and therefore
`field` should be required. Instead, validation automatically passes.

Flipping `return true` fixed the failing test but failed with valid data
that *wasn't* tested. The extra two tests ensure it still passes if the
field is required *and provided*, or absent when 'other' *expects* null.
@dshoreman
Copy link
Contributor Author

@driesvints Mind checking this over please?

From what I can see, the second point in #37128 should be fine for most rules, it's only the unless that has an issue. I think most null/true type cases should be covered with this, but I'd appreciate your input in case I missed something

@driesvints
Copy link
Member

driesvints commented May 5, 2021

Screenshot 2021-05-05 at 13 51 00

In the example you've given the field is not present in the data set. The anotherfield is also not present in the data set. Therefor the validation passes.

If required_unless should be invoked even if baz isn't in the dataset then yes it should fail. But that's not how I understand that things should work.

@taylorotwell we've made the adjustments under the above understanding. If it should be the other way around please let me know and I'll tackle those use cases as well from 6.x and up.

@taylorotwell
Copy link
Member

taylorotwell commented May 5, 2021

IMO it should likely be required. It's required if baz is not true - which it isn't - it is missing. Therefore, IMO, it should be required.

@taylorotwell taylorotwell merged commit 7fdb70f into laravel:8.x May 5, 2021
@driesvints
Copy link
Member

I'll try to update the behavior on 6.x as well soon as well as for the exclude_unless rule.

@dshoreman
Copy link
Contributor Author

Screenshot 2021-05-05 at 13 51 00

In the example you've given the field is not present in the data set. The anotherfield is also not present in the data set. Therefor the validation passes.

To provide the real-world example it was based on, my project was failing with the data ['name' => 'foo']. I have a rule that requires the 'gid' field unless user_group is true. That is, when user_group is provided and true the gid can be calculated automatically, but it must be provided manually otherwise.

The value of anotherfield seems to be of particular importance here. With required_unless:foo,null the early return would produce the expected result (unless we expect for "not set" and "null" to be two different things - I wasn't sure here so I wrote the test assuming unset == null). For non-null values though, the lack of 'anotherfield' usually (but not always) means the opposite.

@driesvints
Copy link
Member

@dshoreman okay, I think we finally got to the bottom of the behavior we want. I already sent in #37291 to backport it to 6.x and will send in a PR to the docs as well.

@dshoreman
Copy link
Contributor Author

Thanks @driesvints, much appreciated.

This was the only thing left broken for me in the most recent version, so I look forward to updating from 8.37 soon! 🚀

@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

3 participants