Skip to content

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

Merged
jasonvarga merged 4 commits into
statamic:6.xfrom
ryanmitchell:perf/implement-permissions-cache-on-eloquent
Jun 9, 2026
Merged

[6.x] Implement permissions cache on eloquent users#14661
jasonvarga merged 4 commits into
statamic:6.xfrom
ryanmitchell:perf/implement-permissions-cache-on-eloquent

Conversation

@ryanmitchell

@ryanmitchell ryanmitchell commented May 13, 2026

Copy link
Copy Markdown
Contributor

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

@jasonvarga jasonvarga left a comment

Copy link
Copy Markdown
Member

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.

@jasonvarga jasonvarga enabled auto-merge (squash) June 9, 2026 21:44
@jasonvarga jasonvarga merged commit a8d25cb into statamic:6.x Jun 9, 2026
18 checks passed
@ryanmitchell ryanmitchell deleted the perf/implement-permissions-cache-on-eloquent branch June 10, 2026 22:34
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