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

Inconsistent Models #516

Open
MGatner opened this issue Apr 27, 2022 · 2 comments
Open

Inconsistent Models #516

MGatner opened this issue Apr 27, 2022 · 2 comments

Comments

@MGatner
Copy link
Collaborator

MGatner commented Apr 27, 2022

Model use throughout the library is inconsistent, src/Config/Services.php being a central discrepancy:

  50     Parameter #1 $groupModel of class Myth\Auth\Authorization\FlatAuthorization constructor expects Myth\Auth\Authorization\GroupModel, CodeIgniter\Model given.            
  50     Parameter #2 $permissionModel of class Myth\Auth\Authorization\FlatAuthorization constructor expects Myth\Auth\Authorization\PermissionModel, CodeIgniter\Model given.  
  52     Parameter #1 $model of method Myth\Auth\Authorization\FlatAuthorization::setUserModel() expects Myth\Auth\Models\UserModel, CodeIgniter\Model given.                    

This library should decide one of two ways:

  • Support any Model class, and either loosen class methods or add interfaces for required methods
  • Require specific models (like GroupModel) and developers can extend those models if they need to
@manageruz
Copy link
Collaborator

And it would be concise if we move GroupModel and PermissionModel inside Authorization folder to the respective Models folder. What do you think about it ?

@MGatner
Copy link
Collaborator Author

MGatner commented Jun 22, 2022

Yes, I lobbied for this on Shield and we went with that. It's a pain always having to remember which Models folder to go looking for 😱

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

No branches or pull requests

2 participants