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

Authorisation Service #2

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Authorisation Service #2

wants to merge 16 commits into from

Conversation

nibra
Copy link
Member

@nibra nibra commented Feb 26, 2018

Proposal for a (generic) interface for Authorisation Services.

*
* @return UserInterface[]
*/
public function getUsers($action, $classOrObject = null): array;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be getActors imho - the same as is in description - user implies humans

*
* @return object[]
*/
public function getObjects($roleOrUser, $action, $class = null): array;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this one should be something more abstract - target or entity as in your description

*
* @param UserInterface $user The user that will take action
* @param string $action The action to be taken
* @param string|object $classOrObject The class or object to act on.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klas
Copy link

klas commented Mar 14, 2018

I basically agree with proposed RCF, but all methods and variables should be named in abstract way not bound to specific implementations, as it is done in description https://github.com/joomla-x/joomla-standards/pull/2/files#diff-b75c3483f869c0812ec55be2719fa3c3R28

@nibra
Copy link
Member Author

nibra commented Mar 14, 2018

@klas Thank you for your feedback, made according changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants