zendframework / zend-form

Form component from Zend Framework
BSD 3-Clause "New" or "Revised" License
69 stars 87 forks source link

Default InputerFilter in Form creates Input, instead of ArrayInput for multi select elements #204

Open eweso opened 6 years ago

eweso commented 6 years ago

The default InputFilter of Forms creates always an Input for Elements within the Form. Even, if the Select Element has the multiple attribute. The problem is, when you create an InputFilter and set it to the Form, then the default InputFilter won't be replaced, it will merge Validators and Filters with the new ones. So you're given ArrayInput will be an Input in the form and the validators will fail, because they receive an array as value, but expect a string|number.

Code to reproduce the issue

Just create a Form with a multi select.

Expected results

The default Input for Select with multiple attribute should be ArrayInput.

Actual results

The default Input for Select is always Input.

eweso commented 6 years ago

Still no feedback? At the moment I do not have time to find the origin of the bug and provide a patch. But it is actually a critical bug and should be fixed. I am wondering, why no one reported it until now.

weierophinney commented 6 years ago

Still no feedback?

Please review the code of conduct; the majority of developers on this project are volunteers, and there are close to 200 repositories we maintain. We simply cannot jump on every issue or pull request as it comes in. Additionally, pull requests, even those that simply provide a failing test, get priority over issues generally, as they provide us with more concrete information on how the reporter is attempting to execute the code, and what their expectations are.

Just create a Form with a multi select.

Please demonstrate how you are doing this, so that we can verify that we are creating the scenario in the exact same manner you are; without that information, we may be unable to reproduce the issue. (E.g., the options you are passing to the multiselect may be invalid, or if you are using configuration, there might be a subtle typo leading to the scenario.)

eweso commented 6 years ago

I see. Thank you for your response! I will provide the relevant code within the next two weeks. I am a little busy right now.

eweso commented 6 years ago
$form = new \Zend\Form\Form;
$form->add([
    'name' => 'multiselect',
    'type' => 'select',
    'options' => [
        'value_options' => ['A', 'B', 'C']
    ],
    'attributes' => [
        'multiple' => true
    ]
]);

$multiSelectInput = get_class($form->getInputFilter()->get('multiselect'));
echo $multiSelectInput; // returns Zend\InputFilter\Input, should be Zend\InputFilter\ArrayInput

Here we go. I hope, this helps. Version of Zend-Form is "2.5.3".

froschdesign commented 6 years ago

@eweso Please check this unit test and then update your value options:

public function testValidationWithOptionMultiple()
{
    // Element
    $element = new SelectElement('foobar');
    $element->setValueOptions(
        [
            'A' => 'A',
            'B' => 'B',
            'C' => 'C',
        ]
    );
    $element->setAttribute('multiple', true);
    $element->setUseHiddenElement(true);

    // Input filter
    $inputFilter = new InputFilter();
    $inputFilter->add($element->getInputSpecification());

    // Tests
    $inputFilter->setData([
        'foobar' => [
            'A',
            'B',
        ],
    ]);
    $this->assertTrue($inputFilter->isValid());

    $inputFilter->setData([
        'foobar' => 'A',
    ]);
    $this->assertTrue($inputFilter->isValid());

    $inputFilter->setData([
        'foobar' => 'D',
    ]);
    $this->assertFalse($inputFilter->isValid());
}

The Zend\InputFilter\ArrayInput is not needed, the validator Explode is used.

eweso commented 6 years ago

If you do not add a filter or validator, you don't need an ArrayInput, this is true. But if you create a dynamic multi-select and you want to validate and filter all of it's entries, you need to have an ArrayInput, to iterate through all items of the select. If not, all other validators, like EmailValidator will return false, because EmailValidator expects a string, not an array.

froschdesign commented 6 years ago

@eweso

If you do not add a filter or validator, you don't need an ArrayInput, this is true.

Filters and validators are already there: https://github.com/zendframework/zend-form/blob/4419eef6dbe9d276e7e27c6a25f022be74534959/src/Element/Select.php#L288

But if you create a dynamic multi-select and you want to validate and filter all of it's entries, you need to have an ArrayInput…

If you set the the value_options before you validate your form, then everything works. No extra filters or validators needed.


I'm sorry, but you're only describing your conclusion and not the actual problem. Please provide an unit test to illustrate the problem.

eweso commented 6 years ago

@froschdesign

If you set the the value_options before you validate your form, then everything works. No extra filters or validators needed.

This is not what I was talking about. I want another validator! I don't want to test, if the value is in the given set of value_options, I want to validate each value, because the client is allowed to manipulate the select values dynamically and the values which are set, are email addresses and those addresses need to be validated, before I work with them.

I'm sorry, but you're only describing your conclusion and not the actual problem. Please provide an unit test to illustrate the problem.

Here we go. If you add another validator, like EmailAddress, than the validation process fails. Because the Input is not an ArrayInput, the Validator validates the array as the EmailAddress-Validator value.

    public function testValidationWithOptionMultiple()
    {
        // Element
        $element = new SelectElement('foobar');

        // Imagine that these values are set after post and on form creation but before form validation
        $element->setValueOptions(
            [
                'info@'           => 'info@',
                'info@test.de'    => 'info@test.de',
                'info@example.de' => 'info@example.de',
            ]
        );
        $element->setAttribute('multiple', true);
        $element->setUseHiddenElement(true);

        // Input filter
        $inputFilter = new InputFilter();
        $inputFilter->add($element->getInputSpecification());

        // Add Email Validator
        $inputFilter->get('foobar')->getValidatorChain()->attach(new EmailAddressValidator);

        // Tests
        $inputFilter->setData([
            'foobar' => [
                'info@test.de',
                'info@example.de',
            ],
        ]);
        $this->assertTrue($inputFilter->isValid());

        $inputFilter->setData([
            'foobar' => 'info@',
        ]);
        $this->assertFalse($inputFilter->isValid());

        $inputFilter->setData([
            'foobar' => 'info@not.de',
        ]);
        $this->assertFalse($inputFilter->isValid());
    }
froschdesign commented 6 years ago

@eweso

This is not what I was talking about. I want another validator!

And why is this info and code example missing in your first post?

Your usage is wrong. You must set the EmailAddress validator to the Explode validator - not to the validator chain!

See: https://docs.zendframework.com/zend-validator/validators/explode/#supported-options


The last line of your code example should be included assertTrue.

kynx commented 6 years ago

@froschdesign allah knows why I’ve been following this one, but I have. None of the solution you have finally given is obvious from the documentation.

GitHub is not the place for support. But you have, finally, given it. Why didn’t you suggest a PR on the docs?

froschdesign commented 6 years ago

@kynx Wait wait. This is no support, it had to be clarified if this issue report a bug or something else. Therefore, no label was set until now.

But you are right, the conclusion of this issue report is to extend the documentation for the Select element, because the hint to the Explode validator is missing. And we should add some more code examples.

https://docs.zendframework.com/zend-form/element/select/

froschdesign commented 6 years ago

@kynx Btw. could you take that over?

eweso commented 6 years ago

@froschdesign

But why don't you use just the ArrayInput on multi select? I mean, what is the usecase for the ArrayInput, if we do not use it on array values? Why does the zf2 use the Explode-Validator as a ValidatorChain replacement, to validate array values? I do not get it, actually.

I mean, first of all thank you for your hint to use the ExplodeValidator instead, but do you really think, that this is a good and clean solution?

I always attach a configured InputFilter by the setInputFilter() method to Forms and because the Form creates a default InputFilter and merge the two InputFilters, you cannot change the type of the Input element. It will just fetch all Validators and Filters of the InputFilter I set and attach it to the given one.

My solution to prevent this error was to use method setUseInputFilterDefaults(false) in the Form. This will prevent the form to create an InputFilter and therefore my InputFilter will not be merged, it will be used as it is.

froschdesign commented 6 years ago

@eweso Long talk short sense: provide a pull request with unit tests for your improvement. But please keep in mind, if your changes ends in a BC break or in a different behaviour, then we have to wait for the next major version. The goal for this issue report is to extend the documentation. The pull request for the usage of ArrayInput will be treated separately. (ping me for review)

Thank you in advance!

eweso commented 6 years ago

@froschdesign

I don't think, that the behaviour will change. Therefore I will create a patch and pull request. But don't expect it in July. Beginning of August sounds realistic.

eweso commented 6 years ago

Update: I will start in September at the earliest. At the moment I have to much to do at work. I am sorry for that!

froschdesign commented 6 years ago

@eweso No problem. Thanks for the feedback!

Erikvv commented 6 years ago

ArrayInput does not support required so you'll run into that @eweso

I agree the current solution is not optimal but it's a decent stop-gap solution. But better focus on improving inputfilters then we can fix this later.

If you look at the suggested improvements in https://github.com/zendframework/zend-inputfilter/issues/13 Input and InputFilter both implement the same interface.

As a consequence you can then merge ArrayInput and CollectionInputFilter into a single class.

eweso commented 6 years ago

@Erikvv

I already pulled a bugfix for the required problem on ArrayInput: https://github.com/zendframework/zend-inputfilter/pull/167

There is no BC break on that.

weierophinney commented 4 years ago

This repository has been closed and moved to laminas/laminas-form; a new issue has been opened at https://github.com/laminas/laminas-form/issues/5.