Method setObject on Zend/Form/Element/Collection overrides count of target element

[webdevilopers]

Given is One Entity Employee with Many Comissions.
The ArrayCollection is binded to the form and the DoctrineModule\Stdlib\Hydrator\DoctrineObject is used.

I add a count of 2 to my 'Zend\Form\Element\Collection'.

        $this->add(array(
            'type'    => 'Zend\Form\Element\Collection',
            'name'    => 'comissions',
            'options' => array(
                'label' => 'Comissions',
                'count' => 2,
                'should_create_template' => true,
                'allow_add' => true,
                'target_element' => array(
                    'type' => 'ComissionFieldset',
                )
            )
        ));

Expected behaviour when creating new Entity:
Though my binded Entity Employee has 0 items the Collection should render 2 Fieldsets with all its element as dummies.
https://github.com/zendframework/zf2/blob/master/library/Zend/Form/Element/Collection.php#L464-478

Current behaviour:
Indeed two Fieldsets are renderer but no element is rendered. Only the legend and span HTML are generated.

Cause:
https://github.com/zendframework/zf2/blob/master/library/Zend/Form/Element/Collection.php#L175
L. 175 overrides the custom count option with the count of existing items in the Entity collection.

Should this be changed?

Original issue: doctrine/DoctrineORMModule#336 (comment)

[acaciovilela]

👍

[ltouro]

@webdevilopers nice catch! Maybe we can fix that like:

$this->count  = count($object) > $this->count ? count($object) : $this->count;

[webdevilopers]

This solution works fine @ltouro !
I can create a PR. But do we need any Test on this?

https://github.com/zendframework/zf2/blob/master/tests/ZendTest/Form/Element/CollectionTest.php

Ps.: The credits belong to @acaciovilela in first place. 🙏

[ltouro]

I think tests is a must for the PR be acceptable
What you think about that:

public function testMininumLenghtIsMaintanedWhenSettingAnSmallerCollection()
    {
        $arrayCollection = array(
            new Element\Color(),
            new Element\Color(),        
        );

        $collection = $this->form->get('colors');
        $collection->setCount(3);
        $collection->setObject($arrayCollection);   
        $this->assertEquals(3, $collection->getCount()));
    }

[gabrielsch]

nice catch, I think this line was added on 2.3.0.

[acaciovilela]

@gabrielsch There's no this line on 2.3.0 version. I just checked. At least at Zend\Form\Element\Collection class. There is the same line:

$this->count = count($object);

[ltouro]

This line was first introduced by @bakura10 on 13d2f65#diff-31fe6580a729ea0519189ac9ef45928a

[acaciovilela]

Is there a special reason for this line has been introduced? It seems unnecessary.

[ltouro]

Supose that you set a count with value 3 for a Element\Collection. If the array of objects binded to the Element\Collection have 4 elements, the form will render only 3 (without that line).

The unwanted side effect is: If the array of objects binded to the collection have 2 elements, the form will render only 2 (with that line). OR, as in your case, if the array collection have 0 elements, the form will render nothing for that Element\Collection.

[acaciovilela]

It's truth... so @ltouro, your suggestion would be the right thing to fix that.

[webdevilopers]

I contacted @bakura10 for a final comment on this. After that I will create the PR and the Test provided by @ltouro ,

[bakura10]

I'm not sure. As always, I cannot really comment this because I don't use Zend\Form anymore. It seems that my line was added for ZF 2.0.3 but merged for 2.3 so this is a bit strange.

However by reading the issue like this, I agree with the expected behavior @webdevilopers. My only concern is that it should be merged for 2.4 as this is a BC break.

Sorry for not providing more feedbacks, I suppose you should create a PR and @weierophinney will have a closer look, but to me this seems like a nice fix :).

[ltouro]

@bakura10, thanks for your comments. Could you please further explain why this change is BC break?

Also, do you mind telling us which technique you are using to render forms and validate users inputs =)?

[bakura10]

It's a BC break because it will change the output of such forms (@WebDevelopers say that it currently renders 0 elements, while after the fix it will render 2 elements). Although it is a bug fixed, it changes the default behavior so this is still considered as BC and should therefore be included into next minor version :).

Regarding forms, I've moved to client-side rendering, so I render my forms in JavaScript, and I validate using ZF2 Input Filters :) (that Zend\Form uses internally, btw). I just don't render forms in server-side anymore :).

[ltouro]

@bakura10 I see now... Thank you very much!

[webdevilopers]

I will try to add the PR this weekend. Is there something I can do when forking to mark it for ZF 2.4?
At least I should note the BC somewhere, right?

@bakura10 : I would love to see some code of that client-side rendering! ;)

[bakura10]

You should make the PR against the develop branch.

Regarding client side rendering, there is nothing special :). I'm using EmberJS, here is how it looks for instance: http://emberjs.com/guides/templates/input-helpers/

The idea is that instead of letting your server render the forms, you delegate the client to create those, and the client sends the raw data, and your server just validate the values.

[webdevilopers]

I created PR https://github.com/zendframework/zf2/pull/6443/commits, @bakura10 .
I hope the branches are correct.

For some reason a commit which failed with travis by @ezimuel was included.

What do i have to do?

[ma-si]

👍 nice

[Ocramius]

Handled in #6443