The Task

Convert the current permissions system to use the new configuration system.

Current implementation

  • Individual permissions are defined by modules in hook_permission().
  • Roles are defined in the admin area at admin/people/permissions/roles and stored in the {role} table (with the exception of the anonymous and authenticated user roles, which are added at install time and locked.)
  • Users are assigned roles on the user edit page, and this is stored in the {users_roles} table
  • Roles have permissions added or removed at admin/people/permissions, and this data is stored in the role {role_permission}.
  • The data in the {role_permission} and {user_role} tables are the central way to determine whether or not a user has permission to do something.

Proposed implementation

This is going to need some discussion about, and I have not done any real thoughtwork behind it other than five minutes of dicussion on irc with xjm and msonnabaum. An interesting thing to think about here is that the permissions themselves are defined in code by modules, but the actual USE of those permissions (aka what permissions are available for what roles) is very much a config issue. If we follow our current trend, we would want to get rid of hook_permission() and have modules define permissions in config files (system.permission.administer_foo.xml). Then there would be a separate file that essentially implements {role_permission} and another for {user_role}. Oh and what are roles anyways? Do roles belong in the config system? If so then how do we attach them to users? One suggestion is that roles are a property on user entities, and this list of available roles is in config.

We are going to see a lot of systems where these kinds of questions come up (think blocks for instance) and these systems that bridge config and content (aka entities) are going to be among some of the most difficult to deal with.

Looking for feedback and ideas to form some consensus around a solution at this point.

Other considerations

This is central to Drupal's entire security model, we want to make sure we get it right.
This issue is related #935062: Change role id to machine name. In that issue, roles are treated as pure config which I think is the right approach as well. So if that patch lands, it may just become a matter of converting roles to CMI and we're done with that part of it.

Follow-up

#1872870: Implement a RoleListController and RoleFormController
#1751274: Do not query user_roles directly
#1872876: Turn role permission assignments into configuration.

Files: 
CommentFileSizeAuthor
#160 drupal-1479454-159.patch52.31 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 50,788 pass(es). View
#152 user.config.151.patch52.31 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,630 pass(es), 1 fail(s), and 0 exception(s). View
#152 interdiff.txt5.37 KBsun
#151 149-151.interdiff.txt656 bytesfubhy
#151 1479454-151.patch53.04 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 50,689 pass(es). View
#149 1479454-149.patch53.02 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 50,559 pass(es), 8 fail(s), and 7 exception(s). View
#149 146-149.interdiff.txt7.71 KBfubhy
#146 user-roles-1479454-146.patch56.11 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,575 pass(es). View
#144 132-144-interdiff.txt3.91 KBalexpott
#144 1479454-144.user-roles.debugging-translation-failures.patch56.13 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1479454-144.user-roles.debugging-translation-failures.patch. Unable to apply patch. See the log in the details link for more information. View
#132 1479454-132.patch52.06 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 50,241 pass(es), 1 fail(s), and 0 exception(s). View
#126 121-126-interdiff.txt2.54 KBalexpott
#126 1479454-126-user-roles.patch51.95 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1479454-126-user-roles.patch. Unable to apply patch. See the log in the details link for more information. View
#121 1479454-121-user-roles.patch51.96 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 49,911 pass(es). View
#121 116-121-interdiff.txt6.54 KBalexpott
#116 115-116-interdiff.txt2.07 KBalexpott
#116 1479454-116-user-roles.patch49.97 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 49,510 pass(es), 9 fail(s), and 13 exception(s). View
#115 1479454-115-user-roles.patch49.88 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 50,713 pass(es). View
#108 1479454-108-user-roles.patch48.82 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 50,552 pass(es). View
#106 1479454-106.patch42.33 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 50,090 pass(es). View
#104 1479454-104.patch42.22 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 50,105 pass(es). View
#104 1479454-104.interdiff.txt1.63 KBfubhy
#102 user-roles-config-1479454.102.interdif.txt1.92 KBlarowlan
#102 user-roles-config-1479454.102.patch42.34 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 50,075 pass(es). View
#98 1479454-97.patch40.88 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 49,428 pass(es). View
#96 1479454-96.patch39.8 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 49,376 pass(es), 10 fail(s), and 0 exception(s). View
#95 1479454-95.patch42.9 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 49,364 pass(es), 10 fail(s), and 0 exception(s). View
#89 1479454-roles-as-config-89.patch50.67 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 49,392 pass(es). View
#85 1479454-roles-as-config-85.patch50.68 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 49,381 pass(es), 1 fail(s), and 3 exception(s). View
#81 1479454-81.patch50.68 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 49,273 pass(es). View
#81 1479454-81-80-interdiff.txt6.84 KBfubhy
#80 1479454-80.patch49.65 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.module. View
#76 1479454-76.patch47.61 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 48,582 pass(es), 74 fail(s), and 60 exception(s). View
#71 1479454-roles-71.patch58.18 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 49,102 pass(es), 60 fail(s), and 34 exception(s). View
#68 1479454-interdiff-68.txt1.04 KBandypost
#68 1479454-roles-68.patch55.65 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,589 pass(es), 69 fail(s), and 67 exception(s). View
#67 1479454-interdiff-62-67.txt3.36 KBandypost
#67 1479454-roles-67.patch55.14 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,911 pass(es), 57 fail(s), and 31 exception(s). View
#66 1479454-roles-65.patch53.76 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 48,579 pass(es), 69 fail(s), and 72 exception(s). View
#64 1479454-roles-63.patch53.51 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#64 interdiff.txt1.41 KBmarcingy
#62 1479454-interdiff-61.txt2.82 KBandypost
#62 1479454-roles-61.patch53.32 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,489 pass(es), 73 fail(s), and 81 exception(s). View
#60 1479454-interdiff-58.txt4.69 KBandypost
#60 1479454-roles-58.patch53.62 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,512 pass(es), 82 fail(s), and 84 exception(s). View
#57 1479454-roles-57.patch50.48 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.install. View
#55 1479454-roles-as-config-55.patch50.48 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/lib/Drupal/user/UserStorageController.php. View
#51 1479454-51.patch50.32 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 46,277 pass(es), 121 fail(s), and 60 exception(s). View
#49 1479454-49.patch47.37 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 46,112 pass(es), 1 fail(s), and 0 exception(s). View
#47 1479454-47.patch47.4 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 46,392 pass(es), 0 fail(s), and 1 exception(s). View
#45 1479454-45.patch47.33 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 46,030 pass(es), 26 fail(s), and 15 exception(s). View
#41 drupal-1479454-41.patch48.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 46,341 pass(es), 1 fail(s), and 0 exception(s). View
#39 interdiff.txt3.89 KBdawehner
#39 drupal-1817582-39.patch8.32 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 46,308 pass(es), 0 fail(s), and 12 exception(s). View
#37 1479454-37.patch45.67 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 46,318 pass(es), 1 fail(s), and 0 exception(s). View
#35 1479454-35.patch45 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 45,484 pass(es), 838 fail(s), and 0 exception(s). View
#34 1479454-34.patch31.78 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 46,409 pass(es). View
#31 interdiff.txt1.34 KBdawehner
#31 drupal-1479454-30.patch30.61 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 46,354 pass(es), 22 fail(s), and 3 exception(s). View
#28 1479454-28.patch29.49 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 46,345 pass(es), 21 fail(s), and 3 exception(s). View
#26 1479454-26.patch26.39 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 46,334 pass(es), 39 fail(s), and 46 exception(s). View
#22 1479454-22-roles-in-cmi.patch18.79 KBgalooph
PASSED: [[SimpleTest]]: [MySQL] 41,326 pass(es). View
#17 1479454-17-roles-in-cmi.patch18.84 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 40,523 pass(es). View
#15 roles-in-cmi-4.patch20.31 KBHugo Wetterberg
PASSED: [[SimpleTest]]: [MySQL] 40,556 pass(es). View
#12 roles-in-cmi-3.patch16.48 KBHugo Wetterberg
FAILED: [[SimpleTest]]: [MySQL] 40,507 pass(es), 38 fail(s), and 296 exception(s). View
#10 roles-in-cmi-2.patch15.83 KBHugo Wetterberg
FAILED: [[SimpleTest]]: [MySQL] 40,504 pass(es), 45 fail(s), and 520 exception(s). View
#8 roles-in-cmi.patch11.55 KBHugo Wetterberg
FAILED: [[SimpleTest]]: [MySQL] 40,566 pass(es), 2 fail(s), and 6 exception(s). View

Comments

heyrocker’s picture

Issue summary: View changes

Adding to pointers to related issue

neclimdul’s picture

I guess I see it as 3 things. I have to admit a bit of grey area in how the listed features actually fall depending on the site. We do basically allow anything to happen so there's a special case for everything.

  1. System state: Compiled information about the system. Built by the application so it fundamentally knows how to run, this is essentially code and doesn't have any user interaction. I see this as things like compiled versions of information from info hooks like hook_permissions.
  2. Exportables. Admin built information that's built once per site and then maintained or never touched. Panels, views and roles these sort of things. They're also essentially code but if broken the site would need developer interaction and couldn't recover. They're also things a site builder would actually create. Roles and the permissions attached to them see like they fit here.
  3. Site data. This is your everyday user interaction. You have normal users create nodes, comments, register for your site and create users. I see a user's roles as part of this because they're going to be managed in relation to users and users aren't something you're going to code into your site(special cases aside). That means you can't really code the connection between individual users and roles.

Right now we don't have a robust solution to 1) and we generally do things like stuff the information in a cache table or some serialized field in a random database table. I personally think the lack of user interaction makes this separate from CMI as it now and had high hopes for the different K/V threads. 2) is clearly CMI and 3) is clearly everything else. db tables, entities, etc.

beejeebus’s picture

i started writing a script to generate XML config files for core modules (and we should provide this tool for contrib too).

then i hit the obvious point - many module's permissions are dependent on which other modules/filters/etc... are enabled at the time their hook_permission() implementation is run. so - how do i ship this as a module?

beejeebus’s picture

i'll post the script to generate the XML files anyway once i finish it...

beejeebus’s picture

here's the Gist to generate XML files for permissions:

https://gist.github.com/2139409

johnbarclay’s picture

This sounds like a good starting point for me to do some cmi work. If no one else is doing this can I assign it to myself?

heyrocker’s picture

I talked to several people about this in Denver, and I think everyone agrees that the list of roles that are available should be configuration, so converting that could be a first step. Then I (personally) feel that a user's assigned roles should be a field on the user entity.

@beejeebus can you provide an example of where permissions are dynamically created based on other modules' info?

Hugo Wetterberg’s picture

Status: Active » Needs review
FileSize
11.55 KB
FAILED: [[SimpleTest]]: [MySQL] 40,566 pass(es), 2 fail(s), and 6 exception(s). View

First draft for getting roles into cmi.

Status: Needs review » Needs work

The last submitted patch, roles-in-cmi.patch, failed testing.

Hugo Wetterberg’s picture

Status: Needs work » Needs review
FileSize
15.83 KB
FAILED: [[SimpleTest]]: [MySQL] 40,504 pass(es), 45 fail(s), and 520 exception(s). View

Tests should be passing now

Status: Needs review » Needs work

The last submitted patch, roles-in-cmi-2.patch, failed testing.

Hugo Wetterberg’s picture

FileSize
16.48 KB
FAILED: [[SimpleTest]]: [MySQL] 40,507 pass(es), 38 fail(s), and 296 exception(s). View

Well, that was a spectacular failure, amazing how many errors you can get from one error in block visibility settings. Long live testing :)

This should put everything back in order.

Hugo Wetterberg’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, roles-in-cmi-3.patch, failed testing.

Hugo Wetterberg’s picture

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

Apparently I had some unsaved files. The the filter & comment module has now been updated to use the new user_role_names method.

Hugo Wetterberg’s picture

Oh yeah! Tests passed, so it's time to describe the changes a bit:

Only the roles have been migrated to cmi, permissions have been left alone. The default config contains the "anonymous" and "authenticated" roles, the "administrator" role is added programatically by the standard install profile.

There's been one API change for the user module. The `user_roles` function which previously returned "An associative array with the role id as the key and the role name as [...] value" now has been changed to return "An associative array with the role id as the key and the role object as [...] value". The function `user_role_names`has been added and it has the exact same characteristics as the `user_roles` function previously had. This is what's brought on the changes in the comment, block and filter modules.

heyrocker’s picture

FileSize
18.84 KB
PASSED: [[SimpleTest]]: [MySQL] 40,523 pass(es). View

I think the API change for user_roles() vs user_role_names() makes sense, the new function name is more clear and they better reflect what they actually do.

+++ b/core/includes/session.incundefined
@@ -111,7 +111,13 @@ function _drupal_session_read($sid) {
+    $role_config = config('user.roles');
+    $rids = db_query("SELECT ur.rid FROM {users_roles} ur WHERE ur.uid = :uid", array(':uid' => $user->uid))->fetchCol();
+    foreach ($rids as $rid) {
+      $role = $role_config->get($rid);
+      $user->roles[$rid] = $role['name'];
+    }

This is probably a bit of a performance hit, but I'm not sure what else to do when we need to link SQL stuff to config data. If we removed the need to have the role names in the $user object, and just stored the rid, we could get this down to one query again. Now that rids are machine names and have some meaning, this might be worthwhile to do as a followup.

+++ b/core/modules/user/user.installundefined
@@ -475,5 +435,25 @@ function user_update_8003() {
+ * Replace the role table with configuration system as a beckend for storing roles.

Typo (backend)

+++ b/core/modules/user/user.moduleundefined
@@ -2356,26 +2375,42 @@ function user_roles($membersonly = FALSE, $permission = NULL) {
+    $filtered = array();
+    $query = db_select('role_permission', 'p');

Assuming we eventually bring {role_permission} into CMI, this is all going to change too eventually.

Other than the typo, most of this is just commentary. It also needed a reroll against head because new update hooks were added. I've done this reroll including the typo but now someone else will have to RTBC.

Nice patch, thanks for the help!

heyrocker’s picture

I actually opened the followup about removing role names - #1768576: Remove role names from the $user->roles array

Dries’s picture

Committed #1768576: Remove role names from the $user->roles array, which this issue depends on.

heyrocker’s picture

Status: Needs review » Needs work

This needs a reroll now

beejeebus’s picture

+    $rids = $query->execute()->fetchCol();
+
+    foreach ($rids as $rid) {
+      $filtered[$rid] = $roles[$rid];
+    }
+    $roles = $filtered;

could just be:

    $roles = array_intersect_key($roles, $query->execute()->fetchAll());

or whatever dbtng method returns things keyed by id...

galooph’s picture

Status: Needs work » Needs review
FileSize
18.79 KB
PASSED: [[SimpleTest]]: [MySQL] 41,326 pass(es). View

Here's the rerolled patch from #17.

I looked at bejeebus' comment in #21, but we need to step through the roles to add the human readable role name anyway, now that the names come from config('user.roles').

heyrocker’s picture

+++ b/core/includes/session.incundefined
@@ -111,7 +111,13 @@ function _drupal_session_read($sid) {
+
+    $role_config = config('user.roles');
+    $rids = db_query("SELECT ur.rid FROM {users_roles} ur WHERE ur.uid = :uid", array(':uid' => $user->uid))->fetchCol();
+    foreach ($rids as $rid) {
+      $role = $role_config->get($rid);
+      $user->roles[$rid] = $role['name'];
+    }

This entire hunk can be removed now, the whole point of #1768576: Remove role names from the $user->roles array was to be able to remove this code.

heyrocker’s picture

Also, before anyone asks, it is quite possible that this will be better represented as ConfigEntity, however there is another patch waiting behind this one that needs to get done before feature freeze, so I'd just as soon get this in and then do the conversion later.

fubhy’s picture

Assigned: Unassigned » fubhy
Status: Needs review » Active

Working on this now as well as on #1751274: Do not query user_roles directly

fubhy’s picture

Status: Active » Needs review
FileSize
26.39 KB
FAILED: [[SimpleTest]]: [MySQL] 46,334 pass(es), 39 fail(s), and 46 exception(s). View

Okay. This patch is the first step... It converts user roles into config entities. The next step would be to convert permissions to CMI.
Next up: Convert permissions to CMI.

fubhy’s picture

Looks like VDC added some more stuff that I didn't fix with the previous patch. Converting permissions to CMI turns out to be a bit more tricky than I initially thought. Need to chat with Greg first to find out how we want to solve that. Will fall back to fixing the Views stuff and probably converting some parts of the Roles UI to entity controllers (follow-up issues).

fubhy’s picture

Issue tags: -VDC-integration
FileSize
29.49 KB
FAILED: [[SimpleTest]]: [MySQL] 46,345 pass(es), 21 fail(s), and 3 exception(s). View

Okay.. Since we can easily work on permissions and roles in separate issues I am splitting this up. Since the previous patch already does the Roles => ConfigEntity conversion (except for Views, @timplunkett will bring that issue up in a VDC meeting shortly) let's make this the "Convert roles to configurables" issue.

Also tagging with VDC and Configurables as requested by Tim.

Also uploading a new patch with some minor fixes. I think the update path is not fully working yet either. Will need some more testing.

Edit: I am going to create the 'Convert permissions to CMI' issue after I was able to get ahold of Greg. Currently, I am not entirely sure how we would implement that in a way that would not completely ruin the DX for permissions.

dawehner’s picture

Title: Convert permissions (and roles?) to config system » Convert user roles to configurables
Issue tags: +VDC, +Configurables, +VDC-integration

Just adding a tag

Status: Needs review » Needs work
Issue tags: +VDC-integration

The last submitted patch, 1479454-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.61 KB
FAILED: [[SimpleTest]]: [MySQL] 46,354 pass(es), 22 fail(s), and 3 exception(s). View
1.34 KB

One problem i have with this patch
when you for example go to the people management you get

Fatal error: Class name must be a valid object or a string in /var/www/d8/core/includes/entity.inc on line 315 Call Stack: 

Status: Needs review » Needs work

The last submitted patch, drupal-1479454-30.patch, failed testing.

fubhy’s picture

Thanks Daniel, Will fix those failing tests tomorrow and also convert the role assignment / permission assignment tables to CMI. I might need a hand for the VDC integration with those as well :)

fubhy’s picture

Status: Needs work » Needs review
FileSize
31.78 KB
PASSED: [[SimpleTest]]: [MySQL] 46,409 pass(es). View

So... This should pass now (at least it passes all previously failed tests locally). I had a quick chat with Greg on IRC yesterday and we agreed that we will skip the hook_permission() conversion for now. Instead the next steps will be to convert the user<->role & role<->permission assignments to CMI. I will open follow-up issues for that. We will have to investigate how this should be tackled because we will loose the ability to join on permissions if we do a straight conversion. So it might be an option to keep a sort of 'cache' in the database with which we would still be able to join while the 'real' assignments reside in config. But I am not even sure yet if we actually need those joins.

EDIT: The conversion of the permission assignments went rather smooth and is not as much -/+ as I thought so I am uploading the patch here.

fubhy’s picture

FileSize
45 KB
FAILED: [[SimpleTest]]: [MySQL] 45,484 pass(es), 838 fail(s), and 0 exception(s). View

Attached patch also include the conversion of {role_permissions}. Will do the same with {user_roles} in the next step.

Note: I think we should open a follow-up for refactoring the whole user_roles / user_permissions API. Some of the functions have very poor DX forcing us to do a lot of array magic for our use-cases.

@dawehner I added a few @todo's for you to solve. I hope you don't mind :).

Status: Needs review » Needs work

The last submitted patch, 1479454-35.patch, failed testing.

fubhy’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1479454-37.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.32 KB
FAILED: [[SimpleTest]]: [MySQL] 46,308 pass(es), 0 fail(s), and 12 exception(s). View
3.89 KB

This just fixes the views integration part, i'm not sure about the failing test.

Status: Needs review » Needs work

The last submitted patch, drupal-1817582-39.patch, failed testing.

dawehner’s picture

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

Ups, this has been the wrong patch. Here is the actual patch.

Status: Needs review » Needs work

The last submitted patch, drupal-1479454-41.patch, failed testing.

heyrocker’s picture

Status: Needs work » Needs review
+++ b/core/modules/comment/comment.moduleundefined
@@ -1276,9 +1276,12 @@ function comment_node_update_index(Node $node, $langcode) {
+        if (config("user.permissions.$role->rid")->get($permission)) {

This kind of made my brain melt the first couple times I read it. I think 'user.permissions.' . $role->rid would be more readable, in fact I'd rather that all instances of double-quote-embedded-variables be changed to concatenation. It's faster and more consistent with the other ConfigEntity implementations.

+++ b/core/modules/user/lib/Drupal/user/UserRoleStorageController.phpundefined
@@ -0,0 +1,77 @@
+  /**
+   * Overrides Drupal\Core\Entity\ConfigStorageController::save().
+   */
+  public function save(EntityInterface $entity) {
+    // Prevent leading and trailing spaces in role names.
+    $entity->name = trim($entity->name);
+
+    if (!isset($entity->weight)) {
+      $roles = entity_load_multiple('entity_roles');
+      // Set a role weight to make this new role last.
+      $max = array_reduce($roles, function($max, $entity) {
+        return $max > $entity['weight'] ? $max : $entity['weight'];
+      });
+      $entity->weight = $max + 1;
+    }
+
+    return parent::save($entity);
+  }

Couldn't this and other methods in here just as easily be handled in hook_role_()? I'm wondering if there is a benefit to subclassing ConfigStorageController. I had this argument with myself when doing ImageStyles and ended up implementing the hooks instead.

+++ b/core/modules/user/user.installundefined
@@ -203,75 +203,6 @@ function user_schema() {
-  $schema['role'] = array(

Every time I see one of these I want to cry with joy.

+++ b/core/modules/user/user.moduleundefined
@@ -142,41 +143,53 @@ function user_theme() {
+  $info['user_role'] = array(

Can we change this to just 'role'? It would be clearer and reduce confusion with the existing 'user_roles' table.

+++ b/core/modules/user/user.moduleundefined
@@ -301,7 +314,7 @@ function user_load_multiple(array $uids = NULL, $reset = FALSE) {
+ * it is a go*puserermissionod idea to assign the result of this function to a different local

Looks like something weird happened here.

I didn't review the Views parts because I'm really not up on that stuff, but given that dawehner wrote it, I'm not particularly concerned.

Thanks for working on this!

heyrocker’s picture

Status: Needs review » Needs work
fubhy’s picture

Status: Needs work » Needs review
FileSize
47.33 KB
FAILED: [[SimpleTest]]: [MySQL] 46,030 pass(es), 26 fail(s), and 15 exception(s). View

Can we change this to just 'role'? It would be clearer and reduce confusion with the existing 'user_roles' table.

All the core module currently have their module name as prefix for entity types. @see taxonomy_vocabulary / taxonomy_term. However, I changed the naming of the entity class to just Role as well as the label. But the 'plugin id' (== entity type) should stay "user_role", no?

Couldn't this and other methods in here just as easily be handled in hook_role_()? I'm wondering if there is a benefit to subclassing ConfigStorageController. I had this argument with myself when doing ImageStyles and ended up implementing the hooks instead.

I am not a big fan of self-implemented hooks. This stuff is core functionality of the role entity and should be baked in to the controller. Actually, we should maybe open an issue to remove those from all other core entities as well and move them into the controllers instead. But that's just me :)

This kind of made my brain melt the first couple times I read it. I think 'user.permissions.' . $role->rid would be more readable, in fact I'd rather that all instances of double-quote-embedded-variables be changed to concatenation. It's faster and more consistent with the other ConfigEntity implementations.

I have no strong opinion about single- vs. doublequotes. I usually tend to use double quotes if the assembled string would get really long otherwise... Which is not the case here. So... Changed it to single quotes!

Looks like something weird happened here.

Yeah, looks like cat->keyboard... Except, I don't have a cat :(

The attached patch also converts the user role entity into a plugin now that we got that in core.

Status: Needs review » Needs work

The last submitted patch, 1479454-45.patch, failed testing.

fubhy’s picture

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

Fixing upgrade path.

klausi’s picture

Status: Needs review » Needs work

user_update_8002() needs to be updated. I guess you could just rewrite it?

fubhy’s picture

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

Oops, now I know why we are storing the name of the providing module in {role_permissions}. We can work around that anyhow.

Status: Needs review » Needs work

The last submitted patch, 1479454-49.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
50.32 KB
FAILED: [[SimpleTest]]: [MySQL] 46,277 pass(es), 121 fail(s), and 60 exception(s). View

Okay. Let's have this tested: I tried something else and moved the permissions into the role entity. So now we got $role->permissions instead. That also comes with quite some more code that we can remove and also allows us to clean up related code quite a bit more.

heyrocker’s picture

I am not a big fan of self-implemented hooks. This stuff is core functionality of the role entity and should be baked in to the controller. Actually, we should maybe open an issue to remove those from all other core entities as well and move them into the controllers instead. But that's just me :)

I talked about this with timplunkett and some others last night. Ultimately we didn't have any consensus about which is better or worse, but we do agree that we should standardize on something. One other thing we realized is that we should benchmark each approach, because this is something that is going to run on every page load including cached pages (I think.) So even a slight performance improvement would be worthwhile.

Will review the new patch later but I like the idea of putting permissions into the role object.

Status: Needs review » Needs work

The last submitted patch, 1479454-51.patch, failed testing.

moshe weitzman’s picture

Seems like we have some test fails here. Would be nice to fix and reroll.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
50.48 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/lib/Drupal/user/UserStorageController.php. View

I don't have time to get into the test failures but here is a reroll to current head.

Status: Needs review » Needs work

The last submitted patch, 1479454-roles-as-config-55.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
50.48 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.install. View
+++ b/core/modules/user/lib/Drupal/user/UserStorageController.php
@@ -34,9 +34,9 @@ function attachLoad(&$queried_users, $load_revision = FALSE) {
-      $queried_users[$record->uid]->roles[$record->rid] = $record->name;
+      queried_users[$record->uid]->roles[$record->rid] = $record->rid;

This was broken

Status: Needs review » Needs work

The last submitted patch, 1479454-roles-57.patch, failed testing.

fubhy’s picture

Some parts of this patch produce early dependency issues on the entity system. That's what's causing most of the test failures. Will look into solving those on monday. I hope we can solve them nicely. Don't feel like adding more workaround hacks to install.core.inc, et all.

andypost’s picture

Status: Needs work » Needs review
FileSize
53.62 KB
FAILED: [[SimpleTest]]: [MySQL] 48,512 pass(es), 82 fail(s), and 84 exception(s). View
4.69 KB

Fixed administration screens and update hook order

Status: Needs review » Needs work

The last submitted patch, 1479454-roles-58.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
53.32 KB
FAILED: [[SimpleTest]]: [MySQL] 48,489 pass(es), 73 fail(s), and 81 exception(s). View
2.82 KB

There was 3 calls to removed user_permission_roles()

Status: Needs review » Needs work

The last submitted patch, 1479454-roles-61.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
53.51 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Hopefully this fixes the upgrade tests

Status: Needs review » Needs work

The last submitted patch, 1479454-roles-63.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
53.76 KB
FAILED: [[SimpleTest]]: [MySQL] 48,579 pass(es), 69 fail(s), and 72 exception(s). View

Not sure why it is failing but translation.install also needs a dependency.

andypost’s picture

FileSize
55.14 KB
FAILED: [[SimpleTest]]: [MySQL] 48,911 pass(es), 57 fail(s), and 31 exception(s). View
3.36 KB

Suppose this should fix broken tests

andypost’s picture

FileSize
55.65 KB
FAILED: [[SimpleTest]]: [MySQL] 48,589 pass(es), 69 fail(s), and 67 exception(s). View
1.04 KB

Let's see how dependencies are related to failures

Status: Needs review » Needs work

The last submitted patch, 1479454-roles-68.patch, failed testing.

marcingy’s picture

The dependency in 68 is backwards for node and user installs. I'll try and roll a patch later.

marcingy’s picture

Status: Needs work » Needs review
FileSize
58.18 KB
FAILED: [[SimpleTest]]: [MySQL] 49,102 pass(es), 60 fail(s), and 34 exception(s). View

Lets see how this goes

Status: Needs review » Needs work

The last submitted patch, 1479454-roles-71.patch, failed testing.

Berdir’s picture

Looks like this causes the user picture update function to run before system.module adds the uuid column to the file_managed table.

andypost’s picture

The problem here no in update functions itself #67 and local install-update works, but in simpletest update.php and authorize.php throws entity_load_multiple() function not found

@marcingy please provide a interdiff of changes

fubhy’s picture

It's an early entity system dependency issue triggered by user_access(). We now require an entity_load() there due to the role<->permission relation now lying in the role entity directly. I didn't look at the latest patches yet but that's what drove my last patch against the wall.

fubhy’s picture

Status: Needs work » Needs review
FileSize
47.61 KB
FAILED: [[SimpleTest]]: [MySQL] 48,582 pass(es), 74 fail(s), and 60 exception(s). View

Okay... I really don't want to add more hacks to update.php, install.core.inc or authorize.php. Considering that the entity system simply isn't yet capable of handling early bootstrap situations yet we should move forward with a simplified version for now and fix the permissions assignments via a $role->permissions property (instead of a separate CMI namespace for that) later.

This patch is pretty much what we already had in #49. Should be green or nearly green.

I will open a follow-up for converting to $role->permissions.

fubhy’s picture

Status: Needs review » Needs work

The last submitted patch, 1479454-76.patch, failed testing.

fubhy’s picture

Looks like I forgot a tiny little piece. Should be fairly simple to fix that, though.

fubhy’s picture

Status: Needs work » Needs review
FileSize
49.65 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.module. View
fubhy’s picture

FileSize
6.84 KB
50.68 KB
PASSED: [[SimpleTest]]: [MySQL] 49,273 pass(es). View

Minor improvements.

heyrocker’s picture

I'm fine with the solution for permissions presented here and taking that topic to the followup issue.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.phpundefined
@@ -0,0 +1,77 @@
+/**
+ * Controller class for user roles.
+ *
+ * This extends the Drupal\Core\Entity\ConfigStorageController class, adding
+ * required special handling for user role objects.
+ */

I'm still wondering whether it is better to extend the storage controller or self-implement hooks. Lacking a standard I suppose I don't care, we can change it in a followup if we want. We should choose one or the other for consistency though.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.phpundefined
@@ -0,0 +1,77 @@
+    // Prevent leading and trailing spaces in role names.
+    $entity->name = trim($entity->name);

I wonder if this is worth putting in the base class? Or maybe even as a part of #1701014: Validate config object names.

I was talking to fubhy in IRC earlier, and one thing we talked about is how do modules change or override permissions with config? For instance, if Foo module wants to force anonymous users to have 'access content'. In this patch, you could not provide default configuration to do this, you would need to manage it in foo_install(). The only way around this would be to have a separate config file for every combination of role and permission (right now the files with permissions are user.permissions.[role].yml, this would have to change to user.permissions.[role].[permission].yml.) I'm not a fan of that change and I'm fine with the hook_install() solution, but it is worth talking about.

Overall I feel like we're really close.

fubhy’s picture

I'm still wondering whether it is better to extend the storage controller or self-implement hooks. Lacking a standard I suppose I don't care, we can change it in a followup if we want. We should choose one or the other for consistency though.

Yes and yes. I am actually 100% pro extending the controller in favor of self-implemented hooks as communicated before but we can of course discuss that in a follow-up issue. And yes, we should have a consistent solution for that for all entity types... whatever we decide is right.

I wonder if this is worth putting in the base class? Or maybe even as a part of #1701014: Validate config object names.

That line existed beforehand as well (@see user_role_save()). Though, making that a part of #1701014: Validate config object names sounds good to me. Other places in core where we do that:

TermFormController::submit(), VocabularyFormController::save()

Currently, in D7, we don't allow modules to provide default role->permission assignments other than by simply going the hook_install() route too. I think that would be a new feature, and a rarely used one too. I am actually in favor of still going for the $role->permissions property in which case we would only be able to provide default config for role->permission assignments in the actual role .yml file (which is fine if you ask me). Modules would then have to assign permissions in hook_install(). Export/Import would still work as expected.

That solution is also much cleaner and might reduce the amount of array magic and helper functions that we currently have in user.module (which we should clean up anyways).

If you ask me, all these things should be handled in follow-ups. Let's leave this at NR for a few more days for others to take a look. There might still be bugs :)

Berdir’s picture

Status: Needs review » Needs work

General remarks:

- A bit worried about performance here, mostly about the permissions list. I think we need to do some benchmarks here, with an actual useful and real permission set. I just checked a bigger site that I worked on and "select count(*) from role_permission;
" returns 2115. Suggestion: Install all the modules, create 10+ roles, assign all the permissions and see how many config() calls we have.

- Upgrade path tests, we already have UserRoleUpgradePathTest from the role machine name change, does this cover everything that we're doing here? Maybe add a few additional permission checks?

+++ b/core/modules/comment/comment.moduleundefined
@@ -1284,9 +1284,12 @@ function comment_node_update_index(Node $node, $langcode) {
+    foreach (user_roles() as $role) {
+      foreach ($perms as $permission => &$roles) {
+        if (config('user.permissions.' . $role->rid)->get($permission)) {
+          $roles[$role->rid] = $role->rid;
+        }
+      }

Right now, this code does count($roles) * count($perms) config() calls, which means config storage loads, which means as much cache queries. #1187726: Follow-up: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) is working on changing that, but then it's still count($roles). Hm...

+++ b/core/modules/translation/translation.installundefined
@@ -10,10 +10,15 @@
+  foreach (config_get_storage_names_with_prefix('user.role.') as $name) {
+    $role = config($name)->get();
+    $config = config('user.permissions.' . $role['rid']);
+    if ($module = $config->get('translate content')) {
+      $config->clear('translate content');
+      $config->set('translate all content', $module);
+      $config->save();

This code seems to be repeated quite a few times, maybe add a helper function to update.inc or so?

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/Role.phpundefined
@@ -0,0 +1,67 @@
+ * Definition of Drupal\user\Plugin\Core\Entity\Role.

Should be Contains.., also in other files.

heyrocker’s picture

FileSize
50.68 KB
FAILED: [[SimpleTest]]: [MySQL] 49,381 pass(es), 1 fail(s), and 3 exception(s). View

Going to look into the performance impact today, in the meantime here's a reroll to current head.

heyrocker’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1479454-roles-as-config-85.patch, failed testing.

heyrocker’s picture

- A bit worried about performance here, mostly about the permissions list. I think we need to do some benchmarks here, with an actual useful and real permission set. I just checked a bigger site that I worked on and "select count(*) from role_permission;
" returns 2115. Suggestion: Install all the modules, create 10+ roles, assign all the permissions and see how many config() calls we have.

I ran this test and the results weren't that different. This is on a fresh D8 install, all core modules installed + all devel modules, 10 roles, all roles given all permissions (1251 entries in {role_permission}.) The pages were visited as a non-admin user, since I know there are places in core where we bypass user access checks for admin. I just chose four pages at semi-random. For each visit, I give the number of times config() is called, as well as how many times the config is actually loaded from files (since the first patches in #1187726: Follow-up: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) have gone in.)

Patch
-----
/admin - 40 (16 reads)
/admin/structure/types/manage/article - 43 (20 reads)
/admin/people - 114 (31 reads)
/ - 33 (16 reads)

No patch
--------
/admin - 38 (14 reads)
/admin/structure/types/manage/article - 40 (17 reads)
/admin/people - 99 (19 reads)
/ - 31 (14 reads)

So we're not really adding that many extra config calls with this patch. config() is 0.1% to 0.2% of wall time consistently.

Right now, this code does count($roles) * count($perms) config() calls, which means config storage loads, which means as much cache queries.

There's no doubt this takes longer. With the same setup as above, we're looking at 6.3ms without the patch and 75.6 with it. However with the new caching in place, at least that hit only happens once per cron run, which shouldn't be too bad.

heyrocker’s picture

FileSize
50.67 KB
PASSED: [[SimpleTest]]: [MySQL] 49,392 pass(es). View

This patch changes 'Definition of' to 'Contains' where necessary. I didn't really see much to be gained by creating a helper function for the update functions, so I just left that code. The language upgrade tests are passing for me locally so giving the bot another chance here.

heyrocker’s picture

Status: Needs work » Needs review
fubhy’s picture

I didn't really see much to be gained by creating a helper function for the update functions, so I just left that code. The language upgrade tests are passing for me locally so giving the bot another chance here.

Yeah.. I think at this point I would be +1 for not adding such an update function too... If we find more places in core where we want to rename permissions we can think about it again.

Regarding the performance... It looks like the decrease in performance is rather minor with this patch. Plus, you have to consider that we should still think about going for $role->permissions in a follow-up which would remove quite a few config() calls.

I would be okay with RTBC...

sun’s picture

Status: Needs review » Needs work

Thanks for working on this — looks pretty good already.

+++ b/core/modules/node/node.install
@@ -723,13 +723,16 @@ function node_update_8012() {
 function node_update_8013() {
...
+  foreach (config_get_storage_names_with_prefix('user.role.') as $rid) {
+    $role = config($rid)->get();
+    $config = config('user.permissions.' . $role['rid']);

+++ b/core/modules/system/system.install
@@ -1961,13 +1961,16 @@ function system_update_8020() {
+    foreach (config_get_storage_names_with_prefix('user.role.') as $rid) {
+      $role = config($rid)->get();
+      $config = config('user.permissions.' . $role['rid']);

+++ b/core/modules/translation/translation.install
@@ -10,10 +10,15 @@
+  foreach (config_get_storage_names_with_prefix('user.role.') as $name) {
+    $role = config($name)->get();
+    $config = config('user.permissions.' . $role['rid']);

This update is changed to rely on the migrated roles and permissions in config.

A) We either need to leave this update alone.

B) Or we need to define a proper dependency via hook_update_dependencies().

+++ b/core/modules/node/node.views.inc
@@ -690,20 +690,17 @@ function node_views_analyze($view) {
+          // `access content` permission.

The backtick is the shell execution operator in PHP. Let's use simple single quotes here, please.

+++ b/core/modules/user/config/user.role.anonymous.yml
@@ -0,0 +1,3 @@
+rid: anonymous
+name: Anonymous user

Is there any reason for why we cannot rename 'rid' to 'id' and 'name' to 'label' directly within this patch?

+++ b/core/modules/user/config/user.role.authenticated.yml
@@ -0,0 +1,3 @@
+rid: authenticated

Hm. Also, should we do #1831928: Support a 'locked' property on ConfigEntities first?

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/Role.php
@@ -0,0 +1,67 @@
+  public function id() {
+    return $this->rid;

Unless we rename 'name' to 'label' in this patch, we also need an override for the label() method.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/argument/RolesRid.php
@@ -23,16 +23,7 @@
+    return array(entity_load('user_role', $this->value)->name);

+++ b/core/modules/user/user.module
@@ -2332,7 +2256,7 @@ function user_user_operations_block($accounts) {
+  $role_name = entity_load('user_role', $rid)->name;

Let's use the ->id() and ->label() methods everywhere instead of directly accessing those properties.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.php
@@ -0,0 +1,77 @@
+ * This extends the Drupal\Core\Entity\ConfigStorageController class, adding
+ * required special handling for user role objects.

We can safely remove this, as it doesn't state anything that wouldn't be obvious.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.php
@@ -0,0 +1,77 @@
+    if (!isset($entity->weight)) {
+      $roles = entity_load_multiple('entity_roles');
+      // Set a role weight to make this new role last.
+      $max = array_reduce($roles, function($max, $entity) {
+        return $max > $entity['weight'] ? $max : $entity['weight'];
+      });
+      $entity->weight = $max + 1;
+    }

Hm. This looks pretty heavy for something that probably shouldn't exist in the first place. I don't see what we gain from this enforced "put it last" sorting.

Can't we set a default weight of -20 and -10 for the anonymous and authenticated roles, and just default the weight for new roles to 0?

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.php
@@ -0,0 +1,77 @@
+    // Clear the user access cache.
+    drupal_static_reset('user_access');
+    drupal_static_reset('user_role_permissions');

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.php
@@ -80,6 +80,8 @@ function testAdministratorRole() {
+    drupal_static_reset('user_access');
+    drupal_static_reset('user_role_permissions');

Hm. Can we provide a helper function à la user_role_cache_reset() for these?

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.php
@@ -34,9 +34,9 @@ function attachLoad(&$queried_users, $load_revision = FALSE) {
+    $result = db_query('SELECT ur.rid, ur.uid FROM {users_roles} ur WHERE ur.uid IN (:uids)', array(':uids' => array_keys($queried_users)));

Minor: This query could be simplified now as the table aliases are no longer needed; i.e., just

SELECT rid, uid FROM {user_roles} WHERE uid IN (:uids)

+++ b/core/modules/user/user.admin.inc
@@ -1007,8 +1002,8 @@ function user_admin_role($form, $form_state, $role) {
 function user_admin_role_submit($form, &$form_state) {
-  $role = (object) $form_state['values']['role'];
-  $status = user_role_save($role);
+  $role = entity_create('user_role', $form_state['values']['role']);
+  $status = $role->save();
   if ($status === SAVED_UPDATED) {
     drupal_set_message(t('The role has been renamed.'));
   }

It's not clear to me why the form submission handler for the user role form creates a new role entity from scratch. Normally this happens in the form constructor or through the entity form controller already?

Given that, I also don't understand how the SAVED_UPDATED condition could ever be reached?

+++ b/core/modules/user/user.install
@@ -148,75 +148,6 @@ function user_schema() {
-  $schema['role_permission'] = array(
-    'description' => 'Stores the permissions assigned to user roles.',

Overall, I'm a bit skeptical about doing the role_permission conversion in the same patch. I didn't really understand why we're using separate config files for each role's permissions instead of a single user.permissions config file that holds all the mappings. I wonder whether we should perform that conversion in a separate follow-up issue?

+++ b/core/modules/user/user.install
@@ -1059,5 +983,48 @@ function user_update_8016() {
+  db_drop_table('role');
...
+  db_drop_table('role_permission');

We're no longer dropping tables in config conversion updates, and instead leave that for #1860986: Drop left-over tables from 8.x.

+++ b/core/modules/user/user.module
@@ -372,47 +373,39 @@ function user_password($length = 10) {
 function user_role_permissions($roles = array()) {
   $cache = &drupal_static(__FUNCTION__, array());
...
+  foreach ($roles as $rid) {
+    if (!isset($cache[$rid])) {
+      $cache[$rid] = config('user.permissions.' . $rid)->get() ?: array();
     }
   }
+  return array_intersect_key($cache, array_flip($roles));

Unless I'm mistaken, then the new config factory cache essentially works the same as a static cache performance-wise, so the additional static cache in user_role_permissions(), which essentially just copies over the values may not be needed?

But again, perhaps it would be better to leave the role-permissions part for a separate issue.

+++ b/core/modules/user/user.module
@@ -2188,17 +2117,11 @@ function user_role_change_permissions($rid, array $permissions = array()) {
 function user_role_grant_permissions($rid, array $permissions = array()) {
   $modules = user_permission_get_modules();
   // Grant new permissions for the role.
+  $config = config('user.permissions.' . $rid);
   foreach ($permissions as $name) {
-    db_merge('role_permission')
-      ->key(array(
-        'rid' => $rid,
-        'permission' => $name,
-      ))
-      ->fields(array(
-        'module' => $modules[$name],
-      ))
-      ->execute();
+    $config->set($name, $modules[$name]);
   }
+  $config->save();

Hm. The module name that defines a permission is not really configuration, but instead state information. By now, it's pretty obvious that it was wrong to add that data to the role_permission table in first place.

I think we'll have to split that permission owner tracking functionality out into a separate state storage first.

+++ b/core/profiles/standard/standard.install
@@ -387,11 +387,12 @@ function standard_install() {
   // Create a default role for site administrators, with all available permissions assigned.
...
+  $admin_role = entity_create('user_role', array(
+    'rid' => 'administrator',
+    'name' => 'Administrator',
+    'weight' => 2,
+  ));
+  $admin_role->save();

Note that other conversion issues interestingly started to convert default configuration in install profiles directly into actual default config files in the profile's ./config directory. We might want to check whether that works here, too.

fubhy’s picture

Awesome review, thanks! I think I know what we gotta do now and will roll another patch + follow ups later today!

tim.plunkett’s picture

Note, you don't need to override Entity::label(), since it uses the entity keys. Otherwise, review points are valid.

fubhy’s picture

Status: Needs work » Needs review
FileSize
42.9 KB
FAILED: [[SimpleTest]]: [MySQL] 49,364 pass(es), 10 fail(s), and 0 exception(s). View

Note that other conversion issues interestingly started to convert default configuration in install profiles directly into actual default config files in the profile's ./config directory. We might want to check whether that works here, too.

Yes, I was hoping that we can do that eventually, too.

It's not clear to me why the form submission handler for the user role form creates a new role entity from scratch. Normally this happens in the form constructor or through the entity form controller already?

Yes, but we don't use a entity form controller yet because it is also used by the list overview which I was planning to do a generic solution for for config entities (same as with tabledrag/weighting functionality) once I get a chance. Will open a follow-up issue to move his stuff into a form controller. This has to be considered a temporary solution. I added a @todo and opened a follow-up issue for this: #1872870: Implement a RoleListController and RoleFormController

I think we'll have to split that permission owner tracking functionality out into a separate state storage first.

Agreed... I opened an issue for that: #1872874: Remove the module name property from the role permission table

But again, perhaps it would be better to leave the role-permissions part for a separate issue.

Agreed.... I opened an issue for that too: #1872876: Turn role permission assignments into configuration.

Okay... So regarding the attached patch:
I applied most of what was pointed out by #93... Except for the stuff that got removed entirely due to the fresh follow-up issue for the permission assignments.
I started converted ->rid to id() (with an 'id' property) and ->name to ->label() (with a 'label' property).

Hm. This looks pretty heavy for something that probably shouldn't exist in the first place. I don't see what we gain from this enforced "put it last" sorting.

This was there before... I guess it has something to do with the javascript for the permission overview? Maybe?

fubhy’s picture

FileSize
39.8 KB
FAILED: [[SimpleTest]]: [MySQL] 49,376 pass(es), 10 fail(s), and 0 exception(s). View

Oh, sorry... I accidentally put my early work for the Form and List controller conversion into this patch too. Pardon me.

Status: Needs review » Needs work

The last submitted patch, 1479454-96.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
40.88 KB
PASSED: [[SimpleTest]]: [MySQL] 49,428 pass(es). View

Fixing the failing tests... Once this is RTBC I also want to open a pure cleanup issue for all that ancient user role touching code.

fubhy’s picture

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

I gave this another lookthrough, and it appears to address all the existing concerns. I agree with moving most of the open permission questions into followups. Putting this to rtbc. Thanks for all the hrd work.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/user.installundefined
@@ -1059,5 +1019,23 @@ function user_update_8016() {
 /**
+ * Turn the {role} table into configuration entities.
+ */
+function user_update_8017() {
+  $roles = db_select('role', 'r')
+    ->fields('r')
+    ->execute()
+    ->fetchAll();
+
+  foreach ($roles as $role) {
+    config('user.role.' . $role->rid)
+      ->set('id', $role->rid)
+      ->set('label', $role->name)
+      ->set('weight', $role->weight)
+      ->save();
+  }
+}
+

We need to use the new update_config_manifest_add() here to create a manifest.user.role file during upgrade. Additionally since roles are such a key part of the system I think we need upgrade tests as well.

larowlan’s picture

Status: Needs work » Needs review
FileSize
42.34 KB
PASSED: [[SimpleTest]]: [MySQL] 50,075 pass(es). View
1.92 KB

Patch adds update_config_manifest_add() call to create the manifest.user.role file and a simple upgrade test that we can load the new user_role entities. Much of the role upgrade process is already tested in Drupal\system\Tests\Upgrade\UserRoleUpgradePathTest::testRoleUpgrade() which is testing the change from serial rids to machine names, but covers much of the role functionality (other than the (config) entity system integration). New tests just make sure we can load the sample user roles by rid using entity_load().

fubhy’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UserRoleUpgradePathTest.phpundefined
@@ -70,4 +70,31 @@ public function testRoleUpgrade() {
+  /**
+   * Tests roles converted to config.
+   */
+  public function testRoleUpgradeToConfig() {
+    $this->assertTrue($this->performUpgrade(), 'The upgrade was completed successfully.');
+
+    $rids = array_merge(array(
+      "anonymous",
+      "authenticated",
+    ), range(3, 6));
+    $roles = entity_load_multiple('user_role', $rids);
+    // Check that upgraded role can be loaded as a config entity.
+    foreach ($rids as $rid) {
+      if (!empty($roles[$rid])) {
+        $role = $roles[$rid];
+        $this->assertTrue($role, format_string('Config entity for "@role" role was found.', array(
+          '@role' => $role->label()
+        )));
+      }
+      else {
+        $this->fail(format_string("Could not load config entity for '@role' role", array(
+          '@role' => $rid
+        )));
+      }
+    }

Let's not use loops or if/else statements in the test itself. It should be enough to simply check for 'authenticated' and 'anonymous' after the upgrade has been performed.

fubhy’s picture

FileSize
1.63 KB
42.22 KB
PASSED: [[SimpleTest]]: [MySQL] 50,105 pass(es). View

EDIT: Oh, I made the interdiff the wrong way round... - == + of course :)

alexpott’s picture

Status: Needs review » Needs work

Sorry also missed the fact that we need to create a UUID and add langcode to the config entity during the 7 to 8 upgrade.

The update should look something like:

function user_update_8017() {
  $uuid = new Uuid();

  $roles = db_select('role', 'r')
    ->fields('r')
    ->execute()
    ->fetchAll();

  foreach ($roles as $role) {
    config('user.role.' . $role->rid)
      ->set('id', $role->rid)
      ->set('uuid', $uuid->generate())
      ->set('label', $role->name)
      ->set('weight', $role->weight)
      ->set('langcode', LANGUAGE_NOT_SPECIFIED)
      ->save();
  }

  update_config_manifest_add('user.role', array_map(function ($role) {
    return $role->rid;
  }, $roles));
}

So that the created config files are the same as if they'd been created by entity->save()

fubhy’s picture

Status: Needs work » Needs review
FileSize
42.33 KB
PASSED: [[SimpleTest]]: [MySQL] 50,090 pass(es). View
alexpott’s picture

Status: Needs review » Needs work

Discovered some issues with views whilst testing this patch - can not add the role field and also we've introduced the possibility for cross site scripting.

To recreate:

  1. Create a view on users with the role as a field
  2. Create a role with the name <script>alert('here');</script>js
  3. Add the role to a user
  4. View the view

The specific code that causes this is here:

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Roles.phpundefined
@@ -47,15 +47,15 @@ function pre_render(&$values) {
     if ($uids) {
-      $query = db_select('role', 'r');
-      $query->join('users_roles', 'u', 'u.rid = r.rid');
-      $query->addField('r', 'name');
+      $roles = user_roles();
+      $query = db_select('users_roles', 'u');
       $query->fields('u', array('uid', 'rid'));
+      $query->condition('u.rid', array_keys($roles));
       $query->condition('u.uid', $uids);
       $query->orderBy('r.name');
       $result = $query->execute();
       foreach ($result as $role) {
-        $this->items[$role->uid][$role->rid]['role'] = check_plain($role->name);
+        $this->items[$role->uid][$role->rid]['role'] = $roles[$role->rid]->label();
         $this->items[$role->uid][$role->rid]['rid'] = $role->rid;
       }

We need to

  • remove $query->orderBy('r.name');
  • ensure that the role name is properly properly escaped with checkPlain()...
    +        $this->items[$role->uid][$role->rid]['role'] = check_plain($roles[$role->rid]->label());
     
  • I guess there is an outstanding question about whether or not we should alphabetically order the role results as they would have been before this patch
  • We are obviously missing test coverage here
alexpott’s picture

FileSize
48.82 KB
PASSED: [[SimpleTest]]: [MySQL] 50,552 pass(es). View

Patch attached fixes issues discover in #107 and adds a test view for user_roles. Unfortunately the way the user roles are added to the view is in the preRender() step which seems a little unorthodox. But fixing that is outside the scope of this patch.

Still todo: fix the ordering of the roles in the view so that is matches the SQL equivalent.

Unfortunately the interdiff is impossible to read since this is also a reroll on top of the blocks as plugins patch.

ParisLiakos’s picture

Status: Needs work » Needs review
alexpott’s picture

Just discovered another issue with this patch... (at least it's an issue in #108)

Changing the order of the roles on admin/people/roles and press "save order" does not work.

damiankloip’s picture

+++ b/core/modules/user/user.installundefined
@@ -1055,5 +1015,31 @@ function user_update_8016() {
+  foreach ($roles as $role) {
+    config('user.role.' . $role->rid)
+      ->set('id', $role->rid)
+      ->set('uuid', $uuid->generate())
+      ->set('label', $role->name)
+      ->set('weight', $role->weight)
+      ->set('langcode', LANGUAGE_NOT_SPECIFIED)
+      ->save();

Instead of setting the uuid here, should this create a new config entity from this data (which will then generate this anyway) and save, al la importCreate type thing? If there is a reason not to do this, apologies!

alexpott’s picture

Yep there is a reason to not use entity_create() - entity_get_info() uses the hook system which should not be invoked during hook_update_N

damiankloip’s picture

Good point, sorry for the noise. Proceed :)

fubhy’s picture

Hey alex, thanks for the continued stream of good reviews! Are you working on a patch for what you pointed out in #110 or should I get my hands dirty?

alexpott’s picture

FileSize
49.88 KB
PASSED: [[SimpleTest]]: [MySQL] 50,713 pass(es). View

The attached patch fixes the role ordering issue for on admin/people/roles

Basically the issue was is fixed by doing this:

--- a/core/modules/user/lib/Drupal/user/RoleStorageController.php
+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.php
@@ -68,7 +68,7 @@ protected function postDelete($entities) {
    */
   protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
     // Sort the queried roles by their weight.
-    uasort($queried_entities, 'drupal_sort_weight');
+    uasort($queried_entities, 'Drupal\Core\Config\Entity\ConfigEntityBase::sort');

     parent::attachLoad($queried_entities, $revision_id);
   }

I've added a test for this too...

-  function testRoleWeightChange() {
+  function testRoleWeightOrdering() {
     $this->drupalLogin($this->admin_user);
-
-    // Pick up a random role and get its weight.
-    $rid = array_rand(user_roles());
-    $role = user_role_load($rid);
-    $old_weight = $role->weight;
-
-    // Change the role weight and submit the form.
-    $edit = array('roles['. $rid .'][weight]' => $old_weight + 1);
+    $roles = user_roles();
+    $weight = count($roles);
+    $new_role_weights = array();
+    $saved_rids = array();
+
+    // Change the role weights to make the roles in reverse order.
+    $edit = array();
+    foreach ($roles as $role) {
+      $edit['roles['. $role->id() .'][weight]'] =  $weight;
+      $new_role_weights[$role->id()] = $weight;
+      $saved_rids[] = $role->id;
+      $weight--;
+    }
     $this->drupalPost('admin/people/roles', $edit, t('Save order'));
     $this->assertText(t('The role settings have been updated.'), 'The role settings form submitted successfully.');

-    // Retrieve the saved role and compare its weight.
-    $role = user_role_load($rid);
-    $new_weight = $role->weight;
-    $this->assertTrue(($old_weight + 1) == $new_weight, 'Role weight updated successfully.');
+    // Load up the user roles with the new weights.
+    drupal_static_reset('user_roles');
+    $roles = user_roles();
+    $rids = array();
+    // Test that the role weights have been correctly saved.
+    foreach ($roles as $role) {
+      $this->assertEqual($role->weight, $new_role_weights[$role->id()]);
+      $rids[] = $role->id;
+    }
+    // The order of the roles should be reversed.
+    $this->assertIdentical($rids, array_reverse($saved_rids));
   }
 }

Interdiff impossible to read due to the fact this is a reroll too :)

alexpott’s picture

FileSize
49.97 KB
FAILED: [[SimpleTest]]: [MySQL] 49,510 pass(es), 9 fail(s), and 13 exception(s). View
2.07 KB

Reviewing the Role storage controller lead me to find some code that will never be fired since entity_create will always generate an object with a weight property.

Patch attached moves this logic to RoleStorageController::create() which means we can remove some business logic from the user_admin_roles() form FTW!

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

Given that this has undergone review from all the CMI maintainers and it appears all the concerns have been addressed, I'm going to RTBC. Also removing the "Needs benchmarks" tag since I did that in #1479454-88: Convert user roles to configurables

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system, +VDC, +Configurables, +VDC-integration

The last submitted patch, 1479454-116-user-roles.patch, failed testing.

alexpott’s picture

Assigned: fubhy » alexpott

I've got a fix for the "Drupal\views\Tests\AnalyzeTest" - could not reproduce the other failures. Just working on the role ordering in views.

alexpott’s picture

Assigned: alexpott » Unassigned
Status: Needs work » Needs review
FileSize
6.54 KB
51.96 KB
PASSED: [[SimpleTest]]: [MySQL] 49,911 pass(es). View

The patch attached fixes "Drupal\views\Tests\AnalyzeTest" and sorts roles listed in the view according to role weight which I think makes the most logical sense. In pre-CMI roles on the view where sorted according to name which meant that a site admin could never change the order without change names. This way they can order the roles in the admin screen and hey presto! A test for this has been added to.

The views test and Drupal\user\Plugin\views\field\Roles code could do with a review from the VDC team in any of them has time - cheers!

fubhy’s picture

Thanks for keeping the patches coming, Alex. Switching to weight sorting instead of name makes sense and is consistent. The test also looks good. I am not a VDC person though... :)

dawehner’s picture

Alex, thank you for fixing these parts! +1 in general

+++ b/core/modules/node/node.views.incundefined
@@ -691,9 +691,9 @@ function node_views_analyze(ViewExecutable $view) {
-          $result = db_select('role_permissions', 'p')
+          $result = db_select('role_permission', 'p')

ups

+++ b/core/modules/node/node.views.incundefined
@@ -701,7 +701,7 @@ function node_views_analyze(ViewExecutable $view) {
+          if (!($roles[DRUPAL_ANONYMOUS_RID]->safe && $roles[DRUPAL_AUTHENTICATED_RID]->safe)) {

I really love to not use constants anymore.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -535,11 +535,14 @@ protected function drupalCreateUser(array $permissions = array()) {
+   * @param string $weight

Is it just me that a weight as string feels wrong?

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFieldRoleTest.phpundefined
@@ -32,15 +32,15 @@ public static function getInfo() {
-    // Add user 1 to the role.
+    // Add roles to user 1.

Feels like a comment inspired by some unix commands :)

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFieldRoleTest.phpundefined
@@ -53,8 +53,7 @@ public function testRole() {
+    $this->assertText($rolename_b . $rolename_a, 'View test_views_handler_field_role displays role assigned to user in the correct order.');
     $this->assertNoText($rolename_not_assigned, 'View test_views_handler_field_role does not display role no assigned to a user.');

Just as a todo: We should adapt these rolenames later as they refer to the PRE-psr0 filenames.

andypost’s picture

This issue also requires 2 follow-ups to convert List/Form controllers

fubhy’s picture

alexpott’s picture

FileSize
51.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1479454-126-user-roles.patch. Unable to apply patch. See the log in the details link for more information. View
2.54 KB
  • Fixed "@param string" => "@param integer"
  • "test_views_handler_field_role" is referring to the view not the PSR-0 classname :)
  • I've also reduced the testing time by making the test more like HandlerFieldUserNameTest and not render the full view just the field that we're testing.

imho this is now good to go as VDC people have had look - many thanks!

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

Alright then!

webchick’s picture

Assigned: Unassigned » catch

Hm. The benchmarks in #88 seems like it could use catch's feedback prior to commit.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system, +VDC, +Configurables, +VDC-integration

The last submitted patch, 1479454-126-user-roles.patch, failed testing.

fubhy’s picture

Assigned: catch » fubhy
fubhy’s picture

Assigned: fubhy » Unassigned
Status: Needs work » Needs review
FileSize
52.06 KB
FAILED: [[SimpleTest]]: [MySQL] 50,241 pass(es), 1 fail(s), and 0 exception(s). View
webchick’s picture

Assigned: Unassigned » catch

Aaaaand back to catch. :)

Status: Needs review » Needs work
Issue tags: -Configuration system, -VDC, -Configurables, -VDC-integration

The last submitted patch, 1479454-132.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review

#132: 1479454-132.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +VDC, +Configurables, +VDC-integration

The last submitted patch, 1479454-132.patch, failed testing.

alexpott’s picture

Drupal\system\Tests\Entity\EntityTranslationTest nor Drupal\translation\Tests\TranslationTest fail locally :(

Will retest...

alexpott’s picture

Status: Needs work » Needs review

#132: 1479454-132.patch queued for re-testing.

fubhy’s picture

Yeah, doesn't fail locally for me either.

Status: Needs review » Needs work

The last submitted patch, 1479454-132.patch, failed testing.

alexpott’s picture

Last test only failed Drupal\translation\Tests\TranslationTest - which does not fail locally :( ... retesting.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system, -VDC, -Configurables, -VDC-integration

#132: 1479454-132.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +VDC, +Configurables, +VDC-integration

The last submitted patch, 1479454-132.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
56.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1479454-144.user-roles.debugging-translation-failures.patch. Unable to apply patch. See the log in the details link for more information. View
3.91 KB

This is annoying ran Drupal\translation\Tests\TranslationTest 30 times locally - no fail...

Adding debug to try and find out what's going on... this will probably go green :(

catch’s picture

Assigned: catch » Unassigned
+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UserRoleUpgradePathTest.phpundefined
@@ -73,4 +73,19 @@ public function testRoleUpgrade() {
+  /**
+   * Tests that roles were converted to config.
+   */
+  public function testRoleUpgradeToConfig() {
+    $this->assertTrue($this->performUpgrade(), 'The upgrade was completed successfully.');
+
+    // Check that the 'anonymous' role has been converted to config.
+    $anonymous = entity_load('user_role', DRUPAL_ANONYMOUS_RID);
+    $this->assertNotEqual(FALSE, $anonymous, "The 'anonymous' role has been converted to config.");
+
+    // Check that the 'authenticated' role has been converted to config.
+    $authenticated = entity_load('user_role', DRUPAL_AUTHENTICATED_RID);
+    $this->assertNotEqual(FALSE, $authenticated, "The 'authenticated' role has been converted to config.");

Could we add one non-magic role to the test? I realise the default ones get saved to the db too so maybe that's excessive, but I wasn't expecting to see these two tested.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.phpundefined
@@ -0,0 +1,78 @@
+  public function create(array $values) {
+    $entity = parent::create($values);
+    $roles = entity_load_multiple('user_role');
+    if (count($roles)) {
+      // Set a role weight to make this new role last.
+      $max = array_reduce($roles, function($max, $entity) {
+        return $max > $entity->weight ? $max : $entity->weight;
+      });
+      $entity->weight = $max + 1;
+    }
+    return $entity;

This is a bit weird in the controller, is it just because we were doing it before?

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.phpundefined
@@ -0,0 +1,78 @@
+  /**
+   * Overrides ConfigStorageController::save().
+   */
+  public function save(EntityInterface $entity) {
+    // Prevent leading and trailing spaces in role names.
+    $entity->label = trim($entity->label);
+    return parent::save($entity);

Why in the API CRUD function and not in validate/submit? (again presumably it was like this before).

All of these could be follow-ups I think.

On the performance information. First of all thanks for checking! heyrocker's number didn't say what we skip - presumably a query against the users_roles table? Also they look OK to me - it's going to be something that we should focus efforts on optimizing CMI vs. this particular case so fine with that.

Unassigning me - if this was green I'd probably commit it but see what happens with the bot :(

tim.plunkett’s picture

FileSize
56.11 KB
PASSED: [[SimpleTest]]: [MySQL] 50,575 pass(es). View

Yes, everything in #145 is there because that's where it was before. Follow-up-fodder.

Rerolled for #1779026: Convert Text Formats to Configuration System

alexpott’s picture

FYI both #144 and #146 contain debug code in core/modules/translation/lib/Drupal/translation/Tests/TranslationTest.php to try to work out why Drupal\translation\Tests\TranslationTest is failing.

sun’s picture

Looks like we're almost done here! :)

Gave this another final review spin:

+++ b/core/modules/translation/lib/Drupal/translation/Tests/TranslationTest.php
@@ -154,36 +154,45 @@ function testContentTranslation() {
   function testLanguageSwitchLinks() {
     // Create a Basic page in English and its translation in Spanish.
     $node = $this->createPage($this->randomName(), $this->randomName(), 'en');
...
+    $this->assertTrue(is_object($node), 'Create a node to translate.');

This potentially fails, because the node is created via the UI, and the web user might not have permissions to create or translate the node.

Also, hm, apparently ::createPage() verifies that $node is an object already.

+++ b/core/modules/user/config/user.role.anonymous.yml
@@ -0,0 +1,3 @@
+id: anonymous
+label: Anonymous user
+weight: 0

+++ b/core/modules/user/config/user.role.authenticated.yml
@@ -0,0 +1,3 @@
+id: authenticated
+label: Authenticated user
+weight: 1

I wonder whether we could skip most of the custom weight-futzing, by changing the weights of these built-in roles to -10 and -9 respectively?

I.e., so that any new role gets a default weight of 0 and everyone's happy. ;)

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.php
@@ -0,0 +1,78 @@
+  public function save(EntityInterface $entity) {
+    // Prevent leading and trailing spaces in role names.
+    $entity->label = trim($entity->label);

This should normally happen in one of these instead:

1) preSave()

2) Form validation handler of the user role form.

This particular issue of trim() apparently repeats itself across all Configurable conversion issues. We should definitely create a dedicated follow-up issue to discuss what the "proper" place is/should be.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.php
@@ -0,0 +1,78 @@
+  protected function postDelete($entities) {
+    $rids = array_keys($entities);
+
+    foreach ($entities as $entity) {
+      db_delete('role_permission')
+        ->condition('rid', $entity->id())
+        ->execute();
+    }

Can't we use array_keys($entities) here and execute a single delete query?

Apparently, that's even prepared in $rid already... ;)

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.php
@@ -0,0 +1,78 @@
+  protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
+    // Sort the queried roles by their weight.
+    uasort($queried_entities, 'Drupal\Core\Config\Entity\ConfigEntityBase::sort');

I'm not sure whether it is legit to put this sorting into attachLoad().

In all other conversions thus far, we're selectively performing the sorting where it is actually needed only. This typically means the admin listing page only.

Is there a technical reason for having to perform it on all loads?

+++ b/core/profiles/standard/standard.install
@@ -226,18 +226,19 @@ function standard_install() {
   // Create a default role for site administrators, with all available permissions assigned.
-  $admin_role = new stdClass();
-  $admin_role->rid = 'administrator';
-  $admin_role->name = 'Administrator';
-  $admin_role->weight = 2;
-  user_role_save($admin_role);
-  user_role_grant_permissions($admin_role->rid, array_keys(module_invoke_all('permission')));
+  $admin_role = entity_create('user_role', array(
+    'id' => 'administrator',
+    'label' => 'Administrator',
+    'weight' => 2,
+  ));
+  $admin_role->save();

This should be moved into a config file of the Standard profile.

fubhy’s picture

FileSize
7.71 KB
53.02 KB
FAILED: [[SimpleTest]]: [MySQL] 50,559 pass(es), 8 fail(s), and 7 exception(s). View

I wonder whether we could skip most of the custom weight-futzing, by changing the weights of these built-in roles to -10 and -9 respectively?

I.e., so that any new role gets a default weight of 0 and everyone's happy. ;)

Maybe, but let's do that in a follow-up (if it's even necessary).

This should normally happen in one of these instead:

1) preSave()

2) Form validation handler of the user role form.

This particular issue of trim() apparently repeats itself across all Configurable conversion issues. We should definitely create a dedicated follow-up issue to discuss what the "proper" place is/should be.

Granted... Moved it to the form validation handler. Will move it to the Form controller in the follow-up and then consider if we should have a base ConfigEntityFormController. Oki?

Can't we use array_keys($entities) here and execute a single delete query?

Apparently, that's even prepared in $rid already... ;)

Ahrm... Yes, good catch... D'oh

I'm not sure whether it is legit to put this sorting into attachLoad().

In all other conversions thus far, we're selectively performing the sorting where it is actually needed only. This typically means the admin listing page only.

Is there a technical reason for having to perform it on all loads?

We required weighting in multiple other places too (e.g. Views UI, etc.). I believe that's why we moved it there.

This should be moved into a config file of the Standard profile.

Agreed.

Status: Needs review » Needs work

The last submitted patch, 1479454-149.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
53.04 KB
PASSED: [[SimpleTest]]: [MySQL] 50,689 pass(es). View
656 bytes

Wow, I am blind!

sun’s picture

FileSize
5.37 KB
52.31 KB
FAILED: [[SimpleTest]]: [MySQL] 50,630 pass(es), 1 fail(s), and 0 exception(s). View

Fixed that test.

Cleaned up the code.

Also changed the ::create() into a ::preSave(), and slightly corrected its logic to exactly match what it did before.

sun’s picture

Status: Needs review » Reviewed & tested by the community

@fubhy confirmed the final tweaks in #152 and asked me to re-assign to @catch...

However, @catch already checked this in #145, so instead of doing so, rtbc'ing.

webchick’s picture

Title: Convert user roles to configurables » Change notice: Convert user roles to configurables
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

This is a lovely patch of loveliness. Nice to see this becoming more consistent everywhere.

Committed and pushed to 8.x. Thanks!

This'll need a change notice.

webchick’s picture

Title: Change notice: Convert user roles to configurables » [Testbot broke] Change notice: Convert user roles to configurables
Category: task » bug
Status: Active » Needs work
-- [[SimpleTest]]: [MySQL] --

* Drupal\user\Tests\Views\HandlerFieldRoleTest (1 pass(es), 1 fail(s), and 0 exception(s))
   - [fail] [Completion check] "The test did not complete due to a fatal error." in HandlerFieldRoleTest.php on line 32 of Drupal\user\Tests\Views\HandlerFieldRoleTest->testRole().

:(

Anyone around by chance to debug this? Else I'll need to roll this back temporarily before bed.

xjm’s picture

I confirmed the fail locally; the error is:

Uncaught PHP Exception Drupal\Core\Entity\EntityMalformedException: "The entity does not have an ID." at /Applications/MAMP/htdocs/d8git/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php line 292
webchick’s picture

Title: [Testbot broke] Change notice: Convert user roles to configurables » Convert user roles to configurables
Category: bug » task

Ok, sorry folks. Reverted 2ae1597227b96 for now. :(

webchick’s picture

Priority: Critical » Normal
xjm’s picture

Status: Needs work » Needs review

#152: user.config.151.patch queued for re-testing.

dawehner’s picture

FileSize
52.31 KB
PASSED: [[SimpleTest]]: [MySQL] 50,788 pass(es). View

Rerolled for the change in views.

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

Just had a quick chat with @dawehner about his patch since there was no interdiff. Setting back to RTBC now that we got this fixed again.

andypost’s picture

+++ b/core/modules/user/user.admin.inc
@@ -826,27 +826,21 @@ function theme_user_permission_description($variables) {
 function user_admin_roles($form, $form_state) {

Needs @todo for #1872870: Implement a RoleListController and RoleFormController

andypost’s picture

Issue summary: View changes

Clarifying wording

tim.plunkett’s picture

We don't need to add an in-code todo for something that isn't changed by this patch, we have that issue.

webchick’s picture

Title: Convert user roles to configurables » Change notice: Convert user roles to configurables
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Ok! Take two! :)

Committed and pushed to 8.x.

Back to change notice.

sun’s picture

FYI: #67769: Add trim to all text field submissions puts an end to the question of "Who performs a trim() on certain property values?" — by removing all trimming from API code and leaving it to the form handling code only. Even better! It's even automated for you for all text-related form input elements, which in turn automatically triggers the regular #required validation for required fields (but you can always opt-out via '#trim' => FALSE).

fubhy’s picture

Assigned: Unassigned » fubhy

Oh god, we never did the change notice for this. On it in a minute.

fubhy’s picture

Status: Active » Needs review
fubhy’s picture

Assigned: fubhy » Unassigned
tstoeckler’s picture

Title: Change notice: Convert user roles to configurables » Convert user roles to configurables
Priority: Critical » Normal
Status: Needs review » Fixed

Made some minor adjustments (http://drupal.org/node/1907430/revisions/view/2548596/2548840) but looked very good and extensive already. Going back to fixed. If someone disagrees with my changes there's always the next revision, but that shouldn't hold up the critical count.

pcambra’s picture

I think we should add also to the change notice the way to generate roles "programatically"

$role = entity_create('user_role', array(
  'id' => 'administrator',
  'label' => 'Administrator',
));
$role->set('weight', $weight);
$role->save();
fubhy’s picture

@pcambra: You don't want that normally. Either you define them in the UI or through .yml. There are rare cases where you will want to define them in code like that.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

Issue summary: View changes

Updated issue summary.