collectionRawValues not populated in setData of CollectionInputFilter

[rkeet]

Narrative: I use the following isValid function in my InputFilter class:

public function isValid($context = null)
{
    if ( ! $this->isRequired() && $this->arrayFilterIsEmpty($this->getRawValues())) {

        return true;
    }

    return parent::isValid();
}

The function arrayFilterIsEmpty() checks whether or not the current $input contains any data and is required. The parent::isValid() calls the normal isValid function.

Any input (normal Input or Collection) has the function getRawValues to get the raw input data. I use it to check if any data is present combined with a required property to see if a both not-required and empty fieldset/collection can be skipped during validation and just be marked as valid.

During isValid in CollectionInputFilter.php (line 195) the function setData is called (here).

However, setData breaks of setting data if the data is an array or Traversable (here). This causes the collectionRawValues property to never be set.

As such, I suggest updating the function to set this value after the initial check, where it's still handling the collection as a whole instead of per item (before the foreach())

public function setData($data)
{
    if ( ! (is_array($data) || $data instanceof Traversable)) {
        throw new InvalidArgumentException(
            sprintf(
                '%s expects an array or Traversable collection; invalid collection of type %s provided',
                __METHOD__,
                is_object($data) ? get_class($data) : gettype($data)
            )
        );
    }

    $this->collectionRawValues = $data; // added line 

    foreach ($data as $item) {
        if (is_array($item) || $item instanceof Traversable) {
            continue;
        }

        throw new InvalidArgumentException(
            sprintf(
                '%s expects each item in a collection to be an array or Traversable; '
                . 'invalid item in collection of type %s detected',
                __METHOD__,
                is_object($item) ? get_class($item) : gettype($item)
            )
        );
    }

    $this->data = $data;
    return $this;
}

Code to reproduce the issue

Would be a lot of code. Setup to reproduce would require following form setup:

Form > BaseFieldset > Fieldset > Collection with Fieldset target FormInputFilter > BaseFieldsetInputFilter > FieldsetInputFilter > CollectionInputFilter pointing to FieldsetInputFilter

Expected results

Expected collectionRawValues to return raw data using getRawValues() function.

Actual results

Returns empty array.

Received Raw data (example data, concerns "shops" array)

As you can see in the image below, data is received for "shops" to be created.

Debugging getRawValues() (fix not applied!)

As you can see here, when I try to get the raw values, the array is empty.

Debugging getRawValues() (fix applied!)

Here with the fix applied

---

Fixing

I've read the contributing guidelines. In case you'd like me to create a PR for this I can do so, might take some time (I'm sneaking this in at work as I bumped into it, face first), when I get around to it at home.

In the mean time I'm open to alternative methods of fixing the setting of raw data, if any present.

[svycka]

$this->collectionRawValues are filled when you call isValid() method but in your usecase

public function isValid($context = null)
{
    if ( ! $this->isRequired() && $this->arrayFilterIsEmpty($this->getRawValues())) {
        return true; // here is no isValid call so no raw values
    }
    return parent::isValid();
}

[rkeet]

@svycka Please refer to last 2 screenshots in which I show results of getRawValues() with and without fix applied.

[svycka]

yes, it works because you also update raw values in setData method just not sure why this was done in isValid method maybe there was a reason because that should be slower. Instead of one assignment, it was n and don't know why, but maybe that was intentional? If not maybe then remove from isValid method?

[rkeet]

yes, it works because you also update raw values in setData

This is literally what I'm trying to do with the linked PR.

not sure why this was done in isValid method maybe there was a reason because that should be slower

Don't know what you mean with the "should be slower" part. But when comparing availability of raw value data, this seems to be the place. the use-case I placed at the top of the issue, which you referenced, works perfectly for other InputFilter classes which are not CollectionInputFilter instances. Both the InputFilter and CollectionInputFilter get that functionality from BaseInputFilter, which is why I thought to make it available at the point where I inserted it with this patch.

Instead of one assignment, it was n and don't know why, but maybe that was intentional? If not maybe then remove from isValid method?

Could you clarify that? I don't understand what you mean with these questions.

---

edit: switched BaseInputFilter and InputFilter names in sentence as I mixed them up

[svycka]

just saying that maybe collectionRawValues was created for some reason just don't know why becouse otherwise it does not make sense to me as we could use $data instead of $collectionRawValues from https://github.com/rkeet/zend-inputfilter/blob/ae5df37e48933a52a92afd6a8678eb65bbad3fd0/src/CollectionInputFilter.php#L165 but maybe it's ok just saying that it looks strange.

[rkeet]

I reckon it should be ok because at some point data should contain the processed input data, where as collectionRawValues should contain raw input data. See also the comment of function getRawValues() here.

Return a list of unfiltered values

[svycka]

I think that's not so true because if you have data with name, email... but input filter with only name input then raw values will be name only as I understand. But with your fix, it will be name, email... but if this does not break other libraries then maybe ok.

[rkeet]

InputFilters such as for Elements are of type InputFilter and not CollectionInputFilter and as such are not affected by this fix.

[svycka]

InputFilters such as for Elements are of type InputFilter and not CollectionInputFilter and as such are not affected by this fix.

that's also not true InputFilter inside CollectionInputFilter will not be used to get raw data.

@rkeet okay can you write a test to compare getRawValues with before isValid() and after when data contains more data than inputs in input filter if it will pass then I think its safe to say it will not break anything

[rkeet]

The way it works is that a CollectionInputFilter has a target InputFilter class (done using setInputFilter function on CollectionInputFilter). When data is received it is determined how many instances need to be created of said target, which are then held by the CollectionInputFilter. As such, a CollectionInputFilter should contain the raw values of the entire collection of data (this is broken and is what this fix is about).

An InputFilter instance should contain only the raw values specific for that InputFilter (this works).

As such, both the InputFilter instances and the CollectionInputFilter instance should be able to return the raw data pertaining to each particular instance. Currently this is broken for the CollectionInputFilter.

I really don't know how to make that any clearer than that.

that's also not true

What else is not true? Because I'm here arguing to get something broken fixed and I'm thinking your understanding of the implementations of CollectionInputFilter and InputFilter is mixed with that of specific Element instances from zend-form, thus not contained to zend-inputfilter.

can you write a test to compare getRawValues with before isValid() and after when data contains more data than inputs in input

Next to "inputs", by which I'm assuming you mean instances of InputFilter (in the code: $firstInputFilter = new InputFilter()), what else would you like me to add? Things that can be added to a CollectionInputFilter are literally limited to instances extending BaseInputFilter as defined in the setInputFilter function here. As I mentioned before, implementation of InputFilter is not changed by this.

If you see any issues, could you provide detailed examples of how this fix negatively impacts existing implementations?

[svycka]

I am not against this fix just saying that $this->getRawValues() is not always equal $this->collectionRawValues because it will be modified here https://github.com/rkeet/zend-inputfilter/blob/ae5df37e48933a52a92afd6a8678eb65bbad3fd0/src/CollectionInputFilter.php#L212 if this does not break anything then this fix is ok

[rkeet]

@svycka Good catch with that function.

After I expanded the test to also test a more complex case, if did fail because of that line. Please have a look at the revised code. Below results also from debugging manually (to confirm, paranoid like that).

---

Testing without setting raw values in isValid

This passes tests.

The results of the raw values in the main and the cloned (more complex after running isValid), are exactly the same. See the screenshot below (no failed test result, so manual debugging).

Test with setting raw values in isValid

This fails. See screenshot below (fail comparison of test result in PhpStorm)

---

If you see any concerns with updated code, please let me know.

[svycka]

I don't know if this is problem but if invalid tada would be provided for example:

$data = [
            'first_collection' => [
                [
                    'second_collection' => [
                        [
                            'input' => 'some value',
                            'not_exists' => 'some invalid value', // not existed before, but will exist in rawValues after your fix
                        ],
                        [
                            'input' => 'some value',
                        ],
                    ],
                ],
            ],
        ];

but is this a problem? I don't know, but everywhere it this is filtered out, for example, https://github.com/zendframework/zend-inputfilter/blob/master/src/BaseInputFilter.php#L443-L452 they could just use $this->data or create $rawValues instead of all this maybe this is for some reason I not know. Just a guess but maybe this is becouse input filters can be set after setData or something.. just guessing :)

[rkeet]

Added in additional data by your suggestion which should show up in collectionRawValues after isValid. It's there, tests pass. Also fixed build, it had failed on line length of test lines, so put params on new lines for test functions.

[svycka]

I guess test should pass now because you removed https://github.com/zendframework/zend-inputfilter/pull/169/files#diff-10beaa7e4e0d140417aea83b8c20ad8eL210 but it does not change the fact that $this->getRawValues() before and after your fix would return different results. But I don't know maybe this is not a problem at all but I don't know where $this->getRawValues() or how is used in other parts of this module or for example in zend-form I don't know if this does not break something else.

maybe someone can answer this? ping @weierophinney