Skip to content
Browse files

Fix FormElementManagerFactory fieldName side effect

  • Loading branch information...
1 parent 639b10b commit 768c3d8347bd76ae86f1a6832e374cbe12c2cef1 @noopable noopable committed
Showing with 23 additions and 0 deletions.
  1. +23 −0 library/Zend/Form/Form.php
View
23 library/Zend/Form/Form.php
@@ -188,6 +188,29 @@ public function prepare()
}
/**
+ * 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
*
* Typically, also passes data on to the composed input filter.

11 comments on commit 768c3d8

@matuszeman

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

@noopable

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

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

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

@nclundsten
$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

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

Can you try it?

$billing->wrapElements(true);
@nclundsten

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

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

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

Please sign in to comment.
Something went wrong with that request. Please try again.