Problem/Motivation

#731724: Convert comment settings into a field to make them work with CMI and non-node entities Will decouple comments from nodes. Support for comment tokens relating to nodes will be retained in D8 for BC but can be removed in D9. This is the issue to remove them.

Proposed resolution

Remove support for node tokens in comment.tokens.inc

Remaining tasks

Write patch, review.

User interface changes

None

API changes

Node tokens no longer supported for comments.

#731724: Convert comment settings into a field to make them work with CMI and non-node entities

CommentFileSizeAuthor
#11 2031901-node-tokens-11.patch5.38 KBandypost
#5 2031901-node-tokens-5.patch5.4 KBandypost
#3 2031901-node-tokens-3.patch3.19 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Component: forum.module » comment.module

Whoops

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 9.x-dev » 8.0.x-dev
Issue summary: View changes

This seems like we'll run into it whichever version they're taken out of. Given migrate and that specific tokens aren't much of an API, moving back to 8.x at least for now.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Needs review
FileSize
2031901-node-tokens-3.patch3.19 KB

Let's see a test coverage.
This [node] tokens could be provided by deprecated contrib module

Status: Needs review » Needs work

The last submitted patch, 3: 2031901-node-tokens-3.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
Related issues: +#920056: [comment:name] duplicates [comment:author], and the latter should use format_username()
FileSize
2031901-node-tokens-5.patch5.4 KB

Fixed patch - added test for tokens that replaces current functionality
CR https://www.drupal.org/node/2371365

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented 
+++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
@@ -63,8 +63,10 @@ function testCommentTokenReplacement() {
+    $tests['[comment:entity:nid]'] = $comment->getCommentedEntityId();

Shouldn't this be id, not nid?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

NO, :id would work only for entities that have ID as key, that's why I extended a test

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue tags: +@deprecated
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community

ah, makes sense - thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2031901-node-tokens-5.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
2031901-node-tokens-11.patch5.38 KB

re-roll

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community

back we go

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
@@ -62,8 +62,10 @@ function testCommentTokenReplacement() {
-    $tests['[comment:node:nid]'] = $comment->getCommentedEntityId();
-    $tests['[comment:node:title]'] = String::checkPlain($node->getTitle());
+    $tests['[comment:entity]'] = String::checkPlain($node->getTitle());
+    // Test node specific tokens.
+    $tests['[comment:entity:nid]'] = $comment->getCommentedEntityId();
+    $tests['[comment:entity:title]'] = String::checkPlain($node->getTitle());

These test changes look odd to me, why would we still test comment:entity:nid or comment:entity:title and support that?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Reviewed & tested by the community

I've added this tests because there's no coverage for [comment:entity:*] tokens and to make sure that removed functionality works by the new way

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review

Why nid and title rather than id and label though?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community

in #7 I said that there's no such tokens - each token is Real entity field (name)

PS: filed new issue #2372465: Add unified entity tokens for ID and Label

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Explanation and follow-up both good.

Committed/pushed to 8.0.x, thanks!

  • catch committed 26a0068 on 8.0.x 
    Issue #2031901 by andypost: Remove node tokens from comment.tokens.inc.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.