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

Feature - Ability to define different PermissionRegistrar #2623

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

illambo
Copy link

@illambo illambo commented Feb 24, 2024

This PR replaces the PR #2618 refactors some code of the PermissionRegistrar and traits.

The goal is the possibility of being able to instantiate multiple PermissionRegistrar to isolate and use the library conveniently on multiple database at the same time (with possible different configurations without having to modify the runtime configuration).

Overview code changes :

  • Improvement PermissionRegistrar
    • refactoring initialization
    • configuration isolation
    • strengthening of properties type declarations
  • Allowed ability to use custom PermissionRegistrar for models and traits
  • Allowed ability to use custom PermissionRegistrar in commands
  • Allowed ability to use custom PermissionRegistrar for some methods in helpers
  • Add tests cases for multiple schema and adapted some tests

The configuration is given (and isolated for the potentially extensible parts) as input during the initialization phase of the PermissionRegistrar.
It is also possible to overwrite the PermissionRegistrar used for models and traits, in this way you can use the library in a "straight" way on multiple schemes without having to change the configuration at runtime (see the battery of tests for greater clarity).

Currently I have not brought the entire configuration internally so as not to apply too many changes, but only the bare minimum that I have found currently implemented.

This approach is similar to the one also present in other libraries, for example see spatie/laravel-medialibrary (see provider and trait).

A possible breaking change is in the setPermissionClass and setRoleClass methods of the PermissionRegistrar where the global configuration is no longer updated as it is irrelevant.
I kept the binding of the contract optional although in my opinion it should be managed on the application side according to the business logic of the case).

I am aware that it is quite substantial as a PR and I probably missed something around because I don't know the entire library in depth, but I still wanted to propose it as a starting point for a future release (or in any case to consider its purpose).

Thanks

- refactoring initialization
- configuration isolation
- strengthening of properties type declarations
Allowed ability to use custom PermissionRegistrar for models and traits
Allowed ability to use custom PermissionRegistrar in commands
Allowed ability to use custom PermissionRegistrar for some methods in helpers
Add tests cases for multischema and adapted some tests
@illambo illambo changed the title Feature - Ability to define multi PermissionRegistrar Feature - Ability to define different PermissionRegistrar Feb 25, 2024
@illambo
Copy link
Author

illambo commented Mar 7, 2024

Hi, any news about that? Thanks

@drbyte
Copy link
Collaborator

drbyte commented Mar 7, 2024

Still reviewing this. As you say, it's quite substantial.

@illambo
Copy link
Author

illambo commented Apr 5, 2024

Hi, were you able to evaluate for a moment the underlying reasons for this PR?
What do you think? Do you think it's a valid approach for this or a next major release?
In enterprise environments, in my opinion, it often happens that we deal with multiple db schema with libraries like this (or medialibrary, ..).
Thanks

@drbyte
Copy link
Collaborator

drbyte commented Apr 19, 2024

Hi, were you able to evaluate for a moment the underlying reasons for this PR?

I believe I understand your objective, and am willing to consider it for future release.
There's a lot to consider when a PR introduces a lot more code to manage :D

@drbyte
Copy link
Collaborator

drbyte commented Apr 19, 2024

Would it still work if instead of calling $permissionRegistrar = static::getPermissionRegistrar(); in multiple functions, it were to be set as a property once when the trait is instantiated?

@illambo
Copy link
Author

illambo commented Apr 19, 2024

Would it still work if instead of calling $permissionRegistrar = static::getPermissionRegistrar(); in multiple functions, it were to be set as a property once when the trait is instantiated?

I haven't tried, it could work but you need to try, you could do some tests so you can verify your thinking and check with the battery of tests.
I know I have adapted a lot of code to "the need" so I ask you to check and if necessary adapt everything as best as possible in terms of style or code for the library.
This PR can also simply be a starting point / suggestion for conversion in this sense in a future release (so feel free to close it if it is too premature), in my opinion we could also take advantage of the opportunity to strengthen the method signature (php >= 8.1 or 8.2) and the cleaning of logic and code.

Thanks for the feedback.

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

Successfully merging this pull request may close these issues.

None yet

2 participants