Permalink
Browse files

Fix FormElementManagerFactory fieldName side effect

  • Loading branch information...
noopable committed May 2, 2013
1 parent 639b10b commit 768c3d8347bd76ae86f1a6832e374cbe12c2cef1
Showing with 23 additions and 0 deletions.
  1. +23 −0 library/Zend/Form/Form.php
View
@@ -187,6 +187,29 @@ public function prepare()
return $this;
}
+ /**
+ * Ensures state is ready for use. Here, we append the name of the fieldsets to every elements in order to avoid
+ * name clashes if the same fieldset is used multiple times
+ *
+ * @param FormInterface $form
+ * @return mixed|void
+ */
+ public function prepareElement(FormInterface $form)
+ {
+ $name = $this->getName();
+
+ foreach ($this->byName as $elementOrFieldset) {
+ if ($form->wrapElements()) {
+ $elementOrFieldset->setName($name . '[' . $elementOrFieldset->getName() . ']');
+ }
+
+ // Recursively prepare elements
+ if ($elementOrFieldset instanceof ElementPrepareAwareInterface) {
+ $elementOrFieldset->prepareElement($form);
+ }
+ }
+ }
+
/**
* Set data to validate and/or populate elements
*

11 comments on commit 768c3d8

@matuszeman

This comment has been minimized.

Show comment
Hide comment
@matuszeman

matuszeman May 6, 2013

Contributor

Could you please explain why is this needed? It looks like it breaks logic around wrapElements().

Contributor

matuszeman replied May 6, 2013

Could you please explain why is this needed? It looks like it breaks logic around wrapElements().

@noopable

This comment has been minimized.

Show comment
Hide comment
@noopable

noopable May 6, 2013

Contributor

First, it doesn't break logic.
The form's elements are wrapped only if its wrapElements flag is true. This logic is the same way at Form::prepare().
It occurs in a rare case that a client wants to wrap elements by the method prepareElement in spite of "wrapElements-flag" : false.
Is it breaking logic?
The method prepareElement prepares the form to be the ideal state. It is not an element wrapper.
"Prepare the form element (mostly used for rendering purposes)"

Second, this is why.
It is an ordinary use as to the interface concept that a client calls prepareElement manually to prepare the form element.
It is a wrong behavior that prepareElements wraps the elements (as is) without checking the form's flag even if the form's wrapElements-flag is false,

Additionally, compatibility to the DI component
Element, Fieldset and Form implement ElementPrepareAwareInterface. AwareInterface raises auto binding process.
Thus the DI component tries to inject FormInterface.
It is the problem of overusing the AwareInterface. But it is BC break to remove "Aware" from the interface.
We can resolve it temporary by feeding a mock object to type-preference.
But it calls prepareElement without checking wrapElements-flag even though nobody wants to prepare.
I think it is not good but temporary needs.
And we can reduce side effect by adding to check the wrapElements flag.

Contributor

noopable replied May 6, 2013

First, it doesn't break logic.
The form's elements are wrapped only if its wrapElements flag is true. This logic is the same way at Form::prepare().
It occurs in a rare case that a client wants to wrap elements by the method prepareElement in spite of "wrapElements-flag" : false.
Is it breaking logic?
The method prepareElement prepares the form to be the ideal state. It is not an element wrapper.
"Prepare the form element (mostly used for rendering purposes)"

Second, this is why.
It is an ordinary use as to the interface concept that a client calls prepareElement manually to prepare the form element.
It is a wrong behavior that prepareElements wraps the elements (as is) without checking the form's flag even if the form's wrapElements-flag is false,

Additionally, compatibility to the DI component
Element, Fieldset and Form implement ElementPrepareAwareInterface. AwareInterface raises auto binding process.
Thus the DI component tries to inject FormInterface.
It is the problem of overusing the AwareInterface. But it is BC break to remove "Aware" from the interface.
We can resolve it temporary by feeding a mock object to type-preference.
But it calls prepareElement without checking wrapElements-flag even though nobody wants to prepare.
I think it is not good but temporary needs.
And we can reduce side effect by adding to check the wrapElements flag.

@nclundsten

This comment has been minimized.

Show comment
Hide comment
@nclundsten

nclundsten Jun 4, 2013

i have a module that depended on the code before your change. spent almost 4 hours finding this.
if wrapElements() would have been changed to return true by default, it would have not been an issue.

i have a module that depended on the code before your change. spent almost 4 hours finding this.
if wrapElements() would have been changed to return true by default, it would have not been an issue.

@noopable

This comment has been minimized.

Show comment
Hide comment
@noopable

noopable Jun 4, 2013

Contributor

@nclundsten
I'm sorry. Could you show me your use case ?

Contributor

noopable replied Jun 4, 2013

@nclundsten
I'm sorry. Could you show me your use case ?

@nclundsten

This comment has been minimized.

Show comment
Hide comment
@nclundsten

nclundsten Jun 4, 2013

$billing = new Form\Address();  //extends zend form
$billing->setName('billing');

$shipping = new Form\Address();
$shipping->setName('shipping');

$registration = new Form\Registration(); // extends zend form
$registration->add($billing)
    ->add($shipping);

lets use the city input for both the billing and shipping addresses

$shipping = $registration->get('shipping');
$billing = $registration->get('billing');

before your change:

$billing->getCity()->getName(); //billing[city]
$shipping->getCity()->getName(); //shipping[city]

after your change

$billing->getCity()->getName(); //city
$shipping->getCity()->getName(); //city

resulting in multiple form elements on a page with the same name, and undesired results.

also i have just tried this:

$registration->setWrapElements(true);
$billing->getCity()->getName(); //[billing][city]

which is not correct either.

$billing = new Form\Address();  //extends zend form
$billing->setName('billing');

$shipping = new Form\Address();
$shipping->setName('shipping');

$registration = new Form\Registration(); // extends zend form
$registration->add($billing)
    ->add($shipping);

lets use the city input for both the billing and shipping addresses

$shipping = $registration->get('shipping');
$billing = $registration->get('billing');

before your change:

$billing->getCity()->getName(); //billing[city]
$shipping->getCity()->getName(); //shipping[city]

after your change

$billing->getCity()->getName(); //city
$shipping->getCity()->getName(); //city

resulting in multiple form elements on a page with the same name, and undesired results.

also i have just tried this:

$registration->setWrapElements(true);
$billing->getCity()->getName(); //[billing][city]

which is not correct either.

@noopable

This comment has been minimized.

Show comment
Hide comment
@noopable

noopable Jun 4, 2013

Contributor

Thanks.
I got your use case .

Which do you call prepare() or prepareElements() ?
And is wrapElements property true or false in your form classes?

Could you show me repro codes or test case?

Contributor

noopable replied Jun 4, 2013

Thanks.
I got your use case .

Which do you call prepare() or prepareElements() ?
And is wrapElements property true or false in your form classes?

Could you show me repro codes or test case?

@noopable

This comment has been minimized.

Show comment
Hide comment
@noopable

noopable Jun 4, 2013

Contributor

Can you try it?

$billing->wrapElements(true);
Contributor

noopable replied Jun 4, 2013

Can you try it?

$billing->wrapElements(true);
@nclundsten

This comment has been minimized.

Show comment
Hide comment
@nclundsten

nclundsten Jun 4, 2013

just added "setWrapElements" on line 186 in the controller i linked:

$shippingAddressForm->setName('shipping')->setWrapElements(true)
            ->setInputFilter($this->getServiceLocator()->get('SpeckAddress\Form\AddressFilter')); 

still is exactly the same: 'city' instead of 'shipping[city]'

just added "setWrapElements" on line 186 in the controller i linked:

$shippingAddressForm->setName('shipping')->setWrapElements(true)
            ->setInputFilter($this->getServiceLocator()->get('SpeckAddress\Form\AddressFilter')); 

still is exactly the same: 'city' instead of 'shipping[city]'

@noopable

This comment has been minimized.

Show comment
Hide comment
@noopable

noopable Jun 4, 2013

Contributor

Thanks.

I got your use case and realize the problem.

Maybe now, Zend\Form\Form::prepare doesn't take account of Nested Form carefully.

Maybe, this can fix by .
https://github.com/zendframework/zf2/blob/master/library/Zend/Form/Form.php#L180-L182
to change it.

                if ($elementOrFieldset instanceof FormInterface) {
                    $elementOrFieldset->prepare();
                } elseif ($elementOrFieldset instanceof ElementPrepareAwareInterface) {
                    $elementOrFieldset->prepareElement($this);
                }

Could you make a test case for your problems?
Or should I?

Contributor

noopable replied Jun 4, 2013

Thanks.

I got your use case and realize the problem.

Maybe now, Zend\Form\Form::prepare doesn't take account of Nested Form carefully.

Maybe, this can fix by .
https://github.com/zendframework/zf2/blob/master/library/Zend/Form/Form.php#L180-L182
to change it.

                if ($elementOrFieldset instanceof FormInterface) {
                    $elementOrFieldset->prepare();
                } elseif ($elementOrFieldset instanceof ElementPrepareAwareInterface) {
                    $elementOrFieldset->prepareElement($this);
                }

Could you make a test case for your problems?
Or should I?

@nclundsten

This comment has been minimized.

Show comment
Hide comment
@nclundsten

nclundsten Jun 4, 2013

the proposed change works, calling prepareElement instead of prepare on the form was definitely the issue.

the proposed change works, calling prepareElement instead of prepare on the form was definitely the issue.

Please sign in to comment.