Currently user_roles is a weird bridge between the list of available roles (which is configuration) and the user (which is an entity). I suggest we move a user's role assignments to the user entity as a default field. This implementation would be similar to the one I proposed for path aliases in #1751210: Convert URL alias form element into a field and field widget.

- Add a multi-instance text field to the user entity, which has a select list of available roles (which will be stored in CMI).
- Set this field to be Format: Hidden by default in all view modes

In the aliases issue I suggested saving the aliases out to denormalized storage for the purposes of performant reading, but I'm not sure that is necessary here.

Files: 
CommentFileSizeAuthor
#56 1751274_56.patch15.34 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,309 pass(es), 65 fail(s), and 21 exception(s). View
#53 1751274_53-interdiff.txt2.77 KBBerdir
#53 1751274_53.patch15.33 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,262 pass(es), 65 fail(s), and 21 exception(s). View
#53 1751274_53-test-only.patch1.71 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,305 pass(es), 0 fail(s), and 1 exception(s). View
#50 1751274_50.patch12.67 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 54,447 pass(es), 2,953 fail(s), and 1,487 exception(s). View
#48 1751274_48.patch13.06 KBchx
FAILED: [[SimpleTest]]: [MySQL] 55,277 pass(es), 2,277 fail(s), and 1,126 exception(s). View
#45 1751274_45.patch14.91 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,559 pass(es). View
#45 interdiff.txt1.82 KBchx
#42 1751274_42.patch14.95 KBchx
PASSED: [[SimpleTest]]: [MySQL] 59,033 pass(es). View
#39 1751274_39.patch14.86 KBchx
FAILED: [[SimpleTest]]: [MySQL] 54,901 pass(es), 2,053 fail(s), and 1,014 exception(s). View
#37 1751274_37.patch14.97 KBchx
FAILED: [[SimpleTest]]: [MySQL] 58,794 pass(es), 1 fail(s), and 0 exception(s). View
#36 1751274_36.patch8.7 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,575 pass(es). View
#33 1751274_32.patch8.12 KBchx
FAILED: [[SimpleTest]]: [MySQL] 58,750 pass(es), 0 fail(s), and 1 exception(s). View
#27 get-user-roles-method-27.patch9.44 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 56,634 pass(es), 131 fail(s), and 149 exception(s). View
#27 get-user-roles-method-27-interdiff.txt2.62 KBBerdir
#25 get-user-roles-method.patch8.85 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

larowlan’s picture

Issue tags: +d8dx

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

heyrocker’s picture

Issue tags: +Configuration system
marcingy’s picture

Assigned: Unassigned » marcingy

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

heyrocker’s picture

heyrocker’s picture

Issue tags: +feature freeze

Tagging as needing to be done by feature freeze

fubhy’s picture

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

marcingy’s picture

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

fubhy’s picture

Assigned: marcingy » fubhy

Okay. Assigning this to me then. :)

sun’s picture

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.

Berdir’s picture

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

fago’s picture

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

fago’s picture

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

fubhy’s picture

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

Berdir’s picture

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.

chx’s picture

Issue tags: -feature freeze

This is my current understanding after talking to heyrocker.

moshe weitzman’s picture

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

fubhy’s picture

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.

webchick’s picture

Priority: Normal » Major

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

fubhy’s picture

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.

marcingy’s picture

Assigned: fubhy » marcingy

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

Berdir’s picture

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.

marcingy’s picture

Assigned: marcingy » Unassigned

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.

djdevin’s picture

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

Berdir’s picture

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

Berdir’s picture

Status: Active » Needs review
FileSize
8.85 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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.

andypost’s picture

Status: Needs review » Needs work
+++ 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')

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
9.44 KB
FAILED: [[SimpleTest]]: [MySQL] 56,634 pass(es), 131 fail(s), and 149 exception(s). View

This should work better.

Status: Needs review » Needs work

The last submitted patch, get-user-roles-method-27.patch, failed testing.

heyrocker’s picture

Priority: Major » Critical

This is critical, if it doesn't happen then we can't deploy user permissions

Berdir’s picture

Priority: Critical » Major

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.

plach’s picture

chx’s picture

Assigned: Unassigned » chx
chx’s picture

Assigned: chx » Unassigned
Status: Needs work » Needs review
FileSize
8.12 KB
FAILED: [[SimpleTest]]: [MySQL] 58,750 pass(es), 0 fail(s), and 1 exception(s). View
chx’s picture

Assigned: Unassigned » chx

Opsie, I didn't want to unassign.

Status: Needs review » Needs work

The last submitted patch, 1751274_32.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
8.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,575 pass(es). View
chx’s picture

Title: Implement user_roles as a field on user entities » Do not query user_roles directly
FileSize
14.97 KB
FAILED: [[SimpleTest]]: [MySQL] 58,794 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 1751274_37.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
14.86 KB
FAILED: [[SimpleTest]]: [MySQL] 54,901 pass(es), 2,053 fail(s), and 1,014 exception(s). View

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.

jibran’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1751274_39.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
14.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,033 pass(es). View

Thanks, jibran!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

amateescu’s picture

  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.

chx’s picture

FileSize
1.82 KB
14.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,559 pass(es). View

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

alexpott’s picture

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

chx’s picture

Status: Needs work » Needs review
FileSize
13.06 KB
FAILED: [[SimpleTest]]: [MySQL] 55,277 pass(es), 2,277 fail(s), and 1,126 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1751274_48.patch, failed testing.

pfrenssen’s picture

Issue tags: -Needs reroll
FileSize
12.67 KB
FAILED: [[SimpleTest]]: [MySQL] 54,447 pass(es), 2,953 fail(s), and 1,487 exception(s). View

Straight reroll.

tim.plunkett’s picture

Status: Needs work » Needs review

The last submitted patch, 1751274_50.patch, failed testing.

Berdir’s picture

Assigned: chx » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,305 pass(es), 0 fail(s), and 1 exception(s). View
15.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,262 pass(es), 65 fail(s), and 21 exception(s). View
2.77 KB

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

The last submitted patch, 53: 1751274_53-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 53: 1751274_53.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
15.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,309 pass(es), 65 fail(s), and 21 exception(s). View

Reoll 1751274_53.patch no longer applies

Status: Needs review » Needs work

The last submitted patch, 56: 1751274_56.patch, failed testing.

YesCT’s picture

adding the more widely used tag.

martin107’s picture

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.

plach’s picture

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

chx’s picture

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

plach’s picture

yep, that

Berdir’s picture

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.

longwave’s picture

Title: *-* Watch The Fluffy Movie 2014, Online Free Full Movie » Do not query user_roles directly
Issue summary: View changes
Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Closed (duplicate)

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 SessionHandler instead of using manual queries, so closing as duplicate of that.