Hotfix/45

[traviskentbeste]

Thanks for your suggestions and comments! All issues I think are now resolved.

[weierophinney]

I think you need more tests, to cover the various different scenarios:

• Retrieving a role added as a child of another role.
• Retrieving a grandchild role, where the child role was added explicitly, but the grandchild added implicitly.
• How does hasRole() act in these scenarios?

See also the questions I provided on #48, as I think there are some fundamental problems with how roles are added; if you can figure out how to resolve those, let us know!

[weierophinney]

This should be a separate test entirely.

[weierophinney]

There is zero possibility of $rv being anything but 0 in this scenario.

[weierophinney]

This section could be condensed dramatically:

if ((is_string($needle) && $roleName === $needle)
    || (is_object($needle) && $needle instanceof Role && $roleName === $needle->getName())
) {
    return true; // no need to do anything else, because method returns based on non-zero $rv value
}
return $this->roleSearchIncludingChildren($role, $needle); // Same rationale here

[weierophinney]

Just like the other block, we could do the following:

if (! is_array($children)) {
    return false;
}
if (! count($children)) {
    return is_object($needle) && $needle instanceof Role && $obje->getName() === $needle->getName();
}

foreach ($children as $child) {
    $roleName = $hcild->getName();
    if (is_string($needle) && $roleName === $needle) {
        return true;
    }
    if (is_object($needle) && $needle instanceof Role && $roleName === $needle->getName()) {
        return true;
    }
    if ($this->roleSearchIncludingChildren($child, $needle)) {
        return true;
    }
}
return false;

Making these changes and the previous ones I suggested would eliminate unnecessary cycles when we've already determined a matching child is present.

[weierophinney]

Since your if block always returns, there's no need for an else; if the condition does not match, the next code must always execute. Promote the else block:

if ($role->getName() === $needle) {
   return $role;
}
$role = $this->getRoleSearchingChildren($role, $needle);
if ($role !== null) {
    return $role;
}

[weierophinney]

Same comment as for getRole() - promote the body of your else block, as you always return from the if block.

[weierophinney]

This part is confusing - do you want to return the first match, or the last? If the last, why?

[weierophinney]

This repository has been closed and moved to laminas/laminas-permissions-rbac; a new issue has been opened at laminas/laminas-permissions-rbac#1.

[weierophinney]

This repository has been moved to laminas/laminas-permissions-rbac. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

• Squash all commits in your branch (git rebase -i origin/{branch})
• Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
• Run the laminas/laminas-migration tool on the code.
• Clone laminas/laminas-permissions-rbac to another directory.
• Copy the files from the second bullet point to the clone of laminas/laminas-permissions-rbac.
• In your clone of laminas/laminas-permissions-rbac, commit the files, push to your fork, and open the new PR.
  We will be providing tooling via laminas/laminas-migration soon to help automate the process.