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.2] Added new validation rule to skip other rules on first validation rule fail #11986

Merged
merged 1 commit into from Jan 26, 2016

Conversation

Joel-James
Copy link

Reference issue - #4789

New validation rule - bail (rule name updated by Taylorotwell)

When we use multiple rules, sometimes it is required to stop validating other rules on a specific rule fails.

For example, if you have an int field in database and you are using numeric rule and unique rule for the field. This may throw a Database error if the input is not numeric. Because validation checks both the rules.
Numeric check will return false, but exists check will also execute with non-integer value.

'user_id' => 'numeric|unique:users'

In this case we can use this optional rule bail to execute validation in rule order and skip other rules if any of the rules fails. For example,

'user_id' => 'bail|numeric|unique:users'

Now only if the numeric rule pass, validation will check unique rule, that will avoid db error.
This rule can be optional. Validation will be skipped only if we use this rule.

You have to give the rules in proper order to execute on by one.

I am sure that this can help many!

@@ -316,6 +316,11 @@ public function passes()
foreach ($this->rules as $attribute => $rules) {
foreach ($rules as $rule) {
$this->validate($attribute, $rule);

// Break validation on first fail if failonfirst is set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this comment.

@GrahamCampbell GrahamCampbell changed the title Added 'Fail On First' validation rule to skip other rules on first validation rule fail [5.2] Added 'Fail On First' validation rule to skip other rules on first validation rule fail Jan 22, 2016
@GrahamCampbell
Copy link
Member

Please squash to one commit.

@Joel-James
Copy link
Author

@GrahamCampbell Thank you. It's done.

}
}

return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Joel-James, why not just

return $this->hasRule($attribute, ['Failonfirst']) && $this->messages->has($attribute);

?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucasmichot That is good but I think the previous one is fast and easily readable? Should I change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather:

if (! $this->hasRule($attribute, ['Failonfirst'])) {
    return false;
}

return $this->messages->has($attribute);

Just my feeling, at the end @taylorotwell can always tweak it.

@CupOfTea696
Copy link
Contributor

Wouldn't it make more sense for Laravel to automatically check if the unique check would cause any database errors?

@Joel-James
Copy link
Author

@CupOfTea696 For database unique queries that may work. But for others?

/**
* Stop on error if failonfirst rule is given.
*
* First check if 'failonfirst' is set,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is too short.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infact, this description is pretty pointless and should just be deleted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by 'line is too short'? Which line?

@garygreen
Copy link
Contributor

I have always wanted something like this in laravel. Taylor mentioned in the issue that laravel by default checks all rules because you should "show user all that's failing" -- what about those that don't want to actually show all errors. It's very common to just show the ->first() error in the view and not all. Also for rules like exists, unique, etc it doesn't make sense to check those and hit a db if it's unnecessary.

As for the implementation in this PR, I'm not sure how I feel about adding it inside the rules, it seems ok I guess but I would also like something global for all the attributes on the validator instance. It could be annoying having to put that rule on every attribute if all you do is output first failing error message.

This could be achieved by adding a method to the validator like so:

Validator::failOnFirst(['username']); // Fail on first error for username field
Validator::failOnFirst(true); // Fail on first for all rules.

@taylorotwell taylorotwell merged commit f4544b9 into laravel:5.2 Jan 26, 2016
@Joel-James
Copy link
Author

@garygreen You are right. But what if there is only one field that you want to add this rule and for all others you need all the errors? I always wanted to get all the error messages like @taylorotwell said. But for few fields with unique check wanted to skip other validations. That is where this new rule can help you.

Like you said setting something globally for all the rules (optionally) is also a good thing. I think we may implement that separately.

@Joel-James Joel-James changed the title [5.2] Added 'Fail On First' validation rule to skip other rules on first validation rule fail [5.2] Added new validation rule to skip other rules on first validation rule fail Jan 26, 2016
@garygreen
Copy link
Contributor

@Joel-James well with the suggested ->bail() method you can either supply a boolean or an array of attributes to bail early on. It would make it very easy to add on i.e. a FormRequest for all your rules/attributes, without having to add 'bail' to every one. Another option might be, like you mentioned, the concept of 'global rules' something like ->addGlobalRule('bail');

@linaspasv
Copy link
Contributor

@Joel-James is there any workaround where bail would work together with a required_if ? #12500

I have the following use-case scenario:

        $data = [
            'module_slug' => 'home',
            'type' => 'website'
        ];

        $validator = validator($data, [
            'module_slug' => 'bail|required_if:type,module|unique:menus_items,slug'
        ]);

The expected result here would be to bail once required_if rule does not go through. Any ideas?

@neophyt3
Copy link

@codewizz did you found any solutions?? I am stuck on same issue
I am trying to validate
'org_support_email' => 'bail|required_iflabel_flag,1|email',
It skips required_if and I dont want to validate email if label_flag is other than 1

@Joel-James
Copy link
Author

Joel-James commented Sep 29, 2016

@neophyt3 Your required_if rule is wrong. You missed a colon after rule name. Change to required_if:label_flag,1

@laravel laravel locked and limited conversation to collaborators Sep 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants