Fix for the implementation of Collection Element #5636
Conversation
The current implementation of Collection Element imposes, in prepareFieldset(), that the child element keys are set sequentially, starting from 0. After that, populateValues() will remove the unused indexes and add new ones, according to the input data. This behaviour breaks the test in this commit, as it expects that the first key will always be '0', which is not always true, as when collection elements are dinamically added or removed with javascript. The changes done here makes Collection Element more flexible and maybe a little faster, as it saves resources by avoiding unnecessary creation of child elements, adding them just when neccessary. Also, self::$lastChildIndex can be sent along with the template, so javascript can set a proper index for the next element to be added. FieldsetPrepareAwareInterface and all its references were removed, as they are no longer necessary.
…ents if they all were removed
| use Zend\Form\FormInterface; | ||
| use Zend\Stdlib\ArrayUtils; | ||
|
|
||
| class Collection extends Fieldset implements FieldsetPrepareAwareInterface | ||
| class Collection extends Fieldset |
There was a problem hiding this comment.
Why are you removing the FieldsetPrepareAwareInterface implementation? or is this due to submitting from an out-of-date master branch?
There was a problem hiding this comment.
Just the Collection class implements FieldsetPrepareAwareInterface, it's being used in L183 in the Fieldset class just to call prepareFieldset(). The changes done makes FieldsetPrepareAwareInterface useless, as prepareFieldset() isn't needed anymore.
|
@saltoledo Please rebase your branch from current master, resolve the conflicts, and do a force push. Currently, it looks like this is based on a very out-of-date version, and I'm worried that the changes introduced either overwrite or break new functionality. |
|
#4492 is not resolved by this PR |
|
@saltoledo Notice that GitHub is reporting that the PR cannot be automatically merged -- this is an indication that the code you've written is out-of-date with current master. In your branch, do the following; assume that You'll likely find that when you do the rebase that you have merge conflicts. use |
|
@saltoledo are u work on this fix anymore? |
…ild elements if they all were removed" This reverts commit 99d16a4.
…ents if they all were removed
|
Sorry, I was under a health treatment and unable to work. @weierophinney, when I execute 'git rebase origin/master', it returns 'Current branch form-collection-fix is up to date.' |
The current implementation of Collection Element imposes, in prepareFieldset(), that the child element keys are set sequentially, starting from 0. After that, populateValues() will remove the unused indexes and add new ones, according to the input data. This behaviour breaks the test in this commit, as it expects that the first key will always be '0', which is not always true, as when collection elements are dinamically added or removed with javascript. The changes done here makes Collection Element more flexible and maybe a little faster, as it saves resources by avoiding unnecessary creation of child elements, adding them just when neccessary. Also, self::$lastChildIndex can be sent along with the template, so javascript can set a proper index for the next element to be added. FieldsetPrepareAwareInterface and all its references were removed, as they are no longer necessary.
…ents if they all were removed
…ild elements if they all were removed" This reverts commit 99d16a4.
…ents if they all were removed
…into form-collection-fix
|
@weierophinney I think everything is all right now. I'm still a little rusty, sorry for the mess. |
|
👍 |
Fix for the implementation of Collection Element
- To retain BC with existing code that implemented it. Marked as deprecated.
Forward port #5636 Conflicts: library/Zend/Form/Element/Collection.php library/Zend/Form/Fieldset.php
… one more test to demonstrate regression since zendframework#5636
| $values[$name][$childName] = $fieldset->extract(); | ||
| } | ||
| foreach ($values as $name => $object) { | ||
| $fieldset = $this->addNewTargetElementInstance($name); |
There was a problem hiding this comment.
This line makes this issue #6118
The current implementation of Collection Element imposes, in prepareFieldset(), that the child element keys are set sequentially, starting from "0". After that, populateValues() will remove the unused indexes and add new ones, according to the input data. This behaviour breaks the test in this commit, as it expects that the first key will always be "0", which is not always true (as when collection elements are dinamically added or removed with javascript). The changes done here makes Collection Element more flexible and maybe a little faster, as it saves resources by avoiding unnecessary creation of child elements, adding them just when neccessary. Also, self::$lastChildIndex can be sent along with the template, so javascript can set a proper index for the next element to be added. FieldsetPrepareAwareInterface and all its references were removed, as they are no longer necessary.