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 Cookie AuthenticationProvider instead of using manual queries intents to clean this up anyway.
Comment | File | Size | Author |
---|---|---|---|
#33 | user-role-storage-2248977-32-interdiff.txt | 4.49 KB | Berdir |
#33 | user-role-storage-2248977-32.patch | 37.26 KB | Berdir |
Comments
Comment #1
BerdirBefore 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.
Comment #2
tstoecklerYeah, postponed at least on #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities.
Comment #3
BerdirComment #4
BerdirI 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.
Comment #5
BerdirSee 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.
Comment #6
BerdirStuff 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.
Comment #7
BerdirOne 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.
Comment #9
BerdirRerolled 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.
Comment #11
BerdirRe-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?
Comment #12
BerdirSomehow the files didn't make it, will upload tonight.
Comment #13
BerdirComment #15
BerdirGetting there...
Comment #19
BerdirFixing tests.
Scared of looking into entity query integration :(
Comment #21
BerdirRe-roll.
Comment #23
plachThis is at very least major.
Comment #24
Berdir@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...
Comment #25
plachDo you think we should refactor the SQL entity query backend in this issue?
Comment #26
BerdirNot 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.
Comment #27
plachOk, let's try that way then. If turns it's too complex, we may defer that to another issue.
Comment #28
BerdirWell, that was almost too easy. Those tests are passing, let's see if something else broke.
Comment #29
plachLooks great, just minor stuff :)
"Users", maybe?
This could be simplified to a single line:
Comment #30
yched CreditAttribution: yched commentedAwesome patch !
followed by :
Those comments need to be updated, this if/else is not about configurable/base fields anymore (yay)
Not sure I get why this is needed ?
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 ?
:-)
- 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 ?
Comment #31
yched CreditAttribution: yched commentedAlso, 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.
Comment #32
BerdirThanks 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:
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.
Comment #33
BerdirAw. patches.
Comment #34
BerdirComment #35
BerdirComment #36
andypostjust nitpics, is the "user__roles" table populated automatically?
unnecessary change :(
debug
Comment #37
andypostComment #38
BerdirYes, user__roles is a field table, just like any other and is automatically created and managed.
Will remove the debug on the next update.
Comment #39
plachThis 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.
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedNo time to read the patch now, but +1 to the concept. Tagging per #39.
Comment #41
yched CreditAttribution: yched commentedYeah, 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 ?
Comment #42
BerdirOpened #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?
Comment #43
yched CreditAttribution: yched commentedNope, not for me :-)
Comment #44
plachRTBC + 1
Comment #45
Dries CreditAttribution: Dries commentedCommitted to 8.0.x. Thank you.
Comment #47
yched CreditAttribution: yched commented@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.