Fixing Issue #234#235
Conversation
There was a problem hiding this comment.
Nice catch.
>>> in_array(0, ['true', 't', 'yes', 'y', 'on', '1'])
=> true
# Because of
>>> (int) 'foo'
=> 0Turn the check strict introduce a BC break when the value is an instance of class that implements __toString().
I suggest to:
-
Add test without taken
true_valuesorfalse_values.So the test to add is
$t->is($v->clean(0), false) -
Add test when the value is an instance of class that implements
__toString() -
The patch can be to cast the value to string
| protected function doClean($value) | ||
| { | ||
| if (in_array($value, $this->getOption('true_values'))) | ||
| $stringValue = $value !== false ? (string) $value : '0'; |
There was a problem hiding this comment.
I just think that to string conversion trigger more problem than it solve (when the given value is an array).
We have a bug with 0 then only handle this case to be strict, and this minimum patch is enough to make it pass added tests, is'nt it ?
$checkValue = $value;
// Zero is equals to any string as it is the result of string casting `int`.
if (0 === $checkValue) {
$checkValue = '0';
}Full brain storming is following
No need to check whether the $value is not false as a string casting of false is an empty string.
>>> (string) false
=> ""Moreover this condition add an hard-coded configuration. Think when a child class add there own configuration.
So after checking the latest Symfony version validator which have another context.
I suggest this following logic
if (true === $value) {
$stringValue = 'true';
} elseif (false === $value) {
$stringValue = 'false';
} else {
$stringValue = (string) $value;
}But we must be BC compatible so as Boolean values are an edge case then it is better to skip the casting to string.
|
Thanks @pathumhdes , merged! |
Fixing Issue #234 - sfValidatorBoolean - clean method returns true if the tainted value is 0 (integer zero)