Collection in Form not binds values when form has no object and hydrator set #3373

Closed
michaelboke opened this Issue Jan 7, 2013 · 6 comments

3 participants

@michaelboke

When having a form with only one collection as a base fieldset, the binded objects in the fieldsets are not getting their values binded when validating the form.

A quick fix is to set a fake object and a hydrator on the form to trigger the bindValues on the objects in the collection/fieldset.
See the setObject en setHydrator in testCollectionForm
->setObject( new \stdClass() )
->setHydrator(new \Zend\Stdlib\Hydrator\ClassMethods())
the testObject model is getting their values set.

Is this expected behavior?

I believe the fault is in Zend Form, function bindValues(), there is a early out when the is no object set on the form. But what about collections who are set as the base fieldset. They probably dont exchange their objects to the form.

Possible fix
When i change the line 272 in Zend/Form/Form.php
From:

public function bindValues(array $values = array())
{
    if (!is_object($this->object)) {
        return;
    }
    .....

To:

public function bindValues(array $values = array())
{
    if (!is_object($this->object)) {
        if ( $this->baseFieldset === null || $this->baseFieldset->allowValueBinding() == false )
            return;
    }
    .....

Now it works as expected.

Test classes
This is what is used as the test classes

class TestCollectionForm extends \Zend\Form\Form
{
    public function __construct( $name = null, $options = array )
    {
        parent::__construct($name, $options);

        $this->setAttribute('method', 'post')
            ->setBindOnValidate(Form::BIND_ON_VALIDATE)
            //->setObject( new \stdClass() ) // <<- fake class it will bind values of the objects in the fieldset
            //->setHydrator(new \Zend\Stdlib\Hydrator\ClassMethods()) //<<- need a hydrator
            ->setInputFilter(new InputFilter());

        //adds a collection of 5
        $this->add(
            array(
                'type' => '\Zend\Form\Element\Collection',
                'name' => 'test',
                'options' => array(
                    'use_as_base_fieldset' => true,
                    'count' => 5,
                    'should_create_template' => true,
                    'allow_add' => true,
                    'target_element' => array(
                        'type' => '\TestFieldset'
                    ),
                )
            )
        );

        $this->add(
            array(
                'name' => 'submit',
                'attributes' => array(
                    'type' => 'submit',
                    'value' => 'Send'
                )
            )
        );

        $this->setValidationGroup(
            array(
                'test' => array(
                    'name',
                ),
            )
        );
    }
}

class TestFieldset extends \Zend\Form\Fieldset implements \Zend\InputFilter\InputFilterProviderInterface
{
    public function __construct( $name = null, $options = array() )
    {
        parent::__construct( $name, $options );

        $this->setHydrator(new \Zend\Stdlib\Hydrator\ArraySerializable())
                ->setObject(new TestObject());

        $this->add(array('name' => 'name'));
    }

    public function getInputFilterSpecification()
    {
        return array(
            'name' => array(
                'required' => false,
                'filters' => array(),
                'validators' => array(),
            )
        );
    }
}

class TestObject
{
    /**
     * Name
     * @var string
     */
    public $name;

    public function getArrayCopy()
    {
        return get_object_vars($this);
    }

    public function exchangeArray($data = array())
    {
        $this->name = $data['name'];
    }
}

Let me know if you need more info, tests etc.

@bartmcleod

I looked into this issue and have reproduced your case. Your fix works for me too. Your question is whether it is intended behavior or not. Since I did not develop the component, I don't know. What I do know is what I read in the manual. The manual says two things about it:

  1. Form should hydrate all sub objects
  2. Subobjects are meant for one-to-many relations with a parent object, so that a complete entity can be hydrated.

Because of 1. I would says that your point is a valid one and that you have discovered an issue. This would mean we have to make a pull request that includes not only your fix, but also a unit test to ensure it stays fixed.

Because of 2. however, I would say that if there is no parent object (you only have children in your form, they are actually orphans) then there is no point in trying to form a complete entity: this must have been the line of thought for the early return (which you have fixed).

But inspite of 2. I would say there is no reason to disallow a form with only orphans. Unit tests will have to demonstrate that no other functionality breaks when allowing them.

Any opinions from the original creator of the component?

@bartmcleod

Anyone interested in allowing this use case (without the workaround)? In that case I'll start running existing tests against the proposed fix and add new tests.

@bakura10

Hi,

I'm the one behind this component. This fix looks sane to me. The problem as you said is that form absolutely needs an object (most of the time, we have to add an object to the fieldset, and the same object to the form). This is something that will need to be refactor for ZF 3 I suppose.

But in case of Collection being the base fieldset (this is an edge case I've never thought about to be honest), it's not really logical to add an object to the form.

Thanks for discovering that !

@bartmcleod

bakura10: Thanks for your comment. I'm currently working on a unit test for the fix.

@bartmcleod bartmcleod added a commit to bartmcleod/zf2 that referenced this issue Mar 26, 2013
@bartmcleod bartmcleod fix for #3373 f2daf50
@bartmcleod bartmcleod referenced this issue Mar 26, 2013
Closed

Fix#3373 #4123

@weierophinney weierophinney added a commit that closed this issue Mar 28, 2013
@weierophinney weierophinney Merge branch 'hotfix/4123'
Close #4123
Fixes #3373
dce846c
@michaelboke

Great that is fixed, Will try to provide a unittest the next time

@bartmcleod

It was a pleasure, you're welcome. By the way, I used all of the code you provided for both the fix and the new test assets.

@ghost Unknown pushed a commit that referenced this issue Jul 14, 2013
@bartmcleod bartmcleod fix for #3373 90d3a56
@ghost Unknown pushed a commit that referenced this issue Jul 14, 2013
@weierophinney weierophinney Merge branch 'hotfix/4123'
Close #4123
Fixes #3373
2a192e3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment