Problem/Motivation

The work being done in #731724: Convert comment settings into a field to make them work with CMI and non-node entities will result in that comments can be used on any entity just by creating a comment field in the same way as adding a text field. It will also be possible to tailor each comment form used with other fields, making them perfectly adapted for each entity type used on.

But, there is one very big bottleneck that will severely block the usefulness of this new flexibility. That is that permissions are still globally set for all comment forms throughout the same site. This means that it isn't possible to:

  • Allow anonymous commenting on blog posts
  • Only allow comments for logged in users on articles
  • That comments to product must be approved before being visible

To take it further. A site might even need to have different commenting permissions for two different blog content types on the same site.

This permission control isn't just limited to the entity type itself. It should also be possible to override on individual entity items too. For example:

  • A blog post offering a giveaway for the right answer could then set the comments to that post to be approved before publishing.
  • The writer want to disable comments to a post about a sensitive topic.

This kind of detailed configuration simply isn't possible with global permissions.

Implementing the proposed solution below will not only make the comment module a lot more flexible, it will also give site-owners much more control over their site and who can do what and where.

Proposed resolution

First:
comment_permissions.png

  • Remove the global comment viewing/posting permissions completely.
  • Split the "Administer comments and comment settings " into two:
    • Global comment administration
    • Configure comment settings

Then move all the comment viewing/posting permissions to the field configuration. Permissions needed for:

  • View
  • Post
  • Reply (new)
  • Skip approval
  • Approve comments (new)
  • Edit own
  • Edit all (new)

Anonymous, authenticated roles will be statically available. All other roles can be manually added to the field. If not added, the permissions for the authenticated is used unless the two new global comment permissions kicks in.

It should also be possible to set the default permissions in the global comment settings. Including adding more roles that then automatically are used for new comment fields.

There will also be a new setting for allowing overriding certain settings on individual items. Such as described in the examples above.

When enabled options are needed for:

  • What options can be overridden
    • Turn on/off skip approval
    • Disable/Enable commenting
    • Close/open commenting
  • Who can do it
    • Item author (overrides the need of the "Approve comments" perm)
    • Roles with the "Approve comments" perm

When for example disabling commenting or turning on approval, a custom, overrideable, message should be displayed instead. For example "Comments to this post has been disabled due to ....".

Here too, the global comment admin permission overrides all custom settings.

Remaining tasks

  • Create mockups with proposed UI changes
  • Implement the changes

User interface changes

TBD

API changes

TBD

#10426: Make comment permission more granular. Assigned to: killes@www.drop.org

CommentFileSizeAuthor
#98 move_global_comment_permissions_to_comment_type_level-1903138-98.patch28.51 KBhannakras
#97 move_global_comment_permissions_to_comment_type_level-15216974-97.patch28.66 KBovanes
#95 interdiff_94-95.txt5.46 KBvsujeetkumar
#95 1903138-95.patch27.98 KBvsujeetkumar
#94 1903138-94.patch28.42 KBvsujeetkumar
#92 comment_bundle_permissions-1903138-83-9.5.patch28.29 KBShura80
#90 comment_bundle_permissions-1903138-83-9.5.patch28.29 KBShura80
#83 comment_bundle_permissions-Move_global_comment_permissions_to_comment-type_level-1903138-83-9.x.patch28.72 KBDuneBL
#80 comment_bundle_permissions-1903138-80.patch27.73 KBvsujeetkumar
#73 comment_bundle_permissions-1903138-73.patch27.56 KByivanov
#72 comment_bundle_permissions-1903138-72.patch28.72 KByivanov
#71 comment_bundle_permissions-1903138-70.patch28.11 KBhgeshev
#66 comment_bundle_permissions-1903138-66.patch30.67 KBa_grizzli
#64 comment_bundle_permissions-1903138-64.patch29.22 KBa_grizzli
#58 comment_bundle_permissions-1903138-58.patch29.27 KBjefuri
#49 comment_bundle_permissions-1903138-48.patch30.88 KBjefuri
#47 comment_bundle_permissions-1903138-47.patch28.87 KBjefuri
#46 comment_bundle_permissions-1903138-46.patch28.84 KBjefuri
#44 comment_bundle_permissions-1903138-45.patch30.21 KBjefuri
comment_permissions.png5.3 KBtsvenson
#18 comment-permissions.wip_.patch13.1 KBlarowlan
#28 comment_permissions.patch28.98 KBAntonnavi
#32 comment_bundle_permissions.patch22.22 KBAntonnavi
#35 comment_bundle_permissions_1.patch22.22 KBAntonnavi
#39 interdiff.txt5.45 KBAntonnavi
#39 comment_bundle_permissions_2.patch23.31 KBAntonnavi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

People working on this may be interested in http://drupal.org/node/1903124 and http://drupal.org/node/1903128.

An (apparently) similar discussion to the one reported in the OP happened in #1807776: Support both simple and editorial workflows for translating entities.

andypost’s picture

@plach This awesome change! Is there any suggestions/ideas about UI for that? Following #1807776: Support both simple and editorial workflows for translating entities I found that UI part is planned for contrib to not mess a permissions page.

I we make #731724-291: Convert comment settings into a field to make them work with CMI and non-node entities real so this permissions will be very useful. But I not sure exactly how to apply them to comment-field on per entity/bundle basis

tstoeckler’s picture

Sort of related #366416: Field Permissions UI
I think if that gets in this may be obsolete, but I don't consider that likely at this point.

webchick’s picture

Category: task » feature
Priority: Major » Normal

I don't see anything in the OP that you can do in core right now, so don't understand why this is a major task. It sounds like a feature request to me, and most likely one for D9.

andypost’s picture

Assigned: Unassigned » andypost
Category: feature » task
Status: Active » Postponed
andypost’s picture

@tsvenson once we have agreement on settings for comment form. We can continue here.
Anyway having this global settings currently enough, contrib have field permission and only posting an action that questionable.

I see no reason to introduce this permissions in global scope.

Global
View comments - granularity defined by field
Skip approval - same as now, detailed about anonymous in Form settings

Administer - skip entity access and allows to configure forms.
Approve comments (new) - great idea!

Entity Access
Edit own - entity level
Edit all (new) - entity level
Post comments - global + granularity defined by entity

Reply (new) - useless, It's just a link in formatter (when user allowed to post for the entity type)

andypost’s picture

Here's a new way to extend access for fields introduced in #1883152: Field level access for EntityNG

fubhy’s picture

Why do we need to introduce a new concept for this? If we don't fix generic field level access for D8 we should not build a crippled comment-specific field-level access solution here. Instead, just provide dynamic permissions for each comment field instance in use. If you want/need a different permission model for commenting on nodes vs. commenting on users just make the two two separate comment setting fields. Done.

tsvenson’s picture

@fubhy: Say what? As things are now both those comment settings fields with be governed by the global comment permissions as described in the OP.

fubhy’s picture

From the issue summary:

Then move all the comment viewing/posting permissions to the field configuration. Permissions needed for:

I disagree with that. We should not split up access permissions to fields and global unless done generically. Let's not do this specifially for comments. If we can solve field level access for D8 generally fine, let's do it, if we can't let's not introduce a crippled subset of that functionality specifically for comment module.

Permissions should be controlled via the global permission configuration page. If that is your intent, both the issue summary and title are not correct.

However, if you actually plan to move permission configuration into the field instance configuration (as the issue summary and title suggest) you are introducing a huge comment-module specific UX wtf.

tsvenson’s picture

If we can solve field level access for D8 generally fine, let's do it, if we can't let's not introduce a crippled subset of that functionality specifically for comment module.

I would like to turn that around and say that - As long as we have global permissions for things, such as fields, we are severely crippling the usefulness and flexibility of those features. It forces sites to make a global permission compromise instead of being able to tailor it exactly to how they need it for each individual feature on the site.

This crippling is painfully exposed in this case.

Also see my Drupal Limitations and Bottlenecks and Don’t Make Drupal the New ‘Microsoft Office’! posts that goes into much more detail about this.

fubhy’s picture

I would like to turn that around and say that - As long as we have global permissions for things, such as fields, we are severely crippling the usefulness and flexibility of those features. It forces sites to make a global permission compromise instead of being able to tailor it exactly to how they need it for each individual feature on the site.

I am not saying we should not allow per-comment-field-instance permissions. But we should NOT move them into the field settings. We should keep them on the global permission page. Otherwise people will have serious issues trying to find out where they can set permissions for what. Let's keep it in one spot. That's all.

tsvenson’s picture

How about making them available in both places?

From a UX perspective it would make a massive improvement, and avoid pogo-sticking, if I would be able to get a full overview of both the configuration and the permissions at field level too.

Then on the big permissions page the same are shown, grouped per field and also with a convenient link to the field settings page too.

andypost’s picture

There's global permissions anyway - for comment entity (for 80% of sites enough)
I think better to implement permissions per-comment form as "bundle".

If someone will need a per-field settings - it's ok with contrib module a-la field_permissions for comment-field.
Permissions on field-instance level seems absurd but could be implemented with custom module.
Another case is per-entity permissions or more granular for example the workflow module when commenting should be enabled in some states or roles.

andypost’s picture

The related discussion about access to field/field-item #2018707: Do we still need to support hook_field_prepare_translation() ?

andypost’s picture

Status: Postponed » Active
larowlan’s picture

Assigned: andypost » larowlan

grabbing this one, not sure if you're actively working on it, if so, please assign back and I'll hold-off.

larowlan’s picture

Status: Active » Needs review
FileSize
13.1 KB

here's a wip, still lots of calls to user_access|hasPermission(access comments|post comments|skip comment approval|edit own comments) to update to the new method as follows:
before

$account->hasPermission('access comments');
// Legacy format.
user_access('access comments');

after

$manager = \Drupal::service('comment.manager');
$field = $manager->loadField($field_name, $entity_type);
$manager->checkPermission('access comments', $field, $account);

But before that thoughts on approach.
Offline for a few weeks so feel free to keep kicking this along.

The last submitted patch, comment-permissions.wip_.patch, failed testing.

andypost’s picture

Issue summary: View changes

related

larowlan’s picture

I'm wondering if we might be better to refactor comment permission to make one set of permissions per comment bundle regardless.
For sites upgrading from D7 with stacks of different node-types (which map to bundles on upgrade) this would mean a lot of permissions, but no more than node spits out. And if they're not using said bundle, they can remove the field.
Thoughts?

larowlan’s picture

Title: Move global comment permissions to field level » Move global comment permissions to comment-type level
Status: Needs work » Postponed
larowlan’s picture

larowlan’s picture

Working on this, but deferring most decisions to the CommentAccessController to allow it to be swapped out instead of hard-coded.

tsvenson’s picture

Awesome news @larowlan. I am in the process of getting back to Drupal after a longer, much needed, break. Will take a little time to set everything up, but I plan to devote time to help testing D8 stuff.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Antonnavi’s picture

Hi, all!
I ported patch from comment #18 to use it with 8.2.x version.
Currently it is alpha version (seems to stable). I need help with:

  • Review of patch code
  • Suggestion is used way correct (or should be used another way)
    • Current patch add Comment field based permissions check:
      On adding new comment field You can check checkbox "Create custom permissions for this comment field" and for this field appears personal permissions (access/post/skip approval/edit own) on permissions list page.
  • Find solution for core/modules/comment/src/CommentAccessControlHandler.php checkCreateAccess() method.
    • Currently only in this place not added field based permission check because not found way how to obtain related field config.
Antonnavi’s picture

Status: Active » Needs review
andypost’s picture

The center of authority is CommentItem so code should work on top of field permission.
This perfectly fits into field access model and will allow control UI at widget|formatter level

Comment #14 still open - do we really wanna build access around comment types - read that like "read|answer to threaded comments" or "view only plain comments"?

About UI I'd suggest to add tab to field edit page to manage perwissions

+++ b/core/modules/comment/src/CommentFieldItemList.php
@@ -50,14 +50,16 @@ public function access($operation = 'view', AccountInterface $account = NULL, $r
+      $manager = \Drupal::service('comment.manager');
+      $field_config = $this->getFieldDefinition();
...
-      $result = AccessResult::allowedIfHasPermission($account ?: \Drupal::currentUser(), 'access comments')
-        ->orIf(AccessResult::allowedIfHasPermission($account ?: \Drupal::currentUser(), 'post comments'));
+      $result = AccessResult::allowedIfHasPermission($account ?: \Drupal::currentUser(), $manager->buildFieldPermission('access comments', $field_config))
+        ->orIf(AccessResult::allowedIfHasPermission($account ?: \Drupal::currentUser(), $manager->buildFieldPermission('post comments', $field_config)));

This all over changes...
If we gonna go this way better to add this methods *check|build*FieldPerssions() to field itself

Status: Needs review » Needs work

The last submitted patch, 28: comment_permissions.patch, failed testing.

Antonnavi’s picture

Status: Needs work » Needs review
FileSize
22.22 KB

Thank You for review, andypost!
Agree with You. Patch was re-done to use comment bundles for access restriction.

Status: Needs review » Needs work

The last submitted patch, 32: comment_bundle_permissions.patch, failed testing.

andypost’s picture

Version: 8.2.x-dev » 8.3.x-dev
Issue tags: +Needs product manager review

Overall this looks great! @Anton Thanx a lot!

300 failed tests now shows we need proper defaults and upgrade path for existing sites.

@larowlan I'm sure that serious change and needs your opinion)

+++ b/core/modules/comment/comment.module
@@ -420,51 +420,70 @@ function _comment_entity_uses_integer_id($entity_type_id) {
+function comment_node_update_index_check_access(EntityInterface $node, $field_name) {
+  $access = &drupal_static(__FUNCTION__, array());

I'd better convert that to private function (prefixed "_")
otoh static cache needs to be refactored to some class

Antonnavi’s picture

comment_node_update_index_check_access() function name changed to _comment_node_update_index_check_access(), patch was updated.

Antonnavi’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 35: comment_bundle_permissions_1.patch, failed testing.

larowlan’s picture

Assigned: larowlan » Unassigned

Thanks for moving this along, this is a good feature.

  1. +++ b/core/modules/comment/comment.module
    @@ -420,51 +420,70 @@ function _comment_entity_uses_integer_id($entity_type_id) {
    +function _comment_node_update_index_check_access(EntityInterface $node, $field_name) {
    

    If we're going to extract this, let's move it to a service that can be swapped and unit tested

  2. +++ b/core/modules/comment/src/Controller/CommentController.php
    @@ -277,7 +277,8 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
    +    $access = AccessResult::allowedIfHasPermission($account, 'post comments ' . $comment_type);
    
    @@ -286,7 +287,7 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
    +      $access = $access->andIf(AccessResult::allowedIfHasPermission($account, 'access comments ' . $comment_type));
    

    It feels like we could almost use a helper class here too, just to centralize all the logic, in case we want to tweak it or change it again?

Antonnavi’s picture

Thank You for review larowlan!

1. _comment_node_update_index_check_access() was moved to CommentManager service.

2. Do You mean add new method to CommentManager service that will obtain 2 params:

  • $permission (like "access comments")
  • $comment_type (comment type id)

Will concatenate it and return as result: $permission . ' ' . $comment_type.
This method will be used instead of code like 'access comments ' . $comment_type right?

Antonnavi’s picture

Status: Needs work » Needs review

The last submitted patch, 39: comment_bundle_permissions_2.patch, failed testing.

andypost’s picture

Looks good step forward!

It's really interesting where to put this code knowing that comment manager should get split.
Maybe better to introduce new service?!

#38.2 is exactly about permission so maybe better to add new method to \Drupal\comment\CommentAccessControlHandler

+++ b/core/modules/comment/src/CommentManager.php
@@ -67,6 +67,13 @@
+  protected $typesIndexPermissions = array();

this brings state to service
so you need to use DependencySerializationTrait

jefuri’s picture

@andypost

Reply (new) - useless, It's just a link in formatter (when user allowed to post for the entity type)

I do not agree. We have a use case where we only want non hierarchy comments. So a reply on an existing comment is not wanted in our use case. But some users are allowed to post. Adding the reply permission as apposed to using the post permission in generals would solve this.

Also I see a lot of direct validation through the hasPermission method in the current user in parts like:
- CommentLinkBuilder
or only use the entity access layer:
- CommentLazyBuilder

Why wouldn't we validate access on the URL object of the links we are trying to render? In theory you would ignore any route subscriber access check alterations on the routes of comments when showing a link. When clicking they would get an access denied.
Would we not prefer that the same access validation is done on the rendering of an link as when we go to the url of the link?

jefuri’s picture

Based on my previous comment I added the permission to differentiate between a post and a reply.
I also altered the lazybuilder and linkbuilder classes to use the url object to validate access where we could.

Also changed the reply form access callback to reflect this. Please note that I also see the post and reply as a separate access validation which are not dependent on each other. Meaning that one user with a role could post a message, but might not be allowed to reply and vice versa.

public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NULL) {
    // Check if entity and field exists.
    $fields = $this->commentManager->getFields($entity->getEntityTypeId());
    if (empty($fields[$field_name])) {
      throw new NotFoundHttpException();
    }

    $account = $this->currentUser();

    // Check if the user has the proper permissions.
    $comment_type = $entity->{$field_name}->getSetting('comment_type');

    if (!$pid) {
      $access = AccessResult::allowedIfHasPermission($account, 'post comments ' . $comment_type);

      $status = $entity->{$field_name}->status;
      $access = $access->andIf(AccessResult::allowedIf($status == CommentItemInterface::OPEN)
        ->addCacheableDependency($entity));
    }
    // $pid indicates that this is a reply to a comment.
    else {
      // Check if the user has the proper permissions.
      $access = AccessResult::allowedIfHasPermission($account, 'reply comments ' . $comment_type);

      /// Load the parent comment.
      $comment = $this->entityManager()->getStorage('comment')->load($pid);
      // Check if the parent comment is published and belongs to the entity.
      $access = $access->andIf(AccessResult::allowedIf($comment && $comment->isPublished() && $comment->getCommentedEntityId() == $entity->id()));
      if ($comment) {
        $access->addCacheableDependency($comment);
      }
    }
    return $access;
  }

Status: Needs review » Needs work

The last submitted patch, 44: comment_bundle_permissions-1903138-45.patch, failed testing.

jefuri’s picture

Sorry, uploaded a PHPStorm patch. Fixed it!

jefuri’s picture

Patch from #46 has an bug or something. Recreated the patch.

Status: Needs review » Needs work

The last submitted patch, 47: comment_bundle_permissions-1903138-47.patch, failed testing.

jefuri’s picture

Patch from #47 was missing the CommentPermissions.php.

jefuri’s picture

Status: Needs work » Needs review

The last submitted patch, 46: comment_bundle_permissions-1903138-46.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 49: comment_bundle_permissions-1903138-48.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jonathanshaw’s picture

We have a use case where we only want non hierarchy comments. So a reply on an existing comment is not wanted in our use case. But some users are allowed to post. Adding the reply permission as apposed to using the post permission in generals would solve this.

Agreed, flexibility in permissions here would be good. I can see a use case where I would want sort-of-admin users to be able to reply to comments, but other users could only reply to their own comments (or reply to replies on their own comments), but not reply to other people's comments. It would enable a public conversation between a visitor and a website.

hctom’s picture

I just posted another issue which might be interesting for this issue: #2879087: Use comment access handler instead of hardcoding permissions . As you can see, the entity access checks should be done via the access control handler and not with direct permission checks. This would also have a massive impact on this issue.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jefuri’s picture

Here a reroll for 8.4 and 8.5

drummondf’s picture

I have not read all of the above, but had a thought:

If Comments are now wrapped into fields, couldn't Field Permissions be extended to work with the Comment field? My thought would be that I could install Field Permissions, and that would just add the appropriate permissions controls to the field, and thereby the commenting inherently.

Anas_maw’s picture

The patch in #58 works fine for me.

Anas_maw’s picture

Status: Needs work » Needs review

@drummondf
I think field permission can't do the same feature, because the last patch give the option to skip the comment approval for each comment type also reply on same comment.
The patch in #58 is a great solution

Status: Needs review » Needs work

The last submitted patch, 58: comment_bundle_permissions-1903138-58.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

a_grizzli’s picture

Patch #58 works visually fine! Tested on Drupal 8.5.6. Just added some CodeSniffer suggestions from #62 in new patch.

I think, this patch is a good solution to granulate permissions on comment-type level. However, should be adapted to results of #2879087: Use comment access handler instead of hardcoding permissions . Using AccessControlHandler for comment-type level will bring a nice improvement to this issue also.

a_grizzli’s picture

@jefuri: Patch from #58 (so now also from #64) seems to miss removing some code in function comment_node_update_index. Seems to be not used code, since it has moved to CommentManager::isIndexingAvailable:

  $index_comments = &drupal_static(__FUNCTION__);

  if ($index_comments === NULL) {
    // Do not index in the following three cases:
    // 1. 'Authenticated user' can search content but can't access comments.
    // 2. 'Anonymous user' can search content but can't access comments.
    // 3. Any role can search content but can't access comments and access
    // comments is not granted by the 'authenticated user' role. In this case
    // all users might have both permissions from various roles but it is also
    // possible to set up a user to have only search content and so a user
    // edit could change the security situation so it is not safe to index the
    // comments.
    $index_comments = TRUE;
    $roles = \Drupal::entityManager()->getStorage('user_role')->loadMultiple();
    $authenticated_can_access = $roles[RoleInterface::AUTHENTICATED_ID]->hasPermission('access comments');
    foreach ($roles as $rid => $role) {
      if ($role->hasPermission('search content') && !$role->hasPermission('access comments')) {
        if ($rid == RoleInterface::AUTHENTICATED_ID || $rid == RoleInterface::ANONYMOUS_ID || !$authenticated_can_access) {
          $index_comments = FALSE;
          break;
        }
      }
    }
  }
a_grizzli’s picture

Remove of unused code from #65 and adapt for 8.6.x and 8.7.x (trivial change in CommentAccessControlHandler::checkAccess needed)

a_grizzli’s picture

Status: Needs work » Postponed

Patch from #66 (and before) removes all permissions' checks:

  • access comments
  • post comments
  • ...

...in favour of dedicated comment-type sensitive permissions.

To keep backward compatibility (and pass tests), we should keep old permissions, too. These would stay in sense of:

  • access any comments
  • post any comments
  • ...

Furthermore, postpone until results of #2879087: Use comment access handler instead of hardcoding permissions

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

hgeshev’s picture

I had problems applying the patch for 8.7.x, because of the following errors:

patching file modules/comment/src/CommentAccessControlHandler.php

Hunk #5 FAILED at 126.

patching file modules/comment/src/CommentLazyBuilders.php

Hunk #1 FAILED at 166.

patching file modules/comment/src/Controller/CommentController.php

Hunk #1 FAILED at 119.

Hunk #2 FAILED at 221.

Hunk #3 FAILED at 231.

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2018-09-02/comment_bundle_permission...

[Exception]
Cannot apply patch Move global comment permissions to comment-type level (https://www.drupal.org/files/issues/2018-09-02/comment_bundle_permissio
ns-1903138-66.patch)!

I fixed the errors in the following patch, tested it and it is applying and working:

https://www.drupal.org/files/issues/2019-12-16/comment_bundle_permission...

hgeshev’s picture

yivanov’s picture

Hello, the patch was not applying for 8.8.x, so I rerolled it.

yivanov’s picture

Sorry the previous one was not clean. I am adding the rerolled 8.8.x clean version now.

watson.sm’s picture

#73

Perfect, exactly what I needed. Thx.

*Edit 2020-06-12
I think I've found an edge case.

Situation: A set of users are allowed to post top level comments & replies(Managers). Another set are NOT allowed to post top level comments, however they can post replies(employees).

Permissions:
- Managers
-- (comment type) Post Comments
-- (comment type) Edit Own Comments
-- (comment type) Reply Comments
-- (comment type) Skip Comment Approval
-- (comment type) View Comments
-- (Global) Edit Own Comments
-- (Global) Post Comments
-- (Global) Reply Comments
-- (Global) Skip comment approval
-- (Global) View comments

- Employees
-- (comment type) Post Comments
-- (comment type) Edit Own Comments
-- (comment type) Reply Comments
-- (comment type) Skip Comment Approval
-- (comment type) View Comments
-- (Global) Edit Own Comments
-- (Global) Post Comments
-- (Global) Reply Comments
-- (Global) Skip comment approval
-- (Global) View comments

Since Employees have the permission for (comment type) Reply Comments they are presented with the comment form, however since they are lacking the (comment type) Post Comments permission their comments are being blocked.

Shouldn't their comments be allowed if they have the permission for (comment type) Reply Comments?

Side Question: Are the Global comment permissions still necessary with the more fine-tuned permissions based on type?

Skin’s picture

I`ve just applied #73 patch to Drupal 8.8.x, , I have to test it a bit, but it seems ok. When can we see this option embedded in Drupal's core? I find this patch really useful.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

DuneBL’s picture

Status: Postponed » Needs work

I have installed this patch on 9.1.4 (with few offset) and it is working as expected except that in CommentLazyBuilders.php we must replace twice $entity->urlInfo(xxx) by $entity->toUrl(xxx).
[Sorry, right now, I don't have time to reroll and correct this patch]

I change the status of this patch (no more postponed) because I don't think we need to wait #2879087: Use comment access handler instead of hardcoding permissions .
We don't need to wait because this patch can work out of the box and when #2879087 will land, it will use the new permissions created here.

jonathanshaw’s picture

Issue tags: +Needs reroll

Needs reroll for #78

vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
27.73 KB

Re-roll patch created. And replaced $entity->urlInfo(xxx) by $entity->toUrl(xxx) as mentioned in #78.

Status: Needs review » Needs work

The last submitted patch, 80: comment_bundle_permissions-1903138-80.patch, failed testing. View results

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

DuneBL’s picture

Here is a re-roll for 9.3
Same as #80 expect in core/modules/comment/src/CommentLazyBuilders.php :

      if ($create_url->access()
        && $field_definition->getSetting('default_mode') === CommentManagerInterface::COMMENT_MODE_THREADED) {

What is following the && was not in #80 but was in 9.3... Thus I keep it with the && part as I felt it was good.

jonathanshaw’s picture

#67 suggested postponing this issue on #2879087: Use comment access handler instead of hardcoding permissions . Is that necessary?

#67

To keep backward compatibility (and pass tests), we should keep old permissions, too.

#74

Side Question: Are the Global comment permissions still necessary with the more fine-tuned permissions based on type?

We need to decide on a backwards-compatability and possibly deprecation strategy for the global comment permissions.

#74:

Since Employees have the permission for (comment type) Reply Comments they are presented with the comment form, however since they are lacking the (comment type) Post Comments permission their comments are being blocked.

Sounds like a bug in the patch.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

timohuisman’s picture

The patch from #83 fails against 10.1.x and 11.x.

Shura80’s picture

Hello, I upload a patch that may be applied after the following one in this issue:

https://www.drupal.org/project/drupal/issues/2879087
https://www.drupal.org/files/issues/2022-01-17/2879087-130-9.3.x.patch

Tested on a Drupal 9.5.10 installation.

Shura80’s picture

Shura80’s picture

Reuploaded the patch but it will not pass the test as it has to be applied after this one here:
https://www.drupal.org/files/issues/2022-01-17/2879087-130-9.3.x.patch

Shura80’s picture

vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
28.42 KB

New Patch created for 11.x, Given patch #92 is "Failed to apply", Please have a look.

vsujeetkumar’s picture

Patch created, Fixed CCF issue, Please have a look.

smustgrave’s picture

Status: Needs review » Needs work

Seems to have caused test failures.

ovanes’s picture

Hello there,
I've re-rolled the patch mentioned in comment #83 for the Drupal 10.1.x version.
Thank you.

hannakras’s picture

I re-rolled the patch from #97 for Drupal 10.2. There was one tiny change that broke the patch for me.