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

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

Open
2 tasks done
eweso opened this issue May 17, 2018 · 20 comments
Open
2 tasks done

Comments

@eweso
Copy link

eweso commented May 17, 2018

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
Copy link
Author

eweso commented Jun 4, 2018

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
Copy link
Member

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
Copy link
Author

eweso commented Jun 8, 2018

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
Copy link
Author

eweso commented Jun 28, 2018

$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
Copy link
Member

@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
Copy link
Author

eweso commented Jun 29, 2018

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
Copy link
Member

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

public function getInputSpecification()

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
Copy link
Author

eweso commented Jul 8, 2018

@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
Copy link
Member

@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
Copy link

kynx commented Jul 9, 2018

@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
Copy link
Member

froschdesign commented Jul 10, 2018

@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
Copy link
Member

@kynx
Btw. could you take that over?

@eweso
Copy link
Author

eweso commented Jul 12, 2018

@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
Copy link
Member

@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
Copy link
Author

eweso commented Jul 18, 2018

@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
Copy link
Author

eweso commented Aug 16, 2018

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
Copy link
Member

@eweso
No problem. Thanks for the feedback!

@Erikvv
Copy link
Contributor

Erikvv commented Aug 16, 2018

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 zendframework/zend-inputfilter#13 Input and InputFilter both implement the same interface.

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

@eweso
Copy link
Author

eweso commented Aug 21, 2018

@Erikvv

I already pulled a bugfix for the required problem on ArrayInput:
zendframework/zend-inputfilter#167

There is no BC break on that.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-form; a new issue has been opened at laminas/laminas-form#5.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants