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.

Files: 
CommentFileSizeAuthor
#18 1768576-18-remove-role-name-from-user-object.patch2.55 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 40,523 pass(es). View
#16 1768576-16-remove-role-name-from-user-object.patch2.96 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 40,537 pass(es), 1 fail(s), and 0 exception(s). View
#13 1768576-13-remove-role-name-from-user-object.patch2.01 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 40,518 pass(es), 3 fail(s), and 0 exception(s). View
#10 1768576-10-remove-role-name-from-user-object.patch2.01 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 27,035 pass(es), 11,627 fail(s), and 1,485 exception(s). View
#8 1768576-7-remove-role-name-from-user-object.patch1.99 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#5 1768576-5-remove-role-name-from-user-object.patch1.96 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 29,610 pass(es), 10,618 fail(s), and 1,261 exception(s). View
#3 drupal8.user-roles.3.patch1.97 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,326 pass(es), 91 fail(s), and 8,072 exception(s). View
#2 drupal8.user-roles.1.patch1.21 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,515 pass(es), 3 fail(s), and 0 exception(s). View
#1 1768576-1-remove-role-name-from-user-object.patch778 bytesheyrocker
FAILED: [[SimpleTest]]: [MySQL] 40,319 pass(es), 91 fail(s), and 8,076 exception(s). View

Comments

heyrocker’s picture

Status: Active » Needs review
FileSize
778 bytes
FAILED: [[SimpleTest]]: [MySQL] 40,319 pass(es), 91 fail(s), and 8,076 exception(s). View

Seeing what the bot thinks

sun’s picture

FileSize
1.21 KB
FAILED: [[SimpleTest]]: [MySQL] 40,515 pass(es), 3 fail(s), and 0 exception(s). View

Do eeeeet!

sun’s picture

FileSize
1.97 KB
FAILED: [[SimpleTest]]: [MySQL] 40,326 pass(es), 91 fail(s), and 8,072 exception(s). View

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

Status: Needs review » Needs work

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

heyrocker’s picture

FileSize
1.96 KB
FAILED: [[SimpleTest]]: [MySQL] 29,610 pass(es), 10,618 fail(s), and 1,261 exception(s). View

Duh.

heyrocker’s picture

Status: Needs work » Needs review
heyrocker’s picture

Status: Needs review » Needs work

No that's broken too, hang on

heyrocker’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

Status: Needs review » Needs work

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

heyrocker’s picture

FileSize
2.01 KB
FAILED: [[SimpleTest]]: [MySQL] 27,035 pass(es), 11,627 fail(s), and 1,485 exception(s). View

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.

heyrocker’s picture

FileSize
2.01 KB
FAILED: [[SimpleTest]]: [MySQL] 40,518 pass(es), 3 fail(s), and 0 exception(s). View

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.

heyrocker’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.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
FAILED: [[SimpleTest]]: [MySQL] 40,537 pass(es), 1 fail(s), and 0 exception(s). View

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.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
PASSED: [[SimpleTest]]: [MySQL] 40,523 pass(es). View

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.