search

yiisoft / yii2

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

Inconsistency in use of new InvalidArgumentException in 2.0.14 #15727

Closed Offout closed 6 years ago

Offout commented 6 years ago

2.0.14 version presented new InvalidArgumentException that must be used instead of InvalidParamException. In upgrade instructions it is written: 'Replace usages of yii\base\InvalidParamException with yii\base\InvalidArgumentException'.

I started rewriting my code, and I found that CheckAccessInterface still have PHPDoc with depricated InvalidParamException.

I found some code in framework that uses depricated exception:

  1. FixtureController, BaseActiveRecord, JsonParser. In catch block InvalidParamException instead of InvalidArgumentException. I know that new exception inherits deprecated one, but i think this is inconsistent. I suggest replace deprecated exception with new one in this classes.

  2. ConditionInterface, CheckAccessInterface, ManagerInterface. In PHPDoc @throw InvalidParamException but inside framework realizations InvalidArgumentException is thrown. I suggest change PHPDoc of this interfaces.

  3. CheckAccessInterface. In PHPDoc @throw InvalidParamException but inside framework realizations no such exception is thrown. Instead of exception when '$permissionName does not refer to an existing permission', false returned. In DbManager checkAccess() (@throw InvalidParamException) calls checkAccessFromCache() (no @throw tag) or checkAccessRecursive() (no @throw tag) that calls executeRule() (@throw InvalidConfigException) . In PhpManager checkAccess() (@throw InvalidParamException) calls checkAccessRecursive() (no @throw tag) that calls executeRule() (@throw InvalidConfigException). No other calls in PhpManager or DbManager checkAccess() method does not have any throw tags and does not throw any exceptions. Even tests dosn't expect exception https://github.com/yiisoft/yii2/blob/master/tests/framework/rbac/ManagerTestCase.php#L199 I suggest remove @throw tag from CheckAccessInterface because it misleads, and add 'or false if $permissionName does not refer to an existing permission' to return PHPDoc.

  4. Also some places in tests, ArrayHelperTest, HtmlTest, JsonTest and so on. I suggest rewrite all to use InvalidArgumentException.

I asked about this in official Slack channel, and @samdark replied that I can make PR with fix.

Additional info

Q A
Yii version 2.0.14
PHP version 7.1.14
Operating system Doesn't matter
rob006 commented 6 years ago

FixtureController, BaseActiveRecord, JsonParser. In catch block InvalidParamException instead of InvalidArgumentException. I know that new exception inherits deprecated one, but i think this is inconsistent. I suggest replace deprecated exception with new one in this classes.

This will break BC - old code may throw old exception, we cannot just stop catching it.

I suggest change PHPDoc of this interfaces.

Technically such change breaks BC - you're changing exception that implementation may throw.

Offout commented 6 years ago

FixtureController uses catch InvalidParamException around Yii::getAlias() call, I don't think someone redefined this method and uses a custom Yii class.

BaseActiveRecord uses catch InvalidParamException around $relatedModel->getRelation() so it can be BC breaking when using custom ActiveRecordInterface implementing class.

JsonParser uses catch InvalidParamException around Json::decode() call so it can't be somehow redefined by user.

ConditionInterface - since 2.0.14, so I thought it's not BC breaking. ManagerInterface - can be BC breaking. CheckAccessInterface - old PHPdoc is misleading.

In case of PHPDoc in interfaces it's core team's right to decide what to do, because code will continue to work without any problems.

samdark commented 6 years ago

Adjusting phpdoc is fine.