Problem/Motivation
When threading is enabled on comments, there is no mechanism to limit how far a comment will indent. This poses a problem for sites that wish to have a defined maximum threading level for threaded comments.
Proposed resolution
- Add a configuration to allow the maximum threading depth to be defined.
- Allow the site builder to decide 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
None.
User interface changes
User perspective
When the thread depth is limited, depending on the Limited depth settings configuration the user can either reply on the deepest comment, case when the reply will be displayed with the same indent as the parent comment, or replying access is completely denied.
Site builder perspective
The Threading (internally, default_mode
) field setting is converted from a check box into radios, exposing three options:
- Flat list
- Threaded (no depth limit)
- Threaded (limited depth)
The first two options are the current options of the checkbox.
When Threaded (limited depth) is selected, the site builder is able to define a thread deep and the reply policy. Site builders can decide whether replying to the deepest comment is allowed.
Backwards compatibility note
The reason why it was opted to add a new default_mode
is to ensure a more safe backwards compatibility. The current threaded comments fields will work without needing any update path.
API changes
None.
Data model changes
- The comment field
default_mode
setting has a new optionCommentManagerInterface::COMMENT_MODE_THREADED_DEPTH_LIMIT
- New comment field setting:
thread_limit
.
Comment | File | Size | Author |
---|---|---|---|
#70 | 2786587-58-v3-10.1.x.patch | 32.84 KB | evonasek |
#69 | 2786587-58-v2-10.1.x.patch | 32.58 KB | evonasek |
#68 | 2786587-58-10.1.x.patch | 31.8 KB | evonasek |
#65 | 2786587-v3-58-9.5.x.patch | 35.81 KB | evonasek |
#58 | 2786587-58-9.3.x.patch | 81.17 KB | pfrenssen |
Issue fork drupal-2786587
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
mroycroft CreditAttribution: mroycroft at Workday, Inc. commentedAttached proposed patch.
Comment #3
mroycroft CreditAttribution: mroycroft at Workday, Inc. commentedComment #5
mroycroft CreditAttribution: mroycroft at Workday, Inc. commentedOops, forgot the schema change. Attached updated patch and interdiff.
Comment #6
mroycroft CreditAttribution: mroycroft at Workday, Inc. commentedComment #10
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #11
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #12
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #13
jhedstromComment #14
jhedstromSince 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.The max thread depth should probably be a constant on the
CommentInterface
instead of hard-coding the number wherever it's used.Comment #15
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #16
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #18
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #19
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #20
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #21
jhedstromJust a few minor tweaks/questions:
With the update hook, and the default value above, shouldn't this always be set now?
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.Comment #22
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #23
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #26
Kwadz CreditAttribution: Kwadz commentedIs there something missing to include the patch in the next release?
Comment #28
maticb CreditAttribution: maticb commented#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)
Comment #29
thronedigital CreditAttribution: thronedigital commentedbump
Comment #31
Jax CreditAttribution: Jax at Dazzle commentedThis 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:
followed by some additional lines in the else. Wouldn't it be easier to just do:
and leave the existing if/else alone?
Comment #33
claudiu.cristeaThe update number needs... update :)
This not the proper way to update a config entity. Please apply https://www.drupal.org/node/2949630.
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.
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.
You can omit assertion messages. But if you still want to keep them, don't translate them.
Comment #34
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi,
I am picking this issue and working on the #33 feedback.
Thanks,
Comment #35
claudiu.cristeaFixing #33.
Comment #36
claudiu.cristeaUnfortunately 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.
Comment #37
claudiu.cristeaPostponing on #2879087: Use comment access handler instead of hardcoding permissions
Comment #39
claudiu.cristeaThe #2879087: Use comment access handler instead of hardcoding permissions blocker depends also on #2886800: EntityAccessControlHandler::createAccess() and EntityAccessControlHandler::access() return 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.
Comment #40
claudiu.cristeaFor now just rerolled and provided a patch on top of #2879087: Use comment access handler instead of hardcoding permissions for testing purposes.
Comment #41
claudiu.cristeaHad 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.
Comment #42
claudiu.cristeaChanges:
Remaining tasks:
Comment #43
claudiu.cristeaComment #44
claudiu.cristeaChanges:
commented_entity
is already in context when replying.Comment #45
claudiu.cristeaTo Whom It May Concern: Here's an unofficial version of #44 to be used w/ Drupal 8.9.x.
Comment #46
claudiu.cristeaOuch, bad merge.
The patch to be reviewed is #44.
Comment #47
claudiu.cristeaOh!
$sandbox
should be always passed by reference in update & post-update scripts.Comment #48
claudiu.cristeaOK, 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?Comment #49
claudiu.cristeaAdded #3181654: Check that $sandbox is always passed by reference in (post)update functions as Drupal Coder followup
Comment #50
pfrenssenThe code looks pretty good, but I did some extensive testing and found 2 bugs:
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.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:I think the right solution is to also allow "fixing" the depth when the reply mode is set to
THREAD_DEPTH_REPLY_MODE_DENY
.Comment #52
claudiu.cristea@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.
Comment #53
pfrenssenI 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.
Comment #54
claudiu.cristeaMR remarks addressed
Comment #55
pfrenssenThanks! 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.
Comment #58
pfrenssenI 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.
Comment #63
evonasek CreditAttribution: evonasek as a volunteer commentedI 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).
Comment #64
evonasek CreditAttribution: evonasek as a volunteer commentedComment #65
evonasek CreditAttribution: evonasek as a volunteer commentedComment #68
evonasek CreditAttribution: evonasek as a volunteer commentedD10.1.5 patch
Comment #69
evonasek CreditAttribution: evonasek as a volunteer commentedThe previous file is not correct. D10.x
Comment #70
evonasek CreditAttribution: evonasek as a volunteer commentedFixed version v3
Comment #71
paintingguy CreditAttribution: paintingguy commentedRight on! This is a much needed option for the comments. Thank you !
How do I add this for D9?