Make comment links functionality testable and convert CommentLinkTest to PHPUnit

I think comment is no longer tied to to Node, can you explain why NodeInterface is used in CommentLinkBuilder? Thanks

+++ b/core/modules/comment/src/CommentLinkBuilder.php
@@ -0,0 +1,222 @@
+    $fields = $this->commentManager->getFields('node');

I think we should inject 'node' string to this service, it's easier for developer to use it for other entity.

@thehong "node" is a entity type argument, I can't get what you mean

@andypost Instead of

\Drupal::service('comment.link_builder')->buildLinks($node_links, $node, $context);

we should change it to:

\Drupal::service('comment.link_builder')->buildLinks('node', $node_links, $node, $context);

\Drupal::service('comment.link_builder')->buildLinks('node', $node_links, $node, $context);

This doens't make sense. The entity itself ($node) already knows about everything which is needed. (it's entity type ID).
No need to fallback to Drupal 7 APIs.

Just an airport review.

1. +++ b/core/modules/comment/comment.module
   @@ -48,16 +48,6 @@
    /**
   - * Comment form should be displayed on a separate page.
   - */
   -const COMMENT_FORM_SEPARATE_PAGE = 0;
   -
   -/**
   - * Comment form should be shown below post or list of comments.
   - */
   -const COMMENT_FORM_BELOW = 1;
   -
   

   <3

2. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -0,0 +1,222 @@
   + * Defines a class for building markup for comment links on a commented entity.
   + *
   + * Comment links include 'login to post new comment', 'add new comment' etc.
   + */
   

   i really like to have a proper one line description which explains what is going on.

3. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -0,0 +1,222 @@
   +   * Current user proxy.
   +   *
   +   * @var \Drupal\Core\Session\AccountProxyInterface
   +   */
   

   Always typehint the accountinterface and don't talk about the proxy. this is just an implementation detail.

4. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -0,0 +1,222 @@
   +    if ($view_mode == 'search_index' || $view_mode == 'search_result' || $view_mode == 'print') {
   +      // Do not add any links if the node displayed for:
   +      // - search indexing.
   ...
   +      // - print.
   +      return;
   +    }
   +
   ...
   +        if ($view_mode == 'rss') {
   +          // Add a comments RSS element which is a URL to the comments of this
   +          // entity.
   +          $options = array(
   +            'fragment' => 'comments',
   +            'absolute' => TRUE,
   +          );
   +          $entity->rss_elements[] = array(
   +            'key' => 'comments',
   +            'value' => $entity->url('canonical', $options),
   +          );
   +        }
   +        elseif ($view_mode == 'teaser') {
   

   I know this is out of scope but can't we somehow integrate these links properly into the entity display bits? These links should be for example be configurable per view mode.

5. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -0,0 +1,222 @@
   +      $commenting_status = $entity->get($field_name)->status;
   +      if ($commenting_status) {
   

   Oh I nearly got tricked by the fact that status is a part of the comment field. Any reason to not use != CommentItemInterface::HIDDEN ?

6. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -0,0 +1,222 @@
   +                'title' => $this->formatPlural($entity->get($field_name)->comment_count, '1 comment', '@count comments'),
   ...
   +                'html' => TRUE,
   

   Wait, this string does not contain any HTML, or does it?

7. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -0,0 +1,222 @@
   +              if ($this->moduleHandler->moduleExists('history')) {
   +                $links['comment-new-comments'] = array(
   

   Urgs, would be so much better to kinda make it more flexible and less hardcoded

8. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -0,0 +1,222 @@
   +                    'title' => $this->getStringTranslation()->translate('Jump to the first new comment of this posting.'),
   

   You have to use $this->t() otherwise potx cannot find it. Out of scope of this issue.

9. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -0,0 +1,222 @@
   +                  'html' => TRUE,
   

   Same question, this is not even HTML at all.

10. +++ b/core/modules/comment/tests/src/CommentLinkBuilderTest.php
    @@ -0,0 +1,338 @@
    +      ->willReturn("Can't let you do that Dave.");
    +    $this->stringTranslation->expects($this->any())
    +      ->method('translate')
    +      ->willReturnArgument(0);
    +    $this->stringTranslation->expects($this->any())
    +      ->method('formatPlural')
    +      ->willReturnArgument(1);
    

    Can you use the getStringTranslationStub() method here? I could imagine that formatPlural is not supported? It would then be great to add support for it.

Overall it's a big step forward to clean-up spaghetti legacy for links!

1) The order of arguments just mimics hook_node_links_alter() & hook_comment_links_alter()
2) In context of comment services I think that better follow patterns #2188287: Split CommentManager in two by placing all render funcs into one CommentRenderHelper service
3) I'd suggest to extract conversion of constants to separate issue with own change record.

Also this patch just converts node links but I think comment links should be addressed here too.

+++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItemInterface.php
@@ -27,4 +27,14 @@
+  const FORM_SEPARATE_PAGE = 0;
...
+  const FORM_BELOW = 1;

+++ b/core/modules/comment/src/Tests/CommentTestBase.php
@@ -265,7 +266,7 @@ public function setCommentPreview($mode, $field_name = 'comment') {
-    $this->setCommentSettings('form_location', ($enabled ? COMMENT_FORM_BELOW : COMMENT_FORM_SEPARATE_PAGE), 'Comment controls ' . ($enabled ? 'enabled' : 'disabled') . '.', $field_name);
+    $this->setCommentSettings('form_location', ($enabled ? CommentItem::FORM_BELOW : CommentItem::FORM_SEPARATE_PAGE), 'Comment controls ' . ($enabled ? 'enabled' : 'disabled') . '.', $field_name);

+++ b/core/modules/comment/tests/src/CommentLinkBuilderTest.php
@@ -0,0 +1,338 @@
+      $this->getMockNode(FALSE, CommentItem::OPEN, CommentItem::FORM_BELOW, 1),
...
+        $this->getMockNode(TRUE, CommentItem::OPEN, CommentItem::FORM_BELOW, 1),
...
+      'form_location'            => array(CommentItemInterface::FORM_BELOW, CommentItemInterface::FORM_SEPARATE_PAGE),
...
+        if ($combination['comments'] == CommentItem::OPEN) {
...
+            if ($combination['view_mode'] == 'teaser' || ($combination['has_access_comments'] && $combination['comment_count']) || $combination['form_location'] == CommentItem::FORM_SEPARATE_PAGE) {
...
+              if ($combination['form_location'] == CommentItem::FORM_BELOW) {

Use CommentItemInterface for constants

I think that only change record needed, +1 rtbc
@dawehner are you agree?

so let's go

All I can say is: Wow!.

+++ b/core/modules/comment/comment.module
@@ -257,151 +247,7 @@ function comment_node_links_alter(array &$node_links, NodeInterface $node, array
+  \Drupal::service('comment.link_builder')->buildLinks($node_links, $node, $context);

Why not buildNodeLinks? To reflect the hook_node_links_alter() - also I think this should return the array of links and the alter hook should still add them - ie not pass $entity_links at all. That way if we ever move hook_node_links_alter to events we can have a build event and an alter event.

great

Thanks. Committed b6ba35f and pushed to 8.0.x. Thanks!