Fix Collection form element replacing bound objects with dummies upon form validation #4077

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@sjuvonen
Contributor

sjuvonen commented Mar 19, 2013

Currently the Zend\Form\Element\Collection will discard bound objects and replace them with clones of its dummy template when the form is validated. This will result in loss of all properties that do not have a field in the Collection's target element (fieldset).

Also the returned object instances have to be the same that were passed to the form originally or it will cause corruption.

@davidwindell

This comment has been minimized.

Show comment
Hide comment
@davidwindell

davidwindell Mar 20, 2013

Contributor

Does this have any conflicts with zendframework#4077?

Contributor

davidwindell commented Mar 20, 2013

Does this have any conflicts with zendframework#4077?

@sjuvonen

This comment has been minimized.

Show comment
Hide comment
@sjuvonen

sjuvonen Mar 20, 2013

Contributor

Do you mean #4075? Yes, these two PRs do conflict, though my patch would be easy to fix with a single-line check to assert createNewObjects() == false. I don't, though, thoroughly understand why a bug should be restored in the functionality albeit optional.

I noticed there is an attempt to fix this very same bug in commit b0c03a3 but it doesn't test the case extensively. My test case tests for when allow_add == true (in the Collection element) AND initial count is set to less than the actual count of objects contained by the parent object that is bound into the form.

The part of the bug that my PR would fix but the original commit doesn't is to preserve all those entities that increase the amount of Collection's rows from the initially specified value. For example, if count in Collection is initially set to 2 but and object with three sub-objects was bound, the third one would be replaced with a dummy.

Contributor

sjuvonen commented Mar 20, 2013

Do you mean #4075? Yes, these two PRs do conflict, though my patch would be easy to fix with a single-line check to assert createNewObjects() == false. I don't, though, thoroughly understand why a bug should be restored in the functionality albeit optional.

I noticed there is an attempt to fix this very same bug in commit b0c03a3 but it doesn't test the case extensively. My test case tests for when allow_add == true (in the Collection element) AND initial count is set to less than the actual count of objects contained by the parent object that is bound into the form.

The part of the bug that my PR would fix but the original commit doesn't is to preserve all those entities that increase the amount of Collection's rows from the initially specified value. For example, if count in Collection is initially set to 2 but and object with three sub-objects was bound, the third one would be replaced with a dummy.

weierophinney added a commit that referenced this pull request Mar 28, 2013

Merge pull request #4077 from sjuvonen/master
Fix Collection form element replacing bound objects with dummies upon form validation

weierophinney added a commit that referenced this pull request Mar 28, 2013

weierophinney added a commit that referenced this pull request Mar 28, 2013

@ghost ghost assigned weierophinney Mar 28, 2013

Slamdunk added a commit to Slamdunk/zf2 that referenced this pull request Apr 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment