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

Change of behaviour in Validation between Laravel 5.4.12 and 5.4.22 #19140

Closed
eduacostam opened this issue May 10, 2017 · 4 comments
Closed

Change of behaviour in Validation between Laravel 5.4.12 and 5.4.22 #19140

eduacostam opened this issue May 10, 2017 · 4 comments

Comments

@eduacostam
Copy link

  • Laravel Version: 5.4.22
  • PHP Version: 7.1.4
  • Database Driver & Version: n/a

Description:

If we want to validate a field with null as valid input but we also want it to be required, we would use a rule such as required|nullable|whatever which worked as expected in Laravel 5.4.12 but stopped working in Laravel 5.4.22.

Steps To Reproduce:

use Validator;

$data = ['null_field' => null];
// Using $rules = ['null_field' => 'nullable|required|boolean']; yields the same result
$rules = ['null_field' => 'required|nullable|boolean'];

// False in Laravel 5.4.12
// True in Laravel 5.4.22
Validator::make($data, $rules)->fails();

Running the following code I would expect fails() to be false, as the required rule passes given that the attribute is present, and the nullable rule passes given that the value is null.

@devcircus
Copy link
Contributor

There were changes to validation to account for edge cases in 5.4.16. See #18376. Since the definition of 'required' according to the Laravel docs includes "the field must not be empty" and the field is considered empty if "the value is null", then required|nullable doesn't really make sense (even though it used to work). Seems like maybe present|nullable would be what you need.

I'll be honest though, validation can be one of the most confusing things to work with, especially with edge cases, so maybe @themsaid can clear things up for you.

@eduacostam
Copy link
Author

Thanks @devcircus, I completely missed that the present rule existed, that's definitely what I should use here.

I agree that this is confusing, especially having it change behavior between minor versions. One might think that required would imply the same thing as present, meaning, "it is required to have this field present in the request", and not that it should also have a value because the value is validated when specifying a type, eg: boolean. In this case it will fail it the data is null, so why make it fail with required if it will fail anyways when validating the type?

@themsaid
Copy link
Member

This area became very complicated and I don't see it possible to cover all scenarios around this edge case, the changes that were made were based on dozens of bug reports which implied the behaviour you're describing as a bug :)

I'd say you use the present rule instead of required since your field might be null, and null would fail with required.

I'm closing this issue but continuing to follow the issue so feel free to ping me whenever you need.

@eduacostam
Copy link
Author

Thanks for the update @themsaid, I did exactly that, replaced the validations with present|nullable and that was it. The only comment I have left is it would be nice to add the change in behaviour between minor versions (even if it's considered a bugfix) as part of the release notes, or maybe be more explicit in the required rule documentation that if you specifically need to validate the presence but don't care if it's empty, then you should use present|nullable (I know it says that a value should exist, but common sense told me that using nullable should override that).

In any case I consider this fixed, it was just an opinion on the wording about the rules and the (in my opinion) sensible meaning of the word required, but I totally agree that this is a matter of taste and that some people would think that required means that a value should be present while others would say that having that field present is what they are looking for.

Thanks!

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

3 participants