refs #12771 \yii\web\User::can() and guest #12785
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pchapl
force-pushed
the
12771_optimize_check_access_on_guest
branch
2 times, most recently
from
d3b5cac to
c8589e9
Compare
SilverFire
requested changes
Nov 13, 2016
| @@ -94,6 +95,7 @@ Yii Framework 2 Change Log | |||
| - Enh #12580: Make `yii.js` comply with strict and non-strict javascript mode to allow concatenation with external code (mikehaertl) | |||
| - Enh #12664: Added support for wildcards for `optional` at `yii\filters\auth\AuthMethod` (mg-code) | |||
| - Enh #12744: Added `afterInit` event to `yii.activeForm.js` (werew01f) | |||
| - Enh #12499: When AJAX validation in enabled, `yii.activeForm.js` will run it forcefully on form submit to display all possible errors (silverfire) | |||
| * @param Assignment[] $assignments list of user's role assignments | ||
| * @return bool true if empty | ||
| */ | ||
| protected function emptyRoles(array $assignments) |
There was a problem hiding this comment.
We prefix this kind of methods with has or is to make it easier to understand, what does the method do.
Maybe, hasNoAssignments()?
| @@ -220,4 +220,15 @@ protected function executeRule($user, $item, $params) | |||
| throw new InvalidConfigException("Rule not found: {$item->ruleName}"); | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Check that there no any roles in user's role assignments and in default roles | |||
There was a problem hiding this comment.
Method checks, whether $assignments array is empty and [[defaultRoles]] are empty as well.
| /** | ||
| * Check that there no any roles in user's role assignments and in default roles | ||
| * | ||
| * @param Assignment[] $assignments list of user's role assignments |
There was a problem hiding this comment.
@param Assignment[] $assignments array of user's assignments
| * Check that there no any roles in user's role assignments and in default roles | ||
| * | ||
| * @param Assignment[] $assignments list of user's role assignments | ||
| * @return bool true if empty |
There was a problem hiding this comment.
@return bool whether $assignments array is empty and [[defaultRoles]] are empty as well
| */ | ||
| protected function emptyRoles(array $assignments) | ||
| { | ||
| return count($assignments) === 0 && count($this->defaultRoles) === 0; |
There was a problem hiding this comment.
Could be simplified to
return empty($assignments) && empty($this->defaultRoles);There was a problem hiding this comment.
I prefer to use count() === 0 cause I know that is array (eg not zero or empty string). It does not matter. And empty works faster with large arrays, so done.
| @@ -119,6 +119,12 @@ public function init() | |||
| public function checkAccess($userId, $permissionName, $params = []) | |||
| { | |||
| $assignments = $this->getAssignments($userId); | |||
|
|
|||
| // nothing to check, user has no any rights | |||
| @@ -97,6 +97,12 @@ public function init() | |||
| public function checkAccess($userId, $permissionName, $params = []) | |||
| { | |||
| $assignments = $this->getAssignments($userId); | |||
|
|
|||
| // nothing to check, user has no any rights | |||
pchapl
force-pushed
the
12771_optimize_check_access_on_guest
branch
from
c8589e9 to
e7ebacc
Compare
dynasource
suggested changes
Nov 15, 2016
There was a problem hiding this comment.
Thanks for putting effort in improving Yii2.
A few things:
- there is duplication of logic in DbManager & PhpManager which should be prevented. I would expect a new function like 'getActiveAssignements' or something like that.
- inverse logic is applied in the method name 'hasNoAssignments'. This should be formulated positively like 'hasAssignments'
- I a still missing a clear explanation in the method docs why the added method is useful. It states what it does, but not why its useful.
- new methods should get a @SInCE tag
SilverFire
force-pushed
the
12771_optimize_check_access_on_guest
branch
from
8214ec2 to
dd4b5af
Compare
|
A few words what happen to this PR:
|
|
Restored changes from |
|
The PR is merged. Thank yoou |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Skip \yii\rbac\PhpManager::checkAccessRecursive and \yii\rbac\DbManager::checkAccessRecursive if role assignments are empty.
I checked code of
\yii\rbac\PhpManager::checkAccessRecursiveand\yii\rbac\DbManager::checkAccessRecursive, and I'm sure that result always will be "false" if$assignmentsis empty array and\yii\rbac\BaseManager::$defaultRolesis empty array too.On the other hand, I'm not sure it matters.