Deprecate comment_uri()

#2: comment-2010202-2.patch queued for re-testing.

This is missing the fragment. It should be kept like it was before.

Patch in #2 was not applying correctly anymore on HEAD. Rerolled.

@tstoeckler, regarding #5, maybe it's up to the presentation layer to use ->permalink() instead of ->uri().

Entity::uri() should remain the canonical URI of the entity, without any fragment, no ?

I'm not really an expert on the whole comment URI / permalink question. But I remember that choosing this URI was discussed in-depth and had very specific reasoning (which I can't recall now). So if you think the fragment should not be part of the URL, please open another issue. For now, let's just straight port this. That would be awesome, thanks!

I would be glad to finish that as a straight port. Could you please recover some pointers referring to that discussion? I would be happy to get more insight on this.

This patch includes the fragment.

Actually after https://drupal.org/node/2020491 this function does not work because URI is generated via 'links' property and fragment is skipped
Not sure we need to override uri() method without checking $rel and calling parent::uri($rel)
Seems for 'canonical' is not allowed to have 'fragment'

See the original issue where this was added, it is not supported and was therefore dropped.

I don't rreally agree with that discussion/conclusion but that's what has been done.

As we have $comment->permalink() and this code is not used anymore since #1970360-78: Entities should define URI templates and standard links

winning

#1978904: Convert comment_admin() to a Controller needs a reroll then.

According #1978904-64: Convert comment_admin() to a Controller we have 3 different functions to generate permalink and comment/ID links to make DX better

The proper fix for that should combine #2111419: Remove CommentManager::getParentEntityUri() in favor of Comment::permalink()

1) Comment->permalink() should point to actual comment comment/123#comment-123
2) Comment->getCommentedEntityURI() should point to node/1#comment-123

Marked #2111419: Remove CommentManager::getParentEntityUri() in favor of Comment::permalink() as duplicate
Postponing on #2113323: Rename Comment::permalink() to not be ambiguous with ::uri()

related #1970360-78

Given how much of a regression D7's comment permalinks present for drupal.org (cf. #2125397: Enable comment_fragment 7.x-1.x-dev to improve issue comment permalinks), I definitely think we should fix this for D8.

What follows is essentially quoting myself from 2009: #26966-97: Fix comment links when paging is used.

The original change was meant to solve two discrete issues:

1. Broken/dysfunctional links to comments that happen to be located on a subsequent page due to pager settings, whereas (i) pager settings can change at any time and (ii) new comments can move existing comments to a different page (in case comment threading is enabled).
2. Preventing duplicate content indexed by search engines, caused by comment URLs.

(2) is fully resolved by canonical URI meta tags in core today. Any search engine that doesn't respect that is not worth to support.

(1) can be resolved in the exact same way, but more user-friendly today though:

/node/123?comment=456#comment-123

↓

/{context_entity_type]/{context_entity_id}?comment={comment_id}#comment-{comment_id}

1. The /comment/$id URI is replaced with the URI of the context.
2. The requested comment is still targeted by a new ?comment=$id query string.
3. The query string is used to conditionally redirect to the correct page (but in 99.9% of all cases, no redirect happens).
4. Whoever plans to raise performance/caching concerns, especially with regard to reverse-proxies: The existing /comment/$id is a separate and separately cached URI, too: Zero difference.
5. The fragment remains to jump to the right document position, even in case of a redirect.

Now, if you additionally take the architectural concern of comment #20 in #2113323-20: Rename Comment::permalink() to not be ambiguous with ::uri() into account, we actually arrive at this:

/{a/r/b/i/t/r/a/r/y/-/c/o/n/t/e/x/t}?comment={comment_id}#comment-{comment_id}

As a result, the most sane architecture and technical implementation I can think of is this:

1. Turn Comment::permalink() into this:

   class Comment {
     public function uriByContext(\Drupal\Core\Url $context_uri) {
       $uri = clone $context_uri;
       $query = $uri->getOption('query');
       $query['comment'] = $this->id();
       $uri->setOption('query', $query);
       $uri->setOption('fragment', 'comment-' . $this->id());
       return $uri;
     }
   }
   

2. Make {whichever-code} that generates the permalinks for comment fields attached to other entities call this:

   foreach ($comments as $comment) {
     $comment_permalink = $comment->uriByContext($this->entity->uri());
   }
   

   → resulting in e.g. /node/12345?comment=45678#comment-45678

3. The real gist: Add a Request event subscriber to Comment module, which

   1. kicks-in for every Request that points to an entity resource URI (e.g. /node/12345)
   2. checks whether the request->query->has('comment')
   3. checks whether the requested comment is available on the requested URI
   4. if not, issues a redirect response (like now).

I originally planned to code this up myself, but I have too many other issues on my plate right now. My hope is that someone else is able to pick this up and turn this concept into an implementation.

The problem here that we need to query database in request subscriber to make sure that comment exists on given page.
Suppose this solvable except that this subscriber would fire on each entity route

The query string is used to conditionally redirect to the correct page (but in 99.9% of all cases, no redirect happens).
Whoever plans to raise performance/caching concerns, especially with regard to reverse-proxies: The existing /comment/$id is a separate and separately cached URI, too: Zero difference.

That's not zero difference.

In one case you have a GET request which always returns a 200 and doesn't require any runtime checks. Sites with a relatively low number of comments per page will end up doing lots of 302 redirects, which is completely uncacheable.

Let's take a Drupal.org issue thread with over 300 comments, but which hasn't seen an update for several hours. Every time a comment link on the second page gets visited, whether by humans or crawlers, it'll always be a 302 whereas with the current scheme it'll be in varnish.

if not, issues a redirect response (like now).

There's no redirect in Drupal 7, there's a menu_execute_active_handler() call, and a subrequest in 8.x.

Here is my short review regarding the most popular "comment hosting services" :

FACEBOOK comments module :
All comments are visible on a single page with a « display n others comments » button displayed at the top. By default it display last comments and collapsing old ones.

DISQUS :
https://disqus.com/
All comments are visible on a single page with a « load more comments » button displayed at bottom.
permalink example : https://disqus.com/home/channel/selectstart/discussion/channel-selectsta...

LIVEFYRE :
All comments are visible on a single page with a “load comments” button displayed at bottom.
See example on http://mashable.com/ which implements this comment hosting service

MUUT :
https://muut.com/demos/basic-forum.html
All comments are visible on a single page with a “more” button displayed at bottom (the label of the button is a number which indicate the count of hidden comments).
permalink example : https://muut.com/demos/basic-forum.html#!/exhibitions:this-is-my-statement

DISCOURSE :
http://try.discourse.org/t/what-happens-when-a-topic-has-over-1000-repli...
All comments are visible on a single page through an infinite scroll :
Whether the user goes directly to a specific comment with the comment ID, the system will load the previous five and ten next comments which surrounds the specific comment.

WORDPRESS default comments system :
It displays all comments on a single page, without hiding the old ones.
permalink example : wordpress_site/2015/09/30/hello-world/#comment-5
All comments are displayed together, not possible to display a single comment with the permalink.

To sum up : with the modern "comment hosting services", there is never pagination for the comments display.
The more sophisticated solution is the “infinite scroll”.

IS needs update cos deprecation of comment_uri() still makes sense, not sure as it bug

updated IS

Let's see effect of removal

Added deprecation, CR still needed but let's see how often it's used

Updated the issue title, this is just a straightforward deprecation now. Needs a re-roll updating the deprecation version.

Filed CR https://www.drupal.org/node/3384294

and updated patch, looks there's no test for uri_callback annotation in comment entity

Also it needs update hook to clear entity definition cache, and let's see if something will be broken with deprecation only

Looking for better wording as Comment::permalink() is useless #2113323: Rename Comment::permalink() to not be ambiguous with ::uri()

That's reason why issue used to have such title

updated IS

Comment::permalink() isn't useless, it's necessary to link to a comment in the context of the thread (with the correct page and comment fragment). I think we can just tell people to use that until it's renamed. However why not recommend $comment->uri() here?