Skip to content

[3.0] Hand back the permission sets that came from the cache #9629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Sesquipedalian merged 3 commits into from
Sep 6, 2026
Merged

[3.0] Hand back the permission sets that came from the cache #9629

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c18de9
Hands back the permission sets that came from the cache
albertlast Sep 5, 2026
e7dcb3c
Update Sources/Permissions/GroupPermissionSet.php
albertlast Sep 5, 2026
7513031
Returns one set per group rather than two
albertlast Sep 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions Sources/Permissions/GroupPermissionSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,9 @@ public static function load(array|int $profiles, array|int $groups, bool $refres
// to query the database separately below.
self::$query_during_construction = false;

// Initialize the objects.
$loaded[] = new self($profile, $group);
// Initialize the objects. The constructor registers each one in
// self::$loaded, which is where they are collected from below.
new self($profile, $group);

// Restore this to its normal value.
self::$query_during_construction = true;
Expand All @@ -300,6 +301,17 @@ public static function load(array|int $profiles, array|int $groups, bool $refres

// Board permissions.
self::loadBoardPermissionData($profiles, $query_groups);

// A set that came from the cache takes the place of the instance the
// constructor registered, so the ones to hand back are whichever
// ended up in self::$loaded.
foreach ($profiles as $profile) {
foreach ($query_groups as $group) {
if (isset(self::$loaded[$profile][$group])) {

@albertlast albertlast Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied, and you were right that the reset was unnecessary — I had convinced myself your version would leave the bug in place and it does not. Tested it: the stats page stays at its full size across a cache miss and two hits, where before it collapsed to a login page on the second request.

One thing it does change though. The construction loop above already appends the instance it builds, so appending again from self::$loaded returns a group twice once it has been looked up — and on a cache hit those two are different objects, the first being the empty one the cached set replaced:

reviewer's version   group 0: load() returned 2 set(s)
with the append gone group 0: load() returned 1 set(s)

current() happens to pick a correct one, so nothing visibly breaks. But Permissions.php:1324 and PermissionProfile.php:115 both mutate every set the list hands them and call save(), so the pair doubles the writes against two objects that disagree, and only the ordering decides which one lands last.

Since the constructor already registers the instance in self::$loaded, dropping the append is enough — no reset, your loop, and one set per group. That is 7513031.

$loaded[] = self::$loaded[$profile][$group];
}
}
}
Comment thread
albertlast marked this conversation as resolved.
}

return $loaded;
Expand Down Expand Up @@ -421,7 +433,7 @@ protected static function loadBoardPermissionData(array $profiles, array $groups
}
}

if ($hits = \count($profiles)) {
if ($hits === \count($profiles)) {
unset($groups[$g]);
}
}
Expand Down