Convert user roles to configurables

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.

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?

First draft for getting roles into cmi.

Tests should be passing now

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.

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

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.

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

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').

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

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.

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).

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.

Just adding a tag

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: 

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 :)

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.

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 :).

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

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

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.

Fixing upgrade path.

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

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

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.

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

+++ 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

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.

Fixed administration screens and update hook order

There was 3 calls to removed user_permission_roles()

Hopefully this fixes the upgrade tests

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

Suppose this should fix broken tests

Let's see how dependencies are related to failures

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

Lets see how this goes

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

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

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.

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.

Let's re-factor towards a $role->permissions property in #1857210: Move permission assignments into a $role->permissions property on the UserRole entity.

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

Minor improvements.

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 :)

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: 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.

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

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.

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

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

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?

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

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.

I continued work over at #1872874: Remove the module name property from the role permission table and started with a small patch for that. I would like to fix #1872870: Implement a RoleListController and RoleFormController next but it's currently blocked by #80855: Add element #type table and merge tableselect/tabledrag into it and it's followup #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController).

+++ 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.

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().

+++ 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.

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

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()

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

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.

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.

+++ 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!

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

Good point, sorry for the noise. Proceed :)

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?

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 :)

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!

#116: 1479454-116-user-roles.patch queued for re-testing.

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.

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!

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... :)

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.

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

I already created a follow-up for that. #1872870: Implement a RoleListController and RoleFormController

• 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!

Alright then!

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

#126: 1479454-126-user-roles.patch queued for re-testing.

Aaaaand back to catch. :)

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

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

Will retest...

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

Yeah, doesn't fail locally for me either.

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

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

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 :(

+++ 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 :(

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

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.

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.

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.

Wow, I am blind!

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.

@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.

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.

-- [[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.

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

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

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

Rerolled for the change in views.

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.

+++ 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

Clarifying wording

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

Ok! Take two! :)

Committed and pushed to 8.x.

Back to change notice.

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).

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

http://drupal.org/node/1907430

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.

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();

@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.

Updated issue summary.

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

Updated issue summary.