Hotfix/non iterable inputfilter value #163
Conversation
src/BaseInputFilter.php
Outdated
| @@ -523,6 +523,11 @@ protected function populate() | |||
| $value = $this->data[$name]; | |||
|
|
|||
| if ($input instanceof InputFilterInterface) { | |||
| // Fixes #159 | |||
| if (! is_array($value) && !$value instanceof Traversable) { | |||
There was a problem hiding this comment.
add space after not (!) operator
There was a problem hiding this comment.
thanks for pointing that out, I saw the travis build fail indeed. Didn't find time to fix it yet.
|
@samsonasik everything ready to merge? |
|
I’m not maintainer ^^ |
Lol, my bad |
|
@weierophinney ready to merge or do you need me something to change? |
| @@ -523,6 +523,11 @@ protected function populate() | |||
| $value = $this->data[$name]; | |||
|
|
|||
| if ($input instanceof InputFilterInterface) { | |||
| // Fixes #159 | |||
| if (! is_array($value) && ! $value instanceof Traversable) { | |||
There was a problem hiding this comment.
I am not sure this is a good fix. In case of invalid data, validation result can be valid in some situations.
There was a problem hiding this comment.
…validation result can be valid in some situations.
In this case there is no validation result. Only an exception.
There was a problem hiding this comment.
@froschdesign I understand that this fixes exception just not sure if this will not cause other problems. For example, if this nested input filter is optional and invalid data was given it will replace an invalid object with [] and nested input filter will not be able to validate or return an error in case of invalid data.
But I don't know maybe that's not a problem since it was invalid anyway. If we ignore a problem that we can't return error messages for this then this is a good fix.
There was a problem hiding this comment.
if this nested input filter is optional
You can not set an input-filter as optional, therefore an exception is thrown.
Please look also at the related discussion: #159
There was a problem hiding this comment.
okay, I am not sure about this so let's be it if this will be tested in all situations
There was a problem hiding this comment.
Btw. for an optional input-filter you can use "Optional Input Filters".
There was a problem hiding this comment.
I am more concerned not about optional input filter but about its optional inputs but I don't know if this is bad
There was a problem hiding this comment.
The previous link is for an entire input-filter and not for single (optional) input!
|
@MadeRelevant your test does not cover your fix see: https://coveralls.io/builds/15520340/source?filename=src%2FBaseInputFilter.php#L528 |
|
@svycka hmm, thats really weird, I ran the tests and it actually hits a breakpoint if I put it there.. Should be enough to prove it is touched and covered? When I run the tests with coverage it is for some strange reason not covered.. I have no clue why not while it reaches the breakpoint. I must be doing something wrong but not sure what, never had this before. Any clues? |
|
don't know but if the test fails without this fix and does not with it then it is ok |
Yes it fails without it, just tested it. But still I think it is strange.. Bug in xdebug? PHPUnit reporter? Very weird.. |
|
@MadeRelevant oh I see whats wrong you have to move test to BaseInputFilterTest or add annotation like this: /**
* @covers \Zend\InputFilter\BaseInputFilter
*/
public function testNestedInputFilterShouldAllowNonArrayValueForData()
{
// ... |
| @@ -523,6 +523,11 @@ protected function populate() | |||
| $value = $this->data[$name]; | |||
|
|
|||
| if ($input instanceof InputFilterInterface) { | |||
| // Fixes #159 | |||
There was a problem hiding this comment.
I guess comment is not required as it is covered with test
There was a problem hiding this comment.
Nice, coverage is fixed. I left the comment where it was. If it needs to be removed tell me.
…lter-value Hotfix/non iterable inputfilter value
|
Thanks, @MadeRelevant! |
Fixes issue #159
Currently, when you send a non-iterable value, non-null value to a nested input filter, it incorrectly raises an InvalidArgumentException. It should report as if all underlying fields are empty, and treat the value as an empty array.