Skip to content

[6.x] Implement permissions cache on eloquent users#14661

Open
ryanmitchell wants to merge 3 commits into
statamic:6.xfrom
ryanmitchell:perf/implement-permissions-cache-on-eloquent
Open

[6.x] Implement permissions cache on eloquent users#14661
ryanmitchell wants to merge 3 commits into
statamic:6.xfrom
ryanmitchell:perf/implement-permissions-cache-on-eloquent

Conversation

@ryanmitchell
Copy link
Copy Markdown
Contributor

@ryanmitchell ryanmitchell commented May 13, 2026

I noticed the permissions cache is not being used on eloquent users, which would make checking hasPermission multiple times per request much slower than using file users.

This PR brings the base eloquent user in line with file based users.

@duncanmcclean duncanmcclean changed the title [6.x] implement permissions cache on eloquent users [6.x] Implement permissions cache on eloquent users May 13, 2026
Copy link
Copy Markdown
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

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

Two issues here that I think prevent this from working as intended:

1. Permissions are never actually cached

The lookup half is here, but there's no corresponding $cache->put(...) after computing $permissions. Compare with the file user, which ends with:

$permissions = $permissions->unique()->values();

$cache->put($this->id, $permissions);

return $permissions;

As written, every call to permissions() misses the cache and re-runs the group/role flatten/merge. The cache lookup overhead is added but no speedup is delivered.

2. $this->id is undefined on the Eloquent user

Unlike Auth\File\User which declares protected $id, Auth\Eloquent\User has no $id property and no __get. Its identifier comes from the id() method, which returns $this->model()->getKey(). So $this->id is undefined-property access → null, and PermissionCache::get(string $user) is typed string, so the call will throw:

TypeError: ...PermissionCache::get(): Argument #1 ($user) must be of type string, null given

Once this lands, any hasPermission() / permissions() call against an eloquent user fatal-errors.

Suggested fix — use the method, and cast since getKey() is typically an int:

$id = (string) $this->id();

if ($cached = $cache->get($id)) {
    return $cached;
}

$permissions = $this->groups()->flatMap->roles()
    ->merge($this->roles())
    ->flatMap->permissions();

if ($this->get('super', false)) {
    $permissions[] = 'super';
}

$permissions = $permissions->unique()->values();

$cache->put($id, $permissions);

return $permissions;

(Also note the file user normalizes with ->unique()->values() before caching/returning — worth matching here for parity.)

Tests

There are no existing tests for PermissionCache, and none added here. A simple test asserting that two consecutive calls to permissions() only compute once (and that role-permission changes invalidate the cache) would have caught both of the above.

@ryanmitchell
Copy link
Copy Markdown
Contributor Author

Sorry. Fixed now and coverage added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants