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

Validate empty with context #4165

Merged

Conversation

zburnham
Copy link
Contributor

@zburnham zburnham commented Apr 2, 2013

Trying this again, I messed up a rebase.

Currently there is no way for Zend\InputFilter to determine the validity of an empty field based on context; for example, an empty field might be valid if another field is 'Y', but not valid if that other field is 'N'. This adds a continueIfEmpty flag to the Input definition, via a separate interface.

…ty instead of just being marked as valid if empty. Required to test for validity of an empty value based on context.
…ty instead of just being marked as valid if empty. Required to test for validity of an empty value based on context.
…ixing a failing unit test; test was not providing data for a required input.
…f empty, removing redundant tests that mysteriously turned up.
@ghost ghost assigned weierophinney Apr 12, 2013
weierophinney added a commit that referenced this pull request Apr 12, 2013
Validate empty with context

Conflicts:
	library/Zend/InputFilter/Factory.php
weierophinney added a commit that referenced this pull request Apr 12, 2013
- per php-cs-fixer
weierophinney added a commit that referenced this pull request Apr 12, 2013
@weierophinney weierophinney merged commit 1ba74d2 into zendframework:develop Apr 12, 2013
@weierophinney
Copy link
Member

Merged, and will release with 2.2.0. Thanks, Zach!

@zburnham
Copy link
Contributor Author

Excellent. I promise that I'll update the documentation, really I will.

weierophinney added a commit that referenced this pull request Apr 12, 2013
- `InputInterface` had a BC break, as it added `setContinueIfEmpty()` and
  `continueIfEmpty()` -- which were technically already defined in
  `EmptyContextInterface`, which `Input` had been modified to implement.
  Removing the methods from `InputInterface` removes the BC break, and fixes an
  issue that appears in 5.3.3 with one class implementing two interfaces that
  define compatible methods.
@@ -187,8 +187,10 @@ protected function validateInputs(array $inputs)
&& $input->isRequired()
&& $input->allowEmpty()
) {
$this->validInputs[$name] = $input;
continue;
if (!$input->allowEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@zburnham Should this be if (!$input->continueIfEmpty()) {?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, MWOP seemed to think it was ok, but I'll definitely take a look, more
eyes on the code and all that.

Z

On May 4, 2013, at 6:24 PM, Daniel Gimenes notifications@github.com wrote:

In library/Zend/InputFilter/BaseInputFilter.php:

@@ -187,8 +187,10 @@ protected function validateInputs(array $inputs)
&& $input->isRequired()
&& $input->allowEmpty()
) {

  •            $this->validInputs[$name] = $input;
    
  •            continue;
    
  •            if (!$input->allowEmpty()) {
    

@zburnham https://github.com/zburnham Should this be if
(!$input->continueIfEmpty()) {?


Reply to this email directly or view it on
GitHubhttps://github.com//pull/4165/files#r4085890
.

Copy link
Contributor

Choose a reason for hiding this comment

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

See line 188.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: #4426.

@danizord
Copy link
Contributor

danizord commented May 4, 2013

@zburnham You have not fulfilled your promise yet. 😄

@zburnham
Copy link
Contributor Author

zburnham commented May 4, 2013

I know :/ I'm going to be looking at it tonight anyway.

On May 4, 2013, at 6:41 PM, Daniel Gimenes notifications@github.com wrote:

@zburnham https://github.com/zburnham You have not fulfilled your promise
yet. [image: 😄]


Reply to this email directly or view it on
GitHubhttps://github.com//pull/4165#issuecomment-17442654
.

@danizord
Copy link
Contributor

danizord commented May 6, 2013

@zburnham Sorry if I'm wrong, but on second thought, this is useless since you can simply setAllowEmpty(false) and get the same result.

@zburnham
Copy link
Contributor Author

zburnham commented May 6, 2013

It stood a little tweaking, anyway.

On Mon, May 6, 2013 at 8:40 AM, Daniel Gimenes notifications@github.comwrote:

@zburnham https://github.com/zburnham Sorry if I'm wrong, but on second
thought, this is useless since you can simply setAllowEmpty(false) and get
the same result.


Reply to this email directly or view it on GitHubhttps://github.com//pull/4165#issuecomment-17479371
.

@danizord
Copy link
Contributor

danizord commented May 6, 2013

@weierophinney Can you please review this before release RC2?

@weierophinney
Copy link
Member

@danizord Already did. :)

@danizord
Copy link
Contributor

danizord commented May 6, 2013

@weierophinney It seems reasonable? As said setAllowEmpty(false) does the same thing.

@weierophinney
Copy link
Member

@danizord they're in different contexts. allowEmpty() works for individual input objects, but the EmptyContextInterface allows looking at empty in relation to the context of the input (i.e., the other inputs), as well as input filters.

So, it does seem reasonable to me; it provides more validation options, basically.

@danizord
Copy link
Contributor

danizord commented May 6, 2013

@weierophinney

allows looking at empty in relation to the context of the input (i.e., the other inputs), as well as input filters.

setAllowEmpty(false) also.

I don't want to be against you, but this is going to be complicated. Now we have:

  • setRequired()
  • setAllowEmpty()
  • setContinueIfEmpty()

It is impossible to understand without looking at the code of the component. Since the documentation does not cover it all.

@zburnham
Copy link
Contributor Author

zburnham commented May 6, 2013

The documentation is something I'm going to try to fix over the next few
days. The docs for InputFilter are pretty cursory.

On Mon, May 6, 2013 at 1:02 PM, Daniel Gimenes notifications@github.comwrote:

@weierophinney https://github.com/weierophinney

allows looking at empty in relation to the context of the input (i.e., the
other inputs), as well as input filters.

setAllowEmpty(false) also.

I don't want to be against you, but this is going to be complicated. Now
we have:

  • setRequired()
  • setAllowEmpty()
  • setContinueIfEmpty()

It is impossible to understand without looking at the code of the
component. Since the documentation does not cover it all.


Reply to this email directly or view it on GitHubhttps://github.com//pull/4165#issuecomment-17493928
.

weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
…idate_empty_with_context

Validate empty with context

Conflicts:
	library/Zend/InputFilter/Factory.php
weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
…tInterface

- `InputInterface` had a BC break, as it added `setContinueIfEmpty()` and
  `continueIfEmpty()` -- which were technically already defined in
  `EmptyContextInterface`, which `Input` had been modified to implement.
  Removing the methods from `InputInterface` removes the BC break, and fixes an
  issue that appears in 5.3.3 with one class implementing two interfaces that
  define compatible methods.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants