The role instance is in charge of checking the permission type.

[jmleroux]

Another try to resove #9 and #10.

I keep PermissionInterface but it should be removed...

[danizord]

You must allow strings here, it'll receive string most of time, since currently ZfcRbac casts it to string before call Rbac::isGranted()

[jmleroux]

I allow string in implementation. I just didn't know what to do with PermissionInterface in phpDoc.
I'll rewrite mixed

[danizord]

Your PR breaks BC a lot and makes things more strict. Some cases developer need to work with Permission entity rathen than a string. So, I'm 👎

Instead of it, I'd suggest is to accept mixed everywhere and let the RoleInterface implementation check the needed type.

[jmleroux]

Instead of it, I'd suggest is to accept mixed everywhere and let the RoleInterface implementation check the needed type.

Yes, that's what i wanted to do, but i did not dare remove the PermissionInterface. This led to this bad state.

I'll rewrite with mixed

[jmleroux]

For PhpStorm

[danizord]

@jmleroux Since you're breaking backwards compatibility, your PR can't be merged until we start the next release (ZF3). So, if you're breaking BC anyway, why don't you remove PermissionInterface completely? I'm not sure if @bakura10 wants this change, but it's ok to me.

[jmleroux]

@jmleroux Since you're breaking backwards compatibility, your PR can't be merged until we start the next release (ZF3)

It was on purpose. I thought this project was for ZF3.

So, if you're breaking BC anyway, why don't you remove PermissionInterface completely?

Keeping it just for the "string system". And because @bakura10 seems to want it. ;)

I'm not sure if @bakura10 wants this change, but it's ok to me.

I'm not sure either, and frankly the "string system" is all i need.

[bakura10]

Hi,

So. I'd say you guys are "right" regarding the removal of PermissionInterface, however I have one concern: the PermissionInterface job was mostly to enforce the __toString and avoid a lot of

if ($permission instanceof PermissionInterface) {
    $name = $permission->getName();
} else {
   $name = (string) $permission;
}

and to have better performance (__toString is much faster than a lot of conditionals).

However, I agree that this should actually be left to the Role implementation to decide what to expect and what to do. BUT there are several places where this thing is actually useful:

• ZendDeveloperTools: thanks to this interface we can simply show permissions. Without this interface, how could we know how to extract the permission name?
• Assertion plugin manager: since ZfcRbac 2.0 we introduced the assertion plugin manager. To work, we need to index the assertion map with the permission name. How could you write the "addAssertion" method without this interface?

It was on purpose. I thought this project was for ZF3.

Yes, this project is likely to be the Rbac component for ZF3. However it's used for ZfcRbac 2 so I need to keep BC. However no problem for opening an issue with various changes we could add for ZF3 when I'll make the PR.

[danizord]

@bakura10 I agree, but fortunately that places are under ZfcRbac namespace. So... 😃

[bakura10]

It can fit in one line.

[bakura10]

It's okay for everyone to merge this? ping @danizord and @arekkas. In turn I'll be able to merge this one: ZF-Commons/zfc-rbac#183

I think the potential of BC is really small on this one. What do you think?

[aeneasr]

Well what doesn't really makes sense to me is that the Role implementation is dependent on an interface but the Interface, the role implements, is not. So imho: either remove it completely or keep it in the RoleInterface as well.

Regarding ZfcRbac: If a PermissionInterface with __toString is required, why not provide it via the ZfcRbac namespace? I think it doesn't make much sense to keep the Interface here only because it is needed in ZfcRbac

[bakura10]

Completely agree that the permission interface should be moved to ZfcRbac. Actually this can be done without any BC by deprecating it on rbac side, and creating it on ZfcRbac where the ZfcRbac permission interface extends the Rbac one.

Envoyé de mon iPhone

Le 6 févr. 2014 à 11:19, arekkas [email protected] a écrit :

Well what doesn't really makes sense to me is that the Role implementation is dependent on an interface but the Interface, the role implements, is not. So imho: either remove it completely or keep it in the RoleInterface as well.

Regarding ZfcRbac: If a PermissionInterface with __toString is required, why not provide it via the ZfcRbac namespace? I think it doesn't make much sense to keep the Interface here only because it is needed in ZfcRbac

—
Reply to this email directly or view it on GitHub.

[danizord]

Yeah, 👍 let's move it to ZfcRbac.

[bakura10]

I just realized @jmleroux that Role has not been updated and still references PermissionInterface: http://github.com/zf-fr/rbac/blob/master/src/Rbac/Role/Role.php

How do you deal with this?

[jmleroux]

Indeed, this is a problem.
How can we retrieve the permission in hasPermission for example if we don't rely on a string conversion ?
Do we have to iterate on the $permissions to compare the permissions without relying on a string key ?
And what will be the criteria for comparison ?
...

[danizord]

Just silently remove the interface from docblocks and keep the (string) casting ;)

[bakura10]

But what's the point of casting if we do not enforce a PermissionInterface. If we have "mixed" then we cannot cast to string because it may be na object that do not have any __toString defined.

You see, the interface was actually useful :/.

[danizord]

@bakura10 The Rbac\Role\Role implementation would always expect string. So since we don't have string typehint in PHP, we can at least cast it to string. But anyway, the docblock will tell that it expects a string.

[danizord]

Most of time developers will write their own implementation of RoleInterface if they need to accept the permission as object (ZfcRbac does that).