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

Acl Resource rename #13639

Closed
niden opened this Issue Dec 7, 2018 · 8 comments

Comments

Projects
4 participants
@niden
Copy link
Member

niden commented Dec 7, 2018

I suggest to rename Resource, because it's on soft reserved word of PHP.


To rename:

  • Phalcon\Acl\Resource (???)
  • Phalcon\Acl\ResourceInterface
  • Phalcon\Acl\ResourceAware (I doubt in the necessity of this class)

Phalcon\Acl\Resource -> Phalcon\Acl\Subject
Phalcon\Acl\ResourceInterface -> Phalcon\Acl\SubjectInterface
Phalcon\Acl\ResourceAware -> Phalcon\Acl\SubjectAware

Phalcon\Acl\Role -> Phalcon\Acl\Operation
Phalcon\Acl\RoleInterface -> Phalcon\Acl\OperationInterface
Phalcon\Acl\RoleAware -> Phalcon\Acl\OperationAware

@niden niden added this to To do in 4.0 Release via automation Dec 7, 2018

@niden niden added the Breaks BC label Dec 7, 2018

@niden niden self-assigned this Dec 7, 2018

@niden niden added this to the 4.0.0 milestone Dec 7, 2018

@scrnjakovic

This comment has been minimized.

Copy link
Contributor

scrnjakovic commented Dec 9, 2018

+1, especially about rethinking ResourceInterface and ResourceAware, it's bit confusing.

@niden niden changed the title "Resource" rename Acl Resource rename Dec 9, 2018

@niden niden moved this from To do to In progress in 4.0 Release Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 17, 2018

@niden niden referenced this issue Dec 17, 2018

Merged

T13639 acl resource rename #13668

3 of 3 tasks complete

niden added a commit that referenced this issue Dec 17, 2018

Merge branch 'niden-T13639-acl-resource-rename' into 4.0.x
* niden-T13639-acl-resource-rename:
  [#13639] - Updated the changelog
  [#13639] - Corrected tests
  [#13639] - Corrected tests
  [#13639] - Renamed Acl\Role to Acl\Operation
  [#13639] - Renamed Acl\Resource to Acl\Subject
  More corrections ot the interfaces
  Corrected interfaces

@niden niden moved this from In progress to Done in 4.0 Release Dec 17, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 18, 2018

[phalcon#12921] - Merge branch '4.0.x' into T12921-remove-session-fro…
…m-di-factory

* 4.0.x:
  [phalcon#13061] - Updated changelog
  [phalcon#13061] - Added test for the interface
  [phalcon#13061] - Aligned interfaces
  [phalcon#13639] - Updated the changelog
  [phalcon#13639] - Corrected tests
  [phalcon#13639] - Corrected tests
  [phalcon#13639] - Renamed Acl\Role to Acl\Operation
  [phalcon#13639] - Renamed Acl\Resource to Acl\Subject
@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Dec 20, 2018

ResourceAware is interface for implementing anything which can be use as resource in $acl->isAllowed() ResourceInterface on other hand is mainly for defining Resources as classes so you can use $acl->addResource and add object here.

@niden niden closed this Dec 21, 2018

@scrnjakovic

This comment has been minimized.

Copy link
Contributor

scrnjakovic commented Dec 22, 2018

I still maintain this is confusing if not unnecessary, or maybe I’m missing something? Why do we need two interfaces? Anything that needs to be passed to isAllowed as a resource should implement ResourceInterface. Same goes for addResource. I still don’t see the reason/benefit.

@niden

This comment has been minimized.

Copy link
Member Author

niden commented Dec 22, 2018

@scrnjakovic I see your point. Let me talk to the core team and see whether we can remove the aware interface and what use case serves. With 4.x coming up, now is the time to do this.

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Dec 22, 2018

For me it's simple @scrnjakovic just check ResourceInterface, it has methods like getDescription etc. ResourceAware and RoleAware where implemented so you can for example do something like:

class Article extends Model implements ResourceAware
{
    public function getResourceName()
    {
        return 'article';
    }
}
class User extends Model implements RoleAware
{
    public function getRoleName()
    {
        return 'user';
    }
}

And then do:

$acl = new Memory();
$acl->addResource('article', ['update']);
$acl->allow('user', 'article', ['update']);
$article = Article::findFirst();
$user = User::findFirst();
$canUpdate = $acl->isAllowed($user, $article, 'update'); // will return result

So pretty much ResourceAware and RoleAware allows to to pass any objects implementing them to isAllowed, while on other hand RoleInterface and ResourceInterface have methods like getName getDescription and __toString which can have do totally different things in many classes. They are just for defining resources and roles.

Maybe we can figure something else, if you have any idea feel free to write it. I just don't see a good way to solve this issue.

Also i don't think that merging two interfaces that are for totally different things will solve anything. I guess we can possibly do it somehow by just renaming methods in RoleInterface and ResourceInterface.

@danhunsaker

This comment has been minimized.

Copy link
Member

danhunsaker commented Dec 22, 2018

To attempt to clarify, based on what I'm reading above, ResourceInterface is used to identify (and define common methods for) objects which are resources, while ResourceAware is used to identify (and define common methods for) objects which use resources. Similarly, RoleInterface is for roles, while RoleAware is for things that respect those roles.

Is that accurate? Because if so, they do serve very distinct purposes. Though I'd probably argue, at that point, that *Aware might be better as traits instead of interfaces.

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Dec 22, 2018

Exactly this @danhunsaker but it's more like RoleAware/ResourceAware is representation of these Roles/Resources.

@danhunsaker

This comment has been minimized.

Copy link
Member

danhunsaker commented Feb 2, 2019

Sorry for not catching the Operation thing the first pass.

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