Make Validator methods protected

I needed to create a new Validator, and the new validator just needed slightly a modified validateAttribute() method in the UniqueValidator. However, I found I could not extend UniqueValidator because the methods in it are all private.

Will it be OK if I create a pull request that changes the UniqueValidator methods to protected? This will allow a Yii2 developer to extend UniqueValidator to create a new validator of his own, just as you would extend from Validator at the moment. It would probably be a good idea if, at the same time, I change the other validators, because the guide implies that it is possible to extend any child of Validator, not just Validator itself. The guide says this about creating a standalone validator:

A standalone validator is a class extending yii\validators\Validator or its child class. You may implement its validation logic by overriding the yii\validators\Validator::validateAttribute() method. However, this is not true because it is not possible, due to the private methods, to extend any validator apart from Validator itself.

The problem with the way things stand at the moment is that if you need only a minor change to an existing validator, you cannot do it - the only way is to copy the entire validator into your own codebase, then change it there. This is not a good way, because if, later on, a change is made to the Yii2 validator, your local validator will not get the benefit of that fix.

UniqueValidator::validateAttribute() is public, you're free to override it. Can you specify what exactly you're trying to do and which method needs to be protected? Making everything protected is very impractical from BC perspective.

Thanks for posting in our issue tracker. In order to properly assist you, we need additional information:

When does the issue occur?
What do you see?
What was the expected result?
Can you supply us with a stacktrace? (optional)
Do you have exact code to reproduce it? Maybe a PHPUnit tests that fails? (optional)

Thanks!

This is an automated comment, triggered by adding the label status:need more info.

UniqueValidator::validateAttribute() is public, you're free to override it. Can you specify what exactly you're trying to do and which method needs to be protected? Making everything protected is very impractical from BC perspective.

Actually, my view is making everything private is impractical from the viewpoint of a development framework.

While UniqueValidator::validateAttribute() is public, nothing else is. My need is not to have a new validator completely different from UniqueValidator, but rather to have a version of it that behaves slightly differently. This will be the case most times when you extend a class and that is what OOP is all about.

So, my need is basically to have all the same code as validateAttribute() but with a small amount of code added. OOP principles mean that you now have three options:

If the code you are adding is before all the existing code in a method, you extend the method by doing your code, then calling the base class's method.
- If the code you are adding comes after all the existing code, you call the base method, then do your code.
- If the code you are adding is in between the existing code, you modify the code, but probably can't call the base class's method. However, even in this case, the code is still calling other methods in the base class.

My case falls into the 3rd category. It's all the same code, with only 3 lines of code added in the middle. However, the code cannot run as is because the first line in the code calls $targetClass = $this->getTargetClass($model); which can't work because it is private. So I now have no choice but to copy the getTargetClass method into my derived class, only to find it now fails on the call to prepareConditions. Long story short: I have to copy the entire class, I cannot extend it.

When creating an open-source framework, the most appropriate to use is protected to allow extension by sub-classing without allowing access from the outside. Public is used to define the public API - those functions accessible from the outside, and which form the functional identity of the class. Private is used by closed-source developers who want to hide their trade secrets, and typically they would create the private function and another public function that would allow users of the class to use the class without knowing how the class does its work.

Using protected rather than private is especially important in the case of a framework like Yii, because of the number of times that the framework uses advanced coding techniques like reflection and injection, which are not well understood by users of the framework. These techniques, though, are harder to use and increase the likelihood of errors that create security flaws. This, in turn, makes extending a class far better than copying it, because when a security problem is fixed, an extended class gets the benefit of the change, but a copied class is likely to continue to be used with the problem because the developer does not know.

Extending a class also comes with a risk that the user code breaks when the framework is updated. However, this is usually what you want to happen, because something this tells you that something has changed that directly affects that piece of code, such as the framework or the user-code injecting data without checking boundaries.

@RichardPillay OK but would you please describe what you do want to achieve with overriding unique validator? What behavior?

@RichardPillay OK but would you please describe what you do want to achieve with overriding unique validator? What behavior?

You probably remember how I tried to add a lowercase option to UniqueValidator in Pull Request #17776. In the end, we decided to abandon it. We decided to abandon it not because it wasn't useful, but because we couldn't figure out how to do it in a way that worked on all DBs that Yii2 supports without causing performance or portability issues. In my case, my testing showed no performance impact - which was due to several features of Postgres. However, there would have been a performance impact on other systems. Also, it made sense to do the same thing for ExistValidator.

That need still exists for me. Trying to do it within the framework was because I felt it would benefit other users of Yii2 as well. Now that I am doing it by keeping my code in user-space, I again feel these changes - making the framework methods protected - would benefit Yii2 and make it easier for everyone to use. Remember, there is no need for anyone to use a framework to produce a software system. The reason using a framework makes good sense is because everyone that needs to produce a software system finds that they are spending the majority of their time doing the "background" stuff that everybody needs, and that is why a framework is so useful. However, this is exactly why the private vs protected issue is so important - because the framework should contain what everybody needs, but do it in a manner that lets it be extended easily to do those things that only a few people need. If it's not done this way, everybody has to re-invent the wheel every time they want to do something different, or they have to copy large chunks of framework code into userspace.

As to what I specifically want to achieve, it is this. The original code from UniqueValidator::validateAttribute() is just slightly modified as below. If with protected methods, I only have this one method in my derived class. With private methods, I have to copy the entire class, else the calls to getTargetClass, prepareConditions and several others all fail.


    public function validateAttribute($model, $attribute)
    {
        /* @var $targetClass ActiveRecordInterface */
        $targetClass = $this->getTargetClass($model);
        $targetAttribute = $this->targetAttribute === null ? $attribute : $this->targetAttribute;
        $rawConditions = $this->prepareConditions($targetAttribute, $model, $attribute);
        $conditions = [$this->targetAttributeJunction === 'or' ? 'or' : 'and'];

        foreach ($rawConditions as $key => $value) {
            if (is_array($value)) {
                $this->addError($model, $attribute, Yii::t('yii', '{attribute} is invalid.'));
                return;
            }

**Only this has been added**
           if ($this->ignoreCase)
            {

                $newKey         = "lower(" . $key . ")";
                $newValue       = "lower('" . $value . "')";
                $conditions[] = $newKey . " = " . $newValue;
            }
            else

**Everything else is the original code**
                $conditions[] = [$key => $value];
        }

        $db = $targetClass::getDb();

        $modelExists = false;

        if ($this->forceMasterDb && method_exists($db, 'useMaster')) {
            $db->useMaster(function () use ($targetClass, $conditions, $model, &$modelExists) {
                $modelExists = $this->modelExists($targetClass, $conditions, $model);
            });
        } else {
            $modelExists = $this->modelExists($targetClass, $conditions, $model);
        }

        if ($modelExists) {
            if (is_array($targetAttribute) && count($targetAttribute) > 1) {
                $this->addComboNotUniqueError($model, $attribute);
            } else {
                $this->addError($model, $attribute, $this->message);
            }
        }
    }

Would making prepareConditions() protected be enough?

I'll do a test this evening when I get back home, but why do think it would allow access to the other methods?

The code in validateAttribute calls quite a few methods:

getTargetClass
prepareConditions
modelExists
addComboNotUniqueError

This is just quickly looking at the code on my phone, so I may have missed others.

The key thing, though, is not just to fix my particular problem now, but to make it usable for everyone else that wants to extend a class. If you do a search via Google, you will see that this has been an issue for many other people as well. The issue keeps coming up repeatedly, but what changes each time is the class name.

Yes, that's because of Yii 2 inheritance-based design. We've switched to mostly composition for Yii 3. The problem with inheritance is what @rob006 tried to explain. Too many uses you haven't accounted for and protected methods you have to keep backwards compatible. Thus, hard to move framework forward overall without breaking something...

@RichardPillay You may want to read http://fabien.potencier.org/pragmatism-over-theory-protected-vs-private.html and many similar articles. Protected methods are part of the class interface (it is not public, but still), and interfaces should be carefully designed - making everything protected increases framework complexity and promotes hacking random part of framework in unpredictable way.

@RichardPillay You may want to read http://fabien.potencier.org/pragmatism-over-theory-protected-vs-private.html and many similar articles. Protected methods are part of the class interface (it is not public, but still), and interfaces should be carefully designed

Yes, you are absolutely correct in what you say about designing interfaces well. However, I don't think what you are saying applies to this situation. I understand that your concern is that making these changes will cause a problem in the future where some change is made to these classes and that will cause user-code to break. I don't believe it will - I think your concern comes about because you are confusing different things with each other: I think you may be mixing up User and Developer on one hand, and you may be confusing the role of "public API" with OO encapsulation/inheritance on the other.

I hope you will forgive me for getting long-winded here. The only way I can think of to explain why I don't believe what you are worried about is not something you should be worried about is to explain what I mean by these two sets of terms, and how they will be affected by the changes.

Before anything else, let me tell you what I mean by a user and a developer:

The Yii2 user is someone who uses Yii2 to write his own application, and doesn't go into how the framework works. So a user writes the code which uses the Yii2 framework. However, the code written by a user uses the Yii2 framework only through the public API. This is very important to understand.
Yii2 extension and Yii2 modification is performed by a Yii2 developer and by this I mean a person who actually wants to change how the framework works.
Modification is when a developer makes changes to code in vendor/yii2soft. This code will often be submitted back to the project via a change request. A modification affects the Yii2 framework directly, and if these changes are not fed back to the project, the next update will most likely wipe out the changes.
Extension is about adding to the framework. It is usually done by creating a subclass of a class in the framework, such as I want to do now - I want to create a UniqueEmailValidator class which is based on UniqueValidator. These extensions may be submitted back to the framework, but they don't have to be. These extensions live out of the yiisoft folder and will not be wiped out by an update. However, it may be affected by an update in such a way that it may have to be modified after the update.

These two may be the same person at different times but functionally they are completely different people. This is because the user and developer have very different expectations from the system, and part of your hesitation and concern about the changing private to protected comes from the fact that you are confusing the two.

Now the other point:

The public API is the way a user can use your classes, your system or your framework using the published API. In order to do this, you (as a user) do not need to know how the class or the system works. All you need to know is what it takes as input, and what it will produce as output. This is expected to remain relatively unchanged over the life of the system. Some people call this a black-box system, and others refer to a design by contract. If during the life of the system, a bug, security issue or any flaw is discovered, the system can be fixed, but in spite of this, nothing will change for the user. So the public API refers to the published method of using the system. It does not refer to how the API is implemented. Protected methods are not part of the API, since a user cannot access a class via a protected method - only a developer can do this. So by way of example, take the API documentation for UniqueValidator::targetAttribute. This says that the class will do a certain thing for you. As a user, I can expect that when ver 2.0.32 is updated and I now have 2.0.33, this will still do exactly the same thing. As a user, I do not have to know what has changed from one version to the next. As a user, I will not be aware that these methods have changed from private to protected. As a user, I will not even be aware that these methods exist.

Now onto the extension of OO classes, and something OO principles are very clear on: extending a class is done by a Developer, not a User. It is here that I think you are confused as to the benefits of private vs protected methods. I believe that when you are saying protected methods are part of the class interface, what you are really worried about is that protected methods will result in loss of encapsulation. As I said before, the public interface is what is visible from outside the class. Encapsulation, on the other hand, is something different - it is an OO principle that can prevent incorrect access to the internal parts of the class by hiding them from the outside. Changing a class's methods from private to protected will not result in loss of encapsulation. However, in the case of UniqueValidator, it is pointless worrying about encapsulation as there is no encapsulation at all - all the attributes of the class are public and are accessible from the outside anyway. I think you are also worried that changing them to protected will mean that if these classes are modified in the future, it will break user code. It will not. It may break developer code, but that is to be expected: When extending a class as a developer, I am not working with a black box, or just using a public API. A developer has to know the internal working of a class, and he cannot expect that the class will remain unchanged or continue to work in the same manner. A developer that extends a class in ver 2.0.32 cannot assume that the base class will remain unchanged in 2.0.33, and if it changes, he has to deal with the consequences. If the methods in a class are private, this prevents extension completely because no subclass can access the base-class methods without rewriting them. If the methods are protected, you allow extension, but you still keep encapsulation (well, not in this case, but that is an OO principle). This is very important to understand - protected methods do not allow access to methods from unrelated classes, just from subclasses. However, subclasses must understand the structure of their base classes. If these methods are changed from private to protected, the ONLY difference will be that a developer can extend the classes without making a copy of it first. Kept private, these classes have to be copied out of the yiisoft folder and then either modified directly or changed to protected and then extended.

@RichardPillay Sorry, but your post doesn't make sense at all. There is no difference between "user" and "developer", there are only developers. As long as class is not final, everyone can extend it and gain access to protected methods and properties (that is why all protected methods and properties are listed in API documentation), even IDE will suggest you protected methods and properties. And you can't even build Yii 2 app without extending Controller, Component or Model, so inheritance is natural. And even if you decide to use Yii 2 in "user way" and use only public methods, you can still install extensions build by "developer", which uses also protected methods. If upgrade to Yii 2.0.33 will change these protected methods, this change will also affect your app, and being an "user" will not protect you from BC break. That is why BC promise for protected methods is important - if every upgrade can break some extensions you're using, then people will just stop upgrading. And this is far worse that "my copy-pasted code will not get automatic patches from upstream" (which BTW makes your class more stable, which is good), because lack of improvements and bugfixes affects all classes, not only this one with copy-pasted code.

And as a bonus - conclusion from Fabien's article was that private methods often protects you from hurting yourself by using class in wrong way. In your example from https://github.com/yiisoft/yii2/issues/17832#issuecomment-578991119 you just introduces SQL Injection to your version of this validator. The fact that you can't easily extend this validator in a way you want, protects you from creating vulnerability in your app. Using approach from https://github.com/yiisoft/yii2/issues/9014#issuecomment-118685826 will be more safe, and it probably could be generalized in validator by overwriting validateAttribute() only:

public function validateAttribute($model, $attribute) {
    $oldValue = $model->$attribute;
    $model->$attribute = strtolower($model->$attribute);
    $this->targetAttribute[$attribute] = "lower($attribute)";

    parent::validateAttribute($model, $attribute);

    $model->$attribute = $oldValue;
}

You can also use $filter property to set additional conditions to query. So your use case does not really need changes in framework, just a little creativity.

@RichardPillay Sorry, but your post doesn't make sense at all. There is no difference between "user" and "developer", there are only developers.

Yes, that part is a little harder to explain. What I mean when I say user is when you are developing your own code, and not modifying or extending the Yii classes. And when I'm talking about developer, then I mean someone who is changing the Yii classes - you, or @samdark . When you think of me, using the Yii2 api to create my own website, at that point I am a user (only using the framework from outside). But when I want to tinker under the hood of Yii2 and change private to protected, at that point, I am a developer because I am changing the framework itself.

As long as class is not final, everyone can extend it and gain access to protected methods and properties (that is why all protected methods and properties are listed in [API documentation]

Ah, yes, but don't you see, this is exactly what I am talking about? You are saying exactly what I have been saying - everyone can extend it and gain access to protected methods and properties and this is why I want to do change the methods to protected because they are currently private and cannot be accessed by a subclass. Have a look at the code

And as a bonus - conclusion from Fabien's article was that private methods often protects you from hurting yourself by using class in wrong way. In your example from [#17832 (comment)] ... public function validateAttribute($model, $attribute) { $oldValue = $model->$attribute; $model->$attribute = strtolower($model->$attribute);

It's interesting that we have different views on this. Where you are seeing being able to write to the attribute from outside the class as a good thing, I see it as bad thing. OO design teaches that you should not access a class internal attributes from outside, you should go via public getter and setter methods, so that those functions could perform security and error checking. Just imagine for a moment that somebody raises an issue to say there is some kind of database exploit due to some unescaped sql. You fix that by changing the the modelExists method. If that method were protected and I were using it in my subclass, I will get the benefit of the fix. However, that method is private, so doing it the way you suggest, writing directly to the attributes without checking my input, my code will still be vulnerable.

Re-evaluated. Decided to stay with current code because what @rob006 suggested will work.

yiisoft / yii2

Make Validator methods protected #17832