Complete support for multi-value base fields in ContentEntitySchemaHandler and use it for the user.roles field

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.

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.

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.

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.

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.

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.

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?

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

Getting there...

Fixing tests.

Scared of looking into entity query integration :(

Re-roll.

This is at very least major.

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

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

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.

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

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

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:

   array_unshift($rids, $authenticated ? DRUPAL_AUTHENTICATED_RID : DRUPAL_ANONYMOUS_RID);
   

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 ?
   

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.

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:

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

Aw. patches.

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

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.

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.

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

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

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?

Anything left here?

Nope, not for me :-)

RTBC + 1

Committed to 8.0.x. Thank you.

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