[PP-1] Add thread depth configuration to comments

1. +++ b/core/modules/comment/comment.install
   @@ -195,3 +196,29 @@ function comment_update_8400() {
   +/**
   + * Set a value for thread_depth for all existing comments.
   + */
   +function comment_update_8500() {
   

   Since this adds an update hook, the upgrade path needs to be tested. Check out Drupal\Tests\comment\Functional\Update\CommentUpdateTest, as I think a new test method can just be added to that, and verify that the settings are indeed updated.

2. +++ b/core/modules/comment/comment.install
   @@ -195,3 +196,29 @@ function comment_update_8400() {
   +        $field->setSetting('thread_depth', 100);
   
   +++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php
   @@ -112,6 +113,21 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   +      '#max' => 100,
   

   The max thread depth should probably be a constant on the CommentInterface instead of hard-coding the number wherever it's used.

Just a few minor tweaks/questions:

1. +++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php
   @@ -112,6 +113,21 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   +      '#default_value' => ($settings['thread_depth']) ? $settings['thread_depth'] : CommentManagerInterface::COMMENT_MAX_THREAD_DEPTH,
   

   With the update hook, and the default value above, shouldn't this always be set now?

2. +++ b/core/modules/comment/tests/src/Functional/Update/CommentUpdateTest.php
   @@ -71,4 +72,41 @@ public function testPublishedEntityKey() {
   +    $results = $entity_query->execute();
   +
   +    // Check that none of the existing comment fields have the thread_depth setting.
   +    /** @var \Drupal\field\FieldConfigStorage $field_config_storage */
   +    $field_config_storage = \Drupal::entityTypeManager()->getStorage('field_config');
   +    foreach ($results as $id) {
   ...
   +    $results = $entity_query->execute();
   

   I think an assert that these $results are non-empty would be valuable. Otherwise, here, and below, if for whatever reason it didn't load any comments, none of the assertions in the foreach loops would run.

Is there something missing to include the patch in the next release?

#22 works fine, but you should fix the null warning on line 119 in CommentItem.php: '#default_value' => $settings['thread_depth'] ?? 0,

(I added ?? 0)

bump

#22 works fine, but you should fix the null warning on line 119 in CommentItem.php: '#default_value' => $settings['thread_depth'] ?? 0,

This only happens if you don't run the update_N hook since that hook should populate the setting for the existing fields.

The patch has in CommentViewBuilder.php:

-        if ($comment_indent > $current_indent) {
+        if ($comment_indent > $current_indent && $comment_indent <= $thread_depth) {

followed by some additional lines in the else. Wouldn't it be easier to just do:

if ($comment_indent > $thread_depth) {
  $comment_indent = $thread_depth;
}

and leave the existing if/else alone?

1. +++ b/core/modules/comment/comment.install
   @@ -195,3 +196,25 @@ function comment_update_8400() {
   +function comment_update_8500() {
   
   +++ b/core/modules/comment/tests/src/Functional/Update/CommentUpdateTest.php
   @@ -71,4 +72,49 @@ public function testPublishedEntityKey() {
   +   * Tests comment_update_8500().
   ...
   +   * @see comment_update_8500()
   

   The update number needs... update :)

2. +++ b/core/modules/comment/comment.install
   @@ -195,3 +196,25 @@ function comment_update_8400() {
   +  /** @var \Drupal\field\Entity\FieldStorageConfig $field_config_storage */
   +  $field_config_storage = Drupal::entityTypeManager()->getStorage('field_config');
   +  foreach ($results as $id) {
   +    $field = $field_config_storage->load($id);
   +    if ($field) {
   +      $settings = $field->getSettings();
   +      if (empty($settings['thread_depth'])) {
   

   This not the proper way to update a config entity. Please apply https://www.drupal.org/node/2949630.

3. +++ b/core/modules/comment/comment.install
   @@ -195,3 +196,25 @@ function comment_update_8400() {
   +        $field->setSetting('thread_depth', CommentManagerInterface::COMMENT_MAX_THREAD_DEPTH);
   
   +++ b/core/modules/comment/src/CommentManagerInterface.php
   @@ -20,6 +20,11 @@
      /**
   +   * Maximum thread depth for comments.
   +   */
   +  const COMMENT_MAX_THREAD_DEPTH = 100;
   
   +++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php
   @@ -43,6 +43,7 @@ public static function defaultStorageSettings() {
   +      'thread_depth' => 10,
   
   @@ -112,6 +113,21 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   +      '#max' => CommentManagerInterface::COMMENT_MAX_THREAD_DEPTH,
   
   +++ b/core/modules/forum/config/optional/field.field.node.forum.comment_forum.yml
   @@ -25,6 +25,7 @@ default_value:
   +  thread_depth: 10
   
   +++ b/core/profiles/standard/config/install/field.field.node.article.comment.yml
   @@ -25,6 +25,7 @@ default_value:
   +  thread_depth: 10
   

   Not a very big fan of settings a hard limit or suggesting a value. What if existing sites went beyond this value? I would rather allow a "not limited" value, which may be stored as "0". Then we update the existing sites with "0" and also default to "0" on new field instance.

4. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -102,7 +108,7 @@ public function buildComponents(array &$build, array $entities, array $displays,
   -        if ($comment_indent > $current_indent) {
   +        if ($comment_indent > $current_indent && $comment_indent <= $thread_depth) {
   
   @@ -114,6 +120,12 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +          // Adjust indentation relative to the defined thread depth.
   +          if ($comment_indent > $thread_depth) {
   +            $indent_adjustment = $comment_indent - $thread_depth;
   +            $build[$id]['#comment_indent'] -= $indent_adjustment;
   +            $current_indent -= $indent_adjustment;
   +          }
   

   I disagree with the approach. We should not flatten the depth to the max thread depth. Instead we should hack into access control and deny access to reply when the comment has the max depth. Meaning also that the Reply link will disappear.

5. +++ b/core/modules/comment/tests/src/Functional/Update/CommentUpdateTest.php
   @@ -71,4 +72,49 @@ public function testPublishedEntityKey() {
   +    $this->assertNotEmpty($results, t('Unable to grab existing comment field configs or none exist.'));
   ...
   +      $this->assertArrayNotHasKey('thread_depth', $settings, t('thread_depth setting should not exist.'));
   ...
   +    $this->assertNotEmpty($results, t('Unable to grab existing comment field configs or none exist.'));
   ...
   +      $this->assertArrayHasKey('thread_depth', $settings, t('thread_depth setting should exist.'));
   +      $this->assertEquals(CommentManagerInterface::COMMENT_MAX_THREAD_DEPTH, $settings['thread_depth'], t('thread_depth setting should be set to max value'));
   

   You can omit assertion messages. But if you still want to keep them, don't translate them.

Hi,

I am picking this issue and working on the #33 feedback.
Thanks,

Fixing #33.

Unfortunately we cannot switch to an "access based" approach (#33.4) because the access is not properly checked. See #2879087: Use comment access handler instead of hardcoding permissions to find out why. I will block this issue on #2879087: Use comment access handler instead of hardcoding permissions .

Fixed #33.1, 2, 3 and 5. Posting the partial work.

Postponing on #2879087: Use comment access handler instead of hardcoding permissions

The #2879087: Use comment access handler instead of hardcoding permissions blocker depends also on #2886800: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context.

Will work on top of #2879087: Use comment access handler instead of hardcoding permissions to advance with this.

For now just rerolled and provided a patch on top of #2879087: Use comment access handler instead of hardcoding permissions for testing purposes.

Had some more thoughts on #33.4 and I think we can offer a field configuration option. A site builder will be able to chose between allowing replying on the same level as the parent comment or totally disable the reply when the maximum depth is hit. Still needs tests.

Changes:

• Now site builders are able to configure each comment field whether replying to the deepest comment is allowed. If allowed, the reply will be displayed with the same indent as the parent comment. If not allowed, then replies are totally denied.

Remaining tasks:

• Create change notice.
• Update IS.

• Fixed IS
• Added change notice: https://www.drupal.org/node/3181462

Changes:

• The CommentOrphanTest test had a bug that only surfaced with this patch.
• The commented_entity is already in context when replying.
• Fixed coding standards.

To Whom It May Concern: Here's an unofficial version of #44 to be used w/ Drupal 8.9.x.

Ouch, bad merge.

The patch to be reviewed is #44.

Oh! $sandbox should be always passed by reference in update & post-update scripts.

OK, fixed passing $sandbox by reference for 9.2.x. Thinking more on this... this is hard to be cough by the update test if the config being tested is in the first batch, because not passing $sandbox by reference will always run only the first batch. We should trick somehow the update test to limit the batch size to 1. In this way it would be more easy to detect this bug. Or better adding a Drupal Coder rule to check is the var is passed by reference?

Added #3181654: Check that $sandbox is always passed by reference in (post)update functions as Drupal Coder followup

The code looks pretty good, but I did some extensive testing and found 2 bugs:

1. --- a/core/modules/comment/src/CommentAccessControlHandler.php
   +++ b/core/modules/comment/src/CommentAccessControlHandler.php
      protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
   +    // Forbid if this reply, to a threaded comment, is about to exceed the
   +    // maximum thread depth.
   +    if (isset($context['commented_entity']) && isset($context['parent_comment'])) {
   +      // ...
   +    }
        return AccessResult::allowedIfHasPermission($account, 'post comments');
   

   There is a bug here when the thread depth limit is set to 1 and the creation of child comments is denied. In this case it is not allowed to reply to root level comments, but this initial if statement bypasses the access check if $context['parent_comment'] is empty.

   Root level comments do not have a parent comment, so this means that for root level comments the user will always be allowed access to create a child comment.

   It's probably a good idea to add a test to ensure that when the depth level is set to 1 and the mode is set to THREAD_DEPTH_REPLY_MODE_DENY that the comments do not include a "Reply" link.

2. --- a/core/modules/comment/src/CommentViewBuilder.php
   +++ b/core/modules/comment/src/CommentViewBuilder.php
   +    // Compute the comment maximum indent if any.
   +    $max_indent = NULL;
   +    if ($commented_entity) {
   +      // The maximum indent could determined only on non-orphan comments.
   +      $thread_limit_settings = $entity->getCommentedEntity()
   +        ->getFieldDefinition($entity->getFieldName())
   +        ->getSetting('thread_limit');
   +      $max_indent = $thread_limit_settings['mode'] === CommentItemInterface::THREAD_DEPTH_REPLY_MODE_ALLOW ? $thread_limit_settings['depth'] - 1 : NULL;
   +    }
   

   There is a functional disparity between the 2 different reply modes in this section. This is trying to "fix" the comment depths so that comments that are "deeper than allowed" appear on the same level as the parent comment. However since this is limited to the reply mode THREAD_DEPTH_REPLY_MODE_ALLOW I was able to get a comment thread to show up that was deeper than allowed by following these steps:

   1. Create a comment field and configure it with a max depth of 2 and allowing child replies.
   2. Create a long comment thread of minimum 5 comments, by always replying to the last comment.
   3. Observe that the comment thread is shown only up to depth 2 as is expected.
   4. Edit the comment field configuration and change toggle the reply mode to deny any child comments.
   5. Reload the page: now the comment thread is shown up to a depth of 5. This is not expected since the maximum depth is 2.

   I think the right solution is to also allow "fixing" the depth when the reply mode is set to THREAD_DEPTH_REPLY_MODE_DENY.

@pfrenssen thank you for your detailed review. The changes are in the merge request.

#50.1: There's now logic of having the maximum depth < 2. That would be a flat list. On UI, a user cannot enter a depth < 2. On API I've used assert() to do the check.

#50.2: You're right. While fixing this point, I've noticed that, in #48, I've broken the "threaded with no limit" mode. No test complained, so I've added regression testing coverage.

I wanted to practice working with merge requests so I merged in the latest changes from the base branch, but now there is a failure regarding the composer lock file checksum, which doesn't make much sense since this MR doesn't change any dependencies. I am suspecting that the hash is also incorrect in 9.2.x but the latest test is green. Strange. I don't think it looks like we can solve this inside this issue though.

I have just 2 very small remarks. One is to change the data type of a parameter in a test helper method, and a minor nit in a comment. Apart from these this looks good to go from me.

MR remarks addressed

Thanks! This looks good to me, but we need to wait for #2879087: Use comment access handler instead of hardcoding permissions to be merged before this can be set to RTBC. Postponing on that issue.

I rebased the main MR onto 9.4.x and have made a copy of the original branch available as 2786587-thread-depth-limit-9.2.x.

Since the dynamically generated patches will now only apply to 9.4.x, here are backported patches for older versions of Drupal.

I figured out that in recent D9.5.2 some code from recent comment patches is already present, so I create a patch for it. If somebody needs a complete patch for version 9.5.x(PHP 8 .1).

D10.1.5 patch

The previous file is not correct. D10.x

Fixed version v3

Right on! This is a much needed option for the comments. Thank you !
How do I add this for D9?

D10.3.1 patch

I have rebased the merge requests in #73 and #74.

Rebased both branches. For 11.x, #3483599: Convert all procedural hook implementations to Hook classes (I wasn't familiar with) forced me to move a change from comment.module to src/Hook/CommentHooks.php. Hopefully it's ok now.

I'm not sure if the patches above contain all the same changes.

Great! Looks good.

The PP in the title actually means it should be postponed and seems postponed on #2879087: Use comment access handler instead of hardcoding permissions from #36. Once that lands this ticket will need an issue summary update and many of these MRs closed.

Hide patches

D10.5.6