Fix collection hydration problem

[svycka]

as @write2art commented that #14 didn't fixed everything. So this is suggestion how that could be fixed.

feedback is welcome as I am not sure this is the best way to fix it.

[svycka]

There is one invalid test not sure if this is related. It's possible due other bug or not :)

zend-form/src/Form.php

Line 355 in 6da5316

$data = $filter->getValues();

this returns:

array(1) {
  ["addresses"]=>
  array(1) {
    [0]=>
    array(1) {
      ["street"]=>
      string(7) "street1"
    }
  }
}

shouldn't it return phones also? Because it does not return phones

zend-form/src/Form.php

Line 377 in 6da5316

protected function prepareBindData(array $values, array $match)

can't work correctly.

[DASPRiD]

I don't think this is the right place to solve this. What if I don't want to update the collection, but instead it is cleared now?

[write2art]

@DASPRiD I`m totally agree. We still need a way not to send a collection and leave it as is without clearing. But we need a way to send an empty array and clear the collection also.

[svycka]

@DASPRiD how would you determine that you need clear or not? also if you want to leave collection you could just remove collection fieldset from form no?

[svycka]

@DASPRiD I think that not clearing collection on object is not a good idea because form collection is cleared anyway and won't render. This is just hydrator part that ignores that collection in form is empty and should be cleared. What do you think?

[manuakasam]

Can we get any more input on this? This seriously is a big issue that we need to tackle.

On literally every project im running into this code:

$obj = $form->getData();

if (false === isset($_POST['stupid_collection'])) {
  $obj->clearStupidCollection();
}

and that's just insane.

I think that the approach outlined here is perfectly valid. However this changes default behavior in a pretty drastic way. I would suggest to add an option to the hydrator implementation that would enable this feature. Something like clear_collections or clear_unsent_collections, clear_non_posted_collections, something along those lines.

Kind of have to pick my own nose as far as blaming goes, but truthfully we're at 2.6 now and an issue of this scale still persists :D that's kinda not good ^^

pingflood for some more input on the concern @DASPRiD @bakura10 @weierophinney

[svycka]

@manuakasam i don't think that this kind of option is good because form collection is cleared anyway, if you try to render form you wont get any items from collection as they are cleared. As I said this is just hydrator that does not do its job and not clear collection. Maybe this option would be some kind of workaround for BC if any, but not more.

You have few options to not remove collection without this option:

• set custom hydrator that would ignore your collection from hydration(would still have data for rendering)
• remove collection from form(if you don't need collection for rendering)

Also as this is enterprise framework having this issue from version 2.0 to 2.6 is pretty disappointing. I hope this will not go into 3.0

I would also like more input on this how ZF team want this to be handled?

[manuakasam]

Don't get me wrong.
I am absolutely pro simply removing the collection if the key isn't
posted.
This is due to me not really seeing the use-case where i render the
collection (read: have it in the form) but don't want it updated. And if it
is not in the Form it won't be cleared either.

The option solution simply came to mind due to the input that dasprid has
given.

Vytautas Stankus [email protected] schrieb am Fr., 30. Okt. 2015
um 08:16 Uhr:

@manuakasam https://github.com/manuakasam i don't think that this kind
of option is good because form collection is cleared anyway, if you try to
render form you wont get any items from collection as they are cleared. As
I said this is just hydrator that does not do its job and not clear
collection. Maybe this option would be some kind of workaround for BC if
any, but not more.

You have few options to not remove collection without this option:

• set custom hydrator that would ignore your collection from
  hydration(would still have data for rendering)
• remove collection from form(if you don't need collection for
  rendering)

Also as this is enterprise framework having this issue from version 2.0 to
2.6 is pretty disappointing. I hope this will not go into 3.0

I would also like more input on this how ZF team want this to be handled?

—
Reply to this email directly or view it on GitHub
#19 (comment)
.

[FabianKoestring]

👍 for getting this as fast as possible!

[DASPRiD]

This is definitely a general design flaw within Zend\Form. Personally I'd in fact argue that if Zend\Form::setData() receives data and there are missing values (like for instance missing collection arrays due to nothing selected, but also other fields) for elements which are defined by the form, it should ask the elements for default values. A multi select may supply an empty array as default, which text elements may supply empty strings as defaults.

As far as I can judge, this should not be solved in the InputFilter.

[rsplithof]

👍 Need this ASAP!

[FabianKoestring]

Any news on this?

[svycka]

@FabianKoestring I am not sure where we are. @DASPRiD said he doesn't like this. So I am waiting for suggestions how this could be fixed, but I guess ZF team is too busy with ZF3 to answer this :) lets hope they will fix it in ZF3 :D

[weierophinney]

I'm inclined to bring this in verbatim. The current workarounds necessary to allow the forms to validate are not trivial to learn; if a collection is required, an empty set will typically cause the collection to invalidate anyways.

Scheduling for next maintenance release (2.7.1).

[weierophinney]

Hm, interestingly there's a new test failure as a result of this change: ZendTest\Form\Element\CollectionTest::testDoesNotCreateNewObjectsWhenUsingNestedCollections fails, as the returned collection is empty after this change. I'll see if I can figure out why, but I cannot, I'll be tossing it back to your court, @svycka .

[weierophinney]

Found the problem, and really unsure how to go about addressing it.

Essentially, this patch has exposed a subtle bug that was masquerading as a feature. The test was checking to ensure that a value in a nested fieldset, when resubmitted, would not change. What's interesting is that, in stepping through the code, it appears that the reason it worked previously was because the nested fieldset was being silently ignored during binding. The test was passing, but because the scenario it described just happened to also be the result of an issue.

To dive deeper, when I stepped through the code, the input filter was not returning the phones data from getValues(). Without the code change, because the value was not present in the array, the property was ignored during population, which left it intact. With the code change, since the property represents a collection, it's reset to an empty array, overwriting the value.

I've tried a few different approaches:

• Passing $this->data to bindValues() from within Form::isValid() (instead of calling it without an argument)
• Adding a boolean flag to alter the workflow of bindValues() based on whether or not it is called post validation. (I didn't particularly like this solution anyways due to the fact that it deviates from the current interface.)

Neither had an effect. In the first case, the prepared data is still pulled from the input filter, and is thus missing the phones data; in the second, the nesting prevented the flag from propagating correctly.

Essentially, there's a far deeper issue here, and, unfortunately, while it's a bug, the fact is that the patch still represents a change in expected behavior, and has the potential of breaking existing applications.

As such, I'm removing it from the 2.7.1 milestone until that can be resolved.

[svycka]

@weierophinney should I try to fix this bug in this PR? Are you planning to to fix this by yourself in another PR or what? this is pretty annoying bug.

Should I rebase? But I'm not sure this will be merged at all :)

[weierophinney]

@svycka Rebase, and see if you can figure out how to make it work, please, based on the analysis I did above. 😄

[svycka]

@weierophinney I need some help. I think found the problem, but not sure I understand how this should work and if this is a bug. Can you explain what is going on here: https://github.com/zendframework/zend-form/blob/master/src/Form.php#L818-L840

I have added InputFilterProviderInterface for ZendTest\Form\TestAsset\PhoneFieldset and tests pass, but I think they should pass and without this.

[svycka]

@weierophinney I added coveralls fix into this PR because I needed coverage. Can revert if you want.

[svycka]

ping @weierophinney

[FabianKoestring]

I dont want to spam around but why isnt this merged?

[svycka]

rebased, squahed and moved unrelated coveralls fix maybe this will make things faster? :)
Please, say if I missed something or you want anything else.

I am still not sure if this is not a bug, but can't imagine form without InputFilter object or InputFilterProviderInterface and without any explanation or example I can't proceed further. So please merge or explain.

There is the line where empty inputFilter is added:

zend-form/src/Form.php

Line 839 in 82b0cdc

$inputFilter->add(new InputFilter(), $name);

I don't understand the use case for this.

[weierophinney]

I am still not sure if this is not a bug, but can't imagine form without InputFilter object or InputFilterProviderInterface and without any explanation or example I can't proceed further. So please merge or explain.

There is the line where empty inputFilter is added:

zend-form/src/Form.php

Line 839 in 82b0cdc

$inputFilter->add(new InputFilter(), $name);

I don't understand the use case for this.

In that code block, $childFieldset is being tested for:

• if it is a Collection, and
• if it implements InputFilterAwareInterface, and
• if it returns an input filter.

What's interesting about this is that Collection, as a type of Fieldset, does not implement InputFilterAwareInterface by default. That's what the else block you linked is guarding against; we need some input filter in order to ensure validation works and we get back all values; if none is provided or accessible for the child fieldset, we need to provide one.

So, as such, that's not a bug; it's just protective coding. 😁

Reviewing to merge, as I discovered that we also need to add zend-i18n as a direct requirement (view helpers depend on it), forcing a 2.9.0 release.

[weierophinney]

Thanks for fixing this typo. 😄