Skip to content
This repository

BaseInputFilter->add deasn't work (Form Validation breaks since 2.2) #4996

Closed
wants to merge 7 commits into from

7 participants

Marc Bennewitz Matthew Weier O'Phinney David Windell Michaël Gallego Stanislav Paweł Nowak Emmanuel Belair
Marc Bennewitz

I have a form with one element and set it as required in input filter but it doesn't work because the form adds default input specifications before I set my own and merging of both doesn't work as it leaves the defaut specifications:

MyForm:

class NewsletterLayoutForm extends \Zend\Form\Form
{
    public function __construct()
    {
        parent::__construct('newsletter_layout');
        $this->setInputFilter(new \Zend\InputFilter\InputFilter());
        $this->addNameElement();
    }

    protected function addNameElement()
    {
        $this->add(array(
            'name' => 'name',
            'attributes' => array(
                'type'  => 'text',
            ),
            'options' => array(
                'label' => 'Name',
            ),
        ));

        $this->getInputFilter()->add(array(
            'required' => true,
            'filters' => array(
                array('name' => 'StringTrim'),
            ),
            'validators' => array(
                array(
                    'name' => 'StringLength',
                    'options' => array(
                        'encoding' => 'UTF-8',
                        'min' => 1,
                        'max' => 255,
                    ),
                ),
            ),
        ), 'name');
    }
}

Failing Test for BaseInputFilterTest that shows BaseInputFilter->add() doesn't work as expected (and expected by Zend\Form):

    public function testAddingExistingInputWillMergeIntoExisting()
    {
        $filter = new InputFilter();

        $foo1    = new Input('foo');
        $foo1->setRequired(true);
        $filter->add($foo1);

        $foo2    = new Input('foo');
        $foo2->setRequired(false);
        $filter->add($foo2);

        $this->assertFalse($filter->get('foo')->isRequired());
    }
Matthew Weier O'Phinney
Owner

Pinging @bakura10 @macnibblet @davidwindell -- can I get some assistance on this one, please?

added some commits August 21, 2013
Matthew Weier O'Phinney [#4996] Correct order of merging
- Added test showing issue with merging
- Fixed merging logic to merge new input with old
720b71d
Matthew Weier O'Phinney [#4996] Additional test showing intended use case for input merging
- Provided by @Bakura10
d35efe8
Matthew Weier O'Phinney [#4996] Set fieldset as base fieldset
- to ensure input filter does not need to look for fieldset name
861abea
David Windell

@weierophinney looks like you've sorted this?

Matthew Weier O'Phinney
Owner

@davidwindell Not quite -- @bakura10 and I are working on it in #zftalk.dev right now -- there's some subtle issues with how the Form component expects merging to happen that we're trying to fix now.

David Windell

Keep me posted, I'll run against our apps test suite once merged to develop.

Matthew Weier O'Phinney
Owner

@davidwindell Test away -- solved the issue @bakura10 discovered, and still have the original issue as posed by @marc-mabe resolved. :)

Michaël Gallego

We found the reason and @weierophinney fixed that. @marc-mabe changed make sense, but it breaks an assumption I've made when I wrote the form. The specific order in input filter was here so taht when you have an element that had default settings (like required => true), you could override them in a getInputFilterSpecification.

But in fact the Form itself was wrong, as it was first checking getInputFilterSpecification AND THEN adding elements that had their defaults validation rules. Mwop changed the order, so @marc-mabe fixed could be applied.

The changes he made make sense ;-).

Matthew Weier O'Phinney weierophinney closed this in 24970da August 21, 2013
Stanislav

Some of my form elements became invalid after this update. Here is an example

use Zend\Form\Factory as FormFactory;

$spec = array(
    'name'         => 'test',
    'elements'     => array(
        array(
            'spec' => array(
                'name'    => 'element',
                'type'    => 'Zend\Form\Element\Checkbox',
                'options' => array(
                    'use_hidden_element' => true,
                    'checked_value'      => '1',
                    'unchecked_value'    => '0'
                )
            )
        )
    ),
    'input_filter' => array(
        'element' => array(
            'required'   => false,
            'filters'    => array(
                array(
                    'name' => 'Boolean'
                )
            ),
            'validators' => array(
                array(
                    'name'    => 'InArray',
                    'options' => array(
                        'haystack' => array(
                            "0",
                            "1"
                        )
                    )
                )
            ),
        )
    )
);

$factory = new FormFactory();
$form = $factory->createForm($spec);
$form->setData(array('element' => '0'));
var_dump($form->isValid()) . "\n";
var_dump($form->getMessages()) . "\n";

The output is:

bool(false) 
array(1) { 'element' => array(1) { 'isEmpty' => string(36) "Value is required and can't be empty" } } 
Stanislav

I realised that it's not much related with this update. The form is invalid because of the boolean filter. Is it a kind of bug then?
Before the update the form was valid because of 'required' => false parameter

Paweł Nowak

Hello there,

after the 2.2.3 update more complex input filters stopped merging as they used to. Here's a test case I've prepared to demonstrate what I'm talking about:

use Zend\InputFilter\BaseInputFilter as InputFilter;
use Zend\Form\Form;
use Zend\InputFilter\Factory as InputFactory;

public function testComplexFormInputFilterMergesIntoExisting()
{
    $form = new Form();
    $form->add(array(
        'name' => 'importance',
        'type'  => 'Zend\Form\Element\Select',
        'options' => array(
            'label' => 'Importance',
            'empty_option' => '',
            'value_options' => array(
                'normal' => 'Normal',
                'important' => 'Important'
            ),
        ),
    ));

    $inputFilter = new InputFilter();
    $factory     = new InputFactory();
    $inputFilter->add($factory->createInput(array(
        'name'     => 'importance',
        'required' => false,
    )));

    $this->assertTrue($form->getInputFilter()->get('importance')->isRequired());
    $this->assertFalse($inputFilter->get('importance')->isRequired());
    $form->setInputFilter($inputFilter);
    $this->assertFalse($form->getInputFilter()->get('importance')->isRequired());
}

It would pass till the version of 2.2.2. Unfortunately I'm not quite sure how to fix the issue, reverting this change seems to get it to work: 24970da#L1R72 it causes the testAddingExistingInputWillMergeIntoExisting test to fail, though.

Emmanuel Belair

+1

Michael Gooden MichaelGooden referenced this pull request from a commit in MichaelGooden/zf2 August 23, 2013
Michael Gooden Add test for #4996 regression.
Signed-off-by: Michael Gooden <michael@bluepointweb.com>
2c52613
Matthew Weier O'Phinney weierophinney referenced this pull request from a commit in weierophinney/zf2 August 26, 2013
Matthew Weier O'Phinney Enable prefer form input filter flag by default
- This fixes a regression in 2.2.3 via #4996, whereby the fix to a
  regression caused a new regression in behavior, in which the form
  input filters were no longer being merged in the expected order.
  Essentially, the fix in #4996 now enforced the `preferFormInputFilter`
  flag status, which was de facto enabled due to the hacks previously
  present. This flag is now enabled by default to provide the behavior
  by default (which is what was occurring before the changes, albeit via
  a different mechanism).
ce07861
Filippo Tessarotto Slamdunk referenced this pull request from a commit in Slamdunk/zf2 August 30, 2013
Filippo Tessarotto #4996 broke File filters management 531f4eb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 7 unique commits by 1 author.

Aug 21, 2013
Matthew Weier O'Phinney [#4996] Correct order of merging
- Added test showing issue with merging
- Fixed merging logic to merge new input with old
720b71d
Matthew Weier O'Phinney [#4996] Additional test showing intended use case for input merging
- Provided by @Bakura10
d35efe8
Matthew Weier O'Phinney [#4996] Set fieldset as base fieldset
- to ensure input filter does not need to look for fieldset name
861abea
Matthew Weier O'Phinney [#4996] Fix input retrieval
- It's nested.
5883056
Matthew Weier O'Phinney Merge branch 'hotfix/4996-alt-test' into hotfix/4996
Merge in tests from @Bakura10
4b19803
Matthew Weier O'Phinney [#4996] Ensure input filter specification is merged last
- to preserver order of operations
7db3607
Matthew Weier O'Phinney [#4996] CS fixes
- trailing whitespace
65b5c6a
This page is out of date. Refresh to see the latest.
14  library/Zend/Form/Form.php
@@ -717,13 +717,6 @@ public function attachInputFilterDefaults(InputFilterInterface $inputFilter, Fie
717 717
         $formFactory  = $this->getFormFactory();
718 718
         $inputFactory = $formFactory->getInputFilterFactory();
719 719
 
720  
-        if ($fieldset === $this && $fieldset instanceof InputFilterProviderInterface) {
721  
-            foreach ($fieldset->getInputFilterSpecification() as $name => $spec) {
722  
-                $input = $inputFactory->createInput($spec);
723  
-                $inputFilter->add($input, $name);
724  
-            }
725  
-        }
726  
-
727 720
         if ($fieldset instanceof Collection && $fieldset->getTargetElement() instanceof FieldsetInterface) {
728 721
             $elements = $fieldset->getTargetElement()->getElements();
729 722
         } else {
@@ -752,6 +745,13 @@ public function attachInputFilterDefaults(InputFilterInterface $inputFilter, Fie
752 745
                 $input = $inputFactory->createInput($spec);
753 746
                 $inputFilter->add($input, $name);
754 747
             }
  748
+
  749
+            if ($fieldset instanceof InputFilterProviderInterface) {
  750
+                foreach ($fieldset->getInputFilterSpecification() as $name => $spec) {
  751
+                    $input = $inputFactory->createInput($spec);
  752
+                    $inputFilter->add($input, $name);
  753
+                }
  754
+            }
755 755
         }
756 756
 
757 757
         foreach ($fieldset->getFieldsets() as $childFieldset) {
7  library/Zend/InputFilter/BaseInputFilter.php
@@ -73,9 +73,12 @@ public function add($input, $name = null)
73 73
         }
74 74
 
75 75
         if (isset($this->inputs[$name]) && $this->inputs[$name] instanceof InputInterface) {
76  
-            // The element already exists, so merge the config. Please note that the order is important (already existing
  76
+            // The element already exists, so merge the config. Please note
  77
+            // that the order is important (already existing
77 78
             // input is merged with the parameter given)
78  
-            $input->merge($this->inputs[$name]);
  79
+            $original = $this->inputs[$name];
  80
+            $original->merge($input);
  81
+            return $this;
79 82
         }
80 83
 
81 84
         $this->inputs[$name] = $input;
14  tests/ZendTest/Form/FormTest.php
@@ -1542,4 +1542,18 @@ public function testNestedFormElementNameWrapping()
1542 1542
         $rootForm->prepare();
1543 1543
         $this->assertEquals('form[element]', $element->getName());
1544 1544
     }
  1545
+
  1546
+    /**
  1547
+     * @group 4996
  1548
+     */
  1549
+    public function testCanOverrideDefaultInputSettings()
  1550
+    {
  1551
+        $myFieldset = new TestAsset\MyFieldset();
  1552
+        $myFieldset->setUseAsBaseFieldset(true);
  1553
+        $form = new Form();
  1554
+        $form->add($myFieldset);
  1555
+
  1556
+        $inputFilter = $form->getInputFilter()->get('my-fieldset');
  1557
+        $this->assertFalse($inputFilter->get('email')->isRequired());
  1558
+    }
1545 1559
 }
34  tests/ZendTest/Form/TestAsset/MyFieldset.php
... ...
@@ -0,0 +1,34 @@
  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\InputFilter\InputFilterProviderInterface;
  13
+use Zend\Form\Fieldset;
  14
+
  15
+class MyFieldset extends Fieldset implements InputFilterProviderInterface
  16
+{
  17
+    public function __construct()
  18
+    {
  19
+        parent::__construct('my-fieldset');
  20
+        $this->add(array(
  21
+            'type' => 'Email',
  22
+            'name' => 'email',
  23
+        ));
  24
+    }
  25
+
  26
+    public function getInputFilterSpecification()
  27
+    {
  28
+        return array(
  29
+            'email' => array(
  30
+                'required' => false,
  31
+            ),
  32
+        );
  33
+    }
  34
+}
18  tests/ZendTest/InputFilter/BaseInputFilterTest.php
@@ -765,4 +765,22 @@ public function testGetInputs()
765 765
         $this->assertEquals('foo', $filters['foo']->getName());
766 766
         $this->assertEquals('bar', $filters['bar']->getName());
767 767
     }
  768
+
  769
+    /**
  770
+     * @group 4996
  771
+     */
  772
+    public function testAddingExistingInputWillMergeIntoExisting()
  773
+    {
  774
+        $filter = new InputFilter();
  775
+
  776
+        $foo1    = new Input('foo');
  777
+        $foo1->setRequired(true);
  778
+        $filter->add($foo1);
  779
+
  780
+        $foo2    = new Input('foo');
  781
+        $foo2->setRequired(false);
  782
+        $filter->add($foo2);
  783
+
  784
+        $this->assertFalse($filter->get('foo')->isRequired());
  785
+    }
768 786
 }
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.