zendframework / zend-inputfilter

InputFilter component from Zend Framework
BSD 3-Clause "New" or "Revised" License
64 stars 50 forks source link

Value is required message for optional field #139

Closed autowp closed 6 years ago

autowp commented 7 years ago
// somewhere in config
    'input_filter_specs' => [
        'test_filter' => [
            'value' => [
                'required' => false
            ],
        ],

// usage
$filters = $container->get('InputFilterManager');
$filter = $filters->get('test_filter');
$filter->setData([
    'value' => false
]);
$filter->isValid(); // false
$filter->getMessages(); // 'Value is required and can't be empty' O_O

One of possible fix is https://github.com/zendframework/zend-inputfilter/pull/138

weierophinney commented 6 years ago

Why are you passing an array of data to setData()? That should be $filter->setData(false), shouldn't it?

froschdesign commented 6 years ago

@weierophinney This code example is correct, but the unit test in #139 is wrong.

@autowp No need for the manager. Much simpler: 😉

$inputFilter = new Zend\InputFilter\InputFilter();
$inputFilter->add(
    [
        'name'     => 'foo',
        'required' => false,
    ]
);
$inputFilter->setData(
    [
        'foo' => false,
    ]
);
var_dump($inputFilter->isValid());
var_dump($inputFilter->getMessages());

Output:


boolean false

array (size=1)
  'foo' => 
    array (size=1)
      'isEmpty' => string 'Value is required and can't be empty' (length=36)
weierophinney commented 6 years ago

I figured out the issue with the test. ArrayInputTest extends from InputTest, which results in an exception being raised if the value is not within an array. The appropriate solution is to override that test case within ArrayInputTest in order to pass an array with the false value.

What's interesting is that if I merge both #138 and #157, and adapt the tests as described above (simple false value in the InputTest, overriding the test within ArrayInputTest to pass an array), the test from #157 passes, but the ArrayInputTest variant fails with the same message.

This is due to the fact that the fix for #138 does not also patch ArrayInput line 92, which does the equivalent $empty definition. However, if I make that change, I now get 158 errors, all against ArrayInputTest.

From what I can see, this is due to the various allow_empty and continue_if_empty flag defaults in that input, and how they interact to determine if a false value is valid.

I'll see if I can find a solution, but it's looking pretty complex, which leads me back to my initial inclination that we may not want to support this.

froschdesign commented 6 years ago

This can be used:

$inputFilter = new Zend\InputFilter\InputFilter();
$inputFilter->add(
    [
        'name'              => 'foo',
        'required'          => false,
        'allow_empty'       => true, // important!
        'continue_if_empty' => true, // important!
    ]
);
$inputFilter->setData(
    [
        'foo' => false,
    ]
);
var_dump($inputFilter->isValid()); // true
var_dump($inputFilter->getMessages()); // empty

If an empty string is not allowed, but the value false (like above):

$inputFilter->get('foo')->getValidatorChain()->prependValidator(
    new Zend\Validator\NotEmpty(
        [
            'type' => [
                Zend\Validator\NotEmpty::STRING,
                Zend\Validator\NotEmpty::SPACE,
                Zend\Validator\NotEmpty::NULL,
                Zend\Validator\NotEmpty::EMPTY_ARRAY,
            ],
        ]
    )
);
$inputFilter->setData(
    [
        'foo' => '',
    ]
);
var_dump($inputFilter->isValid()); // false
var_dump($inputFilter->getMessages()); // 'isEmpty' => string 'Value is required and can't be empty'
weierophinney commented 6 years ago

@autowp See the explanation from @froschdesign above; this is already possible, but requires a little configuration on your part: the input must allow_empty to allow a false value to validate.

froschdesign commented 6 years ago

@weierophinney …and continue_if_empty!

autowp commented 6 years ago

But both allow_empty and continue_if_empty deprecated since 2.4.8

weierophinney commented 6 years ago

@autowp Deprecated, but not yet removed, and we do not have a replacement lined up. We'll be keeping them for the foreseeable future, and will have alternatives in place and documented when we're ready to remove them for a future v3 revision (if we end up doing one at all).