Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

fixes #6585 #6614

Closed
wants to merge 13 commits into from
25 changes: 21 additions & 4 deletions library/Zend/Form/Form.php
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,8 @@ public function bind($object, $flags = FormInterface::VALUES_NORMALIZED)

$this->bindAs = $flags;
$this->setObject($object);
$this->extract();
$data = $this->extract();
$this->populateValues($data, true);

return $this;
}
Expand Down Expand Up @@ -480,6 +481,7 @@ public function isValid()

if (!is_array($this->data)) {
$data = $this->extract();
$this->populateValues($data, true);
if (!is_array($data)) {
throw new Exception\DomainException(sprintf(
'%s is unable to validate as there is no data currently set',
Expand Down Expand Up @@ -867,7 +869,24 @@ public function wrapElements()
}

/**
* Recursively extract values for elements and sub-fieldsets, and populate form values
* {@inheritDoc}
*
* @param bool $onlyBase
*/
public function populateValues($data, $onlyBase = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be private or protected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's overriding method from fieldset. So either we need to figure out a new name for this or keep it public :(
We could take that method away from the form all together but that would change the existing behavior a little (Namely that the form would populate every field instead of just the base during bind and validation without data)..Tests seem to pass then also, but I'd be surprised if there wouldn't be any side effects in some app.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's an override then it's fine: I just don't want new API that is an implementation detail :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me neither :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's inherited, could you replace the docblock with {@inheritDoc}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

{
if ($onlyBase && $this->baseFieldset !== null) {
$name = $this->baseFieldset->getName();
if (array_key_exists($name, $data)) {
$this->baseFieldset->populateValues($data[$name]);
}
} else {
parent::populateValues($data);
}
}

/**
* Recursively extract values for elements and sub-fieldsets
*
* @return array
*/
Expand All @@ -876,10 +895,8 @@ protected function extract()
if (null !== $this->baseFieldset) {
$name = $this->baseFieldset->getName();
$values[$name] = $this->baseFieldset->extract();
$this->baseFieldset->populateValues($values[$name]);
} else {
$values = parent::extract();
$this->populateValues($values);
}

return $values;
Expand Down
79 changes: 79 additions & 0 deletions tests/ZendTest/Form/Element/CollectionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,85 @@ public function testCreatesNewObjectsIfSpecified()
$this->assertNotSame($categories[1], $cat2);
}

public function testAddingCollectionElementAfterBind()
{
$form = new Form();
$form->setHydrator(new ObjectPropertyHydrator());

$phone = new \ZendTest\Form\TestAsset\PhoneFieldset();

$form->add(array(
'name' => 'phones',
'type' => 'Collection',
'options' => array(
'target_element' => $phone,
'count' => 1,
'allow_add' => true
),
));

$data = array(
'phones' => array(
array('number' => '1234567'),
array('number' => '1234568'),
)
);

$phone = new Phone();
$phone->setNumber($data['phones'][0]['number']);

$customer = new stdClass();
$customer->phones = array($phone);

$form->bind($customer);
$form->setData($data);
$this->assertTrue($form->isValid());
}

public function testDoesNotCreateNewObjectsWhenUsingNestedCollections()
{
$addressesFieldeset = new \ZendTest\Form\TestAsset\AddressFieldset();
$addressesFieldeset->setHydrator(new \Zend\Stdlib\Hydrator\ClassMethods());
$addressesFieldeset->remove('city');

$form = new Form();
$form->setHydrator(new ObjectPropertyHydrator());
$form->add(array(
'name' => 'addresses',
'type' => 'Collection',
'options' => array(
'target_element' => $addressesFieldeset,
'count' => 1
),
));

$data = array(
'addresses' =>
array(array(
'street' => 'street1',
'phones' =>
array(array('number' => '1234567')),
))
);

$phone = new Phone();
$phone->setNumber($data['addresses'][0]['phones'][0]['number']);

$address = new Address();
$address->setStreet($data['addresses'][0]['street']);
$address->setPhones(array($phone));

$customer = new stdClass();
$customer->addresses = array($address);

$form->bind($customer);
$form->setData($data);

$this->assertTrue($form->isValid());
$phones = $customer->addresses[0]->getPhones();
$this->assertSame($phone, $phones[0]);
}

public function testDoNotCreateExtraFieldsetOnMultipleBind()
{
$form = new \Zend\Form\Form();
Expand Down
22 changes: 22 additions & 0 deletions tests/ZendTest/Form/FieldsetTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Zend\Form\Form;
use Zend\Form\Fieldset;
use Zend\InputFilter\InputFilter;
use Zend\Stdlib\Hydrator;

class FieldsetTest extends TestCase
{
Expand Down Expand Up @@ -522,4 +523,25 @@ public function testShouldValidateAllowObjectBindingByObject()
$this->assertTrue($allowed);
}

public function testBindValuesPreservesNewValueAfterValidation()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you can add a group annotation so it is clear why the test is needed? See #6711

{
$form = new Form();
$form->add(new Element('foo'));
$form->setHydrator(new Hydrator\ObjectProperty);

$object = new \stdClass();
$object->foo = 'Initial value';
$form->bind($object);

$form->setData(array(
'foo' => 'New value'
));

$this->assertSame('New value', $form->get('foo')->getValue());

$form->isValid();

$this->assertSame('New value', $form->get('foo')->getValue());
}

}