Adding a validation method to validate the count of a value #8716
Conversation
| $count = $check1; | ||
| } | ||
|
|
||
| if (!$count) { |
There was a problem hiding this comment.
What if count is 0 and you wanted to ensure the count was 0?
|
@dereuromark how is the null check you propose better than isset() here? |
|
@burzum What you just did is an anti-pattern. See the discussion about wrong usage of isset and !empty() cloaking possible typos etc. Always prefer non-cloaking check if possible, and in this case it is by simply declaring |
if ($count === null) {
return false;
}results in |
|
👍 |
| $int = 0; | ||
| $this->assertTrue(Validation::count($int, '==', 0)); | ||
|
|
||
| $this->assertFalse(Validation::count(null, '==', 0)); |
There was a problem hiding this comment.
You could be extra thorough and include test cases for true and false as well.
| $count = count($check1); | ||
| } elseif (is_string($check1)) { | ||
| $count = mb_strlen($check1); | ||
| } elseif (is_int($check1)) { |
There was a problem hiding this comment.
I think this method should only array, traversable or countable stuff. We already have method for intgers and strings
There was a problem hiding this comment.
Uh where? I think this method is pretty convenient now as it is.
There was a problem hiding this comment.
Validation::equals() validates numbers, Validation::lengthBetween() validates string length. This method should only validate stuff that can be safely iterated.
If you think we need more / better methods for strings or integers, you are very welcome to add them.
There was a problem hiding this comment.
@lorenzo Would you mind to remove it from the PR if you want to get that removed?
There was a problem hiding this comment.
Sure, I can do that tomorrow after work
There was a problem hiding this comment.
I strongly disagree with that but fine, I've removed it. It was just convenient.
There is no equals, just equalTo and it does not allow me to take a string and compare the count of it to something. http://api.cakephp.org/3.2/source-class-Cake.Validation.Validation.html#592 In fact it's just a strong typed comparison.
lengthBetween requirs a min and max value. http://api.cakephp.org/3.2/source-class-Cake.Validation.Validation.html#121
Both don't do the same my implementation allowed you to do.
There was a problem hiding this comment.
I don't mind adding more methods to the validation if you want this for strings
Requested by Jose.
|
Even better now 👍 |
|
@dereuromark well I consider it a huge step back. See my comment on the change. #8716 (comment) |
| */ | ||
| public static function numElements($check1, $operator, $expectedCount) | ||
| { | ||
| if (!is_array($check1) || $check1 instanceof \Countable) { |
There was a problem hiding this comment.
! is needed before $check1 instanceof \Countable. Adding ArrayObject test might be nice.
Used to check the count of a given value of type string, int, array or countable. If a string value is passed the string length is used as count.