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

InlineValidator now binds $method to $model in 2.0.36 #18175

Closed
brandonkelly opened this issue Jul 14, 2020 · 4 comments
Closed

InlineValidator now binds $method to $model in 2.0.36 #18175

brandonkelly opened this issue Jul 14, 2020 · 4 comments
Labels
status:ready for adoption Feel free to implement this issue. type:docs Documentation
Milestone

Comments

@brandonkelly
Copy link
Contributor

As of b642a38, InlineValidator is now binding closures to the model being validated:

} elseif ($method instanceof \Closure) {
$method = $this->method->bindTo($model);
}

This is a change in behavior from prior versions which was not mentioned in the upgrade notes. It can lead to PHP errors from validation methods that were registered from a different class.

For example, Craft allows event-based rule registration with an EVENT_DEFINE_RULES event in craft\base\Model::rules():

https://github.com/craftcms/cms/blob/68d7e40efc7630fc319a27e0536426028d7baa50/src/base/Model.php#L78-L89

So now if an inline validation rule is added via that event, like this:

Event::on(Entry::class, Entry::EVENT_DEFINE_RULES, function(DefineRulesEvent $e) {
    $e->rules[] = ['title', function() {
        $this->someMethod();
    }];
});

A PHP error will occur, since $this within the callable is no longer a reference to the same class that the event handler was registered in; it will be the Entry object.

What steps will reproduce the problem?

  1. Add a way for other classes to register validation rules to a model’s rules() method.
  2. From a different class, register an inline validation rule on that model, which expects $this to be the rule was originally defined in.
  3. Validate the model.

What is the expected result?

$this references the class the rule was defined in.

What do you get instead?

$this references the model’s class.

Additional info

Q A
Yii version 2.0.36
PHP version n/a
Operating system n/a
brandonkelly added a commit to craftcms/commerce that referenced this issue Jul 14, 2020
Inline validation methods are being bound to the model (the line item) in Yii 2.0.36, which can break validation rules defined by purchasable via getLineItemRules(), such as variant line item rules.
brandonkelly added a commit to craftcms/cms that referenced this issue Jul 14, 2020
Inline validation methods are being bound to the model (the line item) in Yii 2.0.36, which can break validation rules defined by purchasable via getLineItemRules(), such as variant line item rules.

Fixes #6372
@brandonkelly
Copy link
Contributor Author

I was able to work around this by wrapping inline validators’ closures in another closure (craftcms/cms@9f4105c), but the change should still at least be documented in the upgrade guide.

@samdark samdark added type:docs Documentation status:ready for adoption Feel free to implement this issue. labels Jul 14, 2020
@samdark samdark added this to the 2.0.37 milestone Jul 14, 2020
nfourtythree added a commit to craftcms/commerce that referenced this issue Jul 15, 2020
brandonkelly added a commit to craftcms/commerce that referenced this issue Jul 21, 2020
@cscrewsandcaptains
Copy link

I used inline validators and js validation, that is broken with 2.0.36.

In the class "yii\validators\InlineValidator" the method "clientValidateAttribute" has a bug. The line:
$method = $this->method->bindTo($model);
must be:
$method = $method->bindTo($model);

@brandonkelly
Copy link
Contributor Author

@cs-crewsAndCaptains Probably worth posting that as a separate issue since it’s more of a direct bug as opposed to a gap in the documentation.

@cscrewsandcaptains
Copy link

@cs-crewsAndCaptains Probably worth posting that as a separate issue since it’s more of a direct bug as opposed to a gap in the documentation.

@brandonkelly okay, if have add: #18204

@samdark samdark closed this as completed in 523e4b3 Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready for adoption Feel free to implement this issue. type:docs Documentation
Projects
None yet
Development

No branches or pull requests

3 participants