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?

Hello,

The comment_uri function is only been used in uri_callbackcomment entity's annotation.

As per https://www.drupal.org/project/drupal/issues/2667040 ticket the uri_callback is also depricated.

In template_preprocess_comment hook $comment->permalink(); is already been used to generate Comment link.

Thanks
Samit K.

Title says to deprecate the function so that still needs to be done

Hi @smustgrave,

As per the changes record [#3384294] it is deprecated in 10.2.0 and will removed with 11.x, so i removed it for 11.x branch.

Thanks
Samit K.

Yes but it got missed so now needs to be properly deprecated

Hi @smustgrave,

Updated the code based on your suggestion. Also updated the changes record [#3384294].

Thanks
Samit K.

Thanks only thing missing is a quick deprecation test

Hi @smustgrave,,

Test also added, Please review.

Thanks
Samit K.

Couple of minor questions/nits on MR, thanks for keeping this moving

Rebased & fixed the merge conflict, looking into the MR feedback.

I have reviewed all the feedback and made the necessary updates. After making the changes, I re-ran the tests, and This is working as expected. Moving this to NR for your review.

Left another small comment on MR.

I am working on it

@smustgrave Thank you for the review & feedback , I have updated the test file name and comment for the test class, Accordingly updated the Phpstan baseline . Re-run test after these changes and working as expected. and pipeline is green so moving this again for your review.
Kindly review.

Left 1 more comment on the thread started by @larowlan

Moving this for your review.

Removed the comment on the return statement, as I believe it was not appropriate.
Kindly review.

This function is still referenced on the Comment entity type attribute as the uri callback, it should be removed there. https://git.drupalcode.org/project/drupal/-/blob/010bbc7476df8c23b0170f5...

@berdir Thank you for the feedback , I have removed the uri_callback and tested the functionality. It is working as expected. However, I am a bit unsure if there is anything else that needs to be updated related to this.

Moving this for NR , Kindly review.

Yes, uri_callback has not actually been used in a very, very long time. If it would have been called, it would have triggered deprecation errors in the tests. Looking at \Drupal\Core\Entity\EntityBase::toUrl(), it only falls back to that if there are no link templates defined. Which isn't the case for the comment entity (or any other entity in core AFAIK).

@berdir, Thank you for the Information :)

Believe all feedback for this one has been addressed.

I read the IS, comments and the MR. This all looks good but I found some things that need work, so setting to NW. See the comments in the MR.

I am not sure about the removal of uri_callback: 'comment_uri',. I did read the comment from @berdir but it is used in entity.api.php and other files. I think that needs some investigation.

the proposed solution in the iS does need an update, but #72 explains why it's not called. uri_callback is a weird leftover from before we had link templates.

Entirely deprecating that concept is done in #2667040: Deprecate uri_callback in routes for entities, this is just one specific dead configuration that is not used.

Addressed all the feedback. Based on #75, I have checked the uri_callback: 'comment_uri' in other places. I found it in EntityInterface.php. Do we need to update this as well?

Based on #76 detail, I am moving this to Needs Review to get confirmation.

@shalini_jha big thing it was sent back for seems to be issue summary update which still appears to be needed

Ah yes i missed to this IS update tag , sorry for the inconvenience, i am working for this.

1 usage in contrib found http://grep.xnddx.ru/search?text=comment_uri&filename=
Removed this line as given url is not working

I Have updated the issue summary based on the changes we have done so far. waiting for the confirmation of removal of uri_callback from other place.

#2010132: Canonical taxonomy term link for forum vocabulary is broken reminded me of this; forum is removing use of uri_callback, once comment does as well then it can be removed from core entirely.

Rebased the MR, updated the deprecation for 11.3, updated issue credit, I think this is ready to go.

Removed these from the IS as they already exist as followups:

#2113323: Rename Comment::permalink() to not be ambiguous with ::uri()
#2667040: Deprecate uri_callback in routes for entities

Committed/pushed to 11.x, thanks!