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

Validator requires one attribute and cannot see the whole model #28

Open
kamarton opened this issue Oct 31, 2019 · 35 comments
Open

Validator requires one attribute and cannot see the whole model #28

kamarton opened this issue Oct 31, 2019 · 35 comments

Comments

@kamarton
Copy link
Contributor

@kamarton kamarton commented Oct 31, 2019

What steps will reproduce the problem?

In yii2, it was easy to access any of the model's attributes. In yii3, In Yii3 this option seems to me not feasible.

https://www.yiiframework.com/doc/guide/2.0/en/input-validation#standalone-validators

$results->addResult($attribute, $rules->validate($dataSet->getValue($attribute)));

This issue also affects the Result class because it is not suitable for adding errors to an attribute.

What is the expected result?

I get the whole dataset in an extra parameter, and change return type to ResultSet

public function validate($value, DataSetInterface $dataSet): ResultSet
// or yii2 style
public function validate(?string $attribute, DataSetInterface $dataSet): ResultSet

public function validate($value): Result

public function addError(string $message): void

public function addResult(string $attribute, Result $result): void

usage:

[
  /* numeric index -> attribute=null */ (new MyComplexRule())->moreParameters()...
]

What do you get instead?

[
  'notRelevantAttributeName' => (new MyComplexRule())->with($this /* or $dataset ??*/)->moreParameters()...
]

But in this case, I have no way of returning an arbitrary attribute error of the model.

Additional info

Also, to keep the benefits of the current solution, the Rule class should be somehow allow the Result class to be used internally in simpler cases.

Use case

I need to check for additional attributes depending on a type attribute.

Q A
Version 3.0.x-dev
PHP version -
Operating system -
@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Oct 31, 2019

Even better, if ResultSet is part of the parameters: #29

public function validate(?string $attribute, DataSetInterface $dataSet, ResultSet $resultSet): ResultSet /* return for override? */
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 31, 2019

How would that work for cases described in readme?

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Oct 31, 2019

How would that work for cases described in readme?

class CountryValidator extends Validator
{
    public function validateAttribute($model, $attribute)
    {
        if($model->type === 'any') {
           $this->addError($model, 'fieldA', '....');
        } else {
           $this->addError($model, 'fieldA', '....');
           $this->addError($model, 'fieldB', '....');
        }
    }
}
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 31, 2019

And the rest of the cases?

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Oct 31, 2019

And the rest of the cases?

I have access to everything in the example code method:

  • I can add an error message to any attribute
  • access to any attribute of the model
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Oct 31, 2019

There are multiple examples:

  1. Validating a single value
  2. Validating a set of data
  3. Grouping common validation rules into rule sets
@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Oct 31, 2019

  1. Validating a set of data
class CountryValidator extends Validator
{
    // check multiple attribute at one context
    public function validateAttribute($model, $attribute)
    {
	if($model->fieldA && $model->fieldB) {
           // together not allowed
           $this->addError($model, 'fieldA', 'together not allowed');
           $this->addError($model, 'fieldB', 'together not allowed');
	}
    }
}
@demonking

This comment has been minimized.

Copy link

@demonking demonking commented Oct 31, 2019

Isn't it a special case and should happened somewhere else?
This wouldn't be a validator anymore, it seems more like an expression or something similar

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Nov 1, 2019

This wouldn't be a validator anymore, it seems more like an expression or something similar

That's just the rule. It is normal for a form to contain attributes that are related to one another. For example in sign up form with password and password re, or email and email re.

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Nov 1, 2019

More problem in code complexity for example delivery address, with more sub fields.

Here, too, the problem is with an alternative delivery address option:

  • or all fields are left blank
  • or, if one is completed, all are required

In addition, the code complexity issue is:

// If the attributes of the dataset cannot be accessed in the rule
[
  'anyattribute' => new Any()->...,
  'delivery_country' => new HasLength()->..
  'delivery_postal_code' => new HasLength()...
  'delivery_...' => ...
]
// A billing address and a shipping address must be provided.
[
  'anyattribute' => new Any()->...,
  'delivery_country' => new HasLength()->..
  'billing_country' => new HasLength()->..
  'delivery_postal_code' => new HasLength()...
  'billing_postal_code' => new HasLength()...
  'delivery_...' => ...
  'billing_...' => ...
]

by issue:

[
  'anyattribute' => new Any()->...,
  /* attribute is not relevant */ (new Address())->attributePrefix('delivery_'), 
  /* attribute is not relevant */ (new Address())->attributePrefix('billing_'), 
]

class Address {
  ...
  public function attributePrefix(..);
  ...
  public function validateValue(?string $attribute, DataSetInterface $dataSet, ResultSet $resultSet): ResultSet {
    if($this->optional && empty($dataSet->getValue($this->prefix.'country')) && ....) {
      return $resultSet;
    }
    $countryResult = (new HasLength())->min...->validateValue($dataSet->getValue($this->prefix.'country'));
    $resultSet->addResult($this->prefix.'country', $countryResult);
    ...
    return $resultSet;
  }
}
@demonking

This comment has been minimized.

Copy link

@demonking demonking commented Nov 1, 2019

It is normal for a form to contain attributes that are related to one another. For example in sign up form with password and password re, or email and email re.

This seems like a use-case for scenarios :)

Edit: After thinking about your last example, i have another example i could fixed easier.
But with cycle orm we will lose the option with scenario i think :/

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 1, 2019

Correct, no screnarios are planned.

@demonking

This comment has been minimized.

Copy link

@demonking demonking commented Nov 1, 2019

@samdark what is your opinion for this issue?
SkipOnError could be fixed without this, but the other special cases not.

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 1, 2019

No final opinion yet. That's fairly typical use case for validation in Yii 2 and, no doubt, there will be similar problem to solve in Yii 3 projects:

  1. If checkbox A is checked, validate field B else ignore it.
  2. One of A, B, C should be filled.

etc.

For these we, indeed, need to:

  1. Access any field.
  2. Trigger validation for any field and getting results.
  3. Add error to any field.

This part is clear. What's not clear is where/how to do this type of comopound validation.

@Insolita

This comment has been minimized.

Copy link

@Insolita Insolita commented Nov 3, 2019

I'm agree that in practical we often need to make validation against attributes that depends on values of another attributes. It often happens when we check data from complex forms by api
And when we validate nested json structure we need to be flexible with attribute that will be target for error message

What's not clear is where/how to do this type of compound validation.

In yii2 we have "when" condition and can create rules like

return [
[['ip'], 'ip', 'ipv6'=>false, 'when'=>function($model){return $model->allowIpv6===false;}],
[['ip'], 'ip', 'ipv6'=>true, 'when'=>function($model){return $model->allowIpv6===true && !$model->ipv6Only;}],
[['ip'], 'ip', 'ipv6'=>true, 'ipv4'=>false,  'when'=>function($model){return $model->allowIpv6===true && $model->ipv6Only===true;}]
]

Why not do something similar

$validator = new Validator([    
   //...
   'allowIpv6'=>[new Boolean()],
   'onlyIpv6'=>[new When(function(DataSetInterface $data){
             if($data->getValue('allowIpv6') == false) {
                 return (new CompareTo(false)->asBoolean());
             }
             return new Boolean(), 
    })],
    'ip' => [
       new When(function(DataSetInterface $data){
         return (new Ip())->allowIpv6($data->getValue('allowIpv6'))
                    ->allowIpv4(!$data->getValue('onlyIpv6'));
      });
    ],
]);
@Insolita

This comment has been minimized.

Copy link

@Insolita Insolita commented Nov 3, 2019

And for previous case something like

$validator = new Validator([    
   //...
   'fieldA' =>[new InRange(['X', 'Y', 'Z'])],
   new ConditionalRules(function(DatasetInterface $data){
        if($data->getValue('fieldA') == 'X'){
             return RuleSetForX::get();
       }
       //..
       return [
             'anyattribute' => new Any()->...,
             'delivery_country' => new HasLength()->..
             'delivery_postal_code' => new HasLength()...
       ]
})
])
@demonking

This comment has been minimized.

Copy link

@demonking demonking commented Nov 3, 2019

I prefer classes instead of in line functions. For small validations it's fine, aber you geht some functions, it will bloat up the code.

When we will use classes for this, then it would be similar with the solution from @kamarton

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 3, 2019

Why not do something similar

In present state Rule and Rules aren't bound to data set. They work with individual value.

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 3, 2019

There are two use cases for validation:

  1. Individual values.
  2. Data sets.

Yii 2 validation was data set oriented and it wasn't convenient to use validation with individual values via fake dynamic model.

@Insolita

This comment has been minimized.

Copy link

@Insolita Insolita commented Nov 3, 2019

In present state Rule and Rules aren't bound to data set. They work with individual value.

In my cases i dont't boud dataset in rule directly. It can be bounded by wrapper and resolved inside
Validator->validate(), but ok...

There are two use cases for validation:
Individual values.
Data sets.
Yii 2 validation was data set oriented and it wasn't convenient to use validation with individual values via fake dynamic model.

I look on validation from point of applicable context also. In yii2 we have models with set of rules and different scenarios.

At now, especially with orm, we probably will validate request before fill model. So we can do it in controller or with separated validation classes like AuthRequestValidation, RegistrationRequestValidation, CreatePostValidation, EditPostValidation... And probably we will need some business-rule-scoped validators/or rule/or rulesets that should be reusable in different requests
For ex. Title - should be string, trimmed, title cased, and limited with 150 chars, Phone should be started with +, numeric, and probably limited with only allowed country codes, like only russian and ukraine. Address - is set of fields like country, street, code etc. And that should be reusable in different request context. And i don't see these ability with current approach. If i create separated Validator instances for all, next i must additionaly combine result-sets. If i create ti as rules - i haven't access to additional data.
So at now i feel that something is missing. Or you mean different use approach that is not clear for me

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 3, 2019

new When(function(DataSetInterface $data){

How would $data be passed?

@Insolita

This comment has been minimized.

Copy link

@Insolita Insolita commented Nov 3, 2019

How would $data be passed?

As i see in https://github.com/yiisoft/validator/blob/master/src/Validator.php#L31

Draft

 public function validate(DataSetInterface $dataSet): ResultSet
    {
        $results = new ResultSet();
        foreach ($this->attributeRules as $attribute => $rules)
        {
           if ($rules instanse of Conditional){ 
              $rules = $rules->resolve($dataSet); 
           }
            $results->addResult($attribute, $rules->validate($dataSet->getValue($attribute), $dataSet));
        }
        return $results;
    }

class Conditional
{
      private $rule;
      public function __construct(callable $rule){ $this->rule = $rule;}
      public function resolve(DataSetInterface $data): Rules{
         return $this->rule($data);
    }
}

And for When, seems we should pass dataset in Rules and apply in similar manner

class When
{
      private $rule;
      public function __construct(callable $rule){ $this->rule = $rule;}
      public function resolve($value, DataSetInterface $data): Rule{
         return $this->rule($value, $data);
    }
}

https://github.com/yiisoft/validator/blob/master/src/Rules.php#L35

 public function validate($value, ?DatasetInterface $dataSet = null): Result
    {
        $compoundResult = new Result();
        foreach ($this->getRules($value) as $rule) {
           if ($rule instance of When && $dataSet !== null){
               $rule = $rule->resolve($value, $dataSet);
         }else{
                continue or throw Exception
          }
            $ruleResult = $rule->validateValue($value);
            if ($ruleResult->isValid() === false) {
                foreach ($ruleResult->getErrors() as $error) {
                    $compoundResult->addError($error);
                }
            }
        }
        return $compoundResult;
    }
@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Nov 4, 2019

There are two use cases for validation:

  1. Individual values.
  2. Data sets.

I think it would be enough for the ResultSet to get to the Rules and (or?) Rule classes. The Individual value could be treated as a data set. At this point, the handling of the variable will remain current, but errors could be applied to any attribute based on the dataset.

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 4, 2019

@Insolita that brings two possible use-cases into Rule class. If we're going to do that, Yii 2 concept fits a bit better.

@kamarton

The Individual value could be treated as a data set. At this point, the handling of the variable will remain current, but errors could be applied to any attribute based on the dataset.

Applying errors to any attribute is not the only thing that is missing. That's only point 3 from #28 (comment)

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Nov 4, 2019

Applying errors to any attribute is not the only thing that is missing. That's only point 3 from #28 (comment)

The return value must be changed to ResultSet. And the ResultSet must be modified so that all errors can be retrieved, with or without attributes. The Result class should be merged with ResultSet.

$resultset = (new Integer())->max(100)->validate(100);
$resultset->isValid();
$resultset->getErrors();

$resultset = (new Complex())->...->validate($dataset);
$resultset->isValid();
$resultset->getErrors();    // all erros
$resultset->getErrors($attribute);    // errors by attribute
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 4, 2019

@kamarton That's OK but how do you plan getting data set into Rule?

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Nov 5, 2019

@kamarton That's OK but how do you plan getting data set into Rule?

  • add Rule::isSupportDataSet():bool
  • modify Validator

public function validate(DataSetInterface $dataSet): ResultSet
{
$results = new ResultSet();
foreach ($this->attributeRules as $attribute => $rules)
{
$results->addResult($attribute, $rules->validate($dataSet->getValue($attribute)));
}
return $results;
}

With the Rules class, I don't know how. Special cases could be treated so that only classes from the Rules inheritance can use the dataset. This is also logical, because in this case, more than one rule applies.

class Address extends Rules
{
  public function isSupportDataSet() {    <---- maybe put it here ??
    return true;
  }
}

This solution implies that Validator::$attributeRules cannot have an attribute index. The attribute name must be moved one dimension deeper.

class Validator
{
/**
* @var Rules[]
*/
private $attributeRules;

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 18, 2019

After thinking more of it, I came to conclusion that it doesn't make sense to have validator that works for a single value. That is the job for assertions.

So having an interface like the one that was present in Yii 2 is the way to go.

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 20, 2019

A validator for a single value is basically assertion. If we can have both that would be good but I suspect that it may complicate library too much.

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Nov 24, 2019

A validator for a single value is basically assertion. If we can have both that would be good but I suspect that it may complicate library too much.

I think this is mandatory rather than optional. For example, if I make my own validator that uses additional validator, I will usually validate single values.

@samdark samdark changed the title Validator requires one attribute and cannot see the whole model. Validator requires one attribute and cannot see the whole model Nov 24, 2019
@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Nov 29, 2019

@samdark needed for the alpha version?

@samdark samdark added this to the 1.0.0-alpha1 milestone Nov 29, 2019
@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Nov 29, 2019

Yes.

@demonking

This comment has been minimized.

Copy link

@demonking demonking commented Dec 2, 2019

@samdark when will be the freeze for alpha?
Would like to help with the validators, so i can port the logic from yii2 ->yii3 in the next days

@samdark

This comment has been minimized.

Copy link
Member

@samdark samdark commented Dec 2, 2019

By the end of the month or around it.

@kamarton

This comment has been minimized.

Copy link
Contributor Author

@kamarton kamarton commented Dec 12, 2019

Also consider the scenario option when designing. #38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.