Rbac::getRole() : check object->getName() #5973

Merged
merged 2 commits into from Mar 21, 2014

Conversation

Projects
None yet
4 participants
Contributor

jmleroux commented Mar 14, 2014

We know that $objectOrName, can only be string or RoleInterface.

So if it's an object, we can check with getName() method.

If found it usefull for some unit tests which did not pass because $leaf and $objectOrName were not the same when $objectOrName was a mock.

In this case, the test $leaf == $objectOrName was false when it should be true because the two objects had the same name property, even if the rest of the objects were different.

@jmleroux jmleroux and 1 other commented on an outdated diff Mar 14, 2014

library/Zend/Permissions/Rbac/Rbac.php
@@ -110,7 +110,9 @@ public function getRole($objectOrName)
$it = new RecursiveIteratorIterator($this, RecursiveIteratorIterator::CHILD_FIRST);
foreach ($it as $leaf) {
- if ((is_string($objectOrName) && $leaf->getName() == $objectOrName) || $leaf == $objectOrName) {
+ if ((is_string($objectOrName) && $leaf->getName() == $objectOrName)
+ || (is_object($objectOrName) && $leaf->getName() == $objectOrName->getName())
+ || $leaf == $objectOrName) {
@jmleroux

jmleroux Mar 14, 2014

Contributor

I don't know if the line is still necessary, but don't want to remove it to avoid BC break.

@danizord

danizord Mar 14, 2014

Contributor

This line is useless. If names are different, then the objects are different as well.

@danizord

danizord Mar 14, 2014

Contributor

Also, the CS check fails, you should move ) { to the next line.

jmleroux closed this Mar 14, 2014

jmleroux reopened this Mar 14, 2014

Member

Ocramius commented Mar 14, 2014

@jmleroux got a test to back this patch?

Contributor

jmleroux commented Mar 14, 2014

In a few days. 😉

Contributor

jmleroux commented Mar 17, 2014

@Ocramius here are the modified tests

@danizord danizord commented on the diff Mar 17, 2014

library/Zend/Permissions/Rbac/AbstractIterator.php
protected $index = 0;
+ /**
@danizord

danizord Mar 17, 2014

Contributor

Missing empty line before this line.

Contributor

jmleroux commented Mar 19, 2014

ping @Ocramius

Member

Ocramius commented Mar 19, 2014

@jmleroux the PR needs a rebase

@jmleroux jmleroux commented on the diff Mar 20, 2014

library/Zend/Permissions/Rbac/AbstractIterator.php
@@ -42,7 +48,7 @@ public function next()
* (PHP 5 &gt;= 5.0.0)<br/>
* Return the key of the current element
* @link http://php.net/manual/en/iterator.key.php
- * @return scalar scalar on success, or null on failure.
+ * @return int|null scalar on success, or null on failure.
@jmleroux

jmleroux Mar 20, 2014

Contributor

scalar is not a valid php type

@jmleroux jmleroux commented on the diff Mar 20, 2014

library/Zend/Permissions/Rbac/Rbac.php
@@ -110,8 +110,10 @@ public function getRole($objectOrName)
$it = new RecursiveIteratorIterator($this, RecursiveIteratorIterator::CHILD_FIRST);
foreach ($it as $leaf) {
- /* @var RoleInterface $leaf */
- if ((is_string($objectOrName) && $leaf->getName() == $objectOrName) || $leaf == $objectOrName) {
+ /** @var RoleInterface $leaf */
@jmleroux

jmleroux Mar 20, 2014

Contributor

I modified in favor of docblock format, since it seems it is your preferred ;)

Contributor

jmleroux commented Mar 20, 2014

@Ocramius
Rebased. I took the opportunity to add some minor change.

@Maks3w Maks3w and 2 others commented on an outdated diff Mar 20, 2014

library/Zend/Permissions/Rbac/Rbac.php
@@ -110,8 +110,10 @@ public function getRole($objectOrName)
$it = new RecursiveIteratorIterator($this, RecursiveIteratorIterator::CHILD_FIRST);
foreach ($it as $leaf) {
- /* @var RoleInterface $leaf */
- if ((is_string($objectOrName) && $leaf->getName() == $objectOrName) || $leaf == $objectOrName) {
+ /** @var RoleInterface $leaf */
+ if ((is_string($objectOrName) && $leaf->getName() == $objectOrName)
+ || (is_object($objectOrName) && $leaf->getName() == $objectOrName->getName())
@Maks3w

Maks3w Mar 20, 2014

Member

At this point we can be sure that objectOrName is an object so the use of is_object is unnecessary

@danizord

danizord Mar 20, 2014

Contributor

I'd say you should convert object -> string before this loop

@Maks3w

Maks3w Mar 20, 2014

Member

Truth. This can be done while validating the argument. Good catch

@jmleroux

jmleroux Mar 20, 2014

Contributor

I'd say you should convert object -> string before this loop

👍
Why did you not said it first ! 😉

@danizord

danizord Mar 20, 2014

Contributor

I realized it now :D

Contributor

jmleroux commented Mar 20, 2014

@Maks3w @danizord @danizord
Checking if role is an object or a string before loop

Maks3w was assigned by jmleroux Mar 21, 2014

@Maks3w Maks3w added a commit that referenced this pull request Mar 21, 2014

@Maks3w Maks3w Merge pull request #5973 b5c9939

@Maks3w Maks3w added a commit that referenced this pull request Mar 21, 2014

@Maks3w Maks3w Merge pull request #5973 in develop ecd2189

@Maks3w Maks3w added a commit that referenced this pull request Mar 21, 2014

@Maks3w Maks3w Merge pull request #5973 in master 2127acd

@Maks3w Maks3w merged commit ae35ff1 into zendframework:master Mar 21, 2014

1 check failed

default The Travis CI build failed
Details

jmleroux deleted the jmleroux:permissions branch Mar 21, 2014

@weierophinney weierophinney pushed a commit to zendframework/zend-permissions-rbac that referenced this pull request May 15, 2015

@Maks3w Maks3w Merge pull request zendframework/zendframework#5973 1523ed0

@weierophinney weierophinney pushed a commit to zendframework/zend-permissions-rbac that referenced this pull request May 15, 2015

@Maks3w Maks3w Merge pull request zendframework/zendframework#5973 in develop 4dbccf4

@weierophinney weierophinney pushed a commit to zendframework/zend-permissions-rbac that referenced this pull request May 15, 2015

@Maks3w Maks3w Merge pull request zendframework/zendframework#5973 in master 9d09cf8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment