Missing URL fragment for the comment's title link

Hi!

This looks good! but with the txt extension this patch won't be tested by the testbot.

Can you please re-upload your patch using the guidelines? https://www.drupal.org/node/707484

Thanks!

Testbot didn't complain so this is RTBC to me.

Thanks!

This needs test coverage to prevent future regressions.

OK, In this new patch I added the test.

Not sure why the testbot fails here, I tried with a slightly different approach this time

Can you provide an interdiff? It's difficult to figure out what's changed.

Sure, this is the interdiff between #9 and #12

Thanks for your time.

1. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
   @@ -65,9 +65,8 @@ public function testCommentPopulatedTitles() {
   -    $comment_permalink = (string)$comment_permalink[0]->attributes()->{'href'};
   ...
   +    $comment_permalink = (string)$comment_permalink[0]['href'];
   

   I guess this was the fix to the test?

2. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
   @@ -65,9 +65,8 @@ public function testCommentPopulatedTitles() {
   +    $this->assertTrue((bool)preg_match('/\/comment\/[0-9]+#comment\-[0-9]+/', $comment_permalink), 'The comment\'s title has the correct link');
   

   It's no longer testing that the title has the correct link. Now it's testing that the comment includes a fragment.

Ok,I just added the comment id just to make sure to we are testing the correct link.

Thanks for your review!

I'll leave it up to you, but maybe we don't need to use the preg_match anymore if that works?

yup, you are right.

Thanks.

Just tested this manually and it is indeed a regression from D7.

The patch looks pretty good, I only found two small things that could be fixed:

1. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
   @@ -64,5 +64,9 @@ public function testCommentPopulatedTitles() {
   +    $comment_permalink = (string)$comment_permalink[0]['href'];
   

   Missing space after "(string)".

2. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
   @@ -64,5 +64,9 @@ public function testCommentPopulatedTitles() {
   +    $this->assertTrue('/comment/' . $comment1->id() . '#comment-' . $comment1->id() ==  $comment_permalink, 'The comment\'s title has the correct link');
   

   We should use assertEqual() here :)

#20.2 now that you mention it, assertLinkByHref() may be an option too.

Thanks for your reviews.

Here the patch with the changes.

I considered using assertLinkByHref but in this particular page we might have the same link twice (see #2641682: permalink breaks in comment) so I prefer to look for this specific link in the title of the comment, thanks for your suggestion, I didn't know about this function.

This is weird, #18 passed before and now it failed, and #22 passed in my local version :-/

And looking at https://www.drupal.org/pift-ci-job/155572 what is that "checkout" think in the URL?

is this might be a malfunctioning of the bot? am I doing something wrong?

Lets see if this time I can make happy the testbot

#25: #18 was likely a false positive due to #2657482: Test bot doesn't apply the patch.

The logic in template_preprocess_comment() is now actually making it a permalink… which means it can use $comment->permalink(), just like $variables['permalink'] already does.

+++ b/core/modules/comment/comment.module
@@ -660,6 +660,7 @@ function template_preprocess_comment(&$variables) {
     $uri->setOption('attributes', $attributes);
+    $uri->setOption('fragment', 'comment-' . $comment->id());
     $variables['title'] = \Drupal::l($comment->getSubject(), $uri);

This is nearly equal to

  public function permalink() {
    $entity = $this->getCommentedEntity();
    $uri = $entity->urlInfo();
    $uri->setOption('fragment', 'comment-' . $this->id());
    return $uri;
  }

The only difference is the $uri->setOption('attributes', $attributes); call.

So, this patch can hence AFAICT be simplified to:

diff --git a/core/modules/comment/comment.module b/core/modules/comment/comment.module
index e1d74b6..a91c0ec
--- a/core/modules/comment/comment.module
+++ b/core/modules/comment/comment.module
@@ -656,7 +656,7 @@ function template_preprocess_comment(&$variables) {
     $variables['permalink'] = \Drupal::l(t('Permalink'), new Url('<front>'));
   }
   else {
-    $uri = $comment->urlInfo();
+    $uri = $comment->permalink();
     $attributes = $uri->getOption('attributes') ?: array();
     $attributes += array('class' => array('permalink'), 'rel' => 'bookmark');
     $uri->setOption('attributes', $attributes);

While I'm agree with you about to we should use the permalink here instead of just add the fragment to the url the thing is the permalink function is broken.

The $comment->permalink(); returns /node/1#comment-1 which is not a the permalink because this function doesn't consider the pagination of the nodes, indeed this was already reported in #2641682: permalink breaks in comment

So in my opinion what we need to do is pospone this issue fix #2641682: permalink breaks in comment and later come back to this one and use $comment->permalink(); this time.

What do you think?

Works for me!

Alright, let's do that.

Alright #2641682: permalink breaks in comment was fixed now we can continue working on this one.

I will assign it to me.

Ok in this new patch I used the permalink function.

The patch at number at #34 needed reroll. I have done that and tested it. It looks good. Thanks @gnuget!

+++ b/core/modules/comment/src/Tests/CommentTitleTest.php
@@ -59,6 +59,10 @@ public function testCommentPopulatedTitles() {
+    $this->assertEqual($comment1->permalink()->toString(), $comment_permalink, 'The comment\'s title has the correct link');

minor: can we just use double quotes here instead of \'?

Should we also check that the href includes the fragment?

Other than that, looks RTBC to me

Accomodate #37 nit Still N/R for question in #37

Hiding #39 patches

Thanks - #38 is RTBC

I return this to RTBC because the patch which failed was #39 and the patch RTBC is 38. (and I just make sure and it still applies correctly)

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Can somebody please close #2793373: permalink for comments not working if it is a duplicate?

@cilefen: done.