feature #61057 [Security] Improve performance of RoleHierarchy::buildRoleMap method (simonjamain-gp, simonjamain)

fabpot · fabpot · commit 44f6c856d9a5 · 2025-08-23T08:28:44.000+02:00
This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [Security] Improve performance of `RoleHierarchy::buildRoleMap` method | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no (perf) | Deprecations? | no | Issues | Fix #57322 | License | MIT use of an optimized role ustacking function ### What it does and why it's needed _Note : Better in-detail explanation in there : https://github.com/symfony/symfony/issues/57322_ Uses the way faster `array_pop()` function to build the role map instead of `array shift` ### If it modifies existing behavior, include a before/after comparison At first, it would look like this function swap could change slightly the ordering of the array produced by the `RoleHierarchy::buildRoleMap` method and it does it in a way. I find that it does not change the behaviour of our app. I would not expect most other apps too break because I don't find many reasons to rely on the ordering of roles in hierarchies. Furthermore, `buildRoleMap` is a protected method serving the public `getReachableRoleNames` which does not imply a particular ordering (rightfully so IMHO). ### Testing As it does not change or introduce any behavious per say, this performance increase rely on old tests passing again. If I am not mistaken, the current `testGetReachableRoleNames()` still passes even tho it asserts a specific ordering. I still changed the assertion so it does not convey any false premises. Commits ------- 508b62f [Security] Improve performance of `RoleHierarchy::buildRoleMap` method
diff --git a/src/Symfony/Component/Security/Core/Role/RoleHierarchy.php b/src/Symfony/Component/Security/Core/Role/RoleHierarchy.php
@@ -54,7 +54,7 @@ protected function buildRoleMap(): void
             $this->map[$main] = $roles;
             $visited = [];
             $additionalRoles = $roles;
-            while ($role = array_shift($additionalRoles)) {
+            while ($role = array_pop($additionalRoles)) {
                 if (!isset($this->hierarchy[$role])) {
                     continue;
                 }
diff --git a/src/Symfony/Component/Security/Core/Tests/Role/RoleHierarchyTest.php b/src/Symfony/Component/Security/Core/Tests/Role/RoleHierarchyTest.php
@@ -23,11 +23,11 @@ public function testGetReachableRoleNames()
             'ROLE_SUPER_ADMIN' => ['ROLE_ADMIN', 'ROLE_FOO'],
         ]);
 
-        $this->assertEquals(['ROLE_USER'], $role->getReachableRoleNames(['ROLE_USER']));
-        $this->assertEquals(['ROLE_FOO'], $role->getReachableRoleNames(['ROLE_FOO']));
-        $this->assertEquals(['ROLE_ADMIN', 'ROLE_USER'], $role->getReachableRoleNames(['ROLE_ADMIN']));
-        $this->assertEquals(['ROLE_FOO', 'ROLE_ADMIN', 'ROLE_USER'], $role->getReachableRoleNames(['ROLE_FOO', 'ROLE_ADMIN']));
-        $this->assertEquals(['ROLE_SUPER_ADMIN', 'ROLE_ADMIN', 'ROLE_FOO', 'ROLE_USER'], $role->getReachableRoleNames(['ROLE_SUPER_ADMIN']));
-        $this->assertEquals(['ROLE_SUPER_ADMIN', 'ROLE_ADMIN', 'ROLE_FOO', 'ROLE_USER'], $role->getReachableRoleNames(['ROLE_SUPER_ADMIN', 'ROLE_SUPER_ADMIN']));
+        $this->assertEqualsCanonicalizing(['ROLE_USER'], $role->getReachableRoleNames(['ROLE_USER']));
+        $this->assertEqualsCanonicalizing(['ROLE_FOO'], $role->getReachableRoleNames(['ROLE_FOO']));
+        $this->assertEqualsCanonicalizing(['ROLE_ADMIN', 'ROLE_USER'], $role->getReachableRoleNames(['ROLE_ADMIN']));
+        $this->assertEqualsCanonicalizing(['ROLE_FOO', 'ROLE_ADMIN', 'ROLE_USER'], $role->getReachableRoleNames(['ROLE_FOO', 'ROLE_ADMIN']));
+        $this->assertEqualsCanonicalizing(['ROLE_SUPER_ADMIN', 'ROLE_ADMIN', 'ROLE_FOO', 'ROLE_USER'], $role->getReachableRoleNames(['ROLE_SUPER_ADMIN']));
+        $this->assertEqualsCanonicalizing(['ROLE_SUPER_ADMIN', 'ROLE_ADMIN', 'ROLE_FOO', 'ROLE_USER'], $role->getReachableRoleNames(['ROLE_SUPER_ADMIN', 'ROLE_SUPER_ADMIN']));
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ protected function buildRoleMap(): void`
`54`	`54`	`$this->map[$main] = $roles;`
`55`	`55`	`$visited = [];`
`56`	`56`	`$additionalRoles = $roles;`
`57`		`- while ($role = array_shift($additionalRoles)) {`
	`57`	`+ while ($role = array_pop($additionalRoles)) {`
`58`	`58`	`if (!isset($this->hierarchy[$role])) {`
`59`	`59`	`continue;`
`60`	`60`	`}`