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

[5.3] Validation: rules with "required" continue to evaluate #15084

Closed
mfn opened this issue Aug 27, 2016 · 11 comments
Closed

[5.3] Validation: rules with "required" continue to evaluate #15084

mfn opened this issue Aug 27, 2016 · 11 comments

Comments

@mfn
Copy link
Contributor

mfn commented Aug 27, 2016

When a validation rule contains required, it will not stop evaluating further rules. This is really a problem/break with previous versions when using custom validation which, until now, could always expect that the value they're passed fulfills the required rule (i.e. it's not something "nully", "empty", etc.).

Example with 5.2:

~/src/laravel/laravel-5.2 $ ./artisan tinker
Psy Shell v0.7.2 (PHP 7.0.9 — cli) by Justin Hileman
>>> \Validator::extend('custom', function($attr, $val) { var_dump($val); });
=> null
>>> \Validator::make(['foo' => null], ['foo' => 'required|custom'])->messages()->all();
=> [
     "The foo field is required.",
   ]

or

>>> \Validator::make(['link' => null], ['link' => 'required|url'])->messages()->all();
=> [
     "The link field is required.",
   ]

Example with 5.3.4:

~/src/laravel/laravel-5.3 $ ./artisan tinker
Psy Shell v0.7.2 (PHP 7.0.9 — cli) by Justin Hileman
>>> \Validator::extend('custom', function($attr, $val) { var_dump($val); });
=> null
>>> \Validator::make(['foo' => null], ['foo' => 'required|custom'])->messages()->all();
NULL
PHP error:  Array to string conversion in /src/laravel/laravel-5.3/vendor/laravel/framework/src/Illuminate/Support/MessageBag.php on line 245

or

>>> \Validator::make(['link' => null], ['link' => 'required|url'])->messages()->all();    => [
     "The link field is required.",
     "The link format is invalid.",
   ]

The NULL form the var_dump($val) should never have happened and the url validator should not have been called.

Why are further rules called, when the value did not match the required criteria?

@themsaid
Copy link
Member

themsaid commented Aug 27, 2016

In 5.2 if an attribute was null, empty string, or empty array it would pass all other validation rules, for example this would pass:

Validator::make(['link' => ''], ['link' => 'url'])->messages()->all()

That's acceptable in case the data is coming from HTML, in which an empty field would result an empty string value, but since validation is also used to validate arrays an API request might have a value as an empty string, empty array, or null that shouldn't pass such check.

For this we added a nullable rule, which will pass validation in case the value is null, otherwise the attribute will be validated (unlike 5.2).

To stop validation when required fails you need to use the bail rule:

'bail|required|url'

This will prevent running any further checks after the required check fails which will prevent the situation you are referring to.

@mfn
Copy link
Contributor Author

mfn commented Aug 27, 2016

@themsaid thank you very much for clearing that up!

In fact I'm using validations for a API-only project and yes, the nullable is very welcome as it was always puzzling why a string validation rule would accept a null.

I see that bail did exist before, so this is great.

Basically, required in Laravel < 5.3 seems to do an implicit bail which now needs to be done explicitly.

Can you confirm this for me? I'm happy to make a PR for the Upgrade Notes because I couldn't find any reference for this.

@themsaid
Copy link
Member

Actually in 5.2 even if you didn't use required the attribute would pass validation if it was empty/null, so using bail|required in 5.3 will have same effect as using required only in 5.2

But be careful that bail will always run the first rule, so bail|required|url is not like bail|url|required

@mfn
Copy link
Contributor Author

mfn commented Aug 27, 2016

Thanks and understood. So basically bail is now required to have the 5.2 behavior for required => breaking change.

@GrahamCampbell
Copy link
Member

Sure, though Laravel doesn't follow semver. We always have breaking changes in minor releases.

@mfn
Copy link
Contributor Author

mfn commented Aug 27, 2016

Fine by me, I just wanted to state it explicitly and have it confirmed so it's good to go for the upgrade guide which doesn't mention it.

@GrahamCampbell
Copy link
Member

👍

@taylorotwell
Copy link
Member

It really to me seems like if the required rule fails there is no point in calling any other rules. Seems weird that we do.

@mfn
Copy link
Contributor Author

mfn commented Aug 27, 2016

I share this sentiment.

I can't come up with a use-case where I would want required to continue evaluation further rules. Maybe @GrahamCampbell or @themsaid can come up with an example?

@themsaid
Copy link
Member

Working on changing that.

@mfn
Copy link
Contributor Author

mfn commented Aug 29, 2016

That's pretty awesome, thanks everyone here and especially for @themsaid for fixing and (and ofc @taylorotwell for ... ya know, the framework and all 😄 )

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

4 participants