Rename "body" comment token to "comment_body"

re-rolling #4. Changes looks good. +1 to RTBC.

I checked the issue and in my opinion it is fine to be on RTBC

Patch re-rolled. Please pay special attention in review, since this is my first re-roll.

thanks

It all looks good to me.

How is this any different from the node body token? What makes comment different? I guess it is because the field is actually called comment_body. Hmmm.. this seems a not nice break to be making now. If someone is using the comment body token this will be a hard break.

Needs a discussion on what to do given that D8.0 was missed, and this causes a hard BC break in UX for anyone using the [body] token for comments.

Strategies:
Soft break: allow [comment_body] as a permitted token alongside [body], mark and document [body] as deprecated
Hard break: allow [comment_body], stop [body]

Options:
1) Soft-break now, hard break D8.1
2) Soft-break now, hard break D9
3) Soft break D8.1, hard break D9
4) No soft break, hard break D8.1
5) No soft break, hard break D9

Re-rolled #27 so that it applies to current HEAD.

As mentioned in #33 and #34, we need to decide when and how we can make a token change like this in our release cycle.

Some ideas on the direction forward, since this issue seems to be stalled due to how to approach deprecation:

• Trigger deprecation warnings when calling [comment:body], this shouldn't impact production code.
• Switch over existing code/testing/usage to use [comment:comment_body] (above patch does this), and add a deprecated body token.
• Leave [comment:body] token in codebase until 9.x, which is only available with a feature flag.
• Existing sites have an update path to turn on this flag. New sites don't get this flag.
• Feature flag is immediately marked as deprecated.

I think these changes would be the safest way to resolve this issue, whilst being able to do it within 8.x lifecycle.

This approach also allows sites to implement a new [comment:body] token without triggering deprecations.

Patch implements above.
Updated modified lines to fix CS issues (\t() to $this->t());

Added to acceptable deprecations.

The constant is neither here nor there, its immediately deprecated to remove for 9.x , or could easily be substituted to a plain string.

1. +++ b/core/modules/comment/comment.install
   @@ -260,3 +260,10 @@ function comment_update_8701() {
   +  \Drupal::state()->set(\COMMENT_LEGACY_BODY_TOKEN, TRUE);
   

   Yeah, I agree with @andypost, lets not introduce this at a global level

2. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -140,6 +140,7 @@ trait DeprecationListenerTrait {
   +      '[comment:body] token will no longer return value of [comment:comment_body].',
   

   We can't do this here, we have to clean up the uses as part of the patch.

The usage is only triggered by tests. I introduced the legacy behaviour for any existing usage of the token, which could be anywhere. Any advice for ignoring the warning for tests only?

Add @legacy annotation to the test if its testing legacy behaviour, this will ignore deprecations

Thanks @andypost + @larowlan for the feedback.

Addressed in attached.

Deprecations like this are hard because it's challenging to reach the people we want to tell and tell them what they need to do.

One thing is that the feature-flag is IMHO unnecessary. Deprecation messages on production are not a concern, yes it does have an overhead, but so are all others, many of which are very likely called *way* more often.

The feature flag would also break examples where this token is used in default configuration, e.g. a module like comment_notify. That would mean that it would be impossible for that module to be compatible with 8.7 and 8.8, assuming it is committed there. So that's definitely not an option.

At most, that flag could hide the token info, which currently it doesn't even do yet. And even that is problematic because with token.module, some places even validate the used tokens and that they exist.

At the same time, a @trigger_error() doesn't reach site admins that put this token anywhere in configuration or even content, because it will never show for them. I'm not really sure what to do about that, to be honest. Sometimes if it's a single configuration that we deprecate (e.g. temporary file directory to settings instead of config), we add a requirements hook to tell users that they need to update their configuration but here, we don't even know where it is used, at best we could somehow log that it was used and tell about that.

+++ b/core/modules/comment/tests/src/Functional/CommentTokenReplaceTest.php
@@ -18,6 +18,7 @@ use Drupal\user\Entity\User;
  *
  * @group comment
+ * @group legacy
  */
 class CommentTokenReplaceTest extends CommentTestBase {

The instructions here were a bit unclear.

We don't want to make existing tests that are also testing other things as @legacy, it should be a new one, e.g. CommentDeprecatedTokenReplaceTest.

Also, @legacy needs to be combined with at least one @expectedDeprecation annotation on the test method, it's a) testing that the old logic still works and b) that the specific trigger_error() also works.

+++ b/core/modules/comment/comment.tokens.inc
@@ -168,6 +172,14 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
+          }
+          // Continues to 'comment_body' token if legacy token behavior is on.
+          @trigger_error('[comment:body] token will no longer return value of [comment:comment_body].', E_USER_DEPRECATED);

Deprecation messages need to follow a specific format, example:

@trigger_error('drupal_render_root() is deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Render\RendererInterface::renderRoot() instead. See https://www.drupal.org/node/2912696', E_USER_DEPRECATED);

* deprecation in which version
* when will it be removed
* Link to change record.