Extract and populate values for nested fieldsets in Collection elements #5093

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

thestanislav commented Sep 9, 2013

If collection element has nested fieldsets, objects are never bound and values are never extracted or populated.
This should fix it

@bakura10 bakura10 commented on an outdated diff Sep 9, 2013

library/Zend/Form/Element/Collection.php
@@ -519,6 +519,30 @@ public function extract()
}
}
+ // Recursively extract and populate values for nested fieldsets
+ foreach ($this->fieldsets as $fieldset) {
+ $name = $fieldset->getName();
+ if (isset($values[$name])) {
+ $object = $values[$name];
+
+ if ($fieldset->allowObjectBinding($object)) {
+ $fieldset->setObject($object);
+ $values[$name] = $fieldset->extract();
+ }else {
@bakura10

bakura10 Sep 9, 2013

Contributor

Add a space before else.

Contributor

bakura10 commented Sep 9, 2013

Change makes sense to me. Could you add a test please ? :)

Contributor

thestanislav commented Sep 11, 2013

Actually this issue is not related with input filters. The fix should just populate values on nested elements and fieldsets those collection has and also bind objects to fieldsets.

@samsonasik samsonasik commented on an outdated diff Sep 14, 2013

library/Zend/Form/Element/Collection.php
@@ -519,6 +519,30 @@ public function extract()
}
}
+ // Recursively extract and populate values for nested fieldsets
+ foreach ($this->fieldsets as $fieldset) {
+ $name = $fieldset->getName();
+ if (isset($values[$name])) {
+ $object = $values[$name];
+
+ if ($fieldset->allowObjectBinding($object)) {
+ $fieldset->setObject($object);
+ $values[$name] = $fieldset->extract();
+ } else {
+ foreach($fieldset->fieldsets as $childFieldset){
@samsonasik

samsonasik Sep 14, 2013

Contributor

add space after foreach before and after open/close parenthesis.

@weierophinney weierophinney added a commit that referenced this pull request Oct 22, 2013

@weierophinney weierophinney Merge pull request #5093 from thestanislav/fix/collection
Extract and populate values for nested fieldsets in Collection elements
b30476e

@weierophinney weierophinney added a commit that referenced this pull request Oct 22, 2013

@weierophinney weierophinney Merge branch 'hotfix/5093' into develop
Forward port #5093
e8fc367

berny46 commented Nov 19, 2013

This new code block in Collection.php breaks my application. I'm using ZF2 with Doctrine 2 and before the change, all my values in nested elements were populated. Removing the new code block leads to a working application again.

Contributor

fabiocarneiro commented Nov 19, 2013

Same here. Ive added a failing test for that. zendframework#5495 . But if you notice its necessary, since without it, the nested fieldsets does not have objects. Something is wrong with that code, but i dont know what.

@danizord danizord added a commit to danizord/FhcsFormTest that referenced this pull request Dec 12, 2013

@danizord danizord Fix the problem of bind() throwing exception
This commit modifies the IndexController to test the really needed behavior: $form->bind()

The problem: $form->bind() method was throwing an unespected exception.
This problem was introduced in ZF 2.2.5 through zendframework/zendframework#5093

So the fatest solution would be downgrade to ZF 2.2.4, then lets make it :D
dcc5a0b

This is wrong! Isnt it? Why the hell would you need to set a hydrator for a collection? This is an array of objects!

Member

Ocramius replied Jan 24, 2014

A form collection is not just an array

Contributor

fabiocarneiro replied Jan 24, 2014

Why not? For me they're arrays of form elements. You bind an array of objects and expect it to pass each object of this array to the fieldset class. The hydration process should be handled by the fieldset. So, why the collection need a hydrator?

Do you really think is productive to have to add a hydrator to every collection you have that will only be responsible to get each element of the array and set in the fieldsets, or would be much more simple to let the collection class handle that?

Contributor

fabiocarneiro replied Jan 24, 2014

If you take a look at the ProductFieldset test asset that was used in this test, you'll notice it also has a collection, but in that case, a hydrator was not set to it. Kind of confusind, huh?

https://github.com/zendframework/zf2/blob/master/tests/ZendTest/Form/TestAsset/ProductFieldset.php#L45

Contributor

thestanislav commented on a017aba Jan 24, 2014

fabiocarneiro Are you a troller?

Contributor

fabiocarneiro replied Jan 24, 2014

@thestanislav No, I'm just someone with a problem in a form, and trying to solve it for more than one month, and believe me, i'm not the only one.

How did this end up in the framework? The code in this foreach is never executed since fieldsets property is out of the scope at this point (protected). It likely should be $fieldset->getFieldsets(). These lines aren't backed up with a test.

Member

Ocramius replied Apr 7, 2014

@netiul can you open an issue?

@thestanislav thestanislav added a commit to thestanislav/zf2 that referenced this pull request Apr 8, 2014

@thestanislav thestanislav Remove PR #5093 code db4bd6a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment