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.

Sign up for GitHub

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

Jump to bottom

Rename Phalcon\Assets\Resource #12082

Closed
3 tasks done
sergeyklay opened this issue Aug 3, 2016 · 5 comments
Closed
3 tasks done

Rename Phalcon\Assets\Resource #12082

sergeyklay opened this issue Aug 3, 2016 · 5 comments
Assignees
@niden
Labels
breaks bc Functionality that breaks Backwards Compatibility discussion Request for comments and discussion
Milestone
4.0.0

Comments

@sergeyklay
Copy link
Contributor

sergeyklay commented Aug 3, 2016 •
edited by niden
Loading

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

To rename:

  • Phalcon\Assets\Resource (Phalcon\Assets\Asset)
  • Phalcon\Assets\Resource\Css (Phalcon\Assets\Asset\Css)
  • Phalcon\Assets\Resource\Js (Phalcon\Assets\Asset\Js)
@sergeyklay sergeyklay added Requires-Discussion discussion Request for comments and discussion labels Aug 3, 2016
@sergeyklay sergeyklay added this to the 4.0.0 milestone Aug 3, 2016
@sergeyklay sergeyklay changed the title Rename Phalcon\Assets\Resource Rename Phalcon\Assets\Resource and Phalcon\Acl\Resource Aug 3, 2016
@Jurigag
Copy link
Contributor

Jurigag commented Aug 3, 2016 •
edited
Loading

ResourceAware and ResourceInterface don't need renaming though. I think if it's in namespace don't need too, just class names like Resource needs renaming.

How we can remove https://github.com/phalcon/cphalcon/blob/master/phalcon/acl/resourceaware.zep ? The ResourceInterface is for other thing. ResourceInterface is for using with addResource/addRole, ResourceAware for using with isAllowed(like for example models can implement it, and in MANY situations you can have getName() method in model.

@sergeyklay sergeyklay added the breaks bc Functionality that breaks Backwards Compatibility label Jul 17, 2017
@stale
Copy link

stale bot commented Apr 16, 2018

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues

@stale stale bot added the stale Stale issue - automatically closed label Apr 16, 2018
@sergeyklay sergeyklay reopened this May 2, 2018
@stale stale bot removed the stale Stale issue - automatically closed label May 2, 2018
@niden
Copy link
Member

niden commented Oct 31, 2018

@sergeyklay Renaming the Assets is easy, should have that done reasonably soon.

As far as the ACL is concerned. I propose the following

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

Also if we move to Subject we will have to change the Role as follows

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

This pretty much changes the verbiage to Subject and Operation two terms that are widely used in the industry when describing access control lists.

@niden
Copy link
Member

niden commented Oct 31, 2018 •
edited
Loading

cc @Jurigag @danhunsaker @Jurigag

@niden niden self-assigned this Oct 31, 2018
@niden niden changed the title Rename Phalcon\Assets\Resource and Phalcon\Acl\Resource Rename Phalcon\Assets\Resource Dec 7, 2018
@niden niden mentioned this issue Dec 7, 2018
Merged
3 tasks
niden added a commit that referenced this issue Dec 9, 2018
@niden
Merge branch 'niden-T12082-resource-rename-2' into 4.0.x
4853488 
* niden-T12082-resource-rename-2:
  [#12082] - Updated the changelog; reenabled cli/integration
  [#12082] - More tests
  [#12082] - PHPCS
  [#12082] - Added more tests for Assets\Inline
  [#12082] - Temporarily disabling other suites
  [#12082] - Added assets/getContent tests
  [#12082] - PHPCS
  [#12082] - Reset the Di
  [#12082] - Added filter tests
  [#12082] - More assets tests assets\asset
  [#12082] - More tests refactored
  [#12082] - Corrected tests and PHPCS
  [#12082] - Added tests for asset getLocal, getPatha
  [#12082] - Moved old tests in the check folder; Added Asset tests
  [#12082] - [#12082] - Renamed folder
  [#12082] - Renamed Assets\Resource to Assets\Asset
@niden
Copy link
Member

niden commented Dec 9, 2018

This has been addressed

@niden niden closed this as completed Dec 9, 2018
@niden niden added the 4.0 label Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@niden niden

Labels
breaks bc Functionality that breaks Backwards Compatibility discussion Request for comments and discussion
Projects
None yet
Milestone
4.0.0
Development

No branches or pull requests
3 participants
@niden @sergeyklay @Jurigag