Remove 'reply' link from comment field when threading is disabled

Nice to see the progress here!

The Issue Summary shows that a remaining task of Review and I don't see a code review in the comments so I will start with that.

1. +++ b/core/modules/comment/src/CommentLazyBuilders.php
   @@ -166,6 +166,7 @@ public function renderLinks($comment_entity_id, $view_mode, $langcode, $is_in_pr
   +    $field_definition = $commented_entity->getFieldDefinition($entity->getFieldName());
   

   Since $field_definition is only used once, inside the following if statement, it should be moved to inside the if statement. I would put it directly before the line where it is used.

2. +++ b/core/modules/comment/tests/src/Functional/CommentLinksTest.php
   @@ -150,4 +151,33 @@ public function testCommentLinks() {
   +   * Tests that reply link is visible when default_mode = 1.
   

   This needs to be clearer. Will the reader know what default_mode = 1 means? I certainly don't.

3. +++ b/core/modules/comment/tests/src/Functional/CommentLinksTest.php
   @@ -150,4 +151,33 @@ public function testCommentLinks() {
   +  public function testReplyLinks() {
   

   I don't think another functional test is needed. The test above this is testCommentLinks() and this new test is also testing links. I think the new assertions can be added to the existing test. The comment for testCommentLinks() should be expanded to include that testing with a comment mode of threaded and flat is also done.

4. +++ b/core/modules/comment/tests/src/Functional/CommentLinksTest.php
   --- a/core/modules/forum/tests/src/Functional/ForumTest.php
   +++ b/core/modules/forum/tests/src/Functional/ForumTest.php
   

   Why is a forum test being changed? I was curious so I applied the patch, removed the fix and ran ForumTest. ForumTest passes.

This also needs a fail test.

Next to look at is the screenshots, in #6 and #8. Both of them show what I presume are flat comments with the link removed. If they are flat comment then that does show it is working. But we also need a screenshot of threaded comments to see that the reply link is still there and not accidentally removed. And the screenshots are easier to find if the IS as well, in this case in the User interface changes section. That section also needs to be updated because this issue is changing the user interface.

Setting to NW for the above items.

Finally, there is a patch and it is the review stage so updating the remaining tasks section of the IS.

@manojithape, thanks for the detailed comment on what you did to test the patch! Great work. A patch that passes tests and is working correctly may not be ready for RTBC. It is also important to read the Issue Summary and comments to see what else may need doing before setting to RTBC. In this case, there is no comment on whether #9 has been addressed satisfactorily.

This isn't quite ready for RTBC. There is no evidence that a code review has been done on the latest patch and there is confirmation that all the points I raised in #9 have been addressed. Setting back to NR.

@paulocs, thank you for fixing everything in #9, As a reviewer I do appreciate the screenshots in the summary.

I retested with the fix for ForumTest removed and indeed the test fails. Whatever I did earlier in #9 for testing was wrong. I applied the patch and read the code, particularly ForumTest. The comment above the change needs to be modifed.

+++ b/core/modules/forum/tests/src/Functional/ForumTest.php
@@ -543,7 +543,7 @@ public function testForumWithNewPost() {
     // Test replying to a comment.

This is no longer replying to a comment, it is adding a new comment.

+++ b/core/modules/comment/tests/src/Functional/CommentLinksTest.php
@@ -7,6 +7,7 @@
+use Drupal\comment\CommentManagerInterface;

Since a change is needed to the patch can we take care of this nit? It is tough to add this alphabetically but after CommentInterface would be better, I think.

I have tested forum comments with and without this patch and this does change the behavior for forums. Is that a problem? I don't know.

Currently, when viewing a forum topic with comments the reply link is present in each comment. When you click reply a new page, 'Add a new comment' appears with the comment you are replying to listed first. When you enter and save the comment it will appear at the bottom of the comments. That makes sense because threading is off. With this patch, there is only the option to 'Add new comment' at the bottom of the forum topic page.

I think a search should be made in the forums issue queue for anything related to this. And this change should be explained in the User Interface section of the Issue Summary.

@larowlan, thanks for that confirmation. I looked at that file and there is no mention of threading. After searching, I learned that it is 'default_mode' and the values are define in CommentManagerInterface.

@paulocs, thank you the two small fixes. And notably, checking forum for related issues. I would have been a bonus to find a duplicate.

That covers all the points from my previous review, which also included testing. I have noted the manual testing in the IS.

Off we go!