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

Update method rules() in next versions. #1

Closed
Auramel opened this issue Apr 2, 2018 · 58 comments
Closed

Update method rules() in next versions. #1

Auramel opened this issue Apr 2, 2018 · 58 comments
Assignees

Comments

@Auramel
Copy link

Auramel commented Apr 2, 2018

Hello!
So, I offer to change syntax in \yii\base\Model::rules()

For example, now we have

public function rules()
{
	return [
		[['login', 'pass'], 'required']
	];
}

I offer try to code something else, for example:

public function rules()
{
    return [
        $this->login => Validator()->required()->string()->min(255)->max(1000)
    ];
}

I like https://github.com/Respect/Validation approach. What do you think?
UPD: I don't know why tabs and spaces are not working...
UPD2: Someone fixed it. Thank you 👍

Russian: хотелось бы писать правила, как в ссылке выше. :)
UPD: Хз почему все переносы и табы слетели... Писал в Notepad++...

UPD2: Кто-то пофиксил. Спасибо 👍

@samdark
Copy link
Member

samdark commented Apr 2, 2018

That's quite similar to schema builder used in migrations. I like the idea.

@samdark
Copy link
Member

samdark commented Apr 2, 2018

Would you like to implement it?

@mgrechanik
Copy link

What is login in $this->login ? New object variable needed to be defined?
How would it look like with two or more identical attributes per rule?

@Auramel
Copy link
Author

Auramel commented Apr 3, 2018

@mgrechanik
So, $this->login it's a SomeModel::$login property. When you call method validate(), you use current values of your class.
So, it depends, how it will be implemented.
For example:

class SomeModel
{
    public $property1;
    
    public $property2;
    
    # variant #1
    public function rules()
    {
        return [
            $this->property1 => Validator::required()->string()->min(6)->max(255),
            $this->property2 => Validator::required()->string()->min(6)->max(255)
        ];
    }
    
    # variant #2
    public function rules()
    {
        [$this->property1, $this->property2] => Validator::required()->string()->min(6)->max(255)
    }
}

@Auramel
Copy link
Author

Auramel commented Apr 3, 2018

@samdark
If you ready to see my shit-code - no problem :)

@samdark
Copy link
Member

samdark commented Apr 3, 2018

I am.

@bizley
Copy link
Member

bizley commented Apr 3, 2018

I like the idea but don't you think about something like:

public function rules()
{
    return [
        'property1' => Validator::required()->string()->min(6)->max(255),
        'property2' => Validator::required()->string()->min(6)->max(255),
    ];
}

@Auramel
Copy link
Author

Auramel commented Apr 3, 2018

@bizley can be and so )

@ddinchev
Copy link

ddinchev commented Apr 3, 2018

I do like the idea to use builder pattern to configure validators - the current strings array is error prone and devs have to always check documentation of the correct validator shortcut (was it exist or exists, was it integer or number validator).

However, I wouldn't like to have to define same rule for multiple fields like this:

    # variant #1
    public function rules()
    {
        return [
            $this->property1 => Validator::required()->string()->min(6)->max(255),
            $this->property2 => Validator::required()->string()->min(6)->max(255)
        ];
    }

And it's a bit hard to imagine how the implementation of Variant #2 would look like, besides the fact that this seems like a confusing way to combine required and string validator.

In the simplest form, we could just preserve the field definition and opt for builder pattern for the validators. Eg:

public function rules()
{
    return [
        [['property1', 'property2'], Validator::required()->build()],
        ['property2' => Validator::string()->min(6)->max(255)->build()],
    ];
}

With this, we could allow combining validators for a field in a more obvious manner:

public function rules()
{
    return [
        ['property2' => Validator::composite()
            ->add(Validator::required())
            ->add(Validator::string()->min(6)->max(255)->when(function (SomeModel $model) {
                return $model->property3 === "awesome";
            }))
            ->build()
        ],
    ];
}

What do you think?

@Auramel
Copy link
Author

Auramel commented Apr 3, 2018

@ddinchev oh shit, it's crazy. Cool. It's will be difficult to write something this (for me). And has a one question - what do we do with previous versions? More projects use current rules. I think it not good to crash them, when they updated. I think we can save current syntax and add something new. Maybe, new method? Or add some 100+ if() to code ?)
coreTeam, what do you think?

UPD: for difficult conditions, I think, we can use custom class that will be extends like a \yii\validators\Validator

@ddinchev
Copy link

ddinchev commented Apr 3, 2018

Well, Yii 2.1 is going to have many breaking changes, this is up to @samdark and the team behind Yii2 to decide. We could have detection if the new pattern is used or the old definition, and let the framework handle both, and maybe remove the old syntax in v2.2.

@Auramel
Copy link
Author

Auramel commented Apr 3, 2018

@ddinchev and it will we awesome decision!

@rob006
Copy link

rob006 commented Apr 3, 2018

It looks like overkill to me. You're trying to create sophisticated syntax to configure object (validator). Why not create object directly?

public function rules()
{
	return [
		[['login', 'pass'], new StringValidator(['min' => 0, 'max' => 100])],
		// with fluent setters
		[['login', 'pass'], (new StringValidator())->min(6)->max(255)],
	];
}

It should be more intuitive, more flexible (works with custom validators), easier to implement and does not conflict with current syntax, so we could keep BC and support both syntax without much effort.

@Auramel
Copy link
Author

Auramel commented Apr 3, 2018

@rob006 not a bad thought 👍
But we will have to rewrite all *Validator classes... It's difficult, no?

@ddinchev
Copy link

ddinchev commented Apr 3, 2018

@rob006 it's maybe confusing to me because right now RequiredValidator class does not have min and max.

If that is just a copy/paste error and you don't intend to combine string and required validators -I don't see much of a difference between (new StringValidator())->min(6)->max(255) and Validator::string()->min(6)->max(255) - I'd be happy with both!

@Auramel
Copy link
Author

Auramel commented Apr 3, 2018

I have an idea to add some ValidatorHelper || ValidationBuilder that replaced your builder syntax to current syntax. Good? Bad?

How example:

<?php

class ExperementalValidator 
{
    public static function required()
    {
        return 'required';
    }
    
    public static function min(int $min)
    {
        return ['min' => $min];
    }
}

@rob006
Copy link

rob006 commented Apr 3, 2018

@ddinchev I fixed my example - it was dump copy-paste :)

@Auramel
Copy link
Author

Auramel commented Apr 4, 2018

Hey, guys, I made some wrapper on current rules. What do you think? https://github.com/Auramel/yii2-validation-rules.

See ValidationRule.php PHPDoc. Example in it.

It has a new syntax and not crash old. Perfect?

UPD: Has idea to call method build() in \yii\base\Model::validate() method. It cut some letters from your code 🤣

@samdark samdark transferred this issue from yiisoft/yii2 Nov 19, 2018
@viktorprogger
Copy link
Contributor

@samdark will this be implemented? And in which form?

I like this variant the most.

@samdark
Copy link
Member

samdark commented Nov 23, 2018

Me too.

viktorprogger referenced this issue in viktorprogger/yii-core Nov 30, 2018
@dicrtarasov
Copy link

Will this chained calls slower then simple array of rules data? Even if a lot of objects?

@samdark
Copy link
Member

samdark commented Dec 3, 2018

Should not.

@viktorprogger
Copy link
Contributor

I can make simple tests to show performance of both the variants. But first we need to finish our discussion about the final syntax in #108.

@rob006
Copy link

rob006 commented Dec 4, 2018

AFAIK validator object will be created from array anyway, so there should be no difference in performance. Especially on PHP7.

@viktorprogger
Copy link
Contributor

I think it's a good idea to make a separate module for this functionality, e.g. yiisoft/validation and require it in yii-core. It will have a RequireValidation interface (requires getValidationAttribute, rules and validate methods) and a CanBeValidated trait (with default implementation of validate method).

Minuses:

  • User should manually call validate method (only if we wan't require this module in yii-core)

Pluses:

  • We will have an equal validation way for everything: models, controllers, lots of middleware, etc.

WDYT?

I think I'll make some free time for this in a month.

@samdark
Copy link
Member

samdark commented Feb 22, 2019

I agree. Validation could be separate library.

@samdark
Copy link
Member

samdark commented Feb 22, 2019

@miolae let's design it first though before implementing it. Would you please make a draft document explaining concepts about how to use it, where to use it, what are dependencies etc.?

@viktorprogger
Copy link
Contributor

Yes, I'll do this, but after some higher priority things are completed. I'm going to show this draft in late march or april.

@rob006
Copy link

rob006 commented Mar 5, 2019

@viktorprogger "Validation" of request and permissions is middlewares job, not validators. Decoupling validators from models may be good move, but using it in controller in this way is terrible example.

@viktorprogger
Copy link
Contributor

It's reasonable

@viktorprogger
Copy link
Contributor

I'll make another example later :)

@viktorprogger
Copy link
Contributor

Here is a new draft of the library itself. It's not finished yet, it's just to show the way I see it. Feel free to ask anything or to give an advise.

@viktorprogger
Copy link
Contributor

viktorprogger commented Mar 28, 2019

@miolae let's design it first though before implementing it. Would you please make a draft document explaining concepts about how to use it, where to use it, what are dependencies etc.?

I've added a readme file with concept describing and usage examples. If anybody needs it to be complemented - feel free to say me what to write there.

I've found a couple of bad places in this draft during creating the readme.

  1. A bit earlier (here and here) we decided to add ability to pass instantiated and configured validators. On the other hand is another decision: @samdark said (can't find it now) objects mustn't know how they are configured, DI container must achieve that. So, validation rules in my implementation can't be configured through their __constructor. And now cool-looking variant [['login', 'pass'], new StringValidator(['min' => 0, 'max' => 100])] turns into [['login', 'pass'], $this->validator->getFactory()->get(StringValidator::class, ['min' => 0, 'max' => 100])]. I don't see an ideal way to get out of the problem. What can you suggest about this?
  2. The only dependency I used is yiisoft/di to use \yii\di\Factory::create() method. I think it's an overhead to use the whole library to use just a single method but I didn't find a better way. Please make suggestions how to exclude this overhead.
  3. [known issue] The result of validation and error existence in a rule are not coupled. You can return false during validation and it doesn't mean there will be an error in a rule instance. The reverse is true too.

@dicrtarasov
Copy link

To implement the rules of validation of business models with a large number of fields, the old syntax is much shorter, efficient and self-desciptive:

            ['views', 'trim'],
            ['views', 'default', 'value' => 0],
            ['views', 'integer', 'min' => 0, 'max' => 65535],
            ['views', 'filter', 'filter' => 'intval'],

then

new StringValidator(['min' => 0, 'max' => 100])]
$this->validator->getFactory()->get(StringValidator::class, ['min' => 0, 'max' => 100])]

@viktorprogger
Copy link
Contributor

The old syntax is still possible, I've kept it.

@viktorprogger
Copy link
Contributor

About option №1: I'm inclined to opinion to add magic configuration through constructor to the BaseRule, but not to the RuleInterface.

@samdark
Copy link
Member

samdark commented Mar 28, 2019

Interface should not care about configuration.

@rob006
Copy link

rob006 commented Mar 28, 2019

2. The only dependency I used is yiisoft/di to use \yii\di\Factory::create() method. I think it's an overhead to use the whole library to use just a single method but I didn't find a better way. Please make suggestions how to exclude this overhead.

IMO requiring library for this is not a problem, but using (new Factory())->create($config) directly is design smell, and really ugly solution. I don't really understand purpose of RuleFactory. Why not create validators instances directly in rules()?

@viktorprogger
Copy link
Contributor

I don't really understand purpose of RuleFactory. Why not create validators instances directly in rules()?

It's decoupling of classes and dividing responsibility. Validator shouldn't care about how to create Rule instances, it's just adapter between Validatable and Rules. It's not about validating provided rule type and selecting a right instantiation way for that type. Its responsibility is to collect some information

  • from Validatable to provide it to rules (before validation)
  • from rules to provide it to Validatable (after validation)

Maybe it's a naming problem. If you have an idea about more clear naming - lets discuss it.

@rob006
Copy link

rob006 commented Mar 29, 2019

It's decoupling of classes and dividing responsibility.

Do we need it if we require to create ValidationRule object inside of rules()?

return [
    ['name', new RequireValidator()],
    ['email', new EmailValiadtor()],
];

@viktorprogger
Copy link
Contributor

We allow to do so, not require. The preferred way is to create rules through DI container either using their string aliases (e.g. 'require' and 'email'), or instantiating them with DI container like in the last example in this comment.

@rob006
Copy link

rob006 commented Mar 29, 2019

The preferred way is to create rules through DI container either using their string aliases (e.g. 'require' and 'email'), or instantiating them with DI container like in the last example in this comment.

Why? If AR relies on specific implementation of validator, then changing it by DI container will change how AR works. IMO DI should not change business logic in this way.

Besides, you didn't answer my question. :P

@viktorprogger
Copy link
Contributor

viktorprogger commented Mar 29, 2019

Why? If AR relies on specific implementation of validator, then changing it by DI container will change how AR works. IMO DI should not change business logic in this way.

If smthng relies on specific implementation, it must use this specific implementation, not some contract through DI. But if you has 10-20 places of email validation in your project (by standard yii validator), and suddenly a business task to change this validation appeared, it could be cool just to set your own email validator implementation through di (RuleFactory in this case). Or set some new rules to save uniformity of rule declarations (e.g. ['attribute', 'my-validation-rule']).

The factory is really useless in case when validation rule instances are created in-place. It's created to cover all other cases.

Besides, you didn't answer my question. :P

I answered as I see)) Could you please repeat it in other words?

@rob006
Copy link

rob006 commented Mar 29, 2019

If smthng relies on specific implementation, it must use this specific implementation, not some contract through DI.

That is my point - validation is internal logic of model, you should not rely on DI here.

But if you has 10-20 places of email validation in your project (by standard yii validator), and suddenly a business task to change this validation appeared, it could be cool just to set your own email validator implementation through di (RuleFactory in this case). Or set some new rules to save uniformity of rule declarations (e.g. ['attribute', 'my-validation-rule']).

And IMO this is great example why using DI for this this is dangerous. You're changing internal logic of models by configuring application, and you're doing it for all models and in implicit way. You don't even know which models may be affected (in some cases it may be undesired), and this is completely invisible from model perspective. It is convenient way to shoot yourself in the foot - creating custom validator (or some global config param) and using it in 20 places will take like ~15 minutes and may save you hours of debugging.

I answered as I see)) Could you please repeat it in other words?

I asked "do we need factory if we require instances from rules()?". You answered "we don't require". I did not asked "do we require?" but "what if we require?".

@viktorprogger
Copy link
Contributor

IMO it is a good practice to allow user to use DI when he needs it and not to use DI when he does't need it. It's users' responsibility to choose if he need to use one way or another. IMHO if somebody uses DI he understands why does he use it.

Another point is that this library will not be used only with AR now. It could be used in any place where you need to have some data checking logic. Middleware, object factories, anywhere. So it is not just "internal logic of models" anymore. Another use cases are coming.

I asked "do we need factory if we require instances from rules()?". You answered "we don't require". I did not asked "do we require?" but "what if we require?".

No, we don't need factory if requiring instances from rules() is the only way of creating those instances.

@viktorprogger
Copy link
Contributor

It could be great if we hear more opinions from YiiSoft team.

@machour
Copy link
Member

machour commented Mar 29, 2019

@viktorprogger overall, this looks to be heading in the good direction, but without more visible use cases to see, I tend to agree with what @rob006 said.

Yii 3 brings DI closer to the developer, and some will want to use it even if they are not experts and do not fully grasp the consequences of their acts.

In real life, if I need to change unique validation to work in a certain way, I would explicitly use my validator in my code, where I need it.

Allowing to replace a validator through configuration may have a lot of unexpected consequences (i.e., core depend on unique validation to work in a certain way, I change it to another way because of my app requirements => trouble).

@rob006
Copy link

rob006 commented Mar 29, 2019

IMHO if somebody uses DI he understands why does he use it.

IMO it is exactly opposite - people does not understand how DI works and what are the consequences of it. Even in your implementation there is fundamental flaw which ruins 2 main goals of DI - implicit dependencies and lack of global state (Factory will crate dependencies from nothing using some global static container).

Also note that if you want, nobody will forbid you to use DI for creating rules - you can still do this in rules() directly. It will work the same, except that it will be explicit and framework will not promote such approach.

@kids-return
Copy link

kids-return commented Mar 29, 2019

my point of view.

  • 3.0 Maintain 2.0 rules
  • 3.X Add new rules
  • 4.0 Remove 2.0 rules

should focus on the yii3 base build.
This will allow for faster release.
I have been trying to run the demo for a year and now I can't run it.

@kids-return
Copy link

Should I use 2.X for new projects?
Refactored above after the release of 3.0

@machour
Copy link
Member

machour commented Mar 29, 2019

@kids-return let's keep this conversation about the new rules implementation please. And yes, you should use 2.x for new projects, as we're all doing currently, I'd hate to see 3.x rushed just for the sake of being published.

@viktorprogger
Copy link
Contributor

Also note that if you want, nobody will forbid you to use DI for creating rules - you can still do this in rules() directly. It will work the same, except that it will be explicit and framework will not promote such approach.

This is a good argument. I don't know why didn't I think about it before. I'm going to change this, but I can't give any dates, because I've changed my employer this week, and I have to join the work ASAP.

@samdark
Copy link
Member

samdark commented Apr 10, 2019

No worries. What you did is a great starting point anyway.

@samdark samdark transferred this issue from yiisoft/yii-core Apr 18, 2019
@samdark samdark self-assigned this May 15, 2019
@samdark
Copy link
Member

samdark commented Dec 26, 2019

This one is now implemented.

@samdark samdark closed this as completed Dec 26, 2019
@what-the-diff what-the-diff bot mentioned this issue Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants