Do not allow setting invalid collections or collection items

[weierophinney]

Unlike the base input filter, CollectionInputFilter::setData() has always allowed any data in. It only raises error conditions in one situation: when validating, if the data is not an array or traversable.

Otherwise, as it iterates through the data, if any item is not an array, it casts it to... an empty array.

This patch updates the behavior of setData() to do the following:

• If the $data argument is not an array or Traversable, it raises an InvalidArgumentException.
• If any item in $data is not an array or Traversable, it raises an InvalidArgumentException.

~~Unfortunately, this change now introduces six errors in the test suite. From my cursory inspection, each of these are malformed tests, in my opinion, as they represent invalid data in the first place.

A later commit will take care of those issues.~~

This change presents a slight behavioral break with previous versions. However, it corrects the previous behavior, as invalid collections and/or invalid items in collections previously would have led to inconsistent state internally (both during validation and when retrieving unknown values), and potentially false positive validation (particularly if any given item was not an array/Traversable, and the collection input filter had no required values).

[adamlundrigan]

@weierophinney good change, but (as you predicted) it broke one of my apps; works fine on 2.7.2 but breaks on 2.7.3 with an InvalidArgumentException. Somewhere in my setup a NULL is getting passed into CollectionInputFilter::setData and <2.7.3 silently dealt with it.

What I have is a janky piece of work dating from the early ZF2 era which uses Form and InputFilter to do CRUD on a Doctrine ORM entity with 3 levels of nested collections. There's also some nasty hacks thrown in for applying RBAC permissions to all those form fields using validation groups. One of those hacks was basically doing this:

if (empty($data)) {
    $data = null;
}

So it was an easy fix, and it's far from a normal setup so I doubt many people will be bitten by the BC break in the same way this was.

[weierophinney]

@adamlundrigan It was a hard call, but we were seeing of behavior from specifically collections that, on analysis, stemmed from the fact that it was either delaying errors until validation, or ignoring invalid sets in the collection, replacing them with empty arrays. I chose to err on the side of correct and consistent internal behavior. Sorry your app broke - but it also sounds like you were able to fix it! Could you please post how you corrected it, so any others who have similar issues can have a guide?

[adamlundrigan]

@weierophinney no worries, it was pretty trivial to track down and fix. The new behaviour is much more dependable so that's a win :+1:

The fix was very application-specific: I had overridden Form::getData so when called with VALUES_AS_ARRAY it would spider through the whole array being returned and find anything that wasn't scalar and either make it scalar or remove it (by setting it null). Back then some form elements expected scalar through setValue but returned objects through getValue (like DateTime, and the ones in DoctrineModule) and this squashed the objects returned by getValue down to scalars by trying variety of methods. The motivation behind this was a crude implementation of PATCH:

$form->bind($entity);
$form->isValid();
$data = $form->getData(FormInterface::VALUES_AS_ARRAY);
$data = array_replace_recursive($data, $patchBody);
$form->setData($data);
// validate, save, etc ...

Yes, two-year-ago-Adam was a terrible person :stuck_out_tongue: