Convert user_access to User::hasPermission()

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.php
@@ -29,6 +31,12 @@ public static function getInfo() {
+    $this->access = drupal_container()->get('user.access');

This should be \Drupal::service('user.access') now.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.php
@@ -23,7 +24,7 @@ public static function getInfo() {
+    $this->access = drupal_container()->get('user.access');

Ibid.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.php
@@ -56,9 +57,9 @@ function testUserPermissionChanges() {
+    drupal_static_reset('Drupal\user\UserAccess::check');

This is right out, because drupal_static() should never be used inside an object.

+++ b/core/modules/user/lib/Drupal/user/UserAccess.php
@@ -0,0 +1,44 @@
+  public function check($string, $account = NULL) {
+    global $user;
+
+    if (!isset($account)) {
+      $account = $user;
+    }

The default-to-global logic is flawed to begin with. Let's just make $account required. From a service perspective, that's what we want as it makes the object idempotent.

+++ b/core/modules/user/lib/Drupal/user/UserAccess.php
@@ -0,0 +1,44 @@
+    // To reduce the number of SQL queries, we cache the user's permissions
+    // in a static variable.
+    // Use the advanced drupal_static() pattern, since this is called very often.
+    static $drupal_static_fast;
+    if (!isset($drupal_static_fast)) {
+      $drupal_static_fast['perm'] = &drupal_static('Drupal\user\UserAccess::check');

drupal_static() is completely unnecessary inside an object. That's what protected object properties are for.

+++ b/core/modules/user/lib/Drupal/user/UserAccess.php
@@ -0,0 +1,44 @@
+      $role_permissions = user_role_permissions($account->roles);

Blargh. Can we turn that function into a service, too? Or, put it on some existing service? Or, something?

Actually, looking at the function, it should just be a method on this service, and then this service depends on the DB connection. Done. Then we can deprecate both functions.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.phpundefined
@@ -21,6 +21,8 @@ class CommentLinksTest extends CommentTestBase {
+  protected $access;

Needs docs.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.phpundefined
@@ -29,6 +31,12 @@ public static function getInfo() {
+    $this->access = \Drupal::service('user.access');

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.phpundefined
@@ -36,8 +36,8 @@ public function resetCache(array $ids = NULL) {
+    \Drupal::service('user.access')->resetCache();
+    \Drupal::service('user.access')->resetRoleCache();

Afaik we should use $this->container->get('user.access')

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.phpundefined
@@ -10,6 +10,7 @@
+  protected $access;

@@ -23,7 +24,7 @@ public static function getInfo() {
+    $this->access = \Drupal::service('user.access');

Maybe accessService would describe better what this class is about.

+++ b/core/modules/user/lib/Drupal/user/UserAccess.phpundefined
@@ -0,0 +1,122 @@
+  /**
+   * Implements \Drupal\Core\ControllerInterface::create().
+   */

Should be {@inheritdoc} now.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.phpundefined
--- /dev/null
+++ b/core/modules/user/lib/Drupal/user/UserAccess.phpundefined

+++ b/core/modules/user/user.services.ymlundefined
@@ -13,3 +13,6 @@ services:
+  user.access:
+    class: Drupal\user\UserAccess
+    arguments: ['@database']

I'm wondering whether this should be something provided by user module or whether this is a central core service.

+++ b/core/modules/user/lib/Drupal/user/UserAccess.phpundefined
@@ -0,0 +1,122 @@
+  public function resetCache() {
+    // @todo -rename to permisisonCache to distinguish from roles cache
+    $this->perm = array();
+  }

I guess it would make sense to have a method which resets both at the same time. What about just ->reset()?

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.phpundefined
@@ -21,6 +21,8 @@ class CommentLinksTest extends CommentTestBase {
+  protected $access;

Needs docs.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.phpundefined
@@ -29,6 +31,12 @@ public static function getInfo() {
+    $this->access = \Drupal::service('user.access');

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.phpundefined
@@ -36,8 +36,8 @@ public function resetCache(array $ids = NULL) {
+    \Drupal::service('user.access')->resetCache();
+    \Drupal::service('user.access')->resetRoleCache();

Afaik we should use $this->container->get('user.access')

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.phpundefined
@@ -10,6 +10,7 @@
+  protected $access;

@@ -23,7 +24,7 @@ public static function getInfo() {
+    $this->access = \Drupal::service('user.access');

Maybe accessService would describe better what this class is about.

+++ b/core/modules/user/lib/Drupal/user/UserAccess.phpundefined
@@ -0,0 +1,122 @@
+  /**
+   * Implements \Drupal\Core\ControllerInterface::create().
+   */

Should be {@inheritdoc} now.

I just hope you try to keep user_access() in this issue (which calls the service then) and people can start and use the service in new code.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.php
@@ -10,6 +10,13 @@
+  /**
+   * A UserAccess service object used for checking permissions within this test.
+   *
+   * @var \Drupal\user\UserAccess
+   */
+  protected $accessService;
   protected $admin_user;
   protected $rid;

At least a line break between the properties here. I won't ask you to document the other two since they're unrelated to this patch, but if you want to while you're in there no one will object. :-) (Also, $adminUser, not $admin_user, for object properties.)

+++ b/core/modules/user/lib/Drupal/user/UserAccess.php
@@ -0,0 +1,138 @@
+class UserAccess implements ControllerInterface {

This class is a pure service, not a controller. It should not be using ControllerInterface. It should just register with the container as service directly via user.services.yml.

We also need an interface defined for user access checking that this class can implement. That way someone else can swap it out with a MongoDB-based one, or whatever.

This also suggests that the service should also be responsible for collecting and saving user/role data, or at least some service should be. That way you can swap out the entire user permission system. Hm... Not sure if that's scope creep or not.

+++ b/core/modules/user/lib/Drupal/user/UserAccess.php
@@ -0,0 +1,138 @@
+      $result = db_query("SELECT rid, permission FROM {role_permission} WHERE rid IN (:fetch)", array(':fetch' => $fetch));

This should be using the injected DB connection.

+++ b/core/modules/user/lib/Drupal/user/UserAccessInterface.php
@@ -0,0 +1,46 @@
+  public function check($string, $account);

I'm not sure check() is the best name here. It's not really self-descriptive. hasPermission()? Which would then, I think, want to take ($account, $permission), not ($string, $account), so that it naturally reads $account hasPermission $permission.

+++ b/core/modules/user/lib/Drupal/user/UserAccessInterface.php
@@ -0,0 +1,46 @@
+   * @param $roles
+   *   An array whose keys are the role IDs of interest, such as $user->roles.

Specify $param array $roles for completeness.

+++ b/core/modules/user/lib/Drupal/user/UserAccessInterface.php
@@ -0,0 +1,46 @@
+  public function userRolePermissions($roles);

This should be type hinted to an array.

We should probably also inject this service into at least one of the other services that us currently calling user_access(). Probably the PermissionAccessCheck class is a good candidate. We don't need to all of them, but at least one to prove that it works. :-)

I'm still torn about converting the entire function set. It makes sense, but I worry about scope creep. Of course, with the other functions still hard coded to the database the swappability here doesn't buy us all that much.

Actually, bejeebus in IRC just pointed me at this issue, which is going to have a radical impact on anything going on here: #1872876: Turn role permission assignments into configuration.. :-(

So, yeah, we should wait for that to go in, and then revisit this issue to move all of the functions into a service.

Actually, bejeebus in IRC just pointed me at this issue, which is going to have a radical impact on anything going on here: #1872876: Turn role permission assignments into configuration.. :-(

So, yeah, we should wait for that to go in, and then revisit this issue to move all of the functions into a service.

I asked whether we can commit #1872876: Turn role permission assignments into configuration. without the last part of cleaning up stale permissions on module uninstallation for now so we can proceed with this patch. If it's a "nay" we should proceed with this. Should be much faster to get this to a clean state than the other patch due to the dependency on #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed that it would imply in that case. It's not that much work to re-roll the patch over at #1872876: Turn role permission assignments into configuration. anyways.

Here is a rerole, which injects the user access service into the permission access checker.

Sadly the tests did not passed due to the following issue: #2015721: Adding a role to a user throws a php error

Oh I think this was dreditor.

Fixing the fails and integrating with AccountInterface from #1825332: Introduce an AccountInterface to represent the current user.

Wonder if we should move this to Drupal\Core now that we got that AccountInterface? Has to be role agnostic then too though.

Whoops, forgot a file.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.phpundefined
@@ -24,48 +43,47 @@ public static function getInfo() {
+    \Drupal::service('user.access')->reset();
...
+    \Drupal::service('user.access')->reset();

Should/could we also call $this->container->get('user.access')->reset()?

Potential follow ups:

• Convert parts of UserPermissionTest to a proper unit test
• Add a Drupal::userAccess() or similar method
• Move diverse things from user to core

Should/could we also call $this->container->get('user.access')->reset()?

Of course! Missed that. Thanks

Potential follow ups:

Convert parts of UserPermissionTest to a proper unit test
Add a Drupal::userAccess() or similar method
Move diverse things from user to core

Agreed. (2) and (3) are consequentially the same though :)

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.phpundefined
@@ -31,6 +38,12 @@ public static function getInfo() {
+  function setUp() {

Should have an @inheritdoc

+++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.phpundefined
@@ -17,6 +18,23 @@
+  protected $userAccess;

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.phpundefined
@@ -10,7 +10,26 @@
+  protected $accessService;

We should make that consistent.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.phpundefined
@@ -36,8 +36,7 @@ public function resetCache(array $ids = NULL) {
+    \Drupal::service('user.access')->reset();

Entity controller can have services injected now.

+++ b/core/modules/user/lib/Drupal/user/UserAccessInterface.phpundefined
@@ -0,0 +1,51 @@
+   * Determines the permissions for one or more roles.
...
+  public function userRolePermissions(array $roles);

So we agreed that there should be a follow up for this as well.

So we agreed that there should be a follow up for this as well.

Okay, moved it out again. I like the idea of having a single service for managing all this stuff in one place as #28 suggests but yeah, that's scope creep.

Anyways there's still something wrong in the upgrade pass. Don't have time to look at those fails now.

Point one: Think about patches if they get smaller :) (UserAccess and UserAccessInterface got lost)

For example "BASIC MINIMAL PROFILE UPGRADE, FREE ACCESS" will fail.

Mh, this fixes some bits but still has small upgrade path fails. You can't create a User object as somehow the container is not ready yet.

Berdir has the point that there should be a method on the account interface (hasPermission) instead of having a separate service.

The reason for doing it as a method is that it's easier to use (we could still have a proxy method, sure), but mostly that after permissions are also stored in the role config entity, this comes down to the following pseudo-code:

function hasPermission($permission) {
  foreach ($roles as $role) {
    if ($role->hasPermission($permission)) {
      return TRUE;
    }
  }
  return FALSE;
}

Maybe with a bit caching and stuff, but no database queries involved anymore. Why make a service of this?

Combine it with passing the current user into the access checks as discussed yesterday, the permission and role checks become really simple and nice (apart form the +/, stuff).

I agree. Let's just make this a method on the User class and be done with it. No service needed.

Retitling.

Yeah.. agreed

Note that there are still a few places where global user is not yet an AccountInterface, e.g. UpgradePathTestBase. #2017207: [meta] Complete conversion of users to Entity Field API has fixes for that.

Just playing around a bit... I think we can drop the static user access cache. Even more so once #1872876: Turn role permission assignments into configuration. lands which moves permissions onto the role entity.

I am not sure if we should do more here.. The rest should probably happen in #1872876: Turn role permission assignments into configuration..

One thing that bothers me is that addRole(), etc. currently use $rid instead of a full $role object. We might want to change that.

Forgot a return statement.

I'm sorry but it feels really wrong to have the same code twice.

We have two quite different classes that implement the same interface, they have to have similar implementations, but it will change more I think. I'm adding a few additional methods to that interface in the issue references in #53, they will have "duplicate "/similar implementations there as well.

I expect they will not be that similar in the end, If we go with msonnabaums idea, we might e.g. add something like a UserOneSession class, that subclasses UserSession and just returns TRUE in hasPermission(). We'll see.

Wow, we have 23 upgrade path tests now ;)

As mentioned before, if you merge in the changes to UpgradePathTestBase from #2017207: [meta] Complete conversion of users to Entity Field API, you should have a green patch.

As mentioned before, if you merge in the changes to UpgradePathTestBase from #2017207: Complete conversion of users to Entity Field API, you should have a green patch.

Checking if that is true!

Okay, @berdir and I agreed that we should do some profiling despite the fact that we both also concluded that it probably won't make much of a performance difference as we are really just doing the static caching to cut on function calls (slightly). So if anyone want's to do that today, go ahead... Otherwise I'll get back to it tomorrow.

Refactoring patch now that #1872876: Turn role permission assignments into configuration. is in.

Here is a unit test for both User and UserSession.

#65: user_access-1966334-65.patch queued for re-testing.

So, there is a problem for phpunit/autoloading if you use the simpletest UI, so let's setup stuff different.

So never assume that simple code always work.

#71: drupal-1966334-71.patch queued for re-testing.

Adding tag

The problem with the current patch seems somehow that the global $user is not the one which is logged in the test.

this should be green

tag

This was certainly a good fix.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.phpundefined
@@ -144,15 +145,12 @@ protected function setUp() {
+    // Load rolest.

Maybe something like "// Load roles of the user."?

yes:)

#80: drupal-1966334-80.patch queued for re-testing.

Thank you!

Very good. Thanks. RTBC +1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

reroll after adding methods to AccountInterface got in

note: size difference is due to exact same changes in UpgradePathTestBase happened there:)

OMG this patch has an error level of over 9000!

Of course, this is sparta! If we blow up tests, we always reach the 9000. Easy. SPARTA!

Overall Diff Summary

which is because user_access is a wrapper now, and it is called 48 times:P

re-rtbcing, i totally missed load -> loadMultiple changed thanks dawehner:)

Committed to 8.x. Two follow-ups:

1) Can we mark user_access as deprecated?

2) Can you explain what profiling you have done? This patch removes some caching. It does not look to be material due to caching at different levels but it would be good to understand what kind of profiling you did.

Thanks!

Switching for a followup :)

Here is the follow up: #2045361: Mark user_access as deprecated

@fubhy, please speak about the caching.

Thanks @dawehner!

This patch removes some caching. It does not look to be material due to caching at different levels but it would be good to understand what kind of profiling you did.

Exactly.. So while we removed the direct static caching in user_access() we are still using the values from the $role entities (which are already in memory) and the role assignments of the user entity (also in memory). Instead of directly returning the cached values from user_access() we now go to through both, user entity and role entity, to determine access:

$user->hasPermission() -> $role->hasPermission() (for all roles of a user)

This makes the whole system much more solid and decoupled as it's only bound to the static entity caching instead of multiple, custom caching layers (which you would have to be aware of in order to do proper cache invalidation upon write / delete operations, e.g. when deleting a role). Now, when you remove a user role assignment, or add a new role to the user, or add a new permission to a role, it is automatically reflected in the next $user->hasPermission() check without the need of any cache invalidation (entity system takes care of that).

The only thing that we add are some (very few) additional function calls which obviously doesn't affect performance much.

EDIT: As to the question "what kind of profiling was done" we would have to ask @ParisLiakos.

Hey King Leonidas? Can you give us some detail on the profiling you did?

Untagging

Hey King Leonidas? Can you give us some detail on the profiling you did?

I believe you are talking to me:p

it was against the D8 HEAD (at the time) front page. First run was before the patch second run after patch..the only change was 48 more function calls which completely make sense since user_access is now a wrapper..when we have converted everything to call hasPermission() directly there will be no performance difference..that 1kb of ram i didnt even bother to check more

Well, what @effulgentsia pointed out is that the real thing to profile would be a site with a lot of roles, and with individual users with lots of roles. I seriously doubt there would actually be a negative impact from the patch, and overall the removal of the cache can only be a good thing, I think, as @fubhy explains in #96.

experimental conversion done

Added a follow-up: #2048171: [meta] Replace user_access() calls with $account->hasPermission() wherever possible.

https://drupal.org/node/2049309

Fixed some formatting issues, change notice looks good to me. Thanks @dawehner.

Sample code in the change notice needs to account for cases where you want to check access for a user other than the one making the request.

Updated