NotEmpty validator doesn't override the required attribute #14

larsnystrom · 2015-08-04T18:59:52Z

A NotEmpty validator should override the required check on an input, but in some cases it doesn't. I'm sorry I haven't been able to pin point the exact location of the bug, but here's the code to reproduce it. In test case 1 both NotEmpty validators overrides the required attribute, which can be seen by the error messages. In test case 2 however, field2 doesn't use it's NotEmpty validator but instead falls back to it's internal check to see if the value is empty.

<?php

require __DIR__ . '/vendor/autoload.php';

class MyFilter extends Zend\InputFilter\InputFilter
{
    public function init()
    {
        $this->add([
            'name' => 'field1',
            'required' => true,
            'validators' => [
                [
                    'name' => 'Zend\Validator\NotEmpty',
                    'break_chain_on_failure' => true,
                    'options' => [
                        'messages' => [
                            'isEmpty' => "Field1 cannot be empty",
                        ],
                    ],
                ],
            ],
        ]);

        $this->add([
            'name' => 'field2',
            'required' => true,
            'validators' => [
                [
                    'name' => 'Zend\Validator\NotEmpty',
                    'break_chain_on_failure' => true,
                    'options' => [
                        'messages' => [
                            'isEmpty' => "Field2 cannot be empty",
                        ],
                    ],
                ],
            ],
        ]);
    }
}

$app = Zend\Mvc\Application::init([
    'modules' => [
    ],

    'module_listener_options' => [
        'module_paths' => [
        ],

        'config_glob_paths' => [
        ],
    ],

    'input_filters' => [
        'invokables' => [
            'MyFilter' => 'MyFilter',
        ],
    ],
]);

$manager = $app->getServiceManager()->get('InputFilterManager');
$filter = $manager->get('MyFilter');

echo "\n";
echo "#------------------------------\n";
echo "# Test case 1\n";
echo "#------------------------------\n";
echo "\n";
$filter->setData([
]);

if (!$filter->isValid()) {
    var_dump($filter->getMessages());
}

echo "\n";
echo "#------------------------------\n";
echo "# Test case 2\n";
echo "#------------------------------\n";
echo "\n";
$filter->setData([
    'field1' => [
        'test',
    ],
]);

if (!$filter->isValid()) {
    var_dump($filter->getMessages());
}

The output I get is:

#------------------------------
# Test case 1
#------------------------------

array(2) {
  'field1' =>
  array(1) {
    'isEmpty' =>
    string(22) "Field1 cannot be empty"
  }
  'field2' =>
  array(1) {
    'isEmpty' =>
    string(22) "Field2 cannot be empty"
  }
}

#------------------------------
# Test case 2
#------------------------------

array(1) {
  'field2' =>
  array(1) {
    [0] =>
    string(17) "Value is required"
  }
}

The text was updated successfully, but these errors were encountered:

tflin · 2015-08-06T03:13:45Z

We had the same issue that 'Value is required' message can not be localized to Chinese as it is hardcoded in the framework code. The custom-message NotEmpty validator cannot override this error message like this case. Please help to make the hardcoded message localizable.

Maks3w · 2015-08-21T03:52:12Z

@tflin please open a new issue

Maks3w · 2015-08-21T03:59:11Z

@larsnystrom I don't understand what do you refer about override required attribute. I can say the second commit of #25 will make the messages consistent using the message of test 2

svycka · 2015-08-21T08:11:33Z

@Maks3w I think he speaks about this

zend-inputfilter/src/BaseInputFilter.php

Line 270 in bded51b

$input->setErrorMessage('Value is required');

and they speak about the same problem so no need for new issue.

larsnystrom · 2015-08-24T07:47:37Z

I've created a repo to make it easier to reproduce the bug.

git clone https://github.com/larsnystrom/zend-inputfilter-ticket14
composer install
php test.php

I haven't tested PR #25, I'll do that if I ever get the time.

weierophinney · 2015-08-26T20:54:04Z

@larsnystrom We've never had setting a NotEmpty validator toggle the required flag; the only correlation between the two is that in past versions, setting the required flag to true would implicitly add a NotEmpty validator. We've changed that logic recently, however (we now check for absence of a value when the required flag is true, and invalidate earlier when that condition arises). Is it possible that's the issue you're seeing?

larsnystrom · 2015-08-28T08:08:20Z

@weierophinney If you look at the MyFilter::init() method in the test code above, you'll see that I've set required to true and also added a NotEmpty validator to both input specifications in the InputFilter. Then, if a field is empty, the NotEmpty validator should run and fail, right? As you yourself wrote in #11

The solution to #11 is to keep the input as required, but to inject your own NotEmpty validator at the top of the chain yourself; Input will detect that it's present, and not inject its own NotEmpty validator.

So, the solution is:

Keep the input required

Inject your own NotEmpty validator with the custom message as the first validator in the chain.

That is exactly what I've done.

Now, if you look at test case 1, you'll see that this works when both fields are empty! Fantastic.

Moving on to test case 2, where only one of the fields are empty. I'd expect the corresponding NotEmpty validator to run and fail, right? That's what happened in the first test case. But (here comes the bug) the NotEmpty validator doesn't run and instead the required attribute and its associated logic takes precedence.

The behaviour is different when (1) all fields are empty and (2) only one field is empty. That's the bug.

Maks3w · 2015-08-28T09:01:31Z

Fields on test 1 are not empty. Are not set. Value is unknown so can't be determined if value is empty or not.

Maks3w · 2015-08-28T09:03:00Z

Please test against current master

larsnystrom · 2015-09-01T20:39:57Z

The current master solves the inconsistence, but makes it completely impossible to customize the "Value is required" message when there isn't a value. But that's discussed in other issues (#11 and #28), so I'll close this issue now.

Another issue is that the fallback value isn't validated. Personally I'd prefer if it was validated/filtered just like any other value. But that's outside the scope of this issue.

Includes these changes: - [14: NotEmpty validator doesn't override the required attribute](zendframework#14) - [16: BC in validators context value](zendframework#16) - [22: Missing required field with fallback does not return input in getValidInput](zendframework#22) - [24: Fix loop validation input context is mutable](zendframework#24) - [25: Fix missing optional fields should not be validated](zendframework#25) - [32: Promote HHVM](zendframework#32) - [36: Fix docblocks declared as regular comment block](zendframework#36) - [40: Remove test about Input setters permutation](zendframework#40) - [41: Refactor tests for group 7448 as a data set matrix](zendframework#41) - [42: Consolidate InputFilterPluginManager tests](zendframework#42) - [43: Consolidate Factory tests + Fixes](zendframework#43) - [44: Consolidate of InputInterface::merge and Fix unsafe merge](zendframework#44) - [45: When merge Inputs don't merge values if source does not have one.](zendframework#45) - [46: Expand test matrix with nonempty value scenarios](zendframework#46) - [47: Feature/minor test improvements](zendframework#47) - [48: Empty values + Allow Empty + Not continue if empty is always true](zendframework#48) - [49: Add tests for exceptions and improve some messages](zendframework#49) - [50: Optional fields without value should be valid ](zendframework#50) - [51: Optional input without value are valid](zendframework#51) - [52: Fix merge two inputs without value, result 'has value' flag should be false](zendframework#52) - [53: Tune NonEmpty options for detect the expected empty values](zendframework#53) - [55: Make symmetric the scenarios when required is true/false](zendframework#55) - [56: Hotfix/minor changes](zendframework#56) - [57: Consolidate tests for InputFilterInterface](zendframework#57) - [61: Provide NotEmpty::IS&zendframework#95;EMPTY validation message for required input](zendframework#61) - [63: Ensure custom error messages are used for required missing inputs](zendframework#63) - [64: Feature/test cleanup](zendframework#64)

Includes these changes: - [14: NotEmpty validator doesn't override the required attribute](zendframework#14) - [16: BC in validators context value](zendframework#16) - [24: Fix loop validation input context is mutable](zendframework#24) - [25: Fix missing optional fields should not be validated](zendframework#25) - [26: Deprecate magic logic for auto attaching a NonEmpty validator with breakChainOnFailure = true](zendframework#26) - [31: Update the InputFilterInterface::add docblock to match existing, shipped implementations](zendframework#31) - [32: Promote HHVM](zendframework#32) - [36: Fix docblocks declared as regular comment block](zendframework#36) - [40: Remove test about Input setters permutation](zendframework#40) - [41: Refactor tests for group 7448 as a data set matrix](zendframework#41) - [42: Consolidate InputFilterPluginManager tests](zendframework#42) - [43: Consolidate Factory tests + Fixes](zendframework#43) - [44: Consolidate of InputInterface::merge and Fix unsafe merge](zendframework#44) - [45: When merge Inputs don't merge values if source does not have one.](zendframework#45) - [46: Expand test matrix with nonempty value scenarios](zendframework#46) - [47: Feature/minor test improvements](zendframework#47) - [48: Empty values + Allow Empty + Not continue if empty is always true](zendframework#48) - [49: Add tests for exceptions and improve some messages](zendframework#49) - [50: Optional fields without value should be valid ](zendframework#50) - [51: Optional input without value are valid](zendframework#51) - [52: Fix merge two inputs without value, result 'has value' flag should be false](zendframework#52) - [56: Hotfix/minor changes](zendframework#56) - [57: Consolidate tests for InputFilterInterface](zendframework#57) - [61: Provide NotEmpty::IS&zendframework#95;EMPTY validation message for required input](zendframework#61) - [63: Ensure custom error messages are used for required missing inputs](zendframework#63) - [64: Feature/test cleanup](zendframework#64)

Includes these changes: - [14: NotEmpty validator doesn't override the required attribute](zendframework#14) - [16: BC in validators context value](zendframework#16) - [24: Fix loop validation input context is mutable](zendframework#24) - [25: Fix missing optional fields should not be validated](zendframework#25) - [26: Deprecate magic logic for auto attaching a NonEmpty validator with breakChainOnFailure = true](zendframework#26) - [31: Update the InputFilterInterface::add docblock to match existing, shipped implementations](zendframework#31) - [36: Fix docblocks declared as regular comment block](zendframework#36) - [40: Remove test about Input setters permutation](zendframework#40) - [41: Refactor tests for group 7448 as a data set matrix](zendframework#41) - [42: Consolidate InputFilterPluginManager tests](zendframework#42) - [43: Consolidate Factory tests + Fixes](zendframework#43) - [44: Consolidate of InputInterface::merge and Fix unsafe merge](zendframework#44) - [45: When merge Inputs don't merge values if source does not have one.](zendframework#45) - [46: Expand test matrix with nonempty value scenarios](zendframework#46) - [47: Feature/minor test improvements](zendframework#47) - [48: Empty values + Allow Empty + Not continue if empty is always true](zendframework#48) - [49: Add tests for exceptions and improve some messages](zendframework#49) - [50: Optional fields without value should be valid ](zendframework#50) - [51: Optional input without value are valid](zendframework#51) - [52: Fix merge two inputs without value, result 'has value' flag should be false](zendframework#52) - [56: Hotfix/minor changes](zendframework#56) - [57: Consolidate tests for InputFilterInterface](zendframework#57) - [61: Provide NotEmpty::IS&zendframework#95;EMPTY validation message for required input](zendframework#61) - [63: Ensure custom error messages are used for required missing inputs](zendframework#63) - [64: Feature/test cleanup](zendframework#64)

Includes these changes: - [14: NotEmpty validator doesn't override the required attribute](zendframework/zend-inputfilter#14) - [16: BC in validators context value](zendframework/zend-inputfilter#16) - [24: Fix loop validation input context is mutable](zendframework/zend-inputfilter#24) - [25: Fix missing optional fields should not be validated](zendframework/zend-inputfilter#25) - [26: Deprecate magic logic for auto attaching a NonEmpty validator with breakChainOnFailure = true](zendframework/zend-inputfilter#26) - [31: Update the InputFilterInterface::add docblock to match existing, shipped implementations](zendframework/zend-inputfilter#31) - [36: Fix docblocks declared as regular comment block](zendframework/zend-inputfilter#36) - [40: Remove test about Input setters permutation](zendframework/zend-inputfilter#40) - [41: Refactor tests for group 7448 as a data set matrix](zendframework/zend-inputfilter#41) - [42: Consolidate InputFilterPluginManager tests](zendframework/zend-inputfilter#42) - [43: Consolidate Factory tests + Fixes](zendframework/zend-inputfilter#43) - [44: Consolidate of InputInterface::merge and Fix unsafe merge](zendframework/zend-inputfilter#44) - [45: When merge Inputs don't merge values if source does not have one.](zendframework/zend-inputfilter#45) - [46: Expand test matrix with nonempty value scenarios](zendframework/zend-inputfilter#46) - [47: Feature/minor test improvements](zendframework/zend-inputfilter#47) - [48: Empty values + Allow Empty + Not continue if empty is always true](zendframework/zend-inputfilter#48) - [49: Add tests for exceptions and improve some messages](zendframework/zend-inputfilter#49) - [50: Optional fields without value should be valid ](zendframework/zend-inputfilter#50) - [51: Optional input without value are valid](zendframework/zend-inputfilter#51) - [52: Fix merge two inputs without value, result 'has value' flag should be false](zendframework/zend-inputfilter#52) - [56: Hotfix/minor changes](zendframework/zend-inputfilter#56) - [57: Consolidate tests for InputFilterInterface](zendframework/zend-inputfilter#57) - [61: Provide NotEmpty::IS_EMPTY validation message for required input](zendframework/zend-inputfilter#61) - [63: Ensure custom error messages are used for required missing inputs](zendframework/zend-inputfilter#63) - [64: Feature/test cleanup](zendframework/zend-inputfilter#64)

This was referenced Aug 4, 2015

Rename continue_if_empty to validate_if_empty and default to true #12

Closed

Make it possible to override the 'Value is required' message #11

Closed

Maks3w mentioned this issue Aug 21, 2015

Fix missing optional fields should not be validated #25

Merged

Maks3w added the bug label Aug 21, 2015

Maks3w added this to the 2.4.8 milestone Aug 21, 2015

larsnystrom closed this as completed Sep 1, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NotEmpty validator doesn't override the required attribute #14

NotEmpty validator doesn't override the required attribute #14

larsnystrom commented Aug 4, 2015

tflin commented Aug 6, 2015

Maks3w commented Aug 21, 2015

Maks3w commented Aug 21, 2015

svycka commented Aug 21, 2015

larsnystrom commented Aug 24, 2015

weierophinney commented Aug 26, 2015

larsnystrom commented Aug 28, 2015

Maks3w commented Aug 28, 2015

Maks3w commented Aug 28, 2015

larsnystrom commented Sep 1, 2015

NotEmpty validator doesn't override the required attribute #14

NotEmpty validator doesn't override the required attribute #14

Comments

larsnystrom commented Aug 4, 2015

tflin commented Aug 6, 2015

Maks3w commented Aug 21, 2015

Maks3w commented Aug 21, 2015

svycka commented Aug 21, 2015

larsnystrom commented Aug 24, 2015

weierophinney commented Aug 26, 2015

larsnystrom commented Aug 28, 2015

Maks3w commented Aug 28, 2015

Maks3w commented Aug 28, 2015

larsnystrom commented Sep 1, 2015