{"action":"created","issue":{"url":"https://api.github.com/repos/spatie/laravel-permission/issues/926","repository_url":"https://api.github.com/repos/spatie/laravel-permission","labels_url":"https://api.github.com/repos/spatie/laravel-permission/issues/926/labels{/name}","comments_url":"https://api.github.com/repos/spatie/laravel-permission/issues/926/comments","events_url":"https://api.github.com/repos/spatie/laravel-permission/issues/926/events","html_url":"https://github.com/spatie/laravel-permission/pull/926","id":378694563,"node_id":"MDExOlB1bGxSZXF1ZXN0MjI5MzM2MjI0","number":926,"title":"Add model/permissions caching","user":{"login":"fullstackfool","id":14146557,"node_id":"MDQ6VXNlcjE0MTQ2NTU3","avatar_url":"https://avatars0.githubusercontent.com/u/14146557?v=4","gravatar_id":"","url":"https://api.github.com/users/fullstackfool","html_url":"https://github.com/fullstackfool","followers_url":"https://api.github.com/users/fullstackfool/followers","following_url":"https://api.github.com/users/fullstackfool/following{/other_user}","gists_url":"https://api.github.com/users/fullstackfool/gists{/gist_id}","starred_url":"https://api.github.com/users/fullstackfool/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fullstackfool/subscriptions","organizations_url":"https://api.github.com/users/fullstackfool/orgs","repos_url":"https://api.github.com/users/fullstackfool/repos","events_url":"https://api.github.com/users/fullstackfool/events{/privacy}","received_events_url":"https://api.github.com/users/fullstackfool/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":7,"created_at":"2018-11-08T11:41:12Z","updated_at":"2018-11-09T22:41:51Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/spatie/laravel-permission/pulls/926","html_url":"https://github.com/spatie/laravel-permission/pull/926","diff_url":"https://github.com/spatie/laravel-permission/pull/926.diff","patch_url":"https://github.com/spatie/laravel-permission/pull/926.patch"},"body":"This adds substantial speed increases by caching the associations between models and permissions. Part of the changes focus on the `PermissionRegistar`, specifically caching for `getPermissions()`, thanks to @klincheg for these changes.\r\n\r\nThe rest is focussed on caching calls to `HasPermissionTo()` in `HasPermissions.php`. \r\n\r\nThe association is cached under a compound key comprised of the package key, the model and the permission:\r\n`spatie.permission.cache.App.Models.User.12.Spatie.Permission.Models.Permissions.view-dashboard`\r\n\r\nThese values are also used as tags: \r\n`spatie.permission.cache`, \r\n`App.Models.User.12`, \r\n`Spatie.Permission.Models.Permissions.view-dashboard`\r\n(if tags are not supported then the package continues as before, as they are essential to this functionality)\r\n\r\nAs we are still using the package cache key, then the already existing cache-busting should not need to be updated to account for these changes. However, the usage of the model and the permissions in the tags allow for future addition of model specific, or permission specific cache-busting, helping prevent the stampeding herd effect that larger applications may experience each time a permission or role is updated.\r\n\r\nPerformance tests involved seeding a database with 250 users, who were assigned between 2 - 4 roles, which were assigned between 30 - 500 permissions. Then each permission for each user was tested:\r\n```php\r\n$this->users->each(function (User $user) {\r\n    $user->getAllPermissions()->each(function (Permission $permission) use ($user) {\r\n        $this->assertTrue($user->can($permission->name));\r\n    });\r\n});\r\n``` \r\n\r\nI went through different variations of the above, some including non-assigned permissions, etc. \r\n\r\nThese changes increase the speed of the checks from several minutes down to 4 - 20 seconds (approx 32000 assertions in a couple of seconds in some tests). And the database calls went from 1800+ to 0.\r\n\r\nThings to note/get feedback on are:\r\n - Is the current cache busting effective enough for these changes? I reviewed the tests and ran through some real-world examples and had no issues, but a second opinion would be great.\r\n - The changes to the config could be breaking\r\n - The naming of `checkHasPermissionTo()` inside `HasPermissions` could be confused with the newly added method `checkPermissionTo()`\r\n - Clutter. I'm concious this has added a fair bit of new methods and checks, and I don't want to clutter up the package.\r\n\r\nCloses #732 \r\nFixes #759 \r\nFixes #789 "},"comment":{"url":"https://api.github.com/repos/spatie/laravel-permission/issues/comments/437517986","html_url":"https://github.com/spatie/laravel-permission/pull/926#issuecomment-437517986","issue_url":"https://api.github.com/repos/spatie/laravel-permission/issues/926","id":437517986,"node_id":"MDEyOklzc3VlQ29tbWVudDQzNzUxNzk4Ng==","user":{"login":"drbyte","id":404472,"node_id":"MDQ6VXNlcjQwNDQ3Mg==","avatar_url":"https://avatars0.githubusercontent.com/u/404472?v=4","gravatar_id":"","url":"https://api.github.com/users/drbyte","html_url":"https://github.com/drbyte","followers_url":"https://api.github.com/users/drbyte/followers","following_url":"https://api.github.com/users/drbyte/following{/other_user}","gists_url":"https://api.github.com/users/drbyte/gists{/gist_id}","starred_url":"https://api.github.com/users/drbyte/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/drbyte/subscriptions","organizations_url":"https://api.github.com/users/drbyte/orgs","repos_url":"https://api.github.com/users/drbyte/repos","events_url":"https://api.github.com/users/drbyte/events{/privacy}","received_events_url":"https://api.github.com/users/drbyte/received_events","type":"User","site_admin":false},"created_at":"2018-11-09T22:41:51Z","updated_at":"2018-11-09T22:41:51Z","author_association":"COLLABORATOR","body":"> The naming of checkHasPermissionTo() inside HasPermissions could be confused with the newly added method checkPermissionTo()\r\n\r\nI wonder if renaming the new `checkHasPermissionTo` to `uncachedHasPermissionTo` would suit. It's more explicit on its intent so doesn't require documenting."}}
{"actor":{"display_login":"drbyte"}}