Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

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

Closed
wants to merge 7 commits into from

Conversation

weierophinney
Copy link
Member

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());
    }

@weierophinney
Copy link
Member

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

- Added test showing issue with merging
- Fixed merging logic to merge new input with old
@ghost ghost assigned weierophinney Aug 21, 2013
- to ensure input filter does not need to look for fieldset name
@davidwindell
Copy link
Contributor

@weierophinney looks like you've sorted this?

@weierophinney
Copy link
Member

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

@davidwindell
Copy link
Contributor

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

@weierophinney
Copy link
Member

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

@bakura10
Copy link
Contributor

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 ;-).

weierophinney added a commit that referenced this pull request Aug 21, 2013
@davidwindell
Copy link
Contributor

👍

@thestanislav
Copy link
Contributor

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" } } 

@thestanislav
Copy link
Contributor

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

@pawelnowak
Copy link

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.

@e-belair
Copy link
Contributor

+1

MichaelGooden added a commit to MichaelGooden/zf2 that referenced this pull request Aug 23, 2013
weierophinney added a commit that referenced this pull request Aug 23, 2013
weierophinney added a commit to weierophinney/zendframework that referenced this pull request Aug 26, 2013
- This fixes a regression in 2.2.3 via zendframework#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 zendframework#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).
Slamdunk added a commit to Slamdunk/zf2 that referenced this pull request Aug 30, 2013
weierophinney added a commit that referenced this pull request Sep 3, 2013
weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
- Added test showing issue with merging
- Fixed merging logic to merge new input with old
weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants