Considering this has most likely never worked, I think attributes should be hinted as string instead.

@iltar I don't think that this is a good idea.

I have a use case where i want to check if some operations (attributes) can be applied on an object (subject).

For sure those operations could be flattened into a string, but one would have to parse them in the voter again which is total nonsense.

Type Hinting for string would be a BC break since one can supply objects as attributes already, but only if the objects implements a__toString() method.

I guess implementing__toString() is already a debatable feature on its own. For the other cases, you have examples of how this would be used? I can't imagine because it's not how it's documented.

@iltar i think its basically a good idea to allow objects there, cause in that case you can easy check if your voter is responsible or not. Like ExpressionVoter:https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/Voter/ExpressionVoter.php#L60

Of course you also can do that with strings, and prefix strings, and hope that there are no conflicts ... but who want this?

@iltar I only implemented the__toString to maintain backwards compatibility. In my opinion I'd rather remove that.

But there are already examples in Symfony itself using non string values. (See@Gladhon's comment) TheExpressionVoter only works because theExpression implements__toString.

I think we should not rely on the__toString() method because you usually want to use it to return a "display" value. For instance, returningAdministrators label for an associatedROLE_ADMIN role. The security system requires that any objects to be used as a role must implement theRoleInterface. So theRoleVoter should probably be upgraded to check it the$attribute is an instance ofRoleInterface instance, so that you can convert it to a string with itsgetRole() method.

Otherwise you must implement your custom voter that support your custom type of attribute.

Btw, the border between bug and new feature is really close in this case. I think it's better to consider it a new feature and submit the PR against themaster branch.

@hhamon I'd like your proposed solution. I implemented the__toString() thing to avoid BC breaks.

I think that it's definitely a bug since the interface clearly says that mixed is accepted. (Which is currently only if you implement__toString.
Could we implement this as a bug and introduce a new feature for master which cleans the behaviour up?

supportsAttribute for RoleVoter should only allow a string or Role(Interface) (Role is actually the only one that is officially supported, because of the string promise).

But when the attributes of a not supported type, the de decision should be abstained?
When passing an unsupported attribute it would have failed with an php error, no?

@sstok Two times yes.

AlsosupportsAttribute does not exist anymore in>= 3.0 I think.

YessupportAttributes as a "public" method was removed from theVoterInterface in Symfony 3.0 because this method was never called by the Security component in Symfony 2. But its piece of logic still resides in theRoleVoter class.

Should I change to check if the attribute is instance of RoleInterface instead of __toString? This will be a small BC break.

@maennchen why exactly do you want to remove__toString support? Imo we should first check forRoleInterface then string/string-object.

That would still be a BC break, but personally i consider it a bugfix.

I refactored it with specific checks forRoleInterface.

I personally don't really care if there is support for objects having a__toString() method. In my opinion it's cleaner just to handle strings &RoleInterface, but with__toString() support there is no BC break.

Thank you@maennchen.