yiisoft / validator

Yii validator library
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
113 stars 40 forks source link

Decouple validators from model #8

Closed samdark closed 5 years ago

samdark commented 8 years ago

From #11883

I found the Validator is coupled with Model class, it is not easy to use Validators for other classes. For example, I want to validate request parameters in Controller which is irrelevant with any Models, is there any recommended way to do this?

And the rules() for Models could not be edit, adding a method addRules() will be fine.

klimov-paul commented 8 years ago

We have already created DynamicModel for this: https://github.com/yiisoft/yii2/blob/master/framework/base/DynamicModel.php

samdark commented 8 years ago

That's still a model... I'm talking about concept similar to https://github.com/go-ozzo/ozzo-validation

klimov-paul commented 8 years ago

Filter validators will be unable to function without a model. Scenarios will be unable to function without a model. And so on.

The only possible way is creating another layer for the validators as a condition checkers, however it does not make much sense. What a benefit of using special class just to check if some value is an integer or it matches some regular expression?

samdark commented 8 years ago

Filter validators will be unable to function without a model.

$data = [ ... ];
$validator = (new Validator())->trim('name')->string('name', 'maxLength' => 255);
$isValid = $validator->validate($data);
$errors = $validator->getErrors();
$filteredData = $validator->getData();
samdark commented 8 years ago

Scenarios will be unable to function without a model.

Yes. It doesn't make sense to have scenarios w/o a model.

samdark commented 8 years ago

What a benefit of using special class just to check if some value is an integer or it matches some regular expression?

  1. To get errors in a standardized way.
  2. There are more complicated checks than just integers or regexes.
klimov-paul commented 8 years ago

To get errors in a standardized way.

Error messages are parsed with the attribute label from model.

$filteredData = $validator->getData();

Looks twisted. Following this pattern there should be a setData() method. Also this will increase memory usage for the model validation since each validator will store an additional copy of validated atttribute value.

samdark commented 8 years ago

Error messages are parsed with the attribute label from model.

Could be set not to include field name by default i.e. Please fill the value. and provide ability to override message.

samdark commented 8 years ago

Looks twisted. Following this pattern there should be a setData() method.

Having getters w/o setters is OK.

samdark commented 8 years ago

Also this will increase memory usage for the model validation since each validator will store an additional copy of validated atttribute value.

Could be. Need to check it...

allyraza commented 8 years ago

the values are only loaded in the validator once the action is executed I don't think it will effect the memory footprint, +1 for decoupling the validators from the model layer

klimov-paul commented 8 years ago

What is exactly wrong with Validator::validate() method for you? It can be used without a model and check if particular value is valid.

samdark commented 8 years ago

Rather lengthy code to do it. Especially if multiple validation rules are involved.

rob006 commented 8 years ago

Why not create Validator::validateData($value, $rules = null) method which will internally create DynamicModel instance, set $value to value attribute, configure validation rules, run validation and return created model. No need to change current logic, no BC break and simple validation should be pretty easy and clean:

// multiple validation rules
$result = Validator::validateData($value, [
    ['trim'],
    ['string', 'maxLength' => 255],
]);
$isValid = !$result->hasErrors();
$filteredData = $result->getData();

// use specific validator with default config
$result = IpValidator::validateData($ip);

// use specific validator with custom config
$result = IpValidator::validateData($ip, ['ipv6' => false]);
nighon commented 8 years ago

@klimov-paul I would like to validate request parameters as a group, instead of one by one.

klimov-paul commented 8 years ago

If your request paramers are the solid group, why do not use model for them?

nighon commented 8 years ago

Some are not, irrelevant with any models.

klimov-paul commented 8 years ago

Some are not, irrelevant with any models.

What does this mean?

At first you say:

I would like to validate request parameters as a group, instead of one by one.

This means request parameters has some group meaning - they represent some data structure, which should match some format and thus validation conditions. This indicates this data structure is a part of some business process. So this data structure is a obvious candidate to be a model - may be a simple on, but a model.

Now you tell me this is irrelevant.

nighon commented 8 years ago

I mean request parameters sometimes irrelevant to models, only meaningful to controllers.

rob006 commented 8 years ago

I mean request parameters sometimes irrelevant to models, only meaningful to controllers.

Why not use DynamicModel then?

klimov-paul commented 8 years ago

I mean request parameters sometimes irrelevant to models, only meaningful to controllers.

Provide a example.

nighon commented 8 years ago

DynamicModel probably works.

@klimov-paul For example, a request like /user/forgot-password?email=somebody@email.com&verifycode=something, or /search?cityId=123&districtId=456&type=abc. I need to validate parameters in controller.

klimov-paul commented 8 years ago

/search?cityId=123&districtId=456&type=abc

In this case you definetely need a search model, just the same kind 'Gii Crud' generates.

/user/forgot-password?email=somebody@email.com&verifycode=something

Just the same - you need a special model for 'frogot password' just like out project templates provide: https://github.com/yiisoft/yii2-app-advanced/blob/master/frontend/models/PasswordResetRequestForm.php

Sounds like you simply too lazy to develop your project in the correct way.

nighon commented 8 years ago

Thanks for your guide. So on any kind of request, we need a corresponding model to use in controller?

klimov-paul commented 8 years ago

This is how MVC works.

ghost commented 8 years ago

+1 for decoupling validators from Models.

Consider the case when JSON encoded data is stored directly in a MySQL Text column! Model Rules, ActiveForm, etc, don't apply as JSON data can represent an unlimited number of values in various formats (numbers, etc). This is the case I'm working with now. http://www.yiiframework.com/forum/index.php/topic/71284-ad-hoc-data-validation-error-messages-are-not-displayed/page__p__300669__fromsearch__1#entry300669

Also, MySQL 5.7 has introduced JSON defined columns which can represent various data. Seems like this is a natural case for decoupling validators from models.

rob006 commented 8 years ago

Consider the case when JSON encoded data is stored directly in a MySQL Text column!

Again: why not use DynamicModel for that?

ghost commented 8 years ago

Yes, indeed! I did use DynamicModel. I spent a lot of time with it but in the end the validation errors generated by DynamicModel were NOT picked up by ActiveForm. I could see the validation error in the response

{"dynamicmodel-data":["Data must be no greater than 900."]}

but I couldn't get ActiveForm to do anything with it - no message displayed, no label highlighting, no input coloring. I wanted the same exact experience you get with normal validation against model rules. So something was missing, not working but it was beyond me to figure this out and I did ask for help in the forum.

Also, the rules for DynamicModel uses a slightly different syntax than model rules and I was not able to achieve the same ruleset in the DynamicModel as I could with the model rules.

samdark commented 8 years ago

Well, for active form to work you'll need a model anyway...

ghost commented 8 years ago

For the record, in this case, there is a model but it represents a text column in a database, not form data.

tom-- commented 8 years ago

@samdark What kind of structure(s) will hold the values you would validate in this way?

You excluded objects that are instances of model classes. Does that leave only scalars and array?

samdark commented 8 years ago

@tom-- usually arrays.

ghost commented 8 years ago

@rob006 Update: DynamicModel worked perfectly...

dynasource commented 8 years ago

@samdark has a point here.

the question is, do we always need inheritance for this or could this problem be solved by composition. Validation can be seen as so generic, why should we always have to validate a 'model' instead of validate against an interface.

samdark commented 8 years ago

What do you mean by validating against interface? The model is an interface in this case, isn't it?

dynasource commented 8 years ago

correct. In a perfect world, every object should be able to validate other objects.

samdark commented 8 years ago

I don't get what you're trying to suggest.

dynasource commented 8 years ago

Of course this is just brainstom. Im pointing to a more abstract approach in which perhabs a concrete implementation like a Model is not required. As validation is about 'validating something' you could think of a generic validation approach in which everything can validate everything. Its purely theoretical.

tom-- commented 7 years ago

New today in PHP 7.1 a neat solution

SilverFire commented 7 years ago

Unfortunately, minimum requirement of PHP 7.1 for Yii2 is not acceptable in observable future.

brandonkelly commented 7 years ago

We have a use case for this in Craft CMS: In Craft 3, Craft plugins are implemented as Modules. Plugins can have settings which must be validated, but it’s not currently possible for a Module to have settings if you need validation. So instead of handling the settings directly on the plugin’s Module class, plugins must also supply a Model that holds/validates the settings. This is a departure from everything else in Craft that has settings, as all other savable component types were able to extend yii\base\Model.

We could possibly use DynamicModel to help us with this, but I’ve heard Yii is going to be favoring composition over inheritance going forward, and considering that, it seems like it would be nicer if any class could decide to be validatable by including some ValidationTrait or ValidationBehavior.

samdark commented 7 years ago

@brandonkelly your idea is quite different from what was originally discussed in the issue...

brandonkelly commented 7 years ago

@samdark how so? Original post lists validating request parameters from a controller as a use case. General idea of being able to validate things other than model properties would equally benefit my use case.

I may have gotten a little carried away with the ValidationTrait/ValidationBehavior suggestion, but I think I understand the general idea, implementation aside.

samdark commented 7 years ago

Yes. Traits/behaviors are what I'm talking about.

samdark commented 5 years ago

This is now achieved with DataSetInterface.