Add static caching for user_roles()

I think that you should use a static variable instead of a global one.

What makes you conclude that user_roles() gets called often, or that it represents a performance bottleneck?

That's odd. user_roles() appears to get called once per admin user edit form. And even then, it's not that expensive -- it's way down the list when I run a profiler against it.

That's what I would think, Chris. I'm marking this "won't fix" for now.

Six years later... I've seen a number of modules that need to access the roles list regularly (for example, access control modules) cache the list of roles with a static variable in their own _module_user_roles() function. (In fact, I've seen namespace collisions between modules that named them merely _user_roles().) It's definitely valuable when performing any sort of batch operations.

Are there any implications I'm missing?

our custom stuff also has a wrapper around user_roles (we have a lot of stuff to do with to roles, permissions). It'd be quite nice to not have to do that.

Agree...I've got several user_load queries that are repeated...several of these showing up firebug>drupal>sql

user_load SELECT r.rid, r.name FROM role r INNER JOIN users_roles ur ON ur.rid = r.rid WHERE ur.uid =

Gave it a go. The parameters for user_roles() made things kind of tricky, but I think I nailed it.

D6 backport.

I managed to sucker catch into looking at this this evening. He pointed out that drupal_static() needs an & (d'oh) and that also the way that $roles is being assigned, the query might not be being run if a $permission value is passed - though after re-applying the patch and looking at the whole function, I think this assessment may be incorrect. I'll try to spend some more time with this over the weekend.

Reroll to add the &. As in my previous post, I can't see how the query wouldn't run when $permission is passed, so I think all is well.

Perhaps a simpler approach?

Even simpler.

…and fixing coding standards.

This looks great, the caching is the same as #14 but it's a bit more self-contained. afaik core still doesn't call this function many times per page, but it definitely happens a lot on contrib modules (which is fair enough since sometimes you want to check roles rather than permissions).

Marking RTBC 7 years later!

#17 looks nice and clean. But do we want to cache cases where $permission is non-null as well?

We certainly could, but it might be a micro-optimization.

Any time I've seen user_roles() show up when profiling its called with no args. If a perm is specified, that query should be indexed, so I don't know that we'd gain much.

Ditto; the likelyhood that two different modules are calling user_roles for the same permissions often enough to benefit seems a bit slim.

+++ b/modules/user/user.moduleundefined
@@ -2785,6 +2785,15 @@ function user_mail_tokens(&$replacements, $data, $options) {
+    $cid = (int) $membersonly;
+    if (isset($user_roles[$cid])) {
+      return $user_roles[$cid];
+    }

This doesn't really make sense to me on reading it and could use an inline comment. I can't see any explanation why we're also not caching if $permission is specified?

1 days to next Drupal core point release.

Yeah we should add a comment for no caching when $permission is specified definitely.

Rerolled for #23 through #25.

Variant with a more detailed comment justifying the exclusion of specific permissions.

This looks quite good to me as well.

Comments were a little redundant. Removed the first one and moved the bottom comment up.

Committed to 8.x, moving to 7.x for webchick.

Personally I would rather leave this caching to the caller. But anyway, adding this static cache could break D7 code that expects to be able to change roles during the request. I would not commit it to 7, personally.

Static caching in the caller doesn't help if you have four or five different contrib modules all doing their own calls to the function - you'll end up with 5 duplicate queries and 5 duplicate caches.

Moving to CNR for 7.x though.

Doesn't this cache need to be cleared when user roles change?

A reasonable request.

Attached patch should take care of added roles

[dupe]

Let's start with this patch. These tests should fail, and we need to make them pass :)

A helper function as proposed in #39 makes sense to me.... and #40 is also a good point (the tests I wrote here won't actually catch that issue - we'd have to extend them to test the user role hooks also if we wanted to deal with that).

#43 looks fine to me. It's a bit more complex but the result is more sensible.

I think I'd like to see a comment above if ($permission || !$user_roles) {, it's pretty confusing as is. Maybe the one below could just be moved up there.

Except it's failing testbot. ;)

#52 looks great. Comments are very clear.

+      // The $roles array is sorted by key so that the DRUPAL_ANONYMOUS_RID
+      // role is always the first element in the array, allowing us to use less
+      // memory by keeping one list of roles. The sort makes it easy to return
+      // a list with or without the anonymous role using array_slice().
+      ksort($roles);

I think this breaks the feature where you're allowed to reorder the list of roles using drag-and-drop.... We need to make sure that user_roles() always returns the role list ordered by weight.

It's also slightly uncomfortable that it relies on a specific numeric value of DRUPAL_ANONYMOUS_RID (in relation to the other roles), though I suppose it's probably not the only code in Drupal that does that.

+    // Prime the user_roles() static cache.
+    user_roles();
....
+    // Clear the static cache, then reload it, so we can be sure that the role
+    // we're about to delete is there, otherwise we're not realing testing for
+    // staleness after the delete.
+    drupal_static_reset('user_roles');
+    user_roles();

I don't think we should bother with this, actually. The goal here is to test that the role API works correctly, not to test the static cache (especially since a properly working static cache should be invisible to outside callers of the API), right? As long as some of the tests will fail when the static cache is broken (which they did), I don't think we should do special manipulations to make sure all of them will fail. It's better just to have the test be clean.

+++ b/modules/user/user_static_cache_test.info

For similar reasons as above, I'd recommend calling this module something more like 'user_role_api_test'?

Re-rolled #52 for the /core change and renamed the test module per #54.

Wrote a new issue summary. Feel free take it to another level.
Rerolled from 57 (which last applied at d13fd18f1e2dbd02a2efba5786dfbaa6dbd58cca)
Cleaned up whitespace and changed DrupalWebTestCase to WebTestBase.
This patch applies cleanly at faa63d2.

Sending to the bot. Thanks @August1914!

Rerolled because of #935062: Change role id to machine name and #1594260: Convert user tests to PSR-0. This was a messy reroll, and should be reviewed. Tests still do not pass.

A couple notes, due to the changes in #935062: Change role id to machine name:

1. The ksort() and array_slice() definitely needs to be reworked, since the key is no longer the role ID but the role's machine name.
2. I think a 7.x backport will need to be reconsidered as well.

Updated the issue summary.

Using the issue summary template, and updating text here and there

Cleaning up wording in first paragraph, other minor tweaks.

+++ b/core/modules/user/user.module
@@ -2638,32 +2638,36 @@ function user_mail_tokens(&$replacements, $data, $options) {
+    if ($membersonly) {
+      $query->condition('r.rid', DRUPAL_ANONYMOUS_RID, '!=');
+    }
...
+    return $membersonly ? array_slice($user_roles, 1, count($user_roles), TRUE) : $user_roles;

This looks like a bug.

Ignore these patches unless you're looking for d6 version of them.

(Again, ignore this, d6 -- was missing an array_filter)

Linkifying comment numbers.

Correcting version.

Correcting version again.

The patch was already committed to Drupal 8 in #6463-32: Add static caching for user_roles() in commit #17329b1

Then the static caching was removed by #2572667: Deleting a role doesn't invalidate user_roles() static cache.

Changing to Drupal 7