Problem/Motivation

Comments are multilingual but no token provided

Proposed resolution

Add "langcode" token like node.tokens.inc does

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because adding "langcode" token is a feature that good to have and can't be considered as a bug because without the token the system doesn't break.
Issue priority Normal Task: This is nice to have but doesn't impact the system if it's not present.
Prioritized changes Making this issue as a prioritized change as it improves usability and user experience improvements by providing a new token to the system.
CommentFileSizeAuthor
#24 langcode-token-to-comment-2378565-24.patch2.64 KBadci_contributor
#22 langcode-token-to-comment-2378565-22.patch2.64 KBadci_contributor
#12 langcode-token-to-comment-2378565-12.patch2.64 KBaneek
#12 interdiff-2378565-8-12.txt797 bytesaneek
#8 langcode-token-to-comment-2378565-8.patch2.15 KBaneek
#8 langcode-token-to-comment-2378565-test-only.patch1019 bytesaneek
#8 interdiff-2378565-5-8.txt2.36 KBaneek
#5 2378565-5.patch1.96 KBrpayanm
#5 2378565-interdiff.txt568 bytesrpayanm
#1 2378565-1.patch1.96 KBrpayanm

Comments

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Active » Needs review
StatusFileSize
new 2378565-1.patch1.96 KB

Trying...

Status: Needs review » Needs work

The last submitted patch, 1: 2378565-1.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented

A lot of unnecessary changes

+++ b/core/modules/comment/comment.tokens.inc
@@ -53,6 +54,10 @@ function comment_token_info() {
+  $node['langcode'] = array(

should be a $comment

andypost’s picture

andypost
he/him
Primary language Russian
commented

A lot of unnecessary changes

+++ b/core/modules/comment/comment.tokens.inc
@@ -53,6 +54,10 @@ function comment_token_info() {
+  $node['langcode'] = array(

should be a $comment

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new 2378565-interdiff.txt568 bytes
new 2378565-5.patch1.96 KB

Oh yes! :)

Status: Needs review » Needs work

The last submitted patch, 5: 2378565-5.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented

yes, also needs extend tests

+++ b/core/modules/comment/comment.tokens.inc
@@ -110,7 +115,7 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
+    $langcode = LanguageInterface::LANGCODE_DEFAULT;

@@ -146,7 +151,17 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
+          $translation = \Drupal::entityManager()->getTranslationFromContext($comment, $langcode, array('operation' => 'comment_tokens'));

unneeded change, addressed in own issue (see related issue)

aneek’s picture

aneek commented
StatusFileSize
new interdiff-2378565-5-8.txt2.36 KB
new langcode-token-to-comment-2378565-test-only.patch1019 bytes
new langcode-token-to-comment-2378565-8.patch2.15 KB

@rpayanm,
Removed not needed code from your patch and added a check in the test "CommentTokenReplaceTest".

Review!!

aneek’s picture

aneek commented
Status: Needs work » Needs review

The last submitted patch, 8: langcode-token-to-comment-2378565-test-only.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented

2 nitpicks, suppose there's no need in sanitization

  1. +++ b/core/modules/comment/comment.tokens.inc
@@ -149,6 +153,10 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
+          $replacements[$original] = $sanitize ? String::checkPlain($comment->language()->getId()) : $comment->language()->getId();

    not sure we need to sanitize the machine name 

  2. +++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
@@ -57,6 +57,7 @@ function testCommentTokenReplacement() {
+    $tests['[comment:langcode]'] = String::checkPlain($comment->language()->getId());

    There's unsanitized test case around, maybe extend it too?

aneek’s picture

aneek commented
StatusFileSize
new interdiff-2378565-8-12.txt797 bytes
new langcode-token-to-comment-2378565-12.patch2.64 KB

@andypost, I'd like to have the String::checkPlain(). I think while creating the patch @rpayanm followed the same pattern from node.token.inc. So it also has the checkplain check for some reason (may be).
But the "unsanitized test case" is added in this patch.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I'm fine with current implementation

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

aneek’s picture

aneek commented
Issue summary: View changes
aneek’s picture

aneek commented
Assigned: Unassigned » aneek
Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: langcode-token-to-comment-2378565-12.patch, failed testing.

aneek’s picture

aneek commented
Status: Needs work » Needs review

git apply --check showed no error. Strange!! Marking for review again.

aneek queued 12: langcode-token-to-comment-2378565-12.patch for re-testing.

aneek’s picture

aneek commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: langcode-token-to-comment-2378565-12.patch, failed testing.

adci_contributor’s picture

adci_contributor commented
Status: Needs work » Needs review
StatusFileSize
new langcode-token-to-comment-2378565-22.patch2.64 KB

Rerolled after #2380023: Clean-up Comment module Test members - ensure property definition and use of camelCase naming convention

Status: Needs review » Needs work

The last submitted patch, 22: langcode-token-to-comment-2378565-22.patch, failed testing.

adci_contributor’s picture

adci_contributor commented
Status: Needs work » Needs review
StatusFileSize
new langcode-token-to-comment-2378565-24.patch2.64 KB

Again

aneek’s picture

aneek commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed b3f2319 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed b3f2319 on 8.0.x 
    Issue #2378565 by aneek, rpayanm, adci_contributor: Add langcode token...
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: -content-language +language-content

Fix tags.

aneek’s picture

aneek commented
Status: Fixed » Closed (fixed)