Problem/Motivation
User roles are configuration items, therefore they should use machine names as identifiers and not serial integers. This is important for the exportability of roles. There is the Role Export module for Drupal 7 that works around the integer identifier in a hackish way. Drupal 8 should be fixed to use role machine names only.
Proposed resolution
Change all occurrences of role ids (rid) in Drupal from integers to strings (machine names).
Remaining tasks
- A change record has to reviewed and improved: http://drupal.org/node/1619504
User interface changes
- When adding a new role a role ID machine name field has to be filled out also.
- The role id is displayed as machine name on the role edit page.
API changes
The user/account object changes regarding roles: The array keys of the roles array are not numeric role ids anymore but machine name strings. The human readable role name (label) is kept as array value for now. Old role system:
$user->roles = array(
2 => 'authenticated user',
3 => 'administrator',
4 => 'site editor',
);
New system:
$user->roles = array(
'authenticated' => 'Authenticated user',
'administrator' => 'Administrator',
'site_editor' => 'Site editor',
);
The function user_role_load_by_name()
has been removed, as the role ID is the machine readable name now.
Original report by sun
Same as #934050: Change format into string, for #933846: Better distro support
Comment | File | Size | Author |
---|---|---|---|
#98 | 935062-role-names-98.patch | 40.87 KB | klausi |
#91 | 935062-role-names-91.patch | 40.87 KB | klausi |
#91 | 935062-role-names-91-interdiff.txt | 963 bytes | klausi |
#83 | 935062-role-names-83.patch | 40.16 KB | klausi |
#83 | 935062-role-names-83-interdiff.txt | 1.3 KB | klausi |
Comments
Comment #2
sunComment #4
sunComment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is undoubtedly Drupal 8 material.
Relying on the role to do *anything* is a bug by itself, as the role is (and has always been) a mere technical artifact. Core provides only one access control system and it is based on *permissions* (whose names are unique), not roles. [I'm aware that the block system rely on roles, but it's another story.
Moreover,
{role}.name
is *already* a machine name:[The fact that it is marked as 'translatable' is a bug, unique things cannot be translated]
As a consequence it is a bug for any module to rely on
{role}.rid
instead of{role}.name
on its exportable data structure.True, role names can change, but any module that rely on it for import/export can easily prevent its modification with a simple hook_form_alter().
Comment #7
klausiThis issue is tackled in D7 by the Role Export module.
For Drupal 8 we should fix this properly:
I believe we did something similar to filter formats late in the D7 development cycle, so we should consider looking into it for best practices, drawbacks and impact on (contrib) modules.
Comment #8
klausiHere is a first raw, untested, incomplete patch that shows some directions how I think it should be.
Comment #9
klausiNew patch, now at least the Drupal minimal profile installation succeeds.
Discussion points:
TODO:
Comment #10
klausiAnother iteration of this patch.
TODO:
Comment #12
klausiReplaced all occurrences of DRUPAL_ANONYMOUS_RID and DRUPAL_AUTHENTICATED_RID. Replaced all occurences of "rid" variables. Let's see what is left to do in the Simpletests.
Comment #14
klausiFixed all tests but the upgrade tests. This will be next.
Comment #15
webchickQuestion: Do we still need the DRUPAL_ANONYMOUS_ROLE et al constants if we go this route? It's more typing than just the machine name, and 'anonymous_user' seems fairly clear to me.
Comment #16
klausiGood question, I'm not sure. A constant has the advantage of IDE autocompletion, so you do not make typing mistakes as easy as with a raw string.
Also, you could redefine the constant to declare a completely different role to be the anonymous user role, but I doubt that this is a valid use case.
We could change it to be shorter, i.e. ANONYMOUS_ROLE and AUTHENTICATED_ROLE (do we need the Drupal prefix for core constants?).
Anyway, this sounds a bit like a follow-up issue. The patch will be big enough, so I suggest to focus first on porting the crap and then fixing it up in followups.
Comment #18
klausiAh excellent, simpletests are all passing except for the upgrade tests. Dumping my most recent work here which contains a largely untested update function for user.install.
Comment #20
klausiNew patch, now the bare upgrade test case should pass.
Adds a preparation function to the upgrade process because the new role name schema is needed for session bootstrapping.
Comment #21
klausiYay, tests pass now. I moved more code from the update function in user.install to update_prepare_d8_role_names() to always have a consistent state of new role names.
Now I wonder what we should do with the remaining rid column in the role table? I cannot remove it immediately during the upgrade because other modules might need the data to update their role references.
Comment #22
klausiamateescu said on IRC that this does not look right and that we are loosing the human readable name there. The reasoning behind it:
in_array('administrator', $user->roles)
instead ofisset($user->roles['administrator'])
to check if the user has a role, which is a bit harder to read, IMO.Therefore I would argue that we keep an array key value mirror for the roles array on the user object.
Comment #23
amateescu CreditAttribution: amateescu commentedI took a closer look at the patch, and aside from the comment in #22 for which I'm still not sure/convinced, this looks like a good (and much needed) improvement.
Comment #24
tstoecklerI agree with #15. And that should be a follow-up issue. The point of the constant is that
$user->roles[0]
is less clear than$user->roles[DRUPAL_ANONYMOUS_RID]
. But the latter and$user->roles['anonymous_user']
is equally clear, so there really is no point.Comment #25
klausiUpdated the issue summary.
Comment #26
fagoI agree too.
$roles['anonymous_user']
is the best readable variant.I don't think the array values are constantly set to labels currently either. E.g. I'm pretty sure that in hook_user_presave() no array values are available. That said, I'd agree that we want to keep the array keyed as it doesn't unnecessarily invalidate current checks and allows for faster checks compared to using
in_array().
.For setting a role I'd see the code to just be:
Thus, array values should be just TRUE.
Well, all the other db_tables are converted to use 'role_name' - we should be consistent here.
According to pattern outlined in #1233394: [Policy, no patch] Agree on a property naming pattern the column should be role_name, but the used property too. However, in the case of roles we usually don't need to deal with role-objects anyway. Thus I think it's fine to keep the current $object->roles or $object['roles'], while using the same array structure as for $user->roles.
This should look at the keys not values, i.e. use array_diff_key()
Comment #27
klausiThanks fago. I think the TRUE flag for user roles makes sense, updated the patch to address that. Also renamed the "role" column in the block_role table to "role_name". And fixed the array_diff_key() thing.
New patch attached, there is also a diff to the previous patch in #21.
One question: how should the role machine name column be named in the role table? Currently it is just "name". Should that be "role_name", too?
Will also update the issue summary to address the changes.
Comment #27.0
klausiissue summary template
Comment #29
klausiThanks testbot, fixed some array key handling in the simpletests and the wrong column in block.module.
interdiff is against #27.
Comment #30
gddtagging
Comment #32
klausiReuploading patch from #29 to not confuse the testbot.
Comment #34
klausiRerolled.
Comment #36
wamilton CreditAttribution: wamilton commentedApplied the patch to a freshly pulled d8, incremented the update hook number to 8002.
for file in $(git ls-files -m); do php -l $file; done
passes for me.
Comment #37
wamilton CreditAttribution: wamilton commented...and flipping status.
Comment #39
klausiFixed some more role constants that were introduced in the meantime. Interdiff is against #36.
Comment #40
ksenzeePatch didn't apply since #1015946: Eliminate $user->cache and {session}.cache in favor of $_SESSION['cache_expiration'][$bin] went in. Quick reroll while I review.
Comment #41
ksenzeeWhew, this is a long patch to review. :) Overall I think it looks great. A few comments:
No need to select r.name twice; just call fetchAllKeyed(0, 0).
The role name column is length 64. I'm not sure what happens here if I have two identical 64-character names, but it's probably bad.
s/pimary/primary/g (several instances of it throughout)
If I'm reading this right, we should be testing whether the role is either anon or auth, and this is testing whether the role is anon.
I believe it's important to get the most recently assigned role (judging from the comments in #11218: Allow default text formats per role, and integrate text format permissions, where this was added). If we use end(array_keys($array)) it should preserve the current behavior.
Lowercase only isn't being enforced at the code level as far as I can tell - we should test with the full range of characters or limit the input to lowercase only.
If we're not going to use user_roles() here, I think we should at least addTag('translatable') so the labels get translated.
Also, one general question: Would it make sense to return an array structure of
'machine_name' => 'Human-readable name'
instead of'machine_name' => TRUE
?Comment #42
klausi'machine_name' => 'Human-readable name'
?Interdiff is against #40.
Comment #44
klausiReverted the role name retrieval in the tests, as the newly added role name is not necessarily at the end of the roles array anymore.
Interdiff is against #42.
Comment #45
joachim CreditAttribution: joachim commentedWhy do we need these constants now since we have machine names? Would anything cause them to change?
Comment #46
tstoecklerI agree (#24), and so does @fago (#25).
Comment #47
klausiYes, but we agreed that removing the constants is a followup issue. This issue is just about porting/removing role IDs.
Comment #48
tstoecklerOh, I guess I missed that. I also think it's weird, since we're already changing the constants here, but I don't really mind.
In order to not forget again, I opened #1500140: Remove DRUPAL_ANONYMOUS_ROLE and DRUPAL_AUTHENTICATED_ROLE constants.
Comment #49
xjmThis is totally awesome. I think this patch is close to ready, but I also think it needs upgrade path tests. Details (plus an armada of nitpicks) follow.
The second sentence here should be moved to its own paragraph.
Shouldn't we delete these lines rather than commenting them out?
"IDs" should have capitalization.
Let's add upgrade path tests for both these updates. See: http://drupal.org/node/1429136
"Lowercase" should be one word.
Can someone confirm that the updates will run in this order? Edit: Never mind, I found the answer further up in the patch. However, I think this information is important enough to move up to the docblock. The inline comment can be its own paragraph, and the @see should be at the end after a blank line.
"Built-in" should be hyphenated here.
t()
is not needed on the assertion message texts because it is not going to be translated; it just adds overhead. Reference: http://drupal.org/simpletest-tutorial-drupal7#t(The same also applies to the other assertion messages changed by this patch, but I thought i'd point it out for these in particular since they're new.)
Comment #50
oriol_e9gFixed: #1, #2, #3, #5, #6, #7 and #8.
Remaining task: #4
Comment #51
oriol_e9gOnly for testbot.
Comment #52
klausiNow with upgrade path tests :-) The following things are checked:
It was a bit tedious to get the upgrade test database working locally as http://drupal.org/node/1429136 lacks information about that. I will followup with a writeup about that later.
Interdiff is against #50.
Comment #53
klausiHere is the writeup about my upgrade path test struggle: http://klau.si/41
Comment #54
klausiTagging.
Comment #55
klausiRerolled to account for the most recent core changes (moving simpletests around).
Comment #56
klausiRerolled to account for the user entity namespace changes.
Also changing the priority to major, since this really has to be fixed in D8.
Comment #57
bojanz CreditAttribution: bojanz commented1) Why did user_admin_roles() stop using user_roles() and switch to a query? Seems unnecessary (but if it is necessary, then it deserves a code comment).
2) Maybe the role name generating code from update_prepare_d8_role_names() should be its own function? Don't feel very strongly about it, but it crossed my mind.
Generally, this feels ready to me. Needs a review from someone nitpickier though.
Comment #58
klausiThanks Bojan!
Fixed user_admin_roles() to use user_roles() as it originally was. I don't think we have to put the special code from update_prepare_d8_role_names() into a dedicated function. It is targeted at a very special use case: to convert unique pseudo machine names to real machine names. I don't see where we would have to re-use that code.
Interdiff is against #56.
Comment #60
klausi#58: 935062-role-names-58.patch queued for re-testing.
Comment #61
bojanz CreditAttribution: bojanz commentedLet's do this!
Comment #62
sunThanks for your hard work on this.
I have one major issue with this patch though. To explain it, I need to provide history:
For D7, we introduced machine names for two configuration objects, taxonomy vocabularies and text formats. And we chose two entirely different ways to convert the two of them:
For vocabularies, we introduced a new 'machine_name' column and auto-generated the machine name for each existing vocabulary, pretty much identical to as being proposed for roles in this patch. This caused major headaches down the line, because the new machine names were not really predictable (and it caused even more confusion, because we retained the old vocabulary IDs in parallel). Not only all runtime code had to be massively updated, but also the upgrade path for all modules depending on or dealing with vocabularies in any way turned out to be very cumbersome.
For text formats, however, we chose a much simpler route: We simply converted the format ID into a string (whereas the format (ID) is equivalent to the rid here). That way, the required changes to runtime code and the upgrade path for all modules were kept minimal.
There's absolutely nothing wrong with retaining the name "rid" as the key/property that holds the machine name identifier. Existing custom roles that are upgraded simply get a numeric machine name. But of course, newly created custom roles on upgraded as well as new sites get a "regular" alphanumeric machine name assigned.
I'd really prefer to go the simple route. We did the same for image styles in the configuration system conversion for D8.
Speaking of, this issue is tagged with Configuration system, but the patch does not seem to convert the basic user role configuration to the configuration system? (the role/permissions association should be a separate issue in any case)
Comment #63
ksenzeeThe Configuration system tag is there because this issue is a blocker for converting user roles to the config system, so CMI needs to track it. It's not filed against the configuration system because it doesn't attempt to convert anything.
Comment #64
klausi@sun: Maybe you are right. I thought about this, but it seemed ridiculous to replace the existing unique role name with the numeric ID. On the other hand we don't lose the name, it is just moved to the label column. And as you said, it makes the upgrade path easier as we can just drop the rid column altogether.
Still, it is ugly that we keep such broken machine names around. Anyway, I will rework this based on your suggestion and we will see what we like best.
Comment #65
klausiAlright, updated patch attached.
Note: for the anonymous and authenticated user roles we cannot take the old rid as machine name. Those two roles are hardcoded into Drupal and their machine name must be right. So contrib module upgrade paths will also have to special case those two roles, but as the machine names are constants and the mapping is always crystal clear I see no problem here.
The "administrator" role is a different story, with this patch it gets the machine name "3" in installations that base on the standard install profile. Ugly, but I guess that's ok.
Comment #66
sunHm. Actually, I also meant to retain the name "rid" everywhere, denoting the role identifier (string).
That's what we've done with text formats, and also what we're going to do with vocabulary IDs ("vid") in #1552396: Convert vocabularies into configuration (dropping the separate/parallel 'machine_name' property instead).
Comment #67
klausiOh. I should have read your post more carefully, sorry.
My brain connects anything Drupal related ending in "id" automatically to serial integers, so I didn't think of actually keeping rid. But I agree, this might be a good idea and would drastically reduce the size of this patch as we would not have to change the two role constants.
I hope to find time tomorrow to work on this.
Comment #68
klausiWork in progress patch attached, this will not pass all tests.
Comment #69
sunUpfront: I'm fairly sure we want to restrict machine names of any kind to [a-zA-Z0-9_]. Optionally also "-", but that's very rare (only used by menu names currently).
I'd recommend to simply drop the " user" suffixes.
Comment #70
klausiAh, the space caused the test fails, copy & paste error.
Shortened the default role IDs as suggested by sun and extended the upgrade path test a bit.
Comment #71
klausiJust noticed that we have now a URL path collision:
admin/people/permissions/roles (Roles overview page)
admin/people/permissions/% (Permissions for one specified role)
admin/people/permissions/list (Permissions overview page)
Previously the rid had to be an integer, so we could easily differentiate them from "roles" and "list", but now this is a problem.
Personally I think we should move the roles pages one level up and we would get rid of the problem. I never quite understood why the roles page is so deeply nested anyway. My suggestion:
admin/people/roles (Roles overview page)
admin/people/permissions/role/% (Permissions for one specified role)
admin/people/permissions/list (Permissions overview page)
What do you think?
Comment #72
klausiShort discussion with xjm on IRC: the path collision is followup stuff, so we leave that as is for now. See also #1262812: Make "Roles" a tab of "admin/people" instead of "admin/people/permissions".
Comment #72.0
klausiupdated new role system to TRUE flags.
Comment #72.1
klausiupdated user interface changes.
Comment #73
sunAlright, this code lives in the platform sandbox now: http://drupalcode.org/sandbox/sun/1255586.git
I took the opportunity to fix, clarify, and clean up a couple of things. From a technical perspective, this patch looks pretty much ready to me.
However, this patch inherently introduces the notion of human-readable role labels. I already checked whether that could be split into a separate issue somehow, but that doesn't seem to be possible due to the D7 upgrade path being involved, which migrates the D7 role names into D8 role labels, while the role IDs are converted and retained as-is as respective machine names. So splitting that ain't possible.
Thus, we also need to cater for the resulting UI a bit. It might be possible to defer larger UI changes to a separate issue, but I guess this patch at least has to ensure some level of sanity.
On the role listing:
Also, a technical note: the form structure for the new role form elements was changed to make the fields not required and use a custom form validation handler instead, in order to allow for the tabledrag/role weighting that's baked into the form in parallel here. That does not seem correct to me; we rather want to change the form structure and potentially leverage #limit_validation_errors to prevent the role addition fields from being validated when pressing the "Save order" button. I'll look into that later.
Comment #74
sunAll of those constant replacements in tests should be backported, so I split that off into #1600892: Tests use magic numbers 1 and 2 instead of user role constants
Comment #76
sunSorry, fixed that.
Comment #77
klausiunrelated change, but I just pretend that I have not seen that.
So the patch looks good, thanks for the finetuning! (I'm not going to RTBC it as I wrote most of it)
Regarding the open points from #73: That is all non-critical followup stuff and should not stop this from being committed.
Comment #78
klausiDidn't mean to change the status, I blame dreditor.
Comment #79
sun#1600892: Tests use magic numbers 1 and 2 instead of user role constants landed.
Comment #80
sun- Adjusted user_roles() to account for the custom role labels (built-in roles are no longer translated).
- Adjusted user_admin_roles() to properly embed the role configuration form into the listing/table without duplicating it.
Comment #81
klausiAlso pushed to platform-roles-935062-klausi http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/platfo...
Comment #82
sun- Minor addition to the role administration test to verify that the delete router paths for built-in roles cannot be accessed.
- Added user_admin_role_validate() form validation callback to prevent rids that currently clash with other User module router paths and UI behavior, using the same logic as we do for node types already. As mentioned in #71 and #72, we'll likely change the router paths entirely in a separate follow-up issue, but I wanted this patch to be complete.
Comment #83
klausiFixed role name capitalization for built-in roles.
This looks RTBC to me, waiting for someone else to double check and change the status.
Comment #85
klausi#83: 935062-role-names-83.patch queued for re-testing.
Comment #87
sunAlso RTBC from my perspective. But I've added too much to this patch, so not eligible to mark as RTBC.
Comment #88
tim.plunkettI read through the patch, the code looks good and so does the grammar :)
Comment #89
Dave ReidThis seems to be missing coverage for the user_admin_role variable and administrative role functionality?
Comment #90
tim.plunkettYep, it's definitely missing the user_admin_role variable upgrade.
Comment #91
klausiRerolled the patch against the recent PSR-0 conversions of simpletests. Added a check for the administrative user role in the role upgrade path test. I think that should cover it?
Comment #92
sunThat looks sufficient. One more excellent reason for why the in-place serial ID to string conversion is just simply KISS :)
Comment #93
sun#91: 935062-role-names-91.patch queued for re-testing.
Comment #94
klausiCool, it even survived the kernel patch :-)
Comment #95
sunExcellent. So back to RTBC.
Comment #96
catch#91: 935062-role-names-91.patch queued for re-testing.
Comment #98
klausiRerolled.
Comment #99
tstoecklerBack to RTBC
Comment #100
catchThanks for the quick re-roll. Committed/pushed to 8.x.
Comment #100.0
catchWe keep rid, we just change it to be a string
Comment #100.1
klausiCorrected the new layout of $user->roles, we still have the labels
Comment #100.2
klausirole array values are not TRUE right now.
Comment #101
klausiBasic change notification created: http://drupal.org/node/1619504
Comment #101.0
klausiRemoved task
Comment #102
sunThanks, I've revised and rewritten that :)
Comment #103
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #105
DuaelFrWould it be possible to backport this to D7 ?
How is the process in this case ? Do we have to open another issue ?
Comment #106
klausiNo, this cannot be backported to D7 as it is quite a heavy API change. Use role_export if you need stable role IDs in D7.
Comment #107
DuaelFrThank you for the advice. I didn't know this module (but it will now be a full part of my drush make file !)
Comment #108
YesCT CreditAttribution: YesCT commentedthis might be not displaying correctly in some places.
Comment #109
dawehnerIt seems to be the case that using umlauts was actually intended to test some of the upgrade functionality, sorry.
Comment #109.0
dawehneradded change record link