Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ensure the wrapElements option in Zend\Form\Form::prepareElement #4383

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

noopable commented May 2, 2013

Check the wrapElements option to avoid a violation of field name,
even if a client calls Zend\Form\Form::prepareElement without checking.

@samsonasik samsonasik commented on an outdated diff May 2, 2013

tests/ZendTest/Form/FormElementManagerFactoryTest.php
@@ -0,0 +1,78 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Mvc

@samsonasik samsonasik commented on an outdated diff May 2, 2013

tests/ZendTest/Form/TestAsset/CustomForm.php
@@ -0,0 +1,63 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Form

@ghost ghost assigned weierophinney May 2, 2013

weierophinney added a commit that referenced this pull request May 2, 2013

Merge pull request #4383 from noopable/patch-FormPrepare
ensure the wrapElements option in Zend\Form\Form::prepareElement

weierophinney added a commit that referenced this pull request May 2, 2013

Contributor

matuszeman commented on 768c3d8 May 6, 2013

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

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.

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.

Contributor

noopable replied Jun 4, 2013

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

$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.

Contributor

noopable replied Jun 4, 2013

Can you try it?

$billing->wrapElements(true);

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]'

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?

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

noopable added a commit to noopable/zf2 that referenced this pull request Jun 4, 2013

noopable added a commit to noopable/zf2 that referenced this pull request Jun 4, 2013

weierophinney added a commit that referenced this pull request Jun 11, 2013

Merge pull request #4582 from noopable/hotfix/nested-form-elementsnam…
…e-wrapping

Fix Nested form element wrapping (relative: #4383)

@noopable noopable deleted the noopable:patch-FormPrepare branch Dec 13, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment