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

Custom Validation Rule & allowEmpty #8925

Closed
2 of 3 tasks
wilsmex opened this issue Jun 2, 2016 · 26 comments
Closed
2 of 3 tasks

Custom Validation Rule & allowEmpty #8925

wilsmex opened this issue Jun 2, 2016 · 26 comments

Comments

@wilsmex
Copy link

wilsmex commented Jun 2, 2016

This is a (multiple allowed):

  • bug
  • enhancement
  • feature-discussion (RFC)
  • CakePHP Version: 3.2

What you did

I need to have my custom validation rules determine whether or not a field should be required or not. This is currently impossible as:

  1. if you set allowEmpty the custom validation rule will not fire if the field is left empty
  2. if you set your custom validation rule, the field is forced to be required before the custom validation rules will trigger.

Expected Behavior

When I create a custom validation rule, it should run it's logic and return true/false whether or not the field is left empty or not.

Actual Behavior

When a custom validation rule is in place, a field cannot be left blank and forces data before the custom validation rules execute.

See http://stackoverflow.com/questions/37176984/cakephp-3-x-custom-validation-field-required

@ADmad
Copy link
Member

ADmad commented Jun 2, 2016

The second param of allowEmpty() can be a callable. You can put your custom logic for deciding whether a field can be empty in the callable and return a boolean.

@ADmad ADmad closed this as completed Jun 2, 2016
@ADmad ADmad added this to the 3.2.11 milestone Jun 2, 2016
@dereuromark
Copy link
Member

dereuromark commented Jun 2, 2016

This only works for the first time not when a Form invalidates and re-renders the view.
I tried and the logic of allowEmpty() callbacks is not sufficient here :(

@ADmad
Copy link
Member

ADmad commented Jun 2, 2016

Please provide example codes and steps to reproduce the issue.

@dereuromark
Copy link
Member

The main problem is that the rules are used both for presenting the form and for validating the posted data (before vs after).
Mixing those together this way makes it inflexible and disallows the above use case.

I bet @wilsmex can provide a step to step example, otherwise I will provide one of the tests I ran with "depending fields" and leaving one of them blank.

@lorenzo
Copy link
Member

lorenzo commented Jun 2, 2016

Yeah, I have also been affected by this but a change on the behavior would likely cause a lot of pain. Maybe a flag for hinting the validator that it should run the validation logic regardless of the field being present or empty?

@lorenzo lorenzo added enhancement and removed defect labels Jun 2, 2016
@dereuromark
Copy link
Member

dereuromark commented Jun 2, 2016

That was also my initial proposal when you and me talked about it, some boolean flag to do that until we can find a more clean approach in the next major or sth.
E.g.

allowBlankForValidation

I don't think there a use case to allow validation when it is not present at all (you can easily enforce that yourself by merging with default values, e.g. empty strings).

@markstory markstory modified the milestones: Future, 3.2.11 Jun 3, 2016
@markstory
Copy link
Member

Would it makes sense to consider deprecating the concept of 'empty', and instead make allowEmpty() a helper method that attaches validation rules? Then we could avoid this state where rules don't run when values are empty, and empty checking becomes just another validation rule.

@chinpei215
Copy link
Contributor

chinpei215 commented Jun 7, 2016

See the following code:

$validator
    ->allowEmpty('value')
    ->integer('value');

In this case, users would expect that integer rule doesn't run if the field is left empty, unlikely the original post on the StackOverflow.

Whatever is the implementation to solve this issue, we would need to introduce a new option or new method to specify how Validator/FormHelper should work.

@markstory
Copy link
Member

@chinpei215 Good point. If allowEmpty() becomes a 'macro'/helper method, it will need to set 'last' => true so other validators won't be invoked. One problematic aspect of making allowEmpty() a helper method is scenarios like this will behave differently:

$validator
  ->integer('value')
  ->allowEmpty('value');

@chinpei215
Copy link
Contributor

Reading source code, the 'last' option seems to be used only after the validation has failed.

Aside from that, how does empty validation rule behave?
If the field is empty, the empty validation rule would return true, and should stop execution. I can understand that.
But if the field is not empty, what happens? If the empty validation rule returns false, the validation would have failed whatever other validation rules return. That is not an expected behavior.

So the empty validation rule must return true always, and behave like the 'last' option if the field is empty. That might be not similar to other validation rules.

@dereuromark
Copy link
Member

I would still rather consider this a bug in the current 3.x implementation as this use case, even though rare, cannot be done with the current tooling, and despite the initial false positive on first post will then often times surprisingly fail on the 2nd post (when "empty" data has been provided via post).
Not an easy solution here, though.

@chinpei215
Copy link
Contributor

What about using on option? We need to allow arrays for on option though.

$validator->add('fieldName', 'ruleName', [
    'rule' => 'custom',
    'on' => 'empty', // Same as 'on' => ['create', 'update', 'empty']
]);

@dereuromark
Copy link
Member

@chinpei215 I didnt play it through, but it could work. If it accounts for the above use cases, sure.

@chinpei215
Copy link
Contributor

Sorry. My approach might not work 😞
Probably, FormHelper would render the field as required, if allowEmpty is false. So we would need to fix allowEmpty in some way.

@ArakTaiRoth
Copy link

It's been a while since this has been talked about. I'm just wondering if any progress on this has been made as of yet?

@chinpei215
Copy link
Contributor

chinpei215 commented Dec 4, 2017

I finally understood the reason why 2.x validation has the allowEmpty option in the each rules. It is easy to create this kind of rules in 2.x. If you set allowEmpty to -1 (a value neither true nor false) for a custom validation rule, it will be triggered always. I guess the original intention was null, as the default value is null. But because of recent HTML update, your browser would reject to submit forms if some field marked as required is left empty. However, it still works if you set the option to -1 as the FormHelper doesn't mark the field as required in this case.

Anyway, this is a kind of regression in 3.x. So I think we need to discuss how to fix this defect. Do you have any ideas?

@markstory
Copy link
Member

@chinpei215 Earlier in the thread I had proposed that allowEmpty() become a macro for generating a validation rule and not an additional state in the field validator. That might help with your situation.

@dereuromark dereuromark modified the milestones: Future, 3.6.0 Dec 5, 2017
@chinpei215
Copy link
Contributor

Thank you for your replay, but I am not sure how it works.

@markstory
Copy link
Member

Currently allowEmpty() is a 'state' on a field. When set to true, empty values are allowed and no validators are run. Instead we could move to a model where $validator->allowEmpty('name', false) appends a new rule that adds a validation error if the field contains an empty value. $validator->allowEmpty('field', true) would effectively be a no-op and other validation methods on the field would run as normal.

I wouldn't overload the current allowEmpty() methods as this would be a breaking change in my opinion. But we could add new methods that add this behavior and deprecate the existing allow empty behavior.

@chinpei215
Copy link
Contributor

chinpei215 commented Dec 6, 2017

Thank you for explanation. How does it work when $validator->allowEmpty('field', true)? In this case, if the next validation rule is integer() it must not run as it doesn't allow empty values. But if the next validation rule is myCustomRule() someone wants it runs (because he wants to judge whether it should pass empty values or not), and someone else doesn't want it runs (because his rule doesn't pass empty values). So I thought we would need to add a new option.

@markstory
Copy link
Member

Lets assume our new behavior is offered by a new method called acceptEmpty(). I think we could afford both scenarios based on the order rules are applied. In the integer case we could define rules:

$validator
  ->acceptEmpty('user_id', true)
  ->integer('user_id);

In this scenario acceptEmpty() precedes the integer rule and should it pass (which it always will) the integer rule is called. If a field had a custom validator that didn't accept empty values they could do:

$validator
  ->acceptEmpty('user_id', false)
  ->custom('user_id', 'myRule');

The rule generated by acceptEmpty() would be a 'last' rule aborting validation when it fails. By ordering the acceptEmpty() rule with other custom/core rules we should be able to solve the scenarios we've been discussing thus far.

@chinpei215
Copy link
Contributor

Thank you. What should people do when they want to judge whether they allow empty or not? Probably, for now, custom rules are not called if field is left empty.

@xavier83ar
Copy link

I'm having a particular situation, and I think that's related to this. I've an entity that has two fields "linked", the user may complete only one, but one of them must be filled mandatory.

    // this was my first attempt
    $validator->allowEmptyString('field_a', function ($context) {
        return !empty($context['data']['field_b']);
    });

    // ... and a similar validation rule on field_b checking for field_a

While this works for validating the entity before saving, it does not work when the form is built for a new entity. Both fields are empty, so both rules determines that the other field can't be empty, as a result, both inputs on the form has the required attribute.

My second attempt was allow empty on both fields and add a custom validation rule to do the check (that one of them is filled), but then is when I find out that if both are empty, validation rules on them aren't applied at all. Which has some sense, since I'm telling the framework that those field are allowed to be empty, but, in the other hand, a validation may be checking also the context data, so there should be a way of telling validator to apply some rules no matter the field is allowed to be empty.

Right now I've solved the issue by applying the validations rules of my first attempt and adding 'required' => false attribute on form inputs forcing them to allow empty values on sumit. I know that's not a big deal, but is there some better way of doing this?

Do we have some way of detecting when validatior is used by the entity context to provide info to the form helper, and when is used to actually validate data?

(I'm using CakePHP 3.7.4)

@markstory
Copy link
Member

Do we have some way of detecting when validatior is used by the entity context to provide info to the form helper, and when is used to actually validate data

No there doesn't exist a way to do this. You could not use allowEmptyString and handle the empty checks in your validation rules or application rules instead.

@xavier83ar
Copy link

Yep, after posting my comment I've checked out other issues about validation and I saw this #12484 (comment)

@github-actions
Copy link

This issue is stale because it has been open for 120 days with no activity. Remove the stale label or comment or this will be closed in 15 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants