yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Validator enhancement #1447

Closed egorpromo closed 10 years ago

egorpromo commented 10 years ago

We can make validators more robust and universial. I have found that we have addError() method that needs 'error message' and 'error params'. These parameters can be obtain if we take new allocated method, for example validateItem(), which goal is to return 'error message' and 'error params'.

abstract class Validator extends Component
{
        public function validateItem($value)
        {
/*
In extended classes this method have to return array with
 elements 'message' and 'params'.
return [
    'message'=>'Error message',
    'params'=>[]
];
Returned elements are usefull to analysing errors of validation.
'message' and 'params' are for passing in addError() method.
Developer may use this method to validate any value without model.
*/
        }

        public function isValid($value)
        {
                $error = $this->validateItem($value);
                if ( empty($error) ) {
                    return true; 
                }
                return false;
        }
}

What will we have if we will use these two methods for any validator?

  1. Any value will can be validated without model via new validateItem() method and returned result will inform developers about problems with validation.
  2. Any value can be validated without model via isValid() method and returned result will only inform developers valid value or invalid, because it returnes true or false only.
  3. We reduce code duplication, because when making validation with model it will have to use validateItem() anyway.

So we need some refactoring to make all validators robust and universal.

My issue related to https://github.com/yiisoft/yii2/pull/1431

egorpromo commented 10 years ago

I can take refactoring on my own.

qiangxue commented 10 years ago

In which case do you need to validate values without models?

egorpromo commented 10 years ago

In many cases. For example I don't use forms with form models. It is simpler for me in some cases.

qiangxue commented 10 years ago

Can you show some concrete code example?

egorpromo commented 10 years ago

If I want to use html-form then I make:

<form method="post" enctype="multipart/form-data" action="">
<input type="file" name="myfile" />
<br/>
<input type="button" name="submit" value="Submit">
</form>

In action it is simpler for me don't use form models.

$file = UploadedFile::getInstanceByName('myfile');
$myFileValidator = new FileValidator();
$error = $myFileValidator->validateItem( $file );
if (empty($error)) {
/* I save my file in proper place */
}

I realy need a validation without models.

qiangxue commented 10 years ago

You are using controller to hold code that doesn't belong to controller. This is a bad practice, even though it seems simpler for simple cases. I don't think this is convincing argument for this issue.

slavcodev commented 10 years ago

@qiangxue The validators which validate value not models will be very helpful. First, in some case I prefer use model class without extends Model or AR. Second, validators can be used in filters, behaviors, etc (IP validator, Email validator) to reduce duplicate code.

slavcodev commented 10 years ago

Only suggestion the method must be static

if (EmailValidator::isValid($mail)) {
   // process mail
}
qiangxue commented 10 years ago

I'm not saying it's useless. As you can see, most validators do support validation without models (e.g. EmailValidator, DateValidator). However, FileValidator is tightly coupled with file upload validation. So I was asking for a convincing argument (this is also because the refactoring is not trivial which could affect the whole architecture of validator design.)

qiangxue commented 10 years ago

Just FYI, we already have Validator::validateValue() for this purpose. Some validators (like FileValidator) do not support this because they are not suitable for this.

egorpromo commented 10 years ago

All validators can support validation without model if new validateItem() as I propose will be invented. As I can see from the code of yii\validators\Validator the validation without model is by architecture (see validateValue() method). But after many years of heavy development the code begins rot and yii developers forget about big opportunities. I think if it is possible to realize for convenient using it must be realized.

qiangxue commented 10 years ago

@egorpromo The returned value proposed by you is very unusual because it requires certain postprocessing to make it meaningful and useful, which is not a good design.

egorpromo commented 10 years ago

@qiangxue If developers want easy way they can use isValid(). validateItem() can be used for wide range of unidirectional tasks.

egorpromo commented 10 years ago

Also validateItem() method is good approach to problem of validation without model.

egorpromo commented 10 years ago

And validateItem() is good approach to problem of code duplication.

egorpromo commented 10 years ago

@qiangxue Look at the yii\validators\StringValidator::validateValue() method and yii\validators\StringValidator::validateAttribute(). It is the duplication. Using new validateItem() method the problem will be eliminated.

egorpromo commented 10 years ago

@qiangxue Now look at validateValue() and validateAttribute() of yii\validators\DateValidator class :)

qiangxue commented 10 years ago

@egorpromo I'm aware of these code duplication from the very beginning. It all depends on the requirement for the validators. The current design is that when you use validators out of models (which is not recommended), you usually only care whether the data validates or not. Anyway, I will reconsider the design.

egorpromo commented 10 years ago

@qiangxue I can do some commits to help you :)

qiangxue commented 10 years ago

No, thanks. This requires very big change. It's more efficient I directly work on it.

slavcodev commented 10 years ago

@qiangxue Will be very good if you refactor design to decoupled validators from models. Because now for every model from collection created instance of validator, but need only one.

qiangxue commented 10 years ago

@slavcodev The current validator design already allows you to use it without models. This redesign is more about internal refactoring.

qiangxue commented 10 years ago

Done: https://github.com/yiisoft/yii2/commit/cb4a8c764c9f8332dbfbf0ef3afd9c8687d92e46