Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/required validator #10

Open
weierophinney opened this issue Dec 31, 2019 · 24 comments
Open

Feature/required validator #10

weierophinney opened this issue Dec 31, 2019 · 24 comments
Labels
Question Further information is requested Work In Progress

Comments

@weierophinney
Copy link
Member

These allows manage the required message like any other ValidatorInterface element.

Note: While you can add the validator manually the flag setRequired must be set too

  $input = new Input();
  $input->setRequired(true);empty value instead *not set*.
  $input->getValidatorChain()->attach(
      new Zend\InputFilter\Validator\Required(),
      true                             // break chain on failure

  );
  $inputSpecification = [
    'required'   => true,
    'validators' => [
      [
        'break_chain_on_failure' => true,
        'name'                   => 'Zend\\InputFilter\\Validator\\Required',
      ],
    ],
  ];

Note: setRequired(false) may not be enough and you will need remove the validator from the chain.


Originally posted by @Maks3w at zendframework/zend-inputfilter#62

@weierophinney weierophinney added Question Further information is requested WIP labels Dec 31, 2019
@weierophinney
Copy link
Member Author

Inject a validator in the chain make difficult reset the input state for to validate different values or transititions set <=> not set


Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I haven't tried running this yet, but I like the idea.

Why must "the flag setRequired" be set when adding the validator manually? Wouldn't it be better to just deprecate the flag?


Originally posted by @larsnystrom at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

Because the validator does not validate the input value (https://github.com/zendframework/zend-inputfilter/pull/62/files#diff-5e41c64803af5c0ad8ad84c3e25c834cR412)

I didn't resolve how to make to replace the whole required funcionallity with the validator.

This is needed too https://github.com/zendframework/zend-inputfilter/blob/master/src/BaseInputFilter.php#L253


Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

Hmm, yeah that's true. Do you think it would be possible to use the $data array as $context instead of the raw values? That would enable the Required-validator to check whether a key exists without having a reference to the input object.

You would have to inject the input's name in the Required-validator's constructor as well, so the validator knew which key to look for.

What do you think?


Originally posted by @larsnystrom at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

IIRC every input is present in $context with the default value of null.


Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

Also rely on $context makes the input dependant of InputFilter implementation and can't be used standalone.


Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

The problem is that they're initialized to null. Null is still a value, and would pass the required-validator. So we need the original input data, where the the input is not present.

I don't think relying on $context makes the Input dependent on the InputFilter implementation. The Input::isValid() method is already accepting a $context parameter. We can control the default value of that parameter inside the Input class.

  1. If no $context parameter is provided, and if there is no value, then set the $context to [].
  2. If no $context parameter is provided, and if there is a value, then set the $context to ['inputName' => $val].
  3. If a $context parameter is provided, then use that $context.

Originally posted by @larsnystrom at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

Even if looking in $context is needed to know what input name to search.


Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

Yes, that's why the input's name must be injected into the validator. If you add a RequiredValidator::setName() method, it will be compatible with the plugin manager.


Originally posted by @larsnystrom at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I start to think the best option could be to have 2 validator chains. One for validate the data and another one for validate the input state.

Something like this

  • Is required or not (RequiredValidator::isValid(input))
  • Data is valid or not (DataValidator::isValid(Input))
    • Data is valid because has X characters (FooValidator::isValid(data))
    • Data is valid because exists on DB

Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I just realized that my idea doesn't work. (The absence of a Required-validator wouldn't be equivalent to setRequired(false), since setRequired(false) implies not running any validators, but an absent Required-validator wouldn't give any information concerning whether to run the other validators.)

Two validator chains might be a good idea.


Originally posted by @larsnystrom at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

IMO, if we are going to have a Required validator, then we really can't also have a required flag without causing confusion.


Originally posted by @akrabat at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I agree with @akrabat


Originally posted by @stefanotorresi at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I also agree with that. Right now it seems like the only way to implement a Required validator and not keep the required flag is to implement two validator chains, like Maks3w suggested.

I'm not sure if that's a good idea or not.


Originally posted by @larsnystrom at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

For to make this works seems It's needed a flag for "break the chain on success" for the ValidatorChain and create an OptionalValidator.

Thoughts? I think its reasonable to have break_chain_on_success and break_chain_on_failure


Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I think we should merge #67 and #73 now since those won't cause any BC breaks.

Then I think we should do a more thorough refactorization for version 3.0.

In 3.0 I hope we can remove the required attribute altogether, make null equivalent to a missing value (thus leaving all validation to the normal validator chain), and have an optional attribute which defaults to false (optional can never fail so no error message required, just skip filters and validation if it's optional and the value is missing). I'd also like to remove fallback, continue_if_empty and allow_empty and instead have a default value, which would be used instead of null when a value is missing, but still be put through the whole filter and validation process.

But that's just a dream right now.


Originally posted by @larsnystrom at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I don't see this as a BC Break because the translation feature seems never has work. Anyway the purpose of this PR is discuss about the required / optional field concept


Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I was serious in my last post where I suggested we completely get rid of the required flag and add an optional flag. That would be a BC break, but a lot simpler to maintain than two different validator chains.


Originally posted by @larsnystrom at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I don't see this as a BC Break because the translation feature seems never has work.

that's not correct, it worked prior to 2.4.


Originally posted by @stefanotorresi at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

@stefanotorresi Prior to 2.5.2/2.4.5 there was not a required message.


Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

it was handled by the notEmpty validator, which was always injected for required inputs before continue_if_empty and allow_empty flags were deprecated.


Originally posted by @stefanotorresi at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I really like this approach, but I share the same concern voiced by @akrabat — unless the required flag is tied to the Required validator, we're going to have a lot of confusion.

It seems like this is mostly the case right now, though; isValid() auto-prepends the validator if the $required flag is true. It's the other side of the equation we need, however: isRequired() should return true if the Required validator is present in the chain.

If you can implement that, I can merge this.


Originally posted by @weierophinney at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

Moved to 2.7.0 milestone; we're ready for 2.6.0 (zend-servicemanager forwards-compatibility).


Originally posted by @weierophinney at zendframework/zend-inputfilter#62 (comment)

@weierophinney
Copy link
Member Author

I keep thinking in new ways for make this better. Probably with something like #87


Originally posted by @Maks3w at zendframework/zend-inputfilter#62 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question Further information is requested Work In Progress
Projects
None yet
Development

No branches or pull requests

2 participants