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

callable in validators #17561

Open
wants to merge 21 commits into
base: master
from

Conversation

@DrDeath72
Copy link
Contributor

commented Sep 16, 2019

Q A
Is bugfix? ✔️
New feature?
Breaks BC?
Tests pass? ✔️
Fixed issues #17560
DrDeath72 added 3 commits Sep 16, 2019
@yii-bot

This comment has been minimized.

Copy link

commented Sep 16, 2019

Thank you for putting effort in the improvement of the Yii framework.
We have reviewed your pull request.

In order for the framework and your solution to remain stable in the future, we have a unit test requirement in place. Therefore we can only accept your pull request if it is covered by unit tests.

Could you add these please?

Thanks!

P.S. If you have any questions about the creation of unit tests? Don't hesitate to ask for support. More information about unit tests

This is an automated comment, triggered by adding the label pr:request for unit tests.

DrDeath72 added 2 commits Sep 16, 2019
@DrDeath72 DrDeath72 changed the title Update ExistValidator.php callable in validators Sep 16, 2019
@DrDeath72 DrDeath72 marked this pull request as ready for review Sep 16, 2019
DrDeath72 added 9 commits Sep 16, 2019
Copy link
Member

left a comment

Please add a line for changelog.

framework/validators/ExistValidator.php Outdated Show resolved Hide resolved
framework/validators/ExistValidator.php Outdated Show resolved Hide resolved
framework/validators/RangeValidator.php Outdated Show resolved Hide resolved
framework/validators/UniqueValidator.php Outdated Show resolved Hide resolved
framework/validators/UniqueValidator.php Outdated Show resolved Hide resolved
DrDeath72 and others added 6 commits Sep 17, 2019
Co-Authored-By: Alexander Makarov <sam@rmcreative.ru>
Co-Authored-By: Alexander Makarov <sam@rmcreative.ru>
Co-Authored-By: Alexander Makarov <sam@rmcreative.ru>
Co-Authored-By: Alexander Makarov <sam@rmcreative.ru>
Co-Authored-By: Alexander Makarov <sam@rmcreative.ru>
@alex-code

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

Is

$this->filter instanceof \Closure

still needed with is_callable?
Could this be simplified to

if (is_callable($this->filter)) {
@DrDeath72

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

i think no, do it like in other place :)

@samdark samdark requested a review from yiisoft/reviewers Sep 17, 2019
@rob006

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

👎 from me. This will create ambiguous where array could be interpreted either as callable or value - this may lead to really weird bugs and even security issues. For example ['Foo', 'bar'] in RangeValidator::$range may be interpreted as list of allowed values. But if you add Foo class with bar method, it will magically call this method and use result as list of allowed values.

@kamarton

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

from me. This will create ambiguous where array could be interpreted either as callable or value - this may lead to really weird bugs and even security issues. For example ['Foo', 'bar'] in RangeValidator::$range may be interpreted as list of allowed values. But if you add Foo class with bar method, it will magically call this method and use result as list of allowed values.

I completely agree.

@samdark samdark removed this from the 2.0.27 milestone Sep 17, 2019
@rob006

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

Just for the record - the use case seems to be valid, but implementation is plain wrong. I would personally disallow is_callable in every place where non-callables are also allowed - this is just asking for troubles and it is really easy to shoot yourself in the foot by simple mistake. #17560 probably could be fixed by introducing some kind of CallableExpression which will wrap callable array/string, so they will never be confused with plain array/string.

@alex-code

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

If you're able to use PHP7.1+ there's https://www.php.net/manual/en/closure.fromcallable.php
e.g.

class foo {
    public static function bar() {
        return 'foo-bar';
    }
}

$closure = Closure::fromCallable(['foo', 'bar']);
echo call_user_func($closure);

Should work with the existing checks then.

@DrDeath72

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

Closure not cacheable

@kamarton

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

Create your own validators and implement the necessary logic in them. Caching validators object is wrong idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.