Null value for inputfilter causes exception

[MadeRelevant]

Providing NULL as the value of nested input filter causes an exception to be thrown.
At the moment I only have my phone to post this issue so I will make a picture of the code to reproduce.
I am in the middle of moving and my ISP messed up :(

If you ommit the value its ok because an empty array will be used. I expected the same behaviour for NULL values.. Please tell me if I miss something here and how this should be fixed at my side in the case I did miss something.

[MadeRelevant]

Passing nested => [] also works btw, which is clear from the code in the BaseInputFilter

[MadeRelevant]

@weierophinney any clues?

[froschdesign]

@MadeRelevant
Please add the code of your test case here or create a PR. Copy & paste from a photo is a little bit complicated. 😉

[weierophinney]

This is by design, currently. InputFilter::setData() expects either an array or a traversable set; any other data type results in an exception.

If you feel this should not be the case, please submit a pull request to allow passing null values.

Thanks!

[MadeRelevant]

I will post code to reproduce tomorrow.
Of this is by design i think it is strange that a user is capabel of producing an exception.. Am i responsible to iterate over all data before sending it into the input filter?

[weierophinney]

@MadeRelevant Let me flip the question around: why are you allowing a null value to be passed for that element?

And, related to that: what should the behavior of the input filter be when the data set is set to null, exactly?

[MadeRelevant]

Thanks. It is not exactly that im allowing them to enter a null value but someone can.

I think the behavior should be the same as if they didnt enter a value at all. Our API client doesnt allow it actually but the issue came forward out of our tests.

[MadeRelevant]

@weierophinney sorry for the delay but here is the code of the screenshot:

public function testBrokenInputFilter() {
        $filter1 = new InputFilter();
        $filter1->add([
            'type' => InputFilter::class,
            'nestedField1' => [
                'required' => false
            ]
        ], 'nested');
        $filter1->setData([]);
        self::assertNull($filter1->getValues()['nested']['nestedField1']); // <-- works

        // this is a value that a client may send when they try to break the API or
        // just don't know what to use as value for example. atm it produces an exception
        $filter1->setData(['nested' => null]);
        self::assertNull($filter1->getValues()['nested']['nestedField1']); // <-- should work
    }

If this is by design it seems inconsistent because entering null or an empty array is both invalid input but produce different results. The empty array produces no exception while the null value does.

Having some inspection run before the call to setData() is made which transforms null values into an empty array if needed seems inefficient and can become very complex imo.

[MadeRelevant]

@weierophinney the changes are not within 2.8.1

[MadeRelevant]

Excuse me, it's there, my bad. A variation of the problem still exists.
If you enter a non null value that is not an array it still throws an exception which is not what I expected because it is still invalid data.

The expected behavior is imo also a bit vague, for example, an array is expected but someone passes an integer or a boolean, it feels strange to turn that into an empty array.. I suppose it is the only option for now?

I suppose changing it into an empty array should be done here: https://github.com/zendframework/zend-inputfilter/blob/master/src/BaseInputFilter.php#L190-L196

What do you think?

[froschdesign]

@MadeRelevant

The expected behavior is imo also a bit vague…

Why vague? The expected data types are null, array and Traversable. Otherwise an InvalidArgumentException is thrown. Works as described:

zend-inputfilter/src/BaseInputFilter.php

Lines 172 to 179 in a35c21a

	/**
	* Set data to use when validating and filtering
	*
	* @param null|array|Traversable $data null is cast to an empty array.
	* @throws Exception\InvalidArgumentException
	* @return InputFilterInterface
	*/
	public function setData($data)

…but someone passes an integer or a boolean…

This makes no sense. An input-filter normalizes and validates a set of inputs. If you only want to check one value then use the Input class.

[MadeRelevant]

I meant by vague that i am not sure what the expected behavior should be if you dont throw an exception if the given datatype is not of type array (at the end).

Sending something else like an integer or boolean doesnt make sense indeed but I dont have control over what a client sends to an input of type InputFilter. They dont care or dont know that it expects an array.

Anyway, where an array, null or traversable is expected but a value not meeting that condition is given should be considered as invalid input and not throw an exception imo.

Now, am exception gives no clue what i am missing or did wrong with my input, or even worse in production any api consumer cant see and dig into logs or whatever. If we threat any invalid value as empty instead of throwing an exception it would indicate in some way what is going on, what is vague to me.

All not perfect if you ask me.

[froschdesign]

…but I dont have control over what a client sends to an input of type InputFilter.

You can control it. For example:

/** @var \Psr\Http\Message\ServerRequestInterface $request */
$inputFilter->setData($request->getQueryParams());

or

/** @var \Zend\Http\Request $request */
$inputFilter->setData($request->getPost());

or

try {
    $inputFilter->setData($data);
} catch (\Zend\InputFilter\Exception\InvalidArgumentException $e) {
    // …
}

[MadeRelevant]

@froschdesign Let me clarify with a simple example, assume the following input filter:

$filter1 = new InputFilter();
        $filter1->add([
            'type' => InputFilter::class,
            'nestedField1' => [
                'required' => false
            ]
        ], 'nested');
        $filter1->setData($request->getParsedBody());

Next, I will demonstrate two cases, from which one produces valid output and the other produces an exception or nothing in production.

Expected
This gives me a nice error that nested should not be empty, sweet.

{
    "nested": null
}

Unexpected
This produces the exception where I have no clue what I have done wrong.
lets say the API consumer 'thought' an integer belonged here and did not intend to let the API crash

{
    "nested": 123 
}

However, I don't have control over what is sent to the InputFilter as you can see, the client has.

I have to agree that sending back exactly the same as for the first example, saying it treats invalid data types as if it was empty, is also not as clear as I would like it to be..

Ofcourse, I could catch the exception but how would I figure out what was wrong? And in the end, I would just report something like 'hi, you did not enter a valid value for X.Y.Z' but resolving the path to X.Y.Z is way to complex if you ask me and should be the job of the InputFilter.

Please let me know if there is something not clear or just doesn't make any sense..

[MadeRelevant]

The value of "nested" should be an array, it threw a exception if null, fixed in 2.8.1. Giving any non-null and non-iterable value produces an exception which is inconsistent. An exception being raised should never be influenced by user input or what is the purpose of the InputFilter? Do I manually need to check on those specific values?

The test where it expects an exception is wrong and inconsistent imo.

I have updated my previous post with correct examples.

[froschdesign]

@MadeRelevant
Please provide always a full (failing) unit test.

[froschdesign]

Btw. The method setData should not be changed, because it works like expected and described.
For nested input-filters please look a step before at the method populate.

[MadeRelevant]

Sorry for late response. I have created a fork with the modified tests.
Please have a look at it here: master...MadeRelevant:master

@froschdesign The values used in the tests are values any user who has access to a http client can send to the input filter. It is the I.F's job to validate this input and not throw an exception when someone sends a boolean where an array is expected, imho.

Let me know if I need to change anything before creating a pull request.

[froschdesign]

@MadeRelevant

Let me know if I need to change anything before creating a pull request.

You missed my last comment! Please see above.

[MadeRelevant]

I didn't miss your comment. setData() calls populate() and the exception is thrown before it calls populate. From your middleware/controller or whatever you use you're supposed to call setData() as well. If you fix it within populate() the problem will still exist in root level input filter in case of invalid data type.

Could you show me how you would implement it in case I missed something?
Other reason I fixed it within setData() is because mwop also fixed it in there.

@weierophinney could you have a look as well?

[froschdesign]

…and the exception is thrown before it calls populate.

Sorry this is wrong if you use nested input-filters. setData is called multiple times in this case. Look at your own test:

$filter1->setData(['nested' => false]);
…
$filter1->setData(['nested' => 123]);
…
$filter1->setData(['nested' => new stdClass()]);

But this does not means we have to change the expected type for setData:

$filter1->setData(false);

setData expects a set of values or nothing. This means that only array, Traversable and null can be allowed. false is only one value.
So throwing an exception is correct here.

Therefore you must look at the method populate to fix the problem with nested input-filters and wrong values.

Other reason I fixed it within setData() is because mwop also fixed it in there.

null has a different meaning, so the fix from @MWOP in setData is correct.

[MadeRelevant]

I guess we are getting stuck in this conversation because I dont understand how you would fix it within populate if the check is done within setData. Could you show me how my test can succeed in a way without changing setData?

If setData(false) or setData(123) for an input filter is wrong in your opinion then please tell me how I can prevent my API consumers to enter such values.

[froschdesign]

We should not mix the problems here!
The current problem is nested input-filters and setData.

Your API consumers are a different topic, which can be fixed by:

• $inputFilter->setData((array) $inputData);
• Or using a single input if only one value is expected.

[MadeRelevant]

I updated the code. It fixes nested input filters.
This is already an improvement if you ask me, I will create a PR for this if nothing strange is in it.

I still think that an API consumer can cause an exception with invalid input is unexpected in the context of an object that has the purpose to filter and validate that (invalid) input..

[froschdesign]

I still think that an API consumer can cause an exception with invalid input…

Please provide your use case for this. Otherwise we can not reproduce your problem.
This illustrates the typical usage:

$inputFilter->setData($_GET);
$inputFilter->setData($_POST);
$inputFilter->setData($request->getParsedBody());
// …

No problematic data types. How do you use it?

[MadeRelevant]

Please provide your use case for this

At the moment I cant come up with a scenario where this will be problematic, my bad.

No problematic data types. How do you use it?

I forgot that the null check remains within setData so in case the BodyParamsMIddleware I use from Zend Expressive results into an empty value everything will be fine.

Shall I make the PR then?

[froschdesign]

Shall I make the PR then?

Please create a local branch for the bugfix. (see "Contributing Bugfixes or Features")
Thanks! 😃

[MadeRelevant]

PR is made