Use comment access handler instead of hardcoding permissions

Is the patch still needed with Drupal 10?
As it is mentioned as 'required' by the https://www.drupal.org/project/group_comment → module, it would be great to have some update.
Thanks for any feedback.

Attached a patch for 10.1. Patch is based on 01ed69c2. Slightly adjusted for D10.

It seems I introduced a type error in the last patch when the account is not set. Updated patch. (Only addition
$account = $account ?: \Drupal::currentUser(); in CommentFieldItemList.php).

This patch is identical to #136, only I removed one change introduced by this commit. I'm not sure if that permission check is needed (as view access on the parent comment is checked), and feels like it goes against the idea of the patch itself (to use access checks instead of permission checks). We use this patch together with Group Comment module, and that check would only allow replying to comments if we'd use the 'View comments' global permission, and we'd like to avoid that in favor of group permissions.

Tested for version 10.3
#136 and #137 didn't apply...

this is 137 rerolled for 10.3.x

My patch in #139 was missing a use statement, this should work for 10.3.x

liam morland → made their first commit to this issue’s fork.

I have put the patch in #141 into the merge request. The person who opened the merge request, @claudiu.cristea, will need to update the merge request so that it targets branch 11.x.

Done

But why did you squash? We lost the commit history :(

@claudiu.cristea if you want to rebuild you can try looking at the fork, and looking at the tags with `previous-` prefix. @ https://git.drupalcode.org/issue/drupal-2879087/-/network/9.2.x?ref_type... / https://git.drupalcode.org/issue/drupal-2879087/-/tags

Update: I recreated the history from previous/2879087-comment-access-handler/2022-01-31 branch
https://git.drupalcode.org/issue/drupal-2879087/-/tags/previous%2F287908...
Merged 11.x, fixed conflicts, phpcs, phpstan and the tests touched by the patch.

5 tests are failing at the moment, among those these are concerning to me and relate to the changes in CommentController::replyFormAccess:
- core/modules/comment/tests/src/Functional/CommentNonNodeTest.php
- core/modules/comment/tests/src/Functional/CommentAnonymousTest.php

Also #137 may be right, I'll try to investigate those issues.

For now here are patches of the current MR23 commit f2ac23e6.

Important fix for cacheability:
The CommentDefaultFormatterCacheTagsTest was failing because the CommentDefaultFormatter was not applying cacheability to the render array.

---

On that note, we have a need in our project where the create comment permission will have a very high cache variance (per user).
This means that currently, it would kill the Dynamic Page Cache when CommentDefaultFormatter checks for the create permission and adds the lazy_builder and cacheability to the render array.
I propose to move the access check inside the lazy_builder callback, in CommentLazyBuilders::renderForm.

I'd like to keep the scope to a minimum but this touches on the same lines and lots of projects may be in a similar scenario so we should try to preserve the page cache as much as possible. Viewing comment is less likely to have such requirements.
The change is very small, but this could also be reverted and a separate issue or follow up instead if anyone objects.

I pushed an attempt now to address #118 to #125 (reply to {$pid}).
By calling $parent_comment->access('reply') in CommentController::replyFormAccess and doing the actual check in CommentAccessControlHandler::checkAccess

Also, I had a look at the other issue ✨ [PP-1] Add thread depth configuration to comments Postponed which requires this but it seems to rely only on CommentAccessControlHandler::checkCreateAccess at the moment. Since we now have the reply access operation, I'm not even sure we need all the changes to $context (parent_comment, commented_entity).
I'll sync with @claudiu.cristea on Monday.

---

PS: still the same 2 (less concerning) test failures which I left out for now since I don't quite understand them yet.

smustgrave → changed the visibility of the branch 9.2.x to hidden.

smustgrave → changed the visibility of the branch 2879087-use-comment-access to hidden.

smustgrave → changed the visibility of the branch 2879087-comment-access-handler-9.4.x to hidden.

smustgrave → changed the visibility of the branch 2879087-comment-access-handler-9.2.x to hidden.

smustgrave → changed the visibility of the branch 9.4.x to hidden.

smustgrave → changed the visibility of the branch 11.x to hidden.

smustgrave → changed the visibility of the branch 10.3.x to hidden.

Issue summary needs to be flushed out.

1. This is adding a new trait but not sure I see that mentioned.
2. All the changes outside the comment module, to me seem out of scope.
3. Adding a test EntityCreateAccessCustomStaticCidTest outside of comment not sure I see the why.

Hiding all branches.