RBAC: decoupled rules from assignments and items, implemented php manager #2974
Conversation
|
Ready for review/merge. |
| * | ||
| * @param Rule $rule the rule that has been changed. | ||
| */ | ||
| public function saveRule(Rule $rule) |
There was a problem hiding this comment.
Perhaps createRule and updateRule would be better.
There was a problem hiding this comment.
Currently Rule can be pretty freeform class so it's hard to wrap its creation into method signature. Also it doesn't look well when implemented. I've dropped it during first draft.
There was a problem hiding this comment.
I also think perhaps createRule() should be separated from saveRule(), similar to that for item and assignment.
There was a problem hiding this comment.
Any idea about the signature?
There was a problem hiding this comment.
How about the following? Note that I'm using the name insert and update so that they are similar to the corresponding methods in AR (this means the methods for saving items and assignments should be changed accordingly for consistency):
insertRule(Rule $rule)
updateRule($name, Rule $rule)
There was a problem hiding this comment.
I don't think it is a good idea to rename createRole and others to insertRole since it's actually creating new object instance from parameters passed to method.
As for Rule, we already have the instance so there's no need to create anything.
| try { | ||
| $this->db->createCommand()->insert($this->ruleTable, ['name' => $rule->name, 'data' => $data])->execute(); | ||
| } catch (\yii\db\Exception $e) { | ||
| $this->db->createCommand()->update($this->ruleTable, ['data' => $data], ['name' => $rule->name])->execute(); |
There was a problem hiding this comment.
If the name of the rule is changed, this implementation will create a new rule and leave the old one there.
There was a problem hiding this comment.
this is why i was mentioning createRule and updateRule as well.
updateRule takes first param $ruleName and second param Rule $newRule. If names are different, the means also a name swap, we remove the previous rule, and call createRule($newRule)
There was a problem hiding this comment.
The previous rule should not be removed. It should be updated (to trigger on update cascade).
|
I have finished reviewing. It looks great to me overall! |
|
👍 |
|
Looks fine to me. You may merge it if no more changes will be done. good job! |
RBAC: decoupled rules from assignments and items, implemented php manager
See #499