permalink breaks in comment

I think this one could be related with #2642260: Missing URL fragment for the comment's title link

I think ideally the permalink should be something like:

http://site.com/comment/XX#comment-XX

We decided to work on this before to fix #2642260: Missing URL fragment for the comment's title link, so I will assign it to me.

Ok, a file with only the test and the patch attached.

Aaaaaargh we broke this again? And there was no test? Thanks for fixing it.

Here's the drupal 7 version of this...

function comment_permalink($cid) {
  if (($comment = comment_load($cid)) && ($node = node_load($comment->nid))) {

    // Find the current display page for this comment.
    $page = comment_get_display_page($comment->cid, $node->type);

    // Set $_GET['q'] and $_GET['page'] ourselves so that the node callback
    // behaves as it would when visiting the page directly.
    $_GET['q'] = 'node/' . $node->nid;
    $_GET['page'] = $page;

    // Return the node view, this will show the correct comment in context.
    return menu_execute_active_handler('node/' . $node->nid, FALSE);
  }
  return MENU_NOT_FOUND;
}

Should we be trying to replicate this functionality? To displace the comment in context?

We have a sub-request in that controller - so I think it's functionally equivalent:

      $redirect_request = Request::create($subrequest_url->getGeneratedUrl(), 'GET', $request->query->all(), $request->cookies->all(), array(), $request->server->all());
      $redirect_request->query->set('page', $page);
      // Carry over the session to the subrequest.
      if ($session = $request->getSession()) {
        $redirect_request->setSession($session);
      }
      // @todo: Convert the pager to use the request object.
      $request->query->set('page', $page);
      $response = $this->httpKernel->handle($redirect_request, HttpKernelInterface::SUB_REQUEST);

Related #2377849: Simplify what CommentLinkBuilder is doing - consider removing a lot of the functionality - comment links are high on my 'too generic to be of use and too rigid to modify' hit list.

I think we at least need to do what we do in D7 while we land #2377849: Simplify what CommentLinkBuilder is doing - consider removing a lot of the functionality

This reveals more problems though...
Steps to reproduce:

1. Apply patch
2. Install standard
3. Set article node type's comment field to display 10 comments per page
4. Add eleven comments
5. Go to the permalink for the 11th commment (comment/11#comment-11)
6. Try to use the pager buttons... fail

Also the metatags on this page are interesting...

<link rel="canonical" href="/node/1">
<link rel="shortlink" href="/node/1">
<link rel="delete-form" href="/node/1/delete">
<link rel="edit-form" href="/node/1/edit">
<link rel="version-history" href="/node/1/revisions">
<link rel="revision" href="/node/1">

I'm trying to work out what this is going to mean for SEO and whether these link relations are at all correct.

This new patch should fix the problem exposed by #13.

Let's see what the testbot says.

I think this is the good one!

Ok the patch at #20 fix the bug exposed by #13, @alexpott please let me know if there is something to I can do to help with the SEO thing.

Thanks!

-1, please don't use <current> all the time. Please do a git blame on those lines and see where those bits are coming from.

The git blame points to https://www.drupal.org/node/2351015 It seems to this was changed on purpose.

I will try to think in a different approach.

Thanks for the review.

New approach. Let's see what the testbot says.

Thank you @gnuget, this is looking so much better!

+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -164,6 +164,8 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+            $build['pager']['#route_name'] = \Drupal::routeMatch()->getRouteObject();
+            $build['pager']['#route_parameters'] = \Drupal::routeMatch()->getRawParameters()->all();

We should really explain this here.

Looking good - one minor nit

+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -164,6 +164,8 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+            $build['pager']['#route_name'] = \Drupal::routeMatch()->getRouteObject();
+            $build['pager']['#route_parameters'] = \Drupal::routeMatch()->getRawParameters()->all();

CommentDefaultFormatter already implements ContainerFactoryPluginInterface so we can inject the route match service instead of using the singleton

Thanks for your reviews.

In this patch I injected the route match and I added a comment for explain why we have to pass the route to the pager.

Looks ready now

+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -164,6 +174,12 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
             $build['pager']['#type'] = 'pager';
+            // The CommentController::commentPermalink calculate the page number
+            // where a specific comment appear and do a subrequest pointing to
+            // that page, we need to pass that subrequest route to our pager to
+            // keep the pager working.

Nits on the comments, did the following fix on commit.

@@ -164,6 +174,12 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
           if ($comments) {
             $build = $this->viewBuilder->viewMultiple($comments);
             $build['pager']['#type'] = 'pager';
+            // CommentController::commentPermalink() calculates the page number
+            // where a specific comment appears and does a subrequest pointing to
+            // that page, we need to pass that subrequest route to our pager to
+            // keep the pager working.
+            $build['pager']['#route_name'] = $this->routeMatch->getRouteObject();
+            $build['pager']['#route_parameters'] =  $this->routeMatch->getRawParameters()->all();
             if ($this->getSetting('pager_id')) {
               $build['pager']['#element'] = $this->getSetting('pager_id');
             }

Committed/pushed to 8.2.x and cherry-picked to 8.1.x, thanks!

Added review credit post-commit.

Leaving at 8.1.x due to constructor changes etc.