Do not query user_roles directly

I think we should add a fapi element and a field type while we're at it, like we did for language. Ie a role reference field type and a #type => role

So chx tells me this is next on my hit list ;)

Are you still on this marcingy? Otherwise I can take over and work on a patch on monday.

@Fubhy feel free to work on it I got distracted by some other core stuff. If you don't get to it I will have a look after badcamp.

Okay. Assigning this to me then. :)

Something bugs me big time here, which makes me believe that this is architecturally not correct. I'm not yet sure what exactly, but let me try:

1) A Field API field (not extra field) can be removed. We don't want that.

2) Anything field-related is a huge dependency. Which might not be available early enough.

3) Field module is a module. Modules can be disabled. User roles are a critically important part of the user system.

4) The summary talks about a multi-valued text field with a select widget. That inherently means additional dependencies on Text (field type) and Options (field widget) modules.

5) We don't want to store user role assignments in a field data table.

6) Field values are translatable and automatically get revisioned, depending on the attached entity. Are field modules able to force-opt-out of these features? I don't think we want translatable user role assignments, nor user role values to be revisioned.

Here's the current plan about fields/properties, which I think would quite nicely address sun's concerns.

The property api has been renamed to field API, the idea is that everything is a field, while the current fields are renamed "configurable fields". The basic idea is that everything that's added by the module and can't be removed is a field, all entities have these, while the current "fiedable" will be renamed to "configurable". Fields can be per-language/revision/multiple as required, but don't have to.

I'm not sure yet how exactly we'd handle the storage of this, but we could make roles such a field on the user entity, probably even keep the existing table, at least for now.

So the answer to sun's points based on this would be:

1) Entity fields are handled by the storage controller. The plan is/was to change configurable fields to this as well, although I don't see how we're going to manage that in the remaining time :(

2) See above. Non-configurable fields are available as early as the storage controller.

3) Same.

4) Not sure how the UI would be handled, could be kept custom for the moment?

5) Could stay in it's own table I think.

6) See above, we could make this field multiple but not per-language/revision.

There are quite some dependencies to take care of before this can happen though, including converting user to EntityNG and either the session refactoring or that other issue that would remove the user dependency from session.inc. But we could maybe also keep it as a custom "special" table and add it to $user without calling it field :)

#10 gets it - so yep, we can implement a field (as defined by the new entity field API) without having to make it configurable. It can come pre-defined with user entity type only.

But we could maybe also keep it as a custom "special" table and add it to $user without calling it field

I'd not suggest that, as it would not be properly part of the entity any more then, e.g. it won't be handled by the entity serialization and REST service.

4) Not sure how the UI would be handled, could be kept custom for the moment?

Yes, we don't have to use field API widgets then. Moreover, it won't work right now as afaik field API is not refactored in any way yet to support non-configurable fields as well.

5) Could stay in it's own table I think.

Yep.

Are user roles becoming configuration entities? If so, we should even be able to use the entity_reference_field field type we already have (for stuff like $comment->nid).

@fago, yes... that issue is over here: #1479454: Convert user roles to configurables

But we could maybe also keep it as a custom "special" table and add it to $user without calling it field

I'd not suggest that, as it would not be properly part of the entity any more then, e.g. it won't be handled by the entity serialization and REST service.

Clarification: I meant that to be a temporary solution until the necessary dependences are resolved, because they are non-trivial.

This is my current understanding after talking to heyrocker.

Roles as CMI and user_roles as entity fields makes a lot of sense. Keep going!

Yeah, I got a bit delayed on this because of the roles as CMI / permissions on those roles / etc. introduces a pretty heavy early-bootstrap dependency on entities. Which breaks in a few ugly places in core (update.php, authorize.php, etc.). Will look into that again after the weekend. I hope that I can unblock that somehow.

Talking to heyrocker, he says we basically have to do this or we have no way to deploy user/role assignments.

Yep! I want this to land too, however it is currently blocked by #1818570: Convert users to the new Entity Field API (which should wait for #1818556: Convert nodes to the new Entity Field API to land) if we wan't to do it right. Otherwise we will indeed run into the issues mentioned in #9.

Going to start on a none entityNG version just to get this started, I realise this may mean some re-roll fun but.....

I'm not sure if that is useful.

The EntityNG conversion of users will *require* (at least the full conversion, possibly not the initial BC decorator based on) that this is a field, a field in the NG sense, which means that it has to be defined with hook_entity_field_info() and an item class needs to exist. See PathItem in the taxonomy term conversion issue for $term->path.

Ah, my ignorance of what EntityNG fully is strikes. In that case it sounds like I should start attempting to tackle the conversion issue for user if I want to help push this forward. First task get up too speed with EntityNG.

Tagging for issue summary update - see https://drupal.org/issue-summaries

As discussed, it's technically a field now and code outside of the storage controller should not mess with that table anymore.

For example: #2024985: Remove obsolete user_build_filter_query.

Some other places where we query this table can simply be replaced, e.g. the code in user_admin_account() can be replaced with $account->getRoles().

Ok, here's a start for this.

This adds a getUserRoles(array $uids) method to the user storage controller and uses it where we can. Also updated some code to use getRoles() or other proper API's instead of manual queries. Doesn't look too bad.

I still haven't found a use case where we need EFQ support for roles, so I'll just add a test for it.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Roles.phpundefined
@@ -26,9 +27,9 @@ class Roles extends PrerenderList {
-  protected $database;
+  protected $storage_controller;

@@ -42,17 +43,17 @@ class Roles extends PrerenderList {
-    $this->database = $database;
+    $this->storageController = $storage_controller;

@@ -79,10 +80,12 @@ public function preRender(&$values) {
+      $users_rids = $this->storage_controller->getUserRoles($uids);

$storageController

+++ b/core/modules/user/user.admin.incundefined
@@ -28,22 +28,21 @@ function user_admin_account() {
+  foreach (user_load_multiple($uids) as $account) {

entity_load_multiple('user', $uids)

+++ b/core/profiles/standard/standard.installundefined
@@ -45,9 +45,9 @@ function standard_install() {
+  $user = user_load(1);

entity_load('user')

This should work better.

No, we can't do that anyway. You can deploy roles and their permissions, but not users<->roles assignments. This issue is not about storing this in CMI, it is at this point simply about deprecating direct access to users_roles and adding support for it in EFQ.

Related issue: #2068329: Convert user SQL queries to the Entity Query API.

Opsie, I didn't want to unassign.

It's not really a field still (I wonder whether that's a problem? you can't do cross backend entity queries and the next entity in the chain, role, is a config entity so the party stops cold at users_roles anyways) but I got entity query working against 'rid'. It's also tested. The amount of boilerplate is horrifying (94 LoC for 1 LoC of actual code adding the users_roles table) but that's not the problem of this issue -- that was #2038707: Entity query sql backend limits storage controllers changes in contrib feel free to reopen or create a followup to find a better solution. Actually, it'll be much easier to refactor this once we have a SQL-extending implementation like this and not just doing some theoretical work.

The drop is always moving! Between the time I began to work on this and I submitted the patch, we have added users_roles to EntityUnitTestBase.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Roles.php
@@ -79,10 +80,15 @@ public function preRender(&$values) {
+            $this->items[$uid][$rid]['role'] = check_plain($roles[$rid]->label());

String::checkPlain()

Thanks, jibran!

Opened a follow up for the DX of entity query extensions https://drupal.org/node/2096161

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php
   @@ -509,7 +509,7 @@ protected function assertBundleOrder($order) {
   +  protected function testMetaData() {
   

   Test methods need to be public afaik..

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/UserRidQueryTest.php
   @@ -0,0 +1,44 @@
   +  protected function testUserRoleQuery() {
   

   Same here.

Still RTBC, cleaned up the tests and acted on #44. Sorry, I didn't quite catch that the issue was filed and I filed another #2096271: Make it easier to extend entity query however that one has a patch so I hope you can forgive me for the duplicate :) We might prefer to commit that first and remove the three empty classes from the patch here first. The patches are independent though so the order doesn't matter much.

Patch no longer applies.

#2096271: Make it easier to extend entity query has landed so we can remove the empty classes.

Straight reroll.

Re-roll, fixed some stuff and extend the test a bit to verify multiple cardinality.

Reoll 1751274_53.patch no longer applies

adding the more widely used tag.

Just a note

This patch introduces a new test UserRidQueryTest.php and the getInfo() function has been removed from core.

#697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)

There is no real problem, the group information needs to be moved into annotations.

+++ b/core/includes/session.inc
@@ -113,8 +113,8 @@ function _drupal_session_read($sid) {
+    $rids = Drupal::entityManager()->getStorageController('user')->getUserRoles(array($values['uid']));

diff --git a/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php
index c01a2e2..f8bfde6 100644

I was wondering whether here we are not loading the user entity just for a performance reason of if there's anything else I am missing.

If performance is the only concern, I think we could load the entity object, as it is almost always going to be loaded during the request anyway.

Moreover, using a non-db based cache backend loading the user might be even more efficient than querying roles.

If we go this way I think we can make the ::getUserRoles() protected. This in turn will enable us to switch to regular multiple field storage in #2044859: Convert user roles to entity_reference_field, without introducing API changes.

I am wondering myself -- since entity caching can't we just load the user fully?

yep, that

Well, yes but....

Then we will run into the same problem as rest does with the t() calls in baseFieldDefinitions() in early bootstrap. that currently leads to a loop if you're not using en. However, we somehow need to solve that anyway, possibly by using the TranslationWrapper class.

One thing that we need to worry about is performance of permission checks, we possibly need to optimize that.

I started working on supporting multi-value base fields in #2248977: Complete support for multi-value base fields in ContentEntitySchemaHandler and use it for the user.roles field, which basically removes all the special methods that we currently need.

Ok, that issue got in, which means that user roles are now a standard field table supported by entity query out of the box.

The only part that was not fixed by that issue AFAIK is the manual query in session storage, but that's what I'm trying to do in #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries, so closing as duplicate of that.