Add new “Content Editor” role to Standard Profile

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

sorry, the previous one was wrongly formatted.

Umami 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)

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

For 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:

• Aggregator: View news feeds
• Book: Add content and child pages to books and manage their hierarchies.
• Book: Create new books
• Book: View printer-friendly books
• Comment: Post comments
• Comment: Skip comment approval
• Comment: View comments
• Contact: Use the site-wide contact form
• Content Moderation: Editorial workflow: Use Archive transition
• Content Moderation: Editorial workflow: Use Create New Draft transition
• Content Moderation: Editorial workflow: Use Publish transition.
• Content Moderation: Editorial workflow: Use Restore transition.
• Content Moderation: Editorial workflow: Use Restore to Draft transition.
• Content Moderation: View any unpublished content
• Content Moderation: View the latest version
• Content Translation: Create translations
• Content Translation: Delete translations
• Content Translation: Edit translations
• Contextual Links: Use contextual links
• File: Access the Files overview page
• Filter: Use the Basic HTML text format
• Media: Access media overview
• Media: Create media
• Media: Delete own media
• Media: Update own media
• Media: View all media revisions
• Media: View own unpublished media
• Node: Article: Create new content
• Node: Basic page: Create new content
• Node: Book: Create new content
• Node: Article: Delete own content
• Node: Basic page: Delete own content
• Node: Book: Delete own content
• Node: Article: Delete revisions
• Node: Basic page: Delete revisions
• Node: Book: Delete revisions
• Node: Article: Edit own content
• Node: Basic page: Edit own content
• Node: Book: Edit own content
• Node: Access the Content overview page
• Node: Revert all revisions
• Node: View all revisions
• Node: View published content
• Node: View own unpublished content
• Path: Administer URL aliases
• Path: Create and edit URL aliases
• Quick Edit: Access in-place editing
• Search: Use search
• Shortcut: Use shortcuts
• Statistics: View content hits
• System: Use the administration pages and help
• System: View the administration theme
• Taxonomy: Tags: Create terms
• Taxonomy: Tags: Edit terms
• Toolbar: Use the toolbar
• Tour: Access tours
• User: Change own username
• User: View user information

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.

Here the patch with the permissions defined in #11

Triggering the tests.

Fixing the tests.

It'd be great if this could be reviewed by Product Managers and Release Managers.

This is just new configuration so it doesn't really need release manager review, but it should definitely be reviewed by a product manager.

This was discussed in last week's Drupal Usability Meeting - 2020-03-05. Here's the recording: https://www.youtube.com/watch?v=HcGHwi4CVjo

I 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:

git apply -v 3059984-role-editor-standard-15.patch
Checking patch core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php...
error: while searching for:
      'search_page' => 2,
      'shortcut' => 2,
      'shortcut_set' => 1,
      'action' => 25,
      'menu' => 8,
      'taxonomy_term' => 15,
      'taxonomy_vocabulary' => 7,
      'tour' => 5,
      'user' => 7,
      'user_role' => 6,
      'menu_link_content' => 10,
      'view' => 16,
      'date_format' => 11,

error: patch failed: core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php:84
error: core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php: patch does not apply
Checking patch core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php...
error: while searching for:
      'search_page' => 2,
      'shortcut' => 6,
      'shortcut_set' => 2,
      'action' => 19,
      'menu' => 6,
      'taxonomy_term' => 24,
      'taxonomy_vocabulary' => 7,
      'tour' => 5,
      'user' => 4,
      'user_role' => 3,
      'menu_link_content' => 12,
      'view' => 16,
      'date_format' => 11,

error: patch failed: core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php:86
error: core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php: patch does not apply
Checking patch core/profiles/standard/config/install/user.role.administrator.yml...
Checking patch core/profiles/standard/config/install/user.role.content_editor.yml...

Patch from #15, applied to 9.1-dev

Patch applies locally and tests locally OK; hoping the testbot agrees.

Setting to 'Needs review'

Fixed sloppy patch construction.

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

Per #17:

This is just new configuration so it doesn't really need release manager review, but it should definitely be reviewed by a product manager.

So … still just blocked on product manager review.

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

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

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

We'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:

'access user profiles'
'change own username'

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:

'delete article revisions' (will go to manager later) 
'delete book revisions'(will go to manager later)
'delete content translations'(will go to manager later)
'delete media'(will go to manager later)
'delete page revisions'(will go to manager later)
'revert all revisions'(will go to manager later) 
'use editorial transition archive'(will go to manager later) 
'use editorial transition archived_published'(will go to manager later) 
'use editorial transition publish'(will go to manager later) 

I updated the patch, removing the two permissions as outlined in #31.

Tagging for DrupalCon NA.

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

• I was unable to unpublish my own content
• I was unable to edit my own comment, which sounds small, but as a content editor in a CMS, that's something I expect to be able to do out of the box.

We 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!

And forgot to change the status to Needs work.

Here 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:

'delete own article content'
'delete own book content'
'delete own page content'

9.3.x is not yet available in the Version drop-down ... setting it to 9.2.x for now.

Changing Status.

It seems like the order of config elements makes a difference, trying again.

9.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?

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

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

I created a merge request (MR) using the patch in #41 so that Tugboat will build a preview site for testing.

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

RTBC based on #45 and #48.

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

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:

'delete own article content'
'delete own book content'
'delete own page content'

Deleting a node is a bigger change than unpublishing, right?

Thanks @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! 🤞

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

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

Isn'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.

Once #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".

Tagging for product manager review since this affects the standard profile.

PM 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!!

Oops, 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.

Wooh 🎉 thanks @webchick 😄

Shouldn'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 :)

Good point!

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