Fix loop validation input context is mutable #24

Maks3w · 2015-08-20T08:13:22Z

Given two input fields the context used in each one will be different. https://github.com/zendframework/zend-inputfilter/pull/24/files#diff-2cd0c1ba642dc3e6de9ca4e2af0f5e71R290

This means inputs need to be order sensitive

$inputs = ['A', 'B'];
$data = [ /* empty */ ];

$inputA->isValid($context = ['A' => null]);
$inputB->isValid($context = ['A' => null, 'B' => null]);

// Inverse order will change $context too.
$inputs = ['B', 'A'];
$data = [ /* empty */ ];

$inputB->isValid($context = ['B' => null]);
$inputA->isValid($context = ['B' => null, 'A' => null]);

This PR provides fix and enhance this feature by building a default input context using the default input values when no custom context is provided.

Fix #16

Supersede and close #30

Maks3w · 2015-08-20T09:17:27Z

Note tests are not failling just the coverage due the different weights given to each line after removal.

svycka · 2015-08-20T16:23:49Z

looks good
also have to note that this adds something new

$inputs = ['A', 'B'];
$data = ['C' => 'something'];

In context now we have:

['A' => null, 'B' => null, 'C' => 'something']

Before 'C' => 'something' would not be added to context.
But I think this is not a bug but good feature. Maybe add test to keep it :)

Maks3w · 2015-08-20T16:52:31Z

Yep I opt for use default input raw value for build the context (usually null, sometimes mixed). Test it's placed for this enhancement.

weierophinney · 2015-08-20T22:14:14Z

As I noted on #25, I'm a bit worried that changing test assumptions is indicative of a BC break; also, like #25, the logic looks reasonable. I'll do a more thorough review early next week.

Maks3w · 2015-08-25T05:47:59Z

src/BaseInputFilter.php

@@ -236,6 +236,8 @@ protected function validateInputs(array $inputs, $data = [], $context = null)
            $data = $this->getRawValues();
        }

+        $inputContext = $context ?: (array_merge($this->getRawValues(), (array) $data));


May this should use the filtered version instead the raw value

Considering that the original from two lines above was using the raw values, we should keep it that way for now. We can address again with the next major version.

Note before the context was always null for optional values. getRawValues already change the behavior if user call setValue before

weierophinney · 2015-08-26T20:31:52Z

@Maks3w Can you rebase this, please? When I merged, there was a conflict, and my attempts to resolve the conflict have left me with failing tests.

Maks3w · 2015-08-26T20:50:21Z

Since #25 optional fields is not call what was expected. I've to rewrite the test

Maks3w · 2015-08-27T16:13:32Z

Done. Now includes changes from #30

This methods provide a flag for distinguish when $value contains a real or the default property value.

Delegate to input isValid grant full control about the fallback value feature. This commit also consolidate Input::setFallbackValue tests

Fix zendframework#16

weierophinney · 2015-09-01T18:19:41Z

src/ArrayInput.php

+        if (! $hasValue && $required) {
+            $this->setErrorMessage('Value is required');
+            return false;
+        }


This looks good per our discussion last week on IRC.

For those reading:

If the value was not present but has a fallback, we set it to the default value and consider it valid.

If the value was not present, but required, we mark it as invalid.

Fix loop validation input context is mutable

Close #24 Fixes #16

Forward port #24

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)

Maks3w mentioned this pull request Aug 20, 2015

BC in validators context value #16

Closed

Maks3w added BC Break WIP labels Aug 20, 2015

Maks3w changed the title ~~Loop validation context is mutable~~ [WIP] Loop validation context is mutable Aug 20, 2015

Maks3w added the question label Aug 20, 2015

Maks3w force-pushed the hotfix/mutable-context branch from 6996b08 to 5c81065 Compare August 20, 2015 15:04

Maks3w added bug and removed question WIP BC Break labels Aug 20, 2015

Maks3w changed the title ~~[WIP] Loop validation context is mutable~~ Loop validation context is mutable Aug 20, 2015

Maks3w force-pushed the hotfix/mutable-context branch 2 times, most recently from b261755 to edc3444 Compare August 20, 2015 15:38

Maks3w changed the title ~~Loop validation context is mutable~~ Loop validation input context is mutable Aug 20, 2015

Maks3w changed the title ~~Loop validation input context is mutable~~ Fix loop validation input context is mutable Aug 20, 2015

Maks3w force-pushed the hotfix/mutable-context branch from edc3444 to 863dc48 Compare August 20, 2015 22:04

Maks3w added this to the 2.4.8 milestone Aug 20, 2015

Maks3w assigned weierophinney Aug 20, 2015

Maks3w force-pushed the hotfix/mutable-context branch 2 times, most recently from 5e2a707 to 6d9c9bc Compare August 24, 2015 07:00

Maks3w reviewed Aug 25, 2015
View reviewed changes

Maks3w force-pushed the hotfix/mutable-context branch from 6d9c9bc to 47ece02 Compare August 27, 2015 15:16

Maks3w added the awaiting maintainer response label Aug 27, 2015

Maks3w added 6 commits September 1, 2015 08:31

[Input] Add hasValue()/resetValue() methods

8c35a20

This methods provide a flag for distinguish when $value contains a real or the default property value.

Delegate to InputInterface::isValid when data not exists

de0e3c9

Delegate to input isValid grant full control about the fallback value feature. This commit also consolidate Input::setFallbackValue tests

Remove legacy setFallbackValue tests

152ee5b

Input fallback feature should work when required and optional

44cd418

Fix loop validation input context is mutable.

aec915e

Fix zendframework#16

Remove old tests

d743878

Maks3w force-pushed the hotfix/mutable-context branch from 47ece02 to aec915e Compare September 1, 2015 07:15

weierophinney removed the awaiting maintainer response label Sep 1, 2015

weierophinney reviewed Sep 1, 2015
View reviewed changes

weierophinney merged commit d743878 into zendframework:master Sep 1, 2015

weierophinney added a commit that referenced this pull request Sep 1, 2015

Merge pull request #24 from Maks3w/hotfix/mutable-context

ddd7fa5

Fix loop validation input context is mutable

weierophinney added a commit that referenced this pull request Sep 1, 2015

Merge branch 'hotfix/24'

25cd01e

Close #24 Fixes #16

weierophinney added a commit that referenced this pull request Sep 1, 2015

Merge branch 'hotfix/24' into develop

241e1a3

Forward port #24

Maks3w mentioned this pull request Sep 1, 2015

'Value is required' message should be localizable #28

Closed

Maks3w deleted the hotfix/mutable-context branch September 1, 2015 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix loop validation input context is mutable #24

Fix loop validation input context is mutable #24

Maks3w commented Aug 20, 2015

Maks3w commented Aug 20, 2015

svycka commented Aug 20, 2015

Maks3w commented Aug 20, 2015

weierophinney commented Aug 20, 2015

Maks3w Aug 25, 2015

weierophinney Aug 26, 2015

Maks3w Aug 26, 2015

weierophinney commented Aug 26, 2015

Maks3w commented Aug 26, 2015

Maks3w commented Aug 27, 2015

weierophinney Sep 1, 2015

Fix loop validation input context is mutable #24

Fix loop validation input context is mutable #24

Conversation

Maks3w commented Aug 20, 2015

Maks3w commented Aug 20, 2015

svycka commented Aug 20, 2015

Maks3w commented Aug 20, 2015

weierophinney commented Aug 20, 2015

Maks3w Aug 25, 2015

Choose a reason for hiding this comment

weierophinney Aug 26, 2015

Choose a reason for hiding this comment

Maks3w Aug 26, 2015

Choose a reason for hiding this comment

weierophinney commented Aug 26, 2015

Maks3w commented Aug 26, 2015

Maks3w commented Aug 27, 2015

weierophinney Sep 1, 2015

Choose a reason for hiding this comment