Skip to content
This repository

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

Closed
wants to merge 3 commits into from

5 participants

Tomoaki Kosugi Abdul Malik Ikhsan Nigel Lundsten Matus Zeman Matthew Weier O'Phinney
Tomoaki Kosugi

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

tests/ZendTest/Form/FormElementManagerFactoryTest.php
... ...
@@ -0,0 +1,78 @@
  1
+<?php
  2
+/**
  3
+ * Zend Framework (http://framework.zend.com/)
  4
+ *
  5
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
  6
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
  7
+ * @license   http://framework.zend.com/license/new-bsd New BSD License
  8
+ * @package   Zend_Mvc
1
Abdul Malik Ikhsan
samsonasik added a note May 02, 2013

remove @package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/ZendTest/Form/TestAsset/CustomForm.php
... ...
@@ -0,0 +1,63 @@
  1
+<?php
  2
+/**
  3
+ * Zend Framework (http://framework.zend.com/)
  4
+ *
  5
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
  6
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
  7
+ * @license   http://framework.zend.com/license/new-bsd New BSD License
  8
+ * @package   Zend_Form
1
Abdul Malik Ikhsan
samsonasik added a note May 02, 2013

remove @package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Matthew Weier O'Phinney weierophinney closed this in 8cdc495 May 02, 2013
Matus Zeman

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

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.

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

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?

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

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.

Tomoaki Kosugi noopable referenced this pull request from a commit June 04, 2013
Commit has since been removed from the repository and is no longer available.
Tomoaki Kosugi noopable referenced this pull request from a commit in noopable/zf2 June 04, 2013
Tomoaki Kosugi Add test nested form element wrapping (relative: #4383) 50a6d12
Tomoaki Kosugi noopable referenced this pull request from a commit in noopable/zf2 June 04, 2013
Tomoaki Kosugi fix #4383 form element wrapping problem (relative: #4383) cc3236d
Deleted user Unknown referenced this pull request from a commit May 02, 2013
Matthew Weier O'Phinney Merge branch 'hotfix/4383'
Close #4383
0137827
Deleted user Unknown referenced this pull request from a commit May 02, 2013
Matthew Weier O'Phinney Merge branch 'hotfix/4383' into develop
Forward port #4383
7dcfe56
Deleted user Unknown referenced this pull request from a commit June 04, 2013
Tomoaki Kosugi fix #4383 form element wrapping problem (relative: #4383) e332ae5
Tomoaki Kosugi noopable deleted the branch December 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
23  library/Zend/Form/Form.php
@@ -188,6 +188,29 @@ public function prepare()
188 188
     }
189 189
 
190 190
     /**
  191
+     * Ensures state is ready for use. Here, we append the name of the fieldsets to every elements in order to avoid
  192
+     * name clashes if the same fieldset is used multiple times
  193
+     *
  194
+     * @param  FormInterface $form
  195
+     * @return mixed|void
  196
+     */
  197
+    public function prepareElement(FormInterface $form)
  198
+    {
  199
+        $name = $this->getName();
  200
+
  201
+        foreach ($this->byName as $elementOrFieldset) {
  202
+            if ($form->wrapElements()) {
  203
+                $elementOrFieldset->setName($name . '[' . $elementOrFieldset->getName() . ']');
  204
+            }
  205
+
  206
+            // Recursively prepare elements
  207
+            if ($elementOrFieldset instanceof ElementPrepareAwareInterface) {
  208
+                $elementOrFieldset->prepareElement($form);
  209
+            }
  210
+        }
  211
+    }
  212
+
  213
+    /**
191 214
      * Set data to validate and/or populate elements
192 215
      *
193 216
      * Typically, also passes data on to the composed input filter.
77  tests/ZendTest/Form/FormElementManagerFactoryTest.php
... ...
@@ -0,0 +1,77 @@
  1
+<?php
  2
+/**
  3
+ * Zend Framework (http://framework.zend.com/)
  4
+ *
  5
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
  6
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
  7
+ * @license   http://framework.zend.com/license/new-bsd New BSD License
  8
+ */
  9
+
  10
+namespace ZendTest\Form;
  11
+
  12
+use ArrayObject;
  13
+use PHPUnit_Framework_TestCase as TestCase;
  14
+use Zend\Mvc\Service\FormElementManagerFactory;
  15
+use Zend\Mvc\Service\DiFactory;
  16
+use Zend\Mvc\Service\DiAbstractServiceFactoryFactory;
  17
+use Zend\Mvc\Service\DiServiceInitializerFactory;
  18
+
  19
+use Zend\ServiceManager\Config;
  20
+use Zend\ServiceManager\ServiceManager;
  21
+use Zend\Mvc\Exception;
  22
+use Zend\Form\FormElementManager;
  23
+
  24
+class FormElementManagerFactoryTest extends TestCase
  25
+{
  26
+    /**
  27
+     * @var ServiceManager
  28
+     */
  29
+    protected $services;
  30
+
  31
+    /**
  32
+     * @var \Zend\Mvc\Controller\ControllerManager
  33
+     */
  34
+    protected $loader;
  35
+
  36
+    public function setUp()
  37
+    {
  38
+        $formElementManagerFactory = new FormElementManagerFactory();
  39
+        $config = new ArrayObject(array('di' => array()));
  40
+        $services = $this->services = new ServiceManager();
  41
+        $services->setService('Zend\ServiceManager\ServiceLocatorInterface', $services);
  42
+        $services->setFactory('FormElementManager', $formElementManagerFactory);
  43
+        $services->setService('Config', $config);
  44
+        $services->setFactory('Di', new DiFactory());
  45
+        $services->setFactory('DiAbstractServiceFactory', new DiAbstractServiceFactoryFactory());
  46
+        $services->setFactory('DiServiceInitializer', new DiServiceInitializerFactory());
  47
+
  48
+        $this->manager = $services->get('FormElementManager');
  49
+
  50
+        $this->standaloneManager = new FormElementManager();
  51
+    }
  52
+
  53
+    public function testWillInstantiateFormFromInvokable()
  54
+    {
  55
+        $form = $this->manager->get('form');
  56
+        $this->assertInstanceof('Zend\Form\Form', $form);
  57
+    }
  58
+
  59
+    public function testWillInstantiateFormFromDiAbstractFactory()
  60
+    {
  61
+        //without DiAbstractFactory
  62
+        $this->assertFalse($this->standaloneManager->has('ZendTest\Form\TestAsset\CustomForm'));
  63
+        //with DiAbstractFactory
  64
+        $this->assertTrue($this->manager->has('ZendTest\Form\TestAsset\CustomForm'));
  65
+        $form = $this->manager->get('ZendTest\Form\TestAsset\CustomForm');
  66
+        $this->assertInstanceof('ZendTest\Form\TestAsset\CustomForm', $form);
  67
+    }
  68
+
  69
+    public function testNoWrapFieldName()
  70
+    {
  71
+        $form = $this->manager->get('ZendTest\Form\TestAsset\CustomForm');
  72
+        $this->assertFalse($form->wrapElements(), 'ensure wrapElements option');
  73
+        $this->assertTrue($form->has('email'), 'ensure the form has email element');
  74
+        $emailElement = $form->get('email');
  75
+        $this->assertEquals('email', $emailElement->getName());
  76
+    }
  77
+}
62  tests/ZendTest/Form/TestAsset/CustomForm.php
... ...
@@ -0,0 +1,62 @@
  1
+<?php
  2
+/**
  3
+ * Zend Framework (http://framework.zend.com/)
  4
+ *
  5
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
  6
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
  7
+ * @license   http://framework.zend.com/license/new-bsd New BSD License
  8
+ */
  9
+
  10
+namespace ZendTest\Form\TestAsset;
  11
+
  12
+use Zend\Form\Form;
  13
+use Zend\Form\Element;
  14
+use Zend\Validator;
  15
+use Zend\Stdlib\Hydrator\ClassMethods as ClassMethodsHydrator;
  16
+
  17
+class CustomForm extends Form
  18
+{
  19
+    public function __construct()
  20
+    {
  21
+        parent::__construct('test_form');
  22
+
  23
+        $this->setAttribute('method', 'post')
  24
+             ->setHydrator(new ClassMethodsHydrator());
  25
+
  26
+        $field1 = new Element('name', array('label' => 'Name'));
  27
+        $field1->setAttribute('type', 'text');
  28
+        $this->add($field1);
  29
+
  30
+        $field2 = new Element('email', array('label' => 'Email'));
  31
+        $field2->setAttribute('type', 'text');
  32
+        $this->add($field2);
  33
+
  34
+        $this->add(array(
  35
+            'name' => 'submit',
  36
+            'attributes' => array(
  37
+                'type' => 'submit'
  38
+            )
  39
+        ));
  40
+    }
  41
+
  42
+    public function getInputFilterSpecification()
  43
+    {
  44
+        return array(
  45
+            'name' => array(
  46
+                'required' => true,
  47
+                'filters'  => array(
  48
+                    array('name' => 'Zend\Filter\StringTrim'),
  49
+                ),
  50
+            ),
  51
+            'email' => array(
  52
+                'required' => true,
  53
+                'filters'  => array(
  54
+                    array('name' => 'Zend\Filter\StringTrim'),
  55
+                ),
  56
+                'validators' => array(
  57
+                    new Validator\EmailAddress(),
  58
+                ),
  59
+            ),
  60
+        );
  61
+    }
  62
+}
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.