Problem/Motivation
Currently there are three Installation Profiles: Minimal, Standard and Demo: Umami Food Magazine.
When you install a new Drupal site with the Standard profile, you get three user roles by default: Admin, Authenticated user (logged-in user when this issue is fixed) and Anonymous.
- Admin role: for site building and development purposes and comes with almost every existing permission by default.
- Authenticated/logged-in role: has the same permissions as the Anonymous user (viewing content) plus is able to post comments.
- Anonymous role: for visitors to the site not logged in.
Drupal is a content management system but we don’t have any default user role for creating, editing or managing content.
Proposed resolution
Based on our discovery, this proposal aims to create an out-of-the-box editor role to serve Drupal’s main purpose: manage content.
We propose that this new role is named “Content Editor.”
Installed with the standard profile, this new role will be created with a new set of permissions and options. Our initial proposal for this set is:
- Create and edit new content — articles and pages
- Use the basic HTML text format
- View unpublished content
- Create and edit terms (tags and categories)
- Access the administration theme
Remaining tasks
- Agree on the name and machine name for the new role
- Define the new set of permissions
- Create a patch
- Code Review
- Create tests?
User interface changes
- Roles and permissions screen:
- Content Editor role will be included.
- The new permissions for the role will be checked.
- Edit or Create user screens should include the Content Editor role in the options to select the role(s).
API changes
No changes to existing API changes?
Data model changes
No changes to existing data models?
Comment | File | Size | Author |
---|---|---|---|
#41 | interdiff-38-41.txt | 703 bytes | ressa |
#41 | 3059984-41.patch | 3.92 KB | ressa |
#38 | interdiff-32-38.txt | 494 bytes | ressa |
#38 | 3059984-38.patch | 3.92 KB | ressa |
#26 | 3059984-after_patch.png | 22.9 KB | Abhijith S |
Issue fork drupal-3059984
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
antonellasev CreditAttribution: antonellasev as a volunteer commentedRevision to clean up formatting
Comment #3
ckrinaComment #4
ckrinaComment #5
ckrinaComment #6
quironThis patch adds the role to a clean installation of the standard profile. The permissions are the closest ones that make sense based on the description.
Not that this will not add the 'Content Editor' role to an installed site upgrading to new versions.
Comment #7
quironsorry, the previous one was wrongly formatted.
Comment #9
shaalUmami comes with an Editor role, maybe we want to take the list permissions from there?
I created a patch to add additional permissions to the Editor role in Umami - #3066570: Adding additional permissions to Editor in Umami.
Another question - should we add all the necessary Editor permissions when all core's modules are enabled?
(and then as soon as modules are enabled, Editor will already have the correct permissions)
Comment #10
quiron@shaal I checked the Umami permissions already. To me looks like the roles there have more options, like manage URL aliases or delete revisions. I'm not sure if this is the concept here.
I worked in many projects where the content editors don't have this kind of permissions, so I keep strict with the task description. Anyway, IMHO a less restricted role will lead to better user experience for new adopters, so I'm +1 to go with it.
About the core's modules permissions I'm also in for it, especially for modules like media and media library.
Ping @ckrina for her's feedback about these topics.
Comment #11
quironFor core modules I would suggest adding to the content editor all permissions related to content management and content editing tools, also accessing help features. This should look like:
Actually, most of these permissions are not added to any role because are not for anonymous neither authenticated, and are in the administrator by default. As long the 'Content Editor' is only in the standard installation profile this should be handled in the profile itself, not in the modules like other permissions added now by the core modules.
Comment #12
quironHere the patch with the permissions defined in #11
Comment #13
quironTriggering the tests.
Comment #15
quironFixing the tests.
Comment #16
ckrinaIt'd be great if this could be reviewed by Product Managers and Release Managers.
Comment #17
catchThis is just new configuration so it doesn't really need release manager review, but it should definitely be reviewed by a product manager.
Comment #19
ckrinaThis was discussed in last week's Drupal Usability Meeting - 2020-03-05. Here's the recording: https://www.youtube.com/watch?v=HcGHwi4CVjo
Comment #21
Manav CreditAttribution: Manav as a volunteer commentedI have applied this patch #15 on my local instance but it failed and even I have tried this on simplytest.me it also gives the same result.
Drupal version: 9.1.x-dev
MariaDB version: 10.1.44
PHP 7.3
Here is the error:
Comment #22
mindbet CreditAttribution: mindbet as a volunteer commentedPatch from #15, applied to 9.1-dev
Patch applies locally and tests locally OK; hoping the testbot agrees.
Comment #23
mindbet CreditAttribution: mindbet as a volunteer commentedSetting to 'Needs review'
Comment #24
mindbet CreditAttribution: mindbet as a volunteer commentedFixed sloppy patch construction.
Comment #26
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #24 in 9.2.x.The patch works fine.The new "Content editor" role is created.
Screenshots:
All permissions mentioned in the proposed resolution are given in patch except for the "Use the Basic HTML text format". Is that okay?
Comment #27
Wim LeersPer #17:
So … still just blocked on product manager review.
Comment #28
webchickThere is an old, crusty part of my brain that seems to fuzzily recall that we talked about this in UX meeting repeatedly for awhile a long time ago, and were waiting on someone... maybe @worldlimemine? ... to bring up a counter-proposal to this, because of concerns about sites that use workflow-based content creation vs. those that don't and disagreements about which of those scenarios is more common among Drupal's user base.
However, up here in my 2021 brain, I'm feeling pretty "the perfect is the enemy of the good" about this, and think that the proposal as it stands (assuming the issue summary is correct) solves a lot more problems than it creates, and we can always tweak it in the future (esp. before release) if we feel something catastrophic will happen if we roll it out to folks.
Consider product management sign-off signed off.
Comment #29
ckrinaThis is great!! 🎉
To be sure the path is as clear as we can for this I've created #3201550: Add new “Content Manager” role to Standard Profile to continue the discussions about which role should get specific permissions like Content Moderation: Editorial workflow: Use Publish transition., the more controversial one during the last UX call where this was discussed (https://www.youtube.com/watch?v=HcGHwi4CVjo). From that call we agreed on getting the minimum permissions for the Content Editor role, but still enough ones to use Content Moderation without having the new role yet. The moment we create a second role to moderate/manage content, a few of these permissions should be moved over there.
Comment #30
ressa CreditAttribution: ressa at Ardea commentedThis is great improvement, thanks for working on it! I tested the latest patch (#24) which applies with latest Drupal 9.2 dev-relase.
It looks great, and works well: I created a new user with the role "Content editor", and the user can create content, edit it, change revisions, add a revision log message, edit the URL alias, add an image, delete the node, and so on.
So everything looks and works as expected, except the user can't unpublish own content ... As a content editor, I would probably expect to be able to publish or unpublish at least own content, by default.
Comment #31
ckrinaWe've been discussing this on today's weekly UX call and from the current patch we would remove this permissions that already belong to any Authenticated user:
And for the rest of the list, the following permissions are OK for now on the Content Editor role, but should be moved into the Content manager role on #3201550: Add new “Content Manager” role to Standard Profile:
Comment #32
ressa CreditAttribution: ressa at Ardea commentedI updated the patch, removing the two permissions as outlined in #31.
Comment #33
ckrinaTagging for DrupalCon NA.
Comment #34
erin.rasmussen CreditAttribution: erin.rasmussen as a volunteer commentedI tested the patch in comment #32 with Drupal core 9.1.6 on SimplyTestMe.
I created a content editor user and created articles and pages, and edited them, and deleted them, and everything worked as expected.
A couple of things were missing.
Comment #35
ckrinaWe discussed @erin.rasmussen 's feedback on the UX call today and agreed on:
- Agreed on that you should be able to edit your own comments.
- Since we are already planning on creating the Editor Manager, if you have the need to publish&unpublish and you are the only user editing a site you should have the 2 roles (Content Editor and Content Manager). Otherwise the Content Manager is the one that should actually have this permissions. Thanks Thomas @worldlinemine for the suggestions!
Comment #36
ckrinaAnd forgot to change the status to Needs work.
Comment #38
ressa CreditAttribution: ressa at Ardea commentedHere is an updated patch for Drupal 9.3.x, which adds the
edit own comments
permission.It's fine to let the Editor Manager role take care of publish&unpublish and deleting revisions, but the Content Editor rule can delete own nodes ... So if the point of not allowing publish&unpublish is for tighter control (see #31), then it seems like these delete own content permissions should also be moved from Content Editor and to the Editor Manager role:
9.3.x is not yet available in the Version drop-down ... setting it to 9.2.x for now.
Comment #39
ressa CreditAttribution: ressa at Ardea commentedChanging Status.
Comment #41
ressa CreditAttribution: ressa at Ardea commentedIt seems like the order of config elements makes a difference, trying again.
Comment #43
ressa CreditAttribution: ressa at Ardea commented9.3.x was just made available, so updating Version.
EDIT: I have tried to trigger the #41 patch with Release 9.3.x via "Add test / retest" > "Custom parameters" but it isn't yet available ... perhaps we just need to wait a while, before it is available?
Comment #44
ressa CreditAttribution: ressa at Ardea commentedThe first fail was probably caused by #3211992: TestSiteApplicationTest::testInstallWithNonExistingFile() fails when another test creates database tables during the test run.
The patch in #41 now pass the test.
Comment #45
benjifisherI reviewed the patch in #41, and it all looks good. The (minor) updates to the migration tests make sense. I compared the added role to the
authenticated
role: the format looks good, and there is no duplication of permissions between the two roles.The only other change in the patch is an adjustment to the weight of the
administrator
role. I am only reviewing, not testing, and I will let the tester decide whether that makes sense.I happen to be following #2571235: [regression] Roles should depend on objects that are building the granted permissions, so I am adding it as a related issue. Whichever issue is fixed second will have to add dependencies to the new role.
1/2 of RTBC.
Comment #47
benjifisherI created a merge request (MR) using the patch in #41 so that Tugboat will build a preview site for testing.
Comment #48
worldlinemine CreditAttribution: worldlinemine commentedI have tested this patch on Tugboat and it behaved as expected. After careful consideration my initial concerns regarding specific permissions set in the role aren't worth repeating in this comment. The role now specifically addresses content generation and own content modification as expected.
Comment #49
benjifisherRTBC based on #45 and #48.
Comment #50
ressa CreditAttribution: ressa at Ardea commented@ckrina: Shouldn't we first decide if the Content Editor role should be allowed to delete own nodes, but not publish&unpublish before proceeding? From #38:
Deleting a node is a bigger change than unpublishing, right?
Comment #51
ckrinaThanks @ressa for all this work!
We've discussed your comment during today's UX weekly meeting and agreed on sticking with the plan commented on #31. Basically, we all agreed with you but until we have the Content Manager role someone needs to have this permissions.
Since this issue is to add only the Content Editor for now, we'll keep the publish&unpublish&delete permissions on this patch for now, and we'll move them to the Content Manager on the follow-up issue to create the Content Manager role: #3201550: Add new “Content Manager” role to Standard Profile. And this way we can get this in and start working on the rest of the related issues.
So, moving it back to RTBTC! 🤞
Comment #52
ressa CreditAttribution: ressa at Ardea commentedThanks @ckrina! That sounds like the right approach for now, let's carry on in #3201550: Add new “Content Manager” role to Standard Profile.
Though, if a Content Editor can create, but is not permitted to delete or unpublish content, a node could get created by an Content Editor by accident, or the Content Editor regret creating it. But since the Content Editor cannot remove it, the node will be left out in the open. One solution would be to allow a Content Editor to create a node, but only in an unpublished state. But that would probably require a whole new permission, like 'create own unpublished article content', which seems over the top ...
It's a bit theoretical, and not a big deal, but I am just putting it out there.
Comment #53
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIsn't that what
'use editorial transition create_new_draft'
is? That only works if you install the Content Moderation module though, which isn't currently part of Standard Profile.Comment #54
benjifisherOnce #2571235: [regression] Roles should depend on objects that are building the granted permissions is fixed, we will either have to add Content Moderation to the Standard profile or remove those permissions from the new roles.
Instead of giving the Content Editor permission to delete, you can add another unpublished workflow state, and give the Content Editor permission to move things (anything? unpublished things?) into that state. Not as part of this issue, and maybe never in the Standard profile, but that is an option for the site builder using Content Moderation. Maybe call the state "Trash" and only Content Manager can "empty the trash".
Comment #55
catchTagging for product manager review since this affects the standard profile.
Comment #56
webchickPM sign-off was already given in #28. :)
However, reviewing this anyway...
My first question was "What's up with the failing test?" but this was explained in #44 and was a one-off problem.
My second question was "What's up with the 'Content Editor' vs. 'Content Moderator' decision?" and looks like from #51 that this distinction has been drawn off into a follow-up issue at #3201550: Add new “Content Manager” role to Standard Profile. I think this makes sense, since it requires us to have other discussions, like whether to include Content Moderation on by default in Standard profile alongside it.
My last question, which I can't seem to find a rationale for... why are we repurposing role ID #2 to mean something different vs. appending a new one to the list? As we see here, this requires changes in tests, which implies it could break code in other places, and if similar changes aren't made, it could possibly in a worst-case scenario unexpectedly grant access to things meant for the "Admin" role to a new "Content Editor" role. :\
Especially given the open conversation happening at #3201550: Add new “Content Manager” role to Standard Profile, which implies we might have to move that role ID again to #4 instead of #3... why not just keep admin as #2 and append additional role IDs after it?
Once that question is answered, and/or the patch is adjusted, this is good to go from my POV! Thanks so much to everyone who worked on this!!
Comment #57
webchickComment #58
webchickOops, haha! I blame my "sequel injection" side effects. ;)
That is not renumbering the ID, that is adjusting the count of roles, which totally makes sense given there are now more of them. :P Great!
Committed and pushed to 9.3.x. Woohoo!! :D I know the UX team has wanted this for a GOOD while; thanks to all who pushed on it to make it happen!
Not sure this will actually make sense as a 9.3.0 release highlight, but can't hurt to flag it.
Comment #60
AaronMcHaleWooh 🎉 thanks @webchick 😄
Comment #61
Wim LeersShouldn't this have a change record? It's a pretty noteworthy change — AFAIK quite a few sites are still built atop the Standard install profile :)
Comment #62
webchickGood point!
Comment #63
alexpottBefore we add a change record we need to land #3221258: Fix content editor role to only have permissions that exist - unfortunately this issue introduces a major bug because it is adding a role which has permissions which are provided by modules that are not installed. Since so many people are happy that this has landed I'd rather plod on forward than asking for a revert but we have some decisions to make in #3221259: Add permissions for optional modules to content editor role as they become enabled to decide how to add the non-existent permissions to the role. Roles configuring permissions that do not exist is a security issue because if you go to admin/people/permissions you can't see them and magically when the module is installed a role gets a permission and there's no information given to the site owner that this has occurred - see #2571235: [regression] Roles should depend on objects that are building the granted permissions for a bigger discussion of this issue.
Comment #65
benjifisher