-
Notifications
You must be signed in to change notification settings - Fork 1
Feature: Exclude and Conditional Rules #8
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
Conversation
|
@jonwaldstein I would appreciate a review on this so I can use it in Next Gen. |
bordoni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small set of questions and a docblock.
| if ($conditions instanceof ConditionSet) { | ||
| $this->conditions = $conditions; | ||
| } else { | ||
| $this->conditions = new ComplexConditionSet(...$conditions); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should allow allow conditions to be overwritten directly on an existing Rule by moving this to a method and calling it here on construct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you have a clear and immediate case in mind, I'm inclined to keep this simple until a use case arrises.
| */ | ||
| public static function fromString(string $options = null): ValidationRule | ||
| { | ||
| return new self(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use new static() in case we want to be able to extend Exclude?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've become very reluctant to use new static() prematurely, as it's bitten me in the past. It's not obvious, but when using new static() one is locking in the constructor for child classes, so to pass static analysis one must use an interface or make __construct() final. Either way, you end up limiting inheritance. So until a use case comes up, I've found it's better to assume child classes will override this method if need be, allowing them to also change the constructor.
| */ | ||
| public function __invoke($value, Closure $fail, string $key, array $values) | ||
| { | ||
| if (($value === '' || $value === null) && $this->conditions->passes($values)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we are checking for an empty string or null or can we just do empty($value)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because empty(0) equals true, and in this case that can be a valid value which shouldn't prevent further validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about if the value was an empty array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that should still go through validation. It doesn't now, but I could conceive of the min/max (or minLength/maxLength) rules still being useful here: 'repeater' => ['min:0', 'max:3']
In any case, we're not really handling arrays, yet. So I'd rather add that later once we've thought through it more and have a need.
|
Thank you for your reviews, @bordoni and @jonwaldstein! I've addressed your obvious fixes and replied for conversation to other comments. Back to you for review or response. 🙂 |
jonwaldstein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JasonTheAdams nice job! i'm interested in seeing these rules in action. 🥋
This PR adds the following rules:
The
Excluderules will prevent any further validation rules from applying to the value, and then exclude the value entirely from theValidator::validated()returned data. This is useful for preventing a complex set of rules from occurring while communicating that the value should not be considered validated.Next, the
IfandUnlesssuffixed rules allow apply the rules only if or unless the dataset conditions apply. For example:In this case, the
t_shirt_sizeis only included in the validated values if the user said they're attending the event. The same conditional thinking applies to theOptionalandNullablecounterparts, while maintaining their distinct rules.