Problem

When generating the $user->roles array, we used to be able to grab the rids and the names in one query. With the move of roles to CMI (#1479454: Convert user roles to configurables), this is no longer possible. We can grab the rids from {user_roles} but then we need to open a config object to match up the names. This happens on cached page loads in _drupal_session_read(), so it would be nice not to have that overhead.

Solution

Remove the role names from the $user->roles array, and just store the rids. This shouldn't be a problem in D8 now that the rids are machine names and human readable.

Unsure what kind of chaos this causes, gonna throw together a patch here and just see what fails.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gdd’s picture

Status: Active » Needs review
FileSize
778 bytes

Seeing what the bot thinks

sun’s picture

FileSize
1.21 KB

Do eeeeet!

sun’s picture

FileSize
1.97 KB

hah! Two brilliant minds and so on... ;)

Status: Needs review » Needs work

The last submitted patch, drupal8.user-roles.3.patch, failed testing.

gdd’s picture

gdd’s picture

Status: Needs work » Needs review
gdd’s picture

Status: Needs review » Needs work

No that's broken too, hang on

gdd’s picture

Status: Needs work » Needs review
FileSize
1.99 KB

Status: Needs review » Needs work

The last submitted patch, 1768576-7-remove-role-name-from-user-object.patch, failed testing.

gdd’s picture

That one is broken too because of debug code I left in, I'm just being dumb now. Although it appears the testbot is being a bit weird today too.

Tor Arne Thune’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1768576-10-remove-role-name-from-user-object.patch, failed testing.

gdd’s picture

OK, indexed arrays in roles is bad. This is the same as the first one, it just uses fetchAllKeyed(0,0) to make the array key and value both be the rid. Sun had actually already done that in his patch with drupal_map_assoc() and I just made them both consistent.

gdd’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1768576-13-remove-role-name-from-user-object.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

This passes for me locally.

I don't think we actually should change user_roles() as that function is used in a lot of places to display a list of roles, and as such should include the role names (that is where the above test failures came from.) We only really want to change the $roles array on the user object, and user_roles() is never actually used for that purpose.

This patch also cleans up a few more hardcoded role names.

Status: Needs review » Needs work

The last submitted patch, 1768576-16-remove-role-name-from-user-object.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review
FileSize
2.55 KB

That failure isn't happening for me locally, but I have a cleanup patch anyways that

- Reverts the removal of the 'Administrative features' comment
- Fixes a small spacing issue

Hopefully a retest will go ok

sun’s picture

Status: Needs review » Reviewed & tested by the community

Awesome!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.