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

Fix loop validation input context is mutable #24

Merged
merged 6 commits into from
Sep 1, 2015

Conversation

Maks3w
Copy link
Member

@Maks3w Maks3w commented Aug 20, 2015

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

Maks3w commented Aug 20, 2015

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

@Maks3w Maks3w changed the title Loop validation context is mutable [WIP] Loop validation context is mutable Aug 20, 2015
@Maks3w Maks3w changed the title [WIP] Loop validation context is mutable Loop validation context is mutable Aug 20, 2015
@Maks3w Maks3w force-pushed the hotfix/mutable-context branch 2 times, most recently from b261755 to edc3444 Compare August 20, 2015 15:38
@Maks3w Maks3w changed the title Loop validation context is mutable Loop validation input context is mutable Aug 20, 2015
@Maks3w Maks3w changed the title Loop validation input context is mutable Fix loop validation input context is mutable Aug 20, 2015
@svycka
Copy link
Contributor

svycka commented Aug 20, 2015

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

Maks3w commented Aug 20, 2015

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

@Maks3w Maks3w added this to the 2.4.8 milestone Aug 20, 2015
@weierophinney
Copy link
Member

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 Maks3w force-pushed the hotfix/mutable-context branch 2 times, most recently from 5e2a707 to 6d9c9bc Compare August 24, 2015 07:00
@@ -236,6 +236,8 @@ protected function validateInputs(array $inputs, $data = [], $context = null)
$data = $this->getRawValues();
}

$inputContext = $context ?: (array_merge($this->getRawValues(), (array) $data));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May this should use the filtered version instead the raw value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@weierophinney
Copy link
Member

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

Maks3w commented Aug 26, 2015

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

@Maks3w
Copy link
Member Author

Maks3w commented Aug 27, 2015

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
if (! $hasValue && $required) {
$this->setErrorMessage('Value is required');
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@weierophinney weierophinney merged commit d743878 into zendframework:master Sep 1, 2015
weierophinney added a commit that referenced this pull request Sep 1, 2015
Fix loop validation input context is mutable
weierophinney added a commit that referenced this pull request Sep 1, 2015
weierophinney added a commit that referenced this pull request Sep 1, 2015
@Maks3w Maks3w deleted the hotfix/mutable-context branch September 1, 2015 21:37
weierophinney added a commit to weierophinney/zend-inputfilter that referenced this pull request Sep 8, 2015
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)
weierophinney added a commit to weierophinney/zend-inputfilter that referenced this pull request Sep 9, 2015
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)
weierophinney added a commit to weierophinney/zend-inputfilter that referenced this pull request Sep 9, 2015
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)
weierophinney added a commit to weierophinney/zend-inputfilter that referenced this pull request Sep 9, 2015
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)
weierophinney added a commit to zendframework/zendframework that referenced this pull request Dec 20, 2016
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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BC in validators context value
3 participants