Test/testcoverage util 1#2115
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens type safety in access-control related utility classes and adds PHPUnit coverage for access group and permission group behavior, including singleton/permission checks in AccessControl.
Changes:
- Added/updated PHP type declarations in
AccessGroupUtils,AccessControlUtils, andAccessControl(nullable properties, scalar parameter types, and return types). - Introduced PHPUnit tests for
AccessGroupUtils,AccessControlUtils, andAccessControl, and updated the sharedTestBase. - Added small test-environment adjustments (e.g., defaulting
$_SERVERvalues) to reduce PHPUnit warnings.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/inc/utils/AccessGroupUtils.php | Adds model import and stricter typing for several access-group utilities. |
| src/inc/utils/AccessControlUtils.php | Adds return/parameter typing for permission-group utilities. |
| src/inc/utils/AccessControl.php | Makes internal state nullable/typed and updates method signatures to support tests and safer initialization. |
| ci/phpunit/TestBase.php | Adds overrides and defaults for server variables used during tests. |
| ci/phpunit/inc/utils/AccessGroupUtilsTest.php | New tests covering access-group behaviors (membership, chunk abort, delete reassignment). |
| ci/phpunit/inc/utils/AccessControlUtilsTest.php | New tests covering permission-group CRUD and permission updates. |
| ci/phpunit/inc/utils/AccessControlTest.php | New tests covering singleton behavior and permission evaluation logic. |
Comments suppressed due to low confidence (1)
src/inc/utils/AccessControlUtils.php:30
getGroups()is declared to return?array, butFactory::getRightGroupFactory()->filter([])returns an array when$single=false. Using a nullable return type here is an inaccurate contract and can spread unnecessarynullchecks into callers; preferarray(orRightGroup[]).
public static function getGroups(): ?array {
return Factory::getRightGroupFactory()->filter([]);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static function updateGroupPermissions(int $groupId, array $perm): bool { | ||
| $group = AccessControlUtils::getGroup($groupId); | ||
| if ($group->getPermissions() == 'ALL') { | ||
| throw new HTException("Administrator group cannot be changed!"); |
There was a problem hiding this comment.
Discussions around handling this potential TypeError ended up in that it is only keys read and passed to these functions, so it is not very likely they will appear.
But also, it would be nice to manage this type of error as the code get more typed and these errors become more likely to appear, in this pr we merge as is, and then make a separate fix that changes the exception handling to deal with Throwable in all handlers.
| public static function addAgent(int $agentId, int $groupId): void { | ||
| $group = AccessGroupUtils::getGroup($groupId); | ||
| $agent = AgentUtils::getAgent($agentId); | ||
|
|
There was a problem hiding this comment.
Discussions around handling this potential TypeError ended up in that it is only keys read and passed to these functions, so it is not very likely they will appear.
But also, it would be nice to manage this type of error as the code get more typed and these errors become more likely to appear, in this pr we merge as is, and then make a separate fix that changes the exception handling to deal with Throwable in all handlers.
No description provided.