Problem/Motivation

#2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities introduced automatic schema handling for base fields. #2337927: SqlContentEntityStorage::onFieldStorageDefinition(Create|Update|Delete)() is broken for base fields and a few other issues later, we almost support base fields with multiple values. The only missing puzzle piece is support for those fields in entity queries.

We currently have a lot of code to handle user -> role relationships: custom table, CRUD methods on the storage controller, hacks for entity reference autocomplete support and there is also a critical issue open about inconsistencies: #2374081: User::preCreate() is broken.

Proposed resolution

Add missing support for multi-value base fields to entity query, which turns out to be very easy as we only need to switch out a check for configurable fields with a more generic check if the field has a separate table.

Drop custom storage for the user.roles references to proof that everything is working, refactor authenticated/anonymous role ID handling so that it works with the default storage: never store those roles on the User entity, instead add it dynamically based on isAuthenticated() in getRoles().

Remaining tasks

- Reviews, especially of the views changes.

- We identified that RoleStorage::deleteRoleReferences() is not only on the wrong entity storage class (it really is part of user storage, not roles), it is also never called in HEAD. #2217749: Entity base class should provide standardized cache tags and built-in invalidation seems to have removed that call accidentally. We should open a follow-up to move the method to UserStorage and call it again (Or drop it, because we don't do this kind of cleanup anywhere else, although this is arguably in a security related context, imagine removing and adding a role with the same rid?).

User interface changes

API changes

The {users_roles} table is renamed to the automatically created {user__roles} with different columns (the same as any configurable field). This table is already considered private to the entity user storage, we only query it from a few places like session storage, hacks for entity reference autocomplete (which are removed here) and views.

It is not directly visible here, but session storage is already highly dependent on the user storage implementation (it queries the user_field_data table), this does not make it any worse than it already was. And #2345611: Load user entity in SessionHandler instead of using manual queries intents to clean this up anyway.

Comments

Berdir’s picture

Before we can do that we need to support user roles with entity query and use that I think. There's an issue somewhere for that.

tstoeckler’s picture

Berdir’s picture

Status:Postponed» Active
Berdir’s picture

I started working on this but I run into a nasty problem with the field cache. The loading of that data happens inside the field methods, which are cached. And the problem is that we only cache configurable fields.

Instead of fixing that, let's just get #597236: Add entity caching to core in.

Berdir’s picture

See https://github.com/Berdir/d8/commit/d46feadd92c33ee22eee345832299b5c1c22... for the first step that is needed here.

With that, the user__roles table is created and values are saved, as mentioned needs the entity cache issue to fix the broken field cache then. Obviously will need a lot more changes to actually remove and stop using the old table.

One question is the changes in user_install(). Should getSchema() be extend to automatically include the schema for the additional multi-value field types? I guess so.

Also had problem in case of user as it ended up in that allowed column names function there, but that was probably only because I call it through user_install() and that will get fixed in @plach's issue afaik.

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new16.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,978 pass(es), 814 fail(s), and 246 exception(s).
[ View ]

Stuff will be broken, I guarantee it.

But, installer works and saving and loading users seems to be working.

One problem is that we do not store anonymous/authenticated user roles, but we currently don't have a way to influence that easily, will need to think about that.

Views integration and so on is obviously a mess. Did some search and replace but that will by far not be enough.

Berdir’s picture

One thing to consider might be to pick a test example to get the ability to do this in because most issues that we will see are specific to user roles, which are special in various ways.

Status:Needs review» Needs work

The last submitted patch, 6: user-role-storage-2248977-6.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new172.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,181 pass(es), 109 fail(s), and 5 exception(s).
[ View ]
new14.04 KB

Rerolled this on top of #2337927: SqlContentEntityStorage::onFieldStorageDefinition(Create|Update|Delete)() is broken for base fields, first patch is combined to run the tests and second is the changes necessary for this to work.

Most test fails above were kernel tests being broken (should be fixed) and the taxonomy term parent field not being marked as custom storage, so that fell apart, that got flagged correctly in the meantime, so let's see how many fails are left.

Status:Needs review» Needs work

The last submitted patch, 9: user-role-storage-2248977-9.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

Re-roll after the referenced issue got in, which also includes a better fix for dedicated field tables that gets rid of the unit test failures.

Fixing some test failures, there shouldn't be too many left. One blocker is now entity query support for multi-value base fields, that currently doesn't work at all, just like field with multiple properties don't work.

Anyway, those fixes, apart from the entity query bug are user roles specific, so multi-value base fields are mostly supported now. Maybe change the issue to make it specific about user role fields?

Berdir’s picture

Status:Needs review» Needs work

Somehow the files didn't make it, will upload tonight.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new20.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,523 pass(es), 196 fail(s), and 11 exception(s).
[ View ]
new9.83 KB

Status:Needs review» Needs work

The last submitted patch, 13: user-role-storage-2248977-11.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new24.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,005 pass(es), 20 fail(s), and 1 exception(s).
[ View ]
new7.7 KB

Getting there...

Status:Needs review» Needs work

The last submitted patch, 15: user-role-storage-2248977-15.patch, failed testing.

The last submitted patch, 15: user-role-storage-2248977-15.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new33.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,035 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new8.84 KB

Fixing tests.

Scared of looking into entity query integration :(

Status:Needs review» Needs work

The last submitted patch, 19: user-role-storage-2248977-17.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new33.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,167 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Re-roll.

Status:Needs review» Needs work

The last submitted patch, 21: user-role-storage-2248977-21.patch, failed testing.

plach’s picture

Priority:Normal» Major

This is at very least major.

Berdir’s picture

@plach: Yeah :) I would be awesome if you could have a look at Tables.php. I think we should be able to simplify that *a lot* by relying on the new schema mapping stuff and also support this. I hope we can get rid of the separation of base and configurable fields completely, I don't think there is any reason that we still need that...

plach’s picture

Do you think we should refactor the SQL entity query backend in this issue?

Berdir’s picture

Not sure if we should, but I've been thinking that it might be easier to do that than to add more hacks on top of the existing hacks to support this use case.

plach’s picture

Ok, let's try that way then. If turns it's too complex, we may defer that to another issue.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new36.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,951 pass(es).
[ View ]
new3.3 KB

Well, that was almost too easy. Those tests are passing, let's see if something else broke.

plach’s picture

Looks great, just minor stuff :)

  1. +++ b/core/modules/user/src/Entity/User.php
    @@ -172,8 +156,18 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +    // User with an ID always have the authenticated user role.

    "Users", maybe?

  2. +++ b/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php
    @@ -43,11 +43,19 @@ public function providerTestHasPermission() {
    +    if ($authenticated) {
    +      array_unshift($rids, DRUPAL_AUTHENTICATED_RID);
    +    }
    +    else {
    +      array_unshift($rids, DRUPAL_ANONYMOUS_RID);
    +    }

    This could be simplified to a single line:

    <?php
    array_unshift
    ($rids, $authenticated ? DRUPAL_AUTHENTICATED_RID : DRUPAL_ANONYMOUS_RID);
    ?>
yched’s picture

Awesome patch !

  1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
    @@ -110,12 +105,14 @@ public function addField($field, $type, $langcode) {
           // If we managed to retrieve a configurable field, process it.
    -      if ($field_storage instanceof FieldStorageConfigInterface) {
    +      if ($field_storage && $table_mapping->requiresDedicatedTableStorage($field_storage)) {

    followed by :

    }
    // This is an entity base field (non-configurable field).
    else {
      ...
    }

    Those comments need to be updated, this if/else is not about configurable/base fields anymore (yay)

  2. +++ b/core/lib/Drupal/Core/Session/UserSession.php
    @@ -126,7 +126,7 @@ public function getRoles($exclude_locked_roles = FALSE) {
    -      $roles = array_diff($roles, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID));
    +      $roles = array_values(array_diff($roles, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID)));

    Not sure I get why this is needed ?

  3. +++ b/core/modules/user/src/RoleStorage.php
    @@ -34,8 +34,8 @@ public function isPermissionInRoles($permission, array $rids) {
       public function deleteRoleReferences(array $rids) {
         // Remove the role from all users.
    -    db_delete('users_roles')
    -      ->condition('rid', $rids)
    +    db_delete('user__roles')
    +      ->condition('target_id', $rids)
           ->execute();
       }

    As mentioned in IRC, this method should live on UserStorage, rather than having RoleStorage assume UserStorage is the default SQL.

    Then again, looks like nothing in core currently calls that method - who cleans records when a role is deleted ?

  4. +++ b/core/modules/user/src/Tests/UserEntityReferenceTest.php
    @@ -78,10 +78,14 @@ function testUserSelectionByRole() {
    +    debug(db_query('SELECT * FROM {users}')->fetchAll());
    +    debug(db_query('SELECT * FROM {user__roles}')->fetchAll());
    +
    ...
    +    debug($matches);
    +++ b/core/modules/user/src/Tests/Views/HandlerFieldRoleTest.php
    @@ -41,11 +41,13 @@ public function testRole() {
    +    debug(db_query('SELECT * FROM {user__roles}')->fetchAll());

    :-)

  5. So if I get things right,
    - the $user->roles field only contains actual roles, not DRUPAL_AUTHENTICATED_RID / DRUPAL_ANONYMOUS_RID
    - $user->getRoles() reads the above and adds DRUPAL_AUTHENTICATED_RID / DRUPAL_ANONYMOUS_RID,
    - $user->addRole($rid) adds the role to $user->roles

    2 remarks :
    - We probably can't prevent code to do $user->roles[] = DRUPAL_AUTHENTICATED_RID directly, but shouldn't addRole() check that the caller is not trying to assign DRUPAL_AUTHENTICATED_RID / DRUPAL_ANONYMOUS_RID ?
    - If a user does $user->roles[] = DRUPAL_AUTHENTICATED_RID, looks like getRoles() will return DRUPAL_AUTHENTICATED_RID twice ?

yched’s picture

Also, the issue title and summary could probably be adjusted to reflect the current patch - multi-valued base fields are supported by the storage now, this patch only fixes EFQ, and then removes the custom storage for $user->roles.

Berdir’s picture

Title:Support multi-value base fields in ContentEntitySchemaHandler» Complete support for multi-value base fields in ContentEntitySchemaHandler and use it for the user.roles field
Issue summary:View changes
Related issues:+#2374081: User::preCreate() is broken., +#2345611: Load user entity in SessionHandler instead of using manual queries

Thanks for the reviews.

#30.2: Minor, the unit tests expected a correctly numbered list of permissions. Could also change the tests, but that's pretty random.

#30.3: (who calls it) Very good question, some digging pointed me to #2217749: Entity base class should provide standardized cache tags and built-in invalidation, which i think deleted too much in Role::postDelete(). Can we deal with this in a separate issue? (move to user storage, call again, add tests..). For the record, this is a special case among entity references for doing such cleanups, we don't bother anywhere else.
#30.5: Added exception to addRole(), for the second part, User::preSave() already contains this:

<?php
   
// Make sure that the authenticated/anonymous roles are not persisted.
   
foreach ($this->get('roles') as $index => $item) {
      if (
in_array($item->target_id, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
       
$this->get('roles')->offsetUnset($index);
      }
    }
?>

So it would only happen between manually setting the role and saving the entity. I don't think that's worth to prevent?

Everything else should be fixed. updated the issue summary and title, please check.

Berdir’s picture

StatusFileSize
new37.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,137 pass(es).
[ View ]
new4.49 KB

Aw. patches.

Berdir’s picture

Issue summary:View changes
Berdir’s picture

Issue summary:View changes
andypost’s picture

just nitpics, is the "user__roles" table populated automatically?

  1. +++ b/core/modules/user/src/Tests/UserEntityReferenceTest.php
    @@ -78,6 +78,7 @@ function testUserSelectionByRole() {
    +

    unnecessary change :(

  2. +++ b/core/modules/user/src/Tests/Views/HandlerFieldRoleTest.php
    @@ -41,11 +41,13 @@ public function testRole() {
    +    debug(db_query('SELECT * FROM {user__roles}')->fetchAll());
    +

    debug

andypost’s picture

Berdir’s picture

Yes, user__roles is a field table, just like any other and is automatically created and managed.

Will remove the debug on the next update.

plach’s picture

This looks great to me and we should really have it committed before we are done with D8 upgrade stuff, but I'll the final word to @yched.

effulgentsia’s picture

Issue tags:+D8 upgrade path

No time to read the patch now, but +1 to the concept. Tagging per #39.

yched’s picture

[yched #30] If a user does $user->roles[] = DRUPAL_AUTHENTICATED_RID, looks like getRoles() will return DRUPAL_AUTHENTICATED_RID twice ?

[Berdir #32] User::preSave() already filters out DRUPAL_ANONYMOUS_RID / DRUPAL_AUTHENTICATED_RID, so it would only happen between manually setting the role and saving the entity. I don't think that's worth to prevent?

Yeah, dunno :-) Having business APIs that enforce runtime consistency for their domain-logic, without assuming a load/save cycle to do some cleanup is a good pattern to have IMO. We should avoid things that work differently for purely runtime entities and entities that are saved and loaded.
I mean, $user->save() changing the value returned by $user->getRoles() is not too great either ?

Berdir’s picture

Opened #2390467: User role permission assignments are not deleted when a role is deleted.

Discussed #41 with @yched, I think I convinced him that protecting against this on the field-API level would be very complicated and not worth it. The business API (addRole()) does protect against it.

Anything left here?

yched’s picture

Status:Needs review» Reviewed & tested by the community

Anything left here?

Nope, not for me :-)

plach’s picture

RTBC + 1

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.0.x. Thank you.

  • Dries committed e20d78e on 8.0.x
    Issue #2248977 by Berdir: Complete support for multi-value base fields...
yched’s picture

@Berdir : I might be wrong, but @larowlan's comment in #2342045-40: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency looks related to the user__roles changes made here.

Status:Fixed» Closed (fixed)

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