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

Remove dependency on Zfr/Rbac #3

Closed
visto9259 opened this issue Oct 2, 2020 · 15 comments · Fixed by #36
Closed

Remove dependency on Zfr/Rbac #3

visto9259 opened this issue Oct 2, 2020 · 15 comments · Fixed by #36
Assignees
Labels
enhancement New feature or request v4 To be implemented in version 4
Milestone

Comments

@visto9259
Copy link
Member

Remove the dependency on Zfr/Rbac.
Integrate the prototypes defined by Zfr/Rbac into the LmcRbac namespace

@visto9259 visto9259 added the enhancement New feature or request label Oct 2, 2020
@visto9259
Copy link
Member Author

This is a copy of Laminas-Commons/LmcRbacMvc#3

@ALTAMASH80
Copy link
Contributor

Hi @visto9259, I wanted to ask you before starting the work. I want to ask you what is your desired interest to proceed forward. I see two ways.

  1. One is to make another repository LmcRbac and move the ZfrRbac code there.
  2. Second is to make use of Laminas\Permission\Rbac and remove Rbac altogether.

Thanks!

@visto9259
Copy link
Member Author

Hi @ALTAMASH80,
I had started to look into this but did not get very far. I want to get rid of ZfrRbac because it's no longer supported by Zfr.
We already have a LmcRbac package which has differences from ZfrRbac and I have not looked at Laminas RBAC. I think ZfrRbac was developed as a prototype for Laminas RBAC but I have not completed that analysis.
May I ask you to investigate what is the better approach: using LmcRbac or using Laminas RBAC and report in this issue's thread?
I am concerned with the breaking changes that this migration will cause so we need to know that they would be. If there are, and that's ok if there are breaking changes, then I would tend to create a version 4 of LmcMvcRbac. My vision of version 4 would be add other improvements and potentially make the library more modular (ie have a core library and move into separate modules guards, strategies, Doctrine, etc.). I just did have the time to look into this.

@ALTAMASH80
Copy link
Contributor

ALTAMASH80 commented Feb 20, 2023

@visto9259, I did look into the Lamias\Permission\Rbac it looks like it is similar to ZfrRbac but with changes in method names and the two classes/services merged as one in Role. In ZfcRbac roles are divided into two services/classes namely Role/Hierarchal role. In comparison, Laminas\Permission\Rbac merged the two classes into one which seemed logical because the only difference was getting children and parents.

So, I think we should use Laminas\Permission\Rbac it will be a learning experience for me as well. Thanks!

@visto9259 visto9259 added the v4 To be implemented in version 4 label Mar 10, 2023
@visto9259
Copy link
Member Author

@ALTAMASH80,
I created a branch called 4.x-dev for new development, including this one.
Use it for your work.

@ALTAMASH80
Copy link
Contributor

Thanks, @visto9259 for creating a separate branch and sharing the roadmap.

@visto9259
Copy link
Member Author

I have been comparing the classes and interfaces defined by LmcRbac, zfr/Rbac and laminas-rbac. They are similar but not enough to simply drop LmcRbac and zfr/Rbac in favor of only laminas-rbac. I think we need to make LmcRbac classes extensions of laminas-rbac.
Ideally, it would be best to drop zfr/Rbac in favor of LmcRbac and have LmcRbac be an extension of laminas-rbac and at the same time, make sure that there are no backward compatibility issues.

@ALTAMASH80
Copy link
Contributor

Hi @visto9259, What I've seen and observed through tests in Laminas\Rbac is that Laminas\Rbac has merged the two services into one. So, in ZfcRbac there were two different classes one was simply named Rbac and the second was HeirachalRole. Laminas\Rbac has merged the two into one class. Because of this tests are failing. I'm close to solving the issue without a backward compatibility break. If a break becomes essential in my finding I'll let you know. Thanks!

@visto9259
Copy link
Member Author

@ALTAMASH80,
I think you are refering to the Role class. The Role class in Laminas combines the Role and HierarchicalRole classes into one. HierarchicalRole is used in the InMemoryRoleProvider class and I think we can replace the logic to use Laminas Role instead.
The bigger concern is the use of the Rbac class. In zfr/rbac, the isGranted() method accepts an array of roles where laminas\Rbac does not. And zfr/Rbac has a constuct to specify a trversal strategy and laminas\Rbac does not. It is only a concern if the use of zfr/rbac is not internal to LmcRbacMvc and I have not checked that.

@ALTAMASH80
Copy link
Contributor

Hi @visto9259, you're spot on with the class name. Thanks for the correction. As for the Zfr/Rbac isGranted() method and its contraction having to accept a traverse strategy can be removed altogether without any backward compatibility break. But, it is just my thought and my way of doing things. If I'm able to pass all the tests which are present in LmcRbac, then I'll be able to show you what I've done and how. At the moment I'm learning TTD through the module itself and progressing at the same time. Thanks!

@ALTAMASH80
Copy link
Contributor

Hi @visto9259, I wanted to share my progress with you on the issue. Does this result suggest I'm ready to push the code? Thanks!
test-result-for-lmc-rbac

@visto9259
Copy link
Member Author

Hi @ALTAMASH80,

I would like to review your changes. Either you create a pull request or you share with me the link to your fork.

Sorry for the delay, I have been busy with not much time to devote to this.

@ALTAMASH80
Copy link
Contributor

Hi @visto9259, please consider my last reply as a rookie mistake in TDD. I tested it on the wrong branch and in the excitement shared the result.

@ALTAMASH80
Copy link
Contributor

ALTAMASH80 commented Jul 6, 2023

Hi @visto9259, I've passed all the tests and created a pull request. #36
Please check it if possible and also, please remove the signed commits check for a week. So, that I can sync the master branch with my master branch. Because at the moment I can sync it. because the last time I pushed an unsigned commit code, it doesn't let me sync it. Thanks!
Screenshot-from-2023-07-08

@visto9259 visto9259 moved this from 🆕 New to 📋 Backlog in LmcRbacMvc Development Feb 12, 2024
@visto9259 visto9259 moved this from 📋 Backlog to 🏗 In progress in LmcRbacMvc Development Feb 15, 2024
@visto9259 visto9259 assigned visto9259 and unassigned ALTAMASH80 Feb 23, 2024
visto9259 added a commit to visto9259/LmcRbacMvc that referenced this issue Feb 29, 2024
@visto9259 visto9259 added this to the 4.0.0 milestone Aug 13, 2024
@visto9259
Copy link
Member Author

fixed by #117

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in LmcRbacMvc Development Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v4 To be implemented in version 4
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants