Made the code of a validation example more robust #9993
Conversation
validation/custom_constraint.rst
Outdated
|
|
||
| class ContainsAlphanumericValidator extends ConstraintValidator | ||
| { | ||
| public function validate($value, Constraint $constraint) | ||
| { | ||
| if (!is_string($value) && !(is_object($value) && method_exists($value, '__toString'))) { |
There was a problem hiding this comment.
Now that =I review this, I've got a comment :). If a constraint allows casting an object to null then it should do so for any scalar. What about changing is_string to is_scalar? I can see that it's done here for example https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/EmailValidator.php#L77.
Also by convention; a constraint should not test empty value, for this a NotNull or NotBlank constraint should be used. So it misses:
if (null === $value || '' === $value) {
return;
}at first.
There was a problem hiding this comment.
I disagree with the proposed is_scalar() change. Even the error message of the validator says: The string "{{ string }}" contains an illegal character ... so this must only work with strings, right?
There was a problem hiding this comment.
I've added this:
if ('' === $value) {
return;
}Should it contain a trim($value) call too? Thanks!
There was a problem hiding this comment.
Hmm, casting should be ok, it's about changing the representation to make it fit, then throw an exception if it's not possible. I think we should stay consistent, if we want to keep is_string that strict, then let's remove the __toString() check.
Trimming is not as it alters the given value. Fortunately this will soon be opt-in symfony/symfony#26484.
There was a problem hiding this comment.
@HeahDude I made more changes according to your comments. Thanks for the reviews and for your patience 🙏
validation/custom_constraint.rst
Outdated
| @@ -69,6 +69,10 @@ The validator class is also simple, and only has one required method ``validate( | |||
| throw new UnexpectedTypeException($value, 'string'); | |||
| } | |||
|
|
|||
| if ('' === $value) { | |||
There was a problem hiding this comment.
But the null value would never be checked. We throw an exception if !is_string() above.
There was a problem hiding this comment.
This check must be moved before indeed, and test null.
There was a problem hiding this comment.
The reason is that the constraint should not throw or invalid empty values, or it would superseed other constraints responsibility.
validation/custom_constraint.rst
Outdated
|
|
||
| class ContainsAlphanumericValidator extends ConstraintValidator | ||
| { | ||
| public function validate($value, Constraint $constraint) | ||
| { | ||
| if (null === $value || '' === $value) { | ||
| return; |
There was a problem hiding this comment.
What about adding a commented code line for putting the reason why we must do so?
Fixes #9979.