-
Notifications
You must be signed in to change notification settings - Fork 90
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
Use 'Acl.' as plugin prefix and pluralized class name in ACL behavior #172
Conversation
hasMany associations should always use the pluralized association name: https://book.cakephp.org/4/en/orm/associations.html#hasmany-associations
This resolves an issue where the Acl Plugin itself used autogenerated Cake\ORM\Table instead of Acl\Model\Table\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in getting back to you, I had the notifications for this repository turned off for some reason.
@@ -1,5 +1,5 @@ | |||
{ | |||
"name": "cakephp/acl", | |||
"name": "itnovum/acl", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be reverted in order to be merged.
@@ -113,7 +114,12 @@ public function node($ref = null, $type = null) | |||
throw new Exception\Exception(__d('cake_dev', 'ref parameter must be a string or an Entity')); | |||
} | |||
|
|||
return $this->_table->{$type}->node($ref); | |||
if(property_exists($this->_table, $type)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(property_exists($this->_table, $type)){ | |
if (property_exists($this->_table, $type)){ |
You'll need to apply the code formatting standards. You can do that with vendor/phpcbf src tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @markstory sorry for my late response to this.
I have created a new branch to merge the upstream changes. I also executed phpcbf
but this had made so many code changes, that I would recommend to do this in a second step.
A TOTAL OF 526 ERRORS WERE FIXED IN 53 FILES
I guess otherwise the diff is getting to large. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can update phpcs separately.
Closing as #180 was merged. |
The AclBehavior used the singular table name to add the
hasMany
association which leats to usage of the wrong table.I also replaced
App::className()
withAcl.
because the Acl Plugin was using generic auto generatedCake\ORM\Table
instead ofAcl\Model\Table\
I checked some other CakePHP plugins and none of it was using
App::className()