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] Conditional rules #38361

Merged
merged 5 commits into from
Aug 12, 2021
Merged

[8.x] Conditional rules #38361

merged 5 commits into from
Aug 12, 2021

Conversation

taylorotwell
Copy link
Member

Adds conditional rule support per @timacdonald's suggestion.

request()->validate([
    'name' => 'required|string|min:6',
    'password' => ['required', 'string', Rule::when(true, ['min:5', 'confirmed'])],
]);

@bonroyage
Copy link
Contributor

Great! This could clean up and simplify some code in FormRequests where if statements are put in the rules() or sometimes() calls in the withValidator().

In terms of what it achieves, it's very similar to the sometimes() method on the Validator. Would it make sense to similarly allow the $condition to be a closure with the inputs as an argument? Someone could put a rule like this example in a macro and use it as Rule::requiredForManyGames(), to reuse in multiple validators.

Rule::when(function ($input) {
    return $input->games >= 100;
}, 'required');

Can you nest multiple when() calls or will only a first level when() work? e.g.

Rule::when($conditionA, [
    'required',
    Rule::when($conditionB, 'in:foo,bar')
])

@taylorotwell
Copy link
Member Author

@bonroyage

Your closure use case seems pretty fine.

Regarding nested conditions, it was hard for me to think of any use case where it was totally necessary, and it seems like it would add quite a bit of complication. Do you have any thoughts or use cases where having nested conditional rules is absolutely required?

@bonroyage
Copy link
Contributor

@taylorotwell

Do you have any thoughts or use cases where having nested conditional rules is absolutely required?

Looking at a few form requests of my own, I found a few rules inside nested ifs. Most of these are used to add rules for multiple fields (like a gate allowing something) as opposed to a single rule to an existing field, so they wouldn't benefit as much from supporting nested conditions. A mergeWhen() behaviour like on the JsonResource would be more appropriate for those situations, but that's outside the scope of this PR.

I asked not because I need it, but I was curious about whether or not it would/should be possible. If not, I think it should be clearly documented that you cannot nest them so avoid confusion as it could give the impression to some that the rules used inside when() can be exactly the same as those outside when(), and therefore also allowing Rule::when() to be used again. Maybe someone else has a valid real world use case for nested conditionals.

@GrahamCampbell GrahamCampbell changed the title Conditional rules [8.x] Conditional rules Aug 12, 2021
public function passes(array $data = [])
{
return is_callable($this->condition)
? call_user_func($this->condition, $data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrapping $data in Illuminate\Support\Fluent would make accessing the inputs slightly cleaner ($inputs->key) through the __get helper that checks if the property exists and otherwise return null, as opposed to needing to use Arr::get($inputs, 'key') or $inputs['key'] ?? null.

Suggested change
? call_user_func($this->condition, $data)
? call_user_func($this->condition, new Fluent($data))

Not sure if it's best to put it here or in the Validator on line 1085. Applied here, the setters would have no side-effects on the other conditionals.

@taylorotwell taylorotwell merged commit fc0a4a7 into 8.x Aug 12, 2021
@taylorotwell taylorotwell deleted the conditional-rules branch August 12, 2021 20:27
@michaeldyrynda
Copy link
Contributor

Thanks for this @taylorotwell, keen to take it for a spin. The number of times I've seen or had to write conditional blocks that break out of the validation rules array is troubling.

@timacdonald
Copy link
Member

❤️ Thanks Taylor! Got some refactoring to do on my project!

/**
* The rules to be added to the attribute.
*
* @var array
Copy link
Contributor

@sebdesign sebdesign Aug 18, 2021

Choose a reason for hiding this comment

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

This should be array|string.

Copy link
Member

Choose a reason for hiding this comment

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

Can you send in a PR?

@austenc
Copy link
Contributor

austenc commented Aug 26, 2021

Looks like this PR changed how the validator returns data for keys that have no rules. Prior to this, keys without rules would still be included in the validated data. Now they are not.

For example:

// imagine this is the request that is posted 
$request = [
    'name' => 'Hello',
    'description' => 'World'
];

$data = $this->validate([
    'name' => 'required',
    'description' => ''
]);

dd(array_keys($data));

// Before
['name', 'description']

// After
['name']

Will try to write a test for this, although I'm unsure at this point what needs to change. The cause seems to be the filter() on this line:
https://github.com/laravel/framework/blob/8.x/src/Illuminate/Validation/ValidationRuleParser.php#L306

Edit: Also this seems like it may have been unintentional behavior how it worked before... but turns out we were relying on it as a way to trick a few extra fields into showing up in the validated data.

Edit 2: Fixed by #38563 before I could make a PR!

@itxshakil
Copy link

How can I access the value inside the callback, when field is an array.

$validator = Validator::make($request->all(), [
'openings.*.id' => ['sometimes', 'exists:days,id', 'distinct'],
'openings.*.from' => ['required_with:openings.*.id', 'date_format:H:i'],
'openings.*.to' => ['required_with:openings.*.id', Rule::when(function($a){
// Value not equal to something
},['date_format:H:i']), 'after:openings.*.from'],
]);

I want to check if format only if value is not something like 24:00

But I can not access ther value of current openings to.

@timacdonald
Copy link
Member

The value should be on the request, so you should just be able to use $this->input('whatever') in the callback to access the values.

@itxshakil
Copy link

Yes, but the only problem here is the field is an array. How can I know the array key on the fly?

  • Key name: openings.*.to

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.

8 participants