I was thinking about this solution as well, but it is gonna work different way that it was in v2:

• when we add child role to the given role after it is attached to rbac:

$rbac =...$role1 =newRole('role1');$rbac->addRole($role1);$child =newRole('role2');$role1->addChild($child);self::assertFalse($rbac->hasRole('role2'));

but in v2 it will betrue.

• createMissingRoles is default false, so it's not gonna add the child roles by default (it was also not the case in v2, as iterator worked there on all roles and all children).

I know that there's a difference between v2 but this is ok, we are on v3 and the scope of my refactoring was about consistence (see#34).
Regarding your example,role2 is not added automatically to$rbac and I think is correct. If you want to have alsorole2 you need to add it as child to$role1 and finally assign it to$rbac usingaddRole. I don't see any issue here.

createMissingRoles was alsofalse in v2. Maybe, we need to be more explicit in the documentation about that.

@ezimuel If you think the case I've provided above is desired then it should be also included in tests and described in the docs.

Since$createMissingRoles is currentlyfalse by default, changing its value totrue would be a BC break.

That said, I thinkuser expectations are that if you add a child role to a role, but not directly to the RBAC, it would still be considered a role in the RBAC. This was the behavior in v2, and the problem reported in#45. In looking through the 3.0.0 CHANGELOG, there is no reference to this change in behavior, which means it is unexpected.

What the CHANGELOG does note is that there is now support for detectingcircular references in the role hierarchy. What this patch doesn't do is include a check to see if the RBAC instancehas the child role before attempting to add it, which makes this a potentially destructive process.

My gut take is that we should:

• Make$createMissingRoles betrue be default, so that the behavior matches user expectations, as well as the existing documentation.
• Add tests to ensure that when we add a role, we do not overwrite existing roles.
• Modify the patch to only add a child role when it does not already exist in the RBAC.

I'll work on those momentarily.

Interestingly, I just switched the default value of$createMissingRoles, andtests continued to pass. Which indicates none of these scenarios were tested fully previously.

I started by adding tests that would not add a role if it already exists in the RBAC, by testing againsthasRole(). Unfortunately, this failed, becausehasRole() has logic that checks if the$role implementsRoleInterface, and, if so, it does a strict equality check against the internal instance and the instance passed.

I considered removing that strict equality check, but that leaves us in a situation where we could have two differentRoleInterface implementations, and these would be considered equivalent in the RBAC.

But this also means we have a scenario where oneRoleInterface implementation could be registered with its own set of parents and/or children, and then another entirely differentRoleInterface instance using the same role added later. In this scenario, the latter would overwrite the original, and we would potentially have completely different inheritance trees within the RBAC using the same role name, but with different instances.

What's really crazy is that if$parents are specified asstrings, these end up being newRole instances, and overwriting any previous versions of those roles in the RBAC.

This feels really confusing to me, and I'm not sure it's something we want to support.

So, my question here is: what should the behavior be? Silently overwriting feels like it has huge potential for misuse and WTF moments. Doing strict checks inhasRole() seems like it has potential for problems, as it can return a false negative when a role of the same name exists, but it is a different instance than that provided to the method (as the method takes either the string nameor aRoleInterface instance) — and even more so, as wemight want to toggle that strictness off when building the RBAC, but on again later, or even vary based on the role we're adding.

Basically, we need a list of scenarios and how they should behave before we can continue with any of this.

I started by adding tests that would not add a role if it already exists in the RBAC, by testing againsthasRole(). Unfortunately, this failed, becausehasRole() has logic that checks if the$role implementsRoleInterface, and, if so, it does a strict equality check against the internal instance and the instance passed.

I considered removing that strict equality check, but that leaves us in a situation where we could have two differentRoleInterface implementations, and these would be considered equivalent in the RBAC.

But this also means we have a scenario where oneRoleInterface implementation could be registered with its own set of parents and/or children, and then another entirely differentRoleInterface instance using the same role added later. In this scenario, the latter would overwrite the original, and we would potentially have completely different inheritance trees within the RBAC using the same role name, but with different instances.

What's really crazy is that if$parents are specified asstrings, these end up being newRole instances, and overwriting any previous versions of those roles in the RBAC.

This feels really confusing to me, and I'm not sure it's something we want to support.

So, my question here is: what should the behavior be? Silently overwriting feels like it has huge potential for misuse and WTF moments. Doing strict checks inhasRole() seems like it has potential for problems, as it can return a false negative when a role of the same name exists, but it is a different instance than that provided to the method (as the method takes either the string nameor aRoleInterface instance) — and even more so, as wemight want to toggle that strictness off when building the RBAC, but on again later, or even vary based on the role we're adding.

Basically, we need a list of scenarios and how they should behave before we can continue with any of this.