[6.x] Implement permissions cache on eloquent users#14661
Conversation
jasonvarga
left a comment
There was a problem hiding this comment.
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.
|
Sorry. Fixed now and coverage added. |
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.