Problem/Motivation

On CommentInterface we have two different methods CommentInterface::permalink and EntityInterface::uri. We also have comment_uri so If we want to remove comment_uri which method will replace it and why?

Proposed resolution

Please clarify the difference and document it.

Remaining tasks

Discussion

User interface changes

None

API changes

None

#2010202: Deprecate comment_uri()
#2111419: Remove CommentManager::getParentEntityUri() in favor of Comment::permalink()
#1978904: Convert comment_admin() to a Controller
#1970360-78: Entities should define URI templates and standard links

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it's a deprecation of an existing interface method, renaming it to prevent ambiguity.
Issue priority Normal, non-critical.
Disruption Impact > Disruption, as there's virtually no disruption (only two changed theme variables and two controller method names), and the impact is to reduce the possibility of confusion of what the difference between Comment::permalink and Comment::uri is.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

I introduced the permalink() method while adding unified Entity URI links support. I have no idea why it's there, but the existing code had separate operations for comment_permalink() and the entity URI. The comment URI is the path that the comment lives at as far as REST is concerned; eg, /comment/123. The permalink is the anchored URI on the node page, eg, /node/123#comment-456.

No, this doesn't make sense to me at all. :-) Honestly the word "permalink" has always struck me as stupid and born of some popular blogging platform being utterly ignorant of how the web works in the first place, and then the term stuck. I left that split in place in the original patch because sorting that out was off topic for that issue.

The canonical entity URI needs to be anchor-less, for REST purposes, and we likely do need to retain a way to link to a comment "in context". Beyond that, and whether "permalink" is even the right name for that method (I'd be happy to purge the word from our lexicon entirely as a relic of ignorant developers 10 years ago who didn't know what they were doing) I don't know and largely don't care. :-)

andypost’s picture

According #1970360-78: Entities should define URI templates and standard links entity URI can't have #fargment part so we need another method (CommentInterface::permalink()) to point the exact comment that's why we have route comment.permalink with CommentController::commentPermalink() handler

According #2010202-18: Deprecate comment_uri() I think that also we need CommentInterface::getCommentedEntityUri with fragment to redirect user after new comment posted which requires us:
1) calculate the page for the new comment
2) apply path alias for entity URI
3) add fragment #comment-123 to link

andypost’s picture

Issue tags: +D8UX usability

Also permalink() method is needed for better UX to allow users to easily copy/paste/share link for the comment

Both uri() and permalink() used in template_preprocess_comment()

uri() mostly used to generate comment related links (edit/delete/translate)

pemalink() used only to navigate from comment administration screen to the comment and for UX pointed above

catch’s picture

Issue tags: -D8UX usability

We definitely need to retain the ability to link to comments in context - that's what allows thinks like 'first unread comment' and 'most recent attachment' to work on issue queues for issues with more than 300 comments (once Drupal.org is running on 7.x).

Doesn't need to be called 'permalink' though, that was taken from http://drupal.org/project/permalink when the original bug fix went in iirc.

jibran’s picture

linclark’s picture

Yeah, agreed with catch. We need a way to show comments in context, but that shouldn't be called permalink because the word permalink does not require that the comments be shown in context. For example, look at the way that permalinks work in Reddit... http://www.reddit.com/r/drupal/comments/1oi244/iama_chx_ama/ccs4wlw.

jibran’s picture

So permalink should be comment/$cid which is same as uri IMO. Then what should we call $commented_entity_type/$commented_entity_id#commnet-$cid?page=whatever. Related #2111419: Remove CommentManager::getParentEntityUri() in favor of Comment::permalink()

andypost’s picture

So I postponed #2010202: Deprecate comment_uri() on the issue

There's more problems that affects D.o migration too

1) currently links to comments in issues does not have ?page=x when page > 0
Suppose we need to make them comment/x#commment-x format

2) SEO content duplication https://groups.drupal.org/node/26674
related issue #180379-45: Fix path matching in robots.txt

So better to have 2 methods to generate:
1) comment/123#comment-123 - to display the 123 comment - this link generation looks like permalinks, we could easily override the router to display "reddit-like" comments without context or redirect to "contexted urls"
2) some_entity_url?page=YYY#comment-XXX - should be used only in case of form redirect

linclark’s picture

Just brainstorming a couple of ideas...

  • threadedCommentUri
  • commentThreadUri
  • contextualDisplayUri
linclark’s picture

So better to have 2 methods to generate:
1) comment/123#comment-123 - to display the 123 comment - this link generation looks like permalinks, we could easily override the router to display "reddit-like" comments without context or redirect to "contexted urls"

If we are already using the URI for the comment (comment/123), then why do we need a fragment there (#comment-123)? Fragment identifiers are meant to refer to subordinate resources. Since we are already pointing to the comment, what does the fragment identify?

andypost’s picture

If we are already using the URI for the comment (comment/123), then why do we need a fragment there (#comment-123)?

We need this to navigate comment on page oncw core by default does not use "reddit-style"

linclark’s picture

What do you want to see rendered at comment/123? Would the full thread get rendered?

I would assume only the comment gets rendered when I go to comment/123, and that the thread gets rendered when I go to some_entity_url?page=YYY#comment-XXX

andypost’s picture

What do you want to see rendered at comment/123? Would the full thread get rendered?

Currently on the page we make sub-request to get proper page of the comment and renders a whole entity and comment thread so we need #comment-123 on it to navigate to needed comment.

Suppose it would be better if after visiting comment/123 we got redirected to proper some_entity_url?page=YYY#comment-XXX so this become actual permalink for comment because in this redirect we can calculated the actual page for comment and this could make comment::permalink() to actually return the needed page

EDIT remember that this calculation is not cheap so docs should point that this function should not be used for render

catch’s picture

For reference the original issue that added this is #26966: Fix comment links when paging is used.. The original bug was was what got me into Drupal core development back in 2007.

@jibran/@andypost/@linclark $commented_entity_type/$commented_entity_id#commnet-$cid?page=whatever. isn't a good link to print anywhere.

Especially with threaded comments when a comment can move between pages, but also if the number of comments per page changes, or if some comments are deleted/unpublished, then the page a comment appears on can change and those links go stale. comment/123#comment-123 guarantees that regardless of any of this it's always shown in the right place. Sticking with comment/123#comment-123 then redirecting also has issues:

1. HTTP redirects take time. Linking to a page that redirects is less than ideal too.

2. Lots of people share links by cutting and pasting from browser address bars, and again the ?page is unreliable with threaded comments.

On the original issue that introduced this, one idea was node/123/comment/321#comment-321 for the comment in context. I think we went for comment/123 in case comments applied to any entity at some point -obviously they do now, so it'd be a question of generating the link knowing the entity type that the comment applies to, which we can find out easily enough but would mean some dependencies in the comment class.

It'd be nice not to have the fragment since it looks redundant, but the only way to nuke that would be using js to scroll to the right place which is a lot less efficient than just using #fragment.

jibran’s picture

I think first we have to identify the problem here.

  1. We want a link to comment entity which only shows comment in full view mode. This is for the rest team so that we can access comment entity with uniform call. Let's suppose it is $uri so edit link will be $uri/edit and delete will be $uri/delete.
  2. We want a link of comment entity on commented entity display.
  3. Now we have two options generate $url for each comment.

    1. Redirect user to commented entity display. In this case redirecting function will handle page number and etc.
    2. Pass $comment->id() as _GET param to commented entity display and scroll to the comment using JS. In this case page number should be part of $url.

@catch is right $commented_entity_type/$commented_entity_id#commnet-$cid?page=whatever is not a good link. But IMO comment/$comment->id() is fine.
My Idea, if we have rest request to comment/$comment->id() it should return $comment entity and if we have HTTP request it should redirect to commented entity display with JS scroll to remove fragment.
Yes redirect is slow but we have permanent link to $comment entity we can use link to share/SEO and we can access the commented entity display where comment entity is displayed.
In this way we can cover all the cases.

catch’s picture

Now we have two options generate $url for each comment.

Redirect user to commented entity display. In this case redirecting function will handle page number and etc.
Pass $comment->id() as _GET param to commented entity display and scroll to the comment using JS. In this case page number should be part of $url.

I don't think these are the only two options - we can also keep the existing link structure which solves both the HTTP redirect and front-end performance issue for the comment in context links.

ianthomas_uk’s picture

I agree with #12, in other REST APIs that I have used comment/123 would display the contents of comment 123 and maybe some identifiers that allowed me to retrieve details of its associated entities. Similarly, if I go to comment/123/edit I'd expect to see the edit form for that comment, I wouldn't expect to be able to edit the node and all other comments.

I propose that

  • EntityInterface::uri() returns comment/$cid, which would show just that comment.
  • ???Interface::contextualUri() returns $parent_uri/comment/$cid#comment-$cid (e.g. node/123/comment/456#comment-456)
  • permalink and comment_uri are deprecated in favour of ::contextualUri()

To avoid duplicate content SEO issues there should be an option to HTTP redirect from the entity URI to the contextual URI, but that would not routinely be used because the contextual URI would be the standard one output.

I'd be happy to make a patch for this if other people think it's a good idea.

catch’s picture

To avoid duplicate content SEO issues there should be an option to HTTP redirect from the entity URI to the contextual URI

Rather than that, we should use rel=canonical - that's already used for the comment/$cid#comment-$cid version (to point to node/n) iirc.

andypost’s picture

Issue summary: View changes

Talked with @klausi in IRC and he said that REST currently does not use render system so it does not matter what is rendered at 'comment/{id}' so proposed solution is:
1) use /comment/{id} for entity uri
2) use some other method $comment->contentualURI() for threaded list

@jibran, @larowlan are you agree?

sun’s picture

Title: What is the difference between $comment->uri() and $comment->permalink() methods? » Rename Comment::permalink() method to not be ambiguous with ::uri()

Adjusting issue title for recent comments/conclusions.

I would think the uri() method can stay as-is — it shouldn't hurt anyone if it contains a fragment?

permalink() is indeed ambiguous with uri(). That said, all themes in core are outputting a (literal) "permalink" link/anchor for each comment, so the name permalink() is at least consistent with that.

Contrary to uri(), permalink() generates a link to the comment entity within the context of a parent entity.

The current implementation hard-codes a bold assumption that the comment will actually appear on the entity view URI of the context entity — which is not necessarily the case, especially with Views in core. → The method should get the context URI injected.

The recent improvements of #2167641: EntityInterface::uri() should use route name and not path and #2153891: Add a Url value object might help us to make some more sense of this:

class Comment {
  public function uriByContext(\Drupal\Core\Url $context_uri) {
    $uri = clone $context_uri;
    $uri->setOption('fragment', 'comment-' . $this->id());
    return $uri;
  }
}
andypost’s picture

Actual trouble with this approach that comment needs own route/controller for that contextual URI to be able to calculate page.
This calculation depends on threading/per-page settings that going to change in #1920044: Move comment field settings that relate to rendering to formatter options

Suppose we need:
1) remove comment_uri() see #2010202-14: Deprecate comment_uri()
2) add new CommentInterface::uriByContext()
3) use 1-2 properly

larowlan’s picture

Issue tags: +Novice
andypost’s picture

Probably "novice" needs better IS

jcnventura’s picture

Issue tags: +drupaldevdays

I'm working on this one in DDD2015

jcnventura’s picture

Status: Active » Fixed
FileSize
19.34 KB

I might have gone overboard on renaming every instance of the term 'Permalink' in Drupal to 'Contextual URI'. If the objective is to change only the method I can also provide a patch with that subset.

jcnventura’s picture

Status: Fixed » Needs review

Mouse wheel malfunction.

jibran’s picture

Status: Needs review » Needs work

We need a beta eval here.

jcnventura’s picture

Title: Rename Comment::permalink() method to not be ambiguous with ::uri() » Deprecate Comment::permalink() method and rename it to not be ambiguous with ::uri()
Issue summary: View changes
FileSize
8.09 KB
13.72 KB

In preparation to the beta eval, I'm changing the patch to deprecate permalink instead of renaming it. We can then remove it later in 9.x. I've also realised that the isPermalink attribute in RSS is part of the guid attributes and should keep using the 'permalink' terminology.

Currently the patch does three things:

  1. Rename use of Comment::permalink() to Comment::contextualUri() (and CommentController::commentPermalinkTitle and CommentController::commentPermalink) in
    • core/modules/comment/comment.routing.yml
    • core/modules/comment/src/CommentInterface.php
    • core/modules/comment/src/Controller/CommentController.php
    • core/modules/comment/src/Entity/Comment.php
    • core/modules/comment/src/Form/CommentAdminOverview.php
  2. Rename the permalink and parent_permalink theme variables and in
    • core/modules/comment/comment.module
    • core/modules/comment/templates/comment.html.twig
    • core/themes/bartik/templates/comment.html.twig
    • core/themes/classy/templates/content/comment.html.twig
  3. Rename the comment__permalink CSS class in
    • core/themes/bartik/css/components/comments.css
    • core/themes/bartik/templates/comment.html.twig

The last two can easily be removed from the patch, if these changes are not beta-compliant. Unfortunately, I don't think that marking them deprecated would work in this case.

jcnventura’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -drupaldevdays
andypost’s picture

Issue tags: +drupaldevdays

@jcnventura thanx for taking that!

Markup for comments is still flux, see #2031883: Markup for: comment.html.twig and related

The main question for me is what to do with

/**
 * Entity URI callback.
 */
function comment_uri(CommentInterface $comment) {
  return new Url(
    'entity.comment.canonical',
    array(
      'comment' => $comment->id(),
    ),
    array('fragment' => 'comment-' . $comment->id())
  );
}

I think it should be removed and that should be done is one issue that brings sanity to comment routing!

Maybe better to override some method in entity link templates land to add the fragment to context?

Status: Needs review » Needs work

The last submitted patch, 28: 2113323-rename_all_permalink-28.patch, failed testing.

heddn’s picture

Issue tags: -Novice

Removing novice tag. Anything with discussion as the next step is not really a good novice.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
13.63 KB

This is a re-roll of #28.

Status: Needs review » Needs work

The last submitted patch, 34: deprecate-2113323-34.patch, failed testing.

Status: Needs work » Needs review

jibran queued 34: deprecate-2113323-34.patch for re-testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

SchnWalter’s picture

FileSize
13.8 KB

I've re-rolled the patch against the latest 8.2.x, with a couple of fixes:

diff --git a/core/modules/comment/src/Form/CommentAdminOverview.php b/core/modules/comment/src/Form/CommentAdminOverview.php
index 97515b2..301af76 100644
--- a/core/modules/comment/src/Form/CommentAdminOverview.php
+++ b/core/modules/comment/src/Form/CommentAdminOverview.php
@@ -182,9 +182,9 @@ public function buildForm(array $form, FormStateInterface $form_state, $type = '
     foreach ($comments as $comment) {
       /** @var $commented_entity \Drupal\Core\Entity\EntityInterface */
       $commented_entity = $commented_entities[$comment->getCommentedEntityTypeId()][$comment->getCommentedEntityId()];
-      $comment_permalink = $comment->permalink();
+      $comment_contextual_uri = $comment->contextualUri();
       if ($comment->hasField('comment_body') && ($body = $comment->get('comment_body')->value)) {
-        $attributes = $comment_permalink->getOption('attributes') ?: array();
+        $attributes = $comment_contextual_uri->getOption('attributes') ?: array();
         $attributes += array('title' => Unicode::truncate($body, 128));
         $comment_permalink->setOption('attributes', $attributes);
       }

The last line should have been $comment_contextual_uri->setOption('attributes', $attributes);. Fixed it in the re-roll.

diff --git a/core/modules/comment/src/Controller/CommentController.php b/core/modules/comment/src/Controller/CommentController.php
index e52e3b9..a542d86 100644
--- a/core/modules/comment/src/Controller/CommentController.php
+++ b/core/modules/comment/src/Controller/CommentController.php
@@ -85,16 +85,16 @@ public static function create(ContainerInterface $container) {
    *   A comment entity.
    *
    * @return \Symfony\Component\HttpFoundation\RedirectResponse.
-   *   Redirects to the permalink URL for this comment.
+   *   Redirects to the contextual URI for this comment.
    */
   public function commentApprove(CommentInterface $comment) {
     $comment->setPublished(TRUE);
     $comment->save();

The \Drupal\comment\Controller\CommentController::commentApprove() didn't have the "Redirects to the permalink URL for this comment", so the re-roll just adds that comment. I'm wondering if it has been removed on purpose since the last re-roll of this patch?

Status: Needs review » Needs work

The last submitted patch, 39: deprecate-2113323-39.patch, failed testing.

SchnWalter’s picture

Fixed failing test by reverted the text from "Parent contextual URI" to "Parent permalink", and from "Contextual URI" to "Permalink" and printing the proper variable.

And made a couple more changes:

  1. The 'comment__contextual__uri' class should be 'comment__contextual_uri'.
  2. Removed usage of deprecated `\Drupal\comment\CommentInterface::permalink()` method.

NOTE: The parent_contextual_uri is not rendered in the comment template and it doesn't have tests.

SchnWalter’s picture

Status: Needs work » Needs review
catch’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -654,16 +654,16 @@ function template_preprocess_comment(&$variables) {
     $variables['title'] = \Drupal::l($comment->getSubject(), new Url('<front>'));
-    $variables['permalink'] = \Drupal::l(t('Permalink'), new Url('<front>'));
+    $variables['contextual_uri'] = \Drupal::l(t('Permalink'), new Url('<front>'));
   }

This will break any custom/contrib templates relying on permalink. Should Stable add back the variable?

SchnWalter’s picture

Yes, and I also realized that there are no tests are running against the stable.theme for the permalink and probably other things. Which I believe is very bad.

I'll provide an updated patch soon.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Version: 8.6.x-dev » 9.0.x-dev
xjm’s picture

Version: 9.0.x-dev » 9.1.x-dev

New deprecations are minor-only changes. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Title: Deprecate Comment::permalink() method and rename it to not be ambiguous with ::uri() » Rename Comment::permalink() to not be ambiguous with ::uri()