CommentLazyBuilders should be language aware

Interesting catch, btw maybe language should be passed as argument?

Thanks, then let's keep it consistent!

NW for tests, probably node already have examples

Thanks, the patch looks good now.

+++ b/core/modules/comment/src/CommentLazyBuilders.php
@@ -144,6 +144,7 @@ public function renderLinks($comment_entity_id, $view_mode, $langcode, $is_in_pr
       $entity = $this->entityTypeManager->getStorage('comment')->load($comment_entity_id);
+      $entity = $entity->getTranslation($langcode);
       $commented_entity = $entity->getCommentedEntity();

when translation is missing (whatever reason) this code will throw exception now

For #13 can we add a try/catch there to be safe?

We have unit-test coverage of that class - can we extend CommentLinkBuilderTest for these cases too?

+++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
@@ -233,4 +233,64 @@ protected function doTestTranslationEdit() {
+      foreach ($links as $label => $expected_href) {
+        $this->assertSession()->elementExists('xpath', "//article[@id='comment-$comment_id']//a[text()='$label']");

One nitpick/micro-optimization:

To avoid calling $this->assert Session() multiple times in the loop. Every call returns a new instance.

   * @return \Drupal\Tests\WebAssert
   *   A new web-assert option for asserting the presence of elements with.
   */
  abstract public function assertSession($name = NULL);

Suggestion:

      $assert_session = $this->assertSession();
      foreach ($links as $label => $expected_href) {
        $assert_session->elementExists('xpath', "//article[@id='comment-$comment_id']//a[text()='$label']");

Changing the target version to 9.1.x.

Hi @jungle
Made changes as you suggested please review.

Thanks @matsbla for the new patch. Would be great to attach an interdiff when possible. See https://www.drupal.org/documentation/git/interdiff

@matsbla, I have tested patch in the comment #20. Patch LGTM just one minor change.

+++ b/core/modules/comment/src/CommentLazyBuilders.php
@@ -137,6 +137,11 @@ public function renderLinks($comment_entity_id, $view_mode, $langcode, $is_in_pr
+      }

Please add the description in the catch block. Say "Invalid translation language ($langcode) specified." or similarly something more descriptive.

      catch (\InvalidArgumentException $exception) {
        // Invalid translation language ($langcode) specified.
      }

Assigning for review.

Tested patch in comment #27. Local task link for comments points to respective translation if present. It LGMT.

1. There's another issue about multi-lingual comments - ah yes it is linked in the issue summary - #2751269-57: Submitting a comment in the non default language redirects you to the default language - which ponders how multilingual comments are supposed to work. I think that we need to do some thinking about what is supposed to happen in the different set-ups:
   • Node has content translation enabled but comment does not
   • Comment has content translation enabled but node does not
   • Node and comment both have content translation enabled

   But I'm not sure that that should block the work here as this change is inline with how entity translations are supposed to work. It's just more evidence that multilingual comments are not well supported or thought about from a core perspective.

2. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +238,65 @@ protected function doTestTranslationEdit() {
   +      $translation = $comment->addTranslation($langcode, $values[$langcode]);
   

   No need to assign $translation here. It is never used.

3. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +238,65 @@ protected function doTestTranslationEdit() {
   +    // Need to check from the beginning, including the base_path, in the url
   +    // since the pattern for the default language might be a substring of
   +    // the strings for other languages.
   +    $base_path = base_path();
   

   Can be removed see below.

4. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +238,65 @@ protected function doTestTranslationEdit() {
   +      $this->drupalGet("comment/$comment_id", ['language' => \Drupal::languageManager()->getLanguage($langcode)]);
   +      $links = [];
   +      if ($langcode == $default_langcode) {
   +        // Site default language does not have langcode prefix in the URL.
   +        $links = [
   +          'Edit' => $base_path . 'comment/' . $comment_id . '/edit',
   +          'Delete' => $base_path . 'comment/' . $comment_id . '/delete',
   +          'Translate' => $base_path . 'comment/' . $comment_id . '/translations',
   +          'Reply' => $base_path . 'comment/reply/node/' . $node_id . '/comment_article/' . $comment_id,
   +          'Approve' => $base_path . 'comment/' . $comment_id . '/approve',
   +        ];
   +      }
   +      else {
   +        $links = [
   +          'Edit' => $base_path . $langcode . '/comment/' . $comment_id . '/edit',
   +          'Delete' => $base_path . $langcode . '/comment/' . $comment_id . '/delete',
   +          'Translate' => $base_path . $langcode . '/comment/' . $comment_id . '/translations',
   +          'Reply' => $base_path . $langcode . '/comment/reply/node/' . $node_id . '/comment_article/' . $comment_id,
   +          'Approve' => $base_path . $langcode . '/comment/' . $comment_id . '/approve',
   +        ];
   +      }
   

   This could be written as

         $options = ['language' => \Drupal::languageManager()->getLanguage($langcode)];
         $this->drupalGet("comment/$comment_id", $options);
         $links = [
           'Edit' => $comment->toUrl('edit-form', $options)->toString(),
           'Delete' => $comment->toUrl('delete-form', $options)->toString(),
           'Translate' => $comment->toUrl('drupal:content-translation-overview', $options)->toString(),
           'Reply' => Url::fromRoute('comment.reply', ['entity_type' => 'node', 'entity' => $node_id, 'field_name' => 'comment_article', 'pid' => $comment_id], $options)->toString(),
           'Approve' => Url::fromRoute('comment.approve' , ['comment' => $comment_id], $options)->toString(),
         ];
         // Strip the CSRF token.
         $links['Approve'] = substr($links['Approve'], 0, strpos($links['Approve'], 'token='));
   

   And then we can remove the base_path stuff above -I think this is a bit neater.

5. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +238,65 @@ protected function doTestTranslationEdit() {
   +      $assert_session = $this->assertSession();
   +      foreach ($links as $label => $expected_href) {
   +        $assert_session->elementExists('xpath', "//article[@id='comment-$comment_id']//a[text()='$label']");
   +        $pattern = '|^' . $expected_href . '.*$|';
   +        $num_match_found = 0;
   +        foreach ($this->xpath("//article[@id='comment-$comment_id']//a[text()='$label']") as $link) {
   +          if (preg_match($pattern, $link->getAttribute('href'), $matches) == TRUE) {
   +            $num_match_found++;
   +          }
   +        }
   +        $this->assertTrue($num_match_found == 1, 'When you view the comment there is 1 ' . $label . ' link, ' . $expected_href . ', for the ' . $langcode . ' translation. (Found ' . $num_match_found . '.)');
   +      }
   

   These assertions can be written like:

         foreach ($links as $label => $expected_href) {
           $links = $this->assertSession()->elementExists('xpath', "//article[@id='comment-$comment_id']")
             ->findAll('xpath', "//a[text()='$label']");
           $this->assertCount(1, $links);
           $this->assertRegExp('|^' . $expected_href . '.*$|', $links[0]->getAttribute('href'));
         }
   

   Using phpunit's more specific assertions over assertTrue is generally preferred.

   And note it's no longer necessary to do $assert_session = $this->assertSession();

According to the comment #31, patch added for point 2, 3, 4 and 5.

Please review.

Thanks & Regards
Manisha Bhawsar

Solving failed test cases.

Thanks everyone, there is getting there.

I started my review by reading the Issue Summary. It is nice to see there are steps to reproduce. They could be improved by starting from the install of Drupal whereas this starts from an existing comment type. If I were to test this my first question would be, what profile was used for the install? Next I was looking for some idea of what the fix is. That is not explained nor is there a 'proposed resolution' section. Also missing is 'remaining tasks', so as a reviewer I need to start from scratch. Please use dreditor and insert the issue summary template and complete it. Tagging needs issue summary update.

I have read through the issue and found that #15.1 is still to do. Setting NW for that.

The test creates two translations, 'it' and 'fr' and then tests rendering those translations. Without the fix the test fails because it has not loaded the 'it' translation. That is good to see. What I don't see is a test of case where the translation does not exist. A test for that needs to be added. Adding 'needs tests' for that.

And I have some of my own questions/comments.

1. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -6,6 +6,7 @@
   +use Drupal\Core\Url;
   

   Please add in alphabetical order as much is possible.

2. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +239,45 @@ protected function doTestTranslationEdit() {
   +    // Check that each of the local tasks links to correct translation.
   

   This is a tough read. Should it be 'Check that each local task links to the correct translation'?

3. +++ b/core/modules/comment/src/CommentLazyBuilders.php
   @@ -137,6 +137,12 @@ public function renderLinks($comment_entity_id, $view_mode, $langcode, $is_in_pr
   +      catch (\InvalidArgumentException $e) {
   +        // Invalid translation language ($langcode) specified.
   

   Feels like this comment needs more information and $langcode is redundant. Maybe 'Ignore invalid translation language because ...'

4. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +239,45 @@ protected function doTestTranslationEdit() {
   +      $links = [];
   

   Can be removed. $links is set in the third line below.

I looks great! Just minor nitpics

Please file separate test-only.patch to prove that test catching the bug

1. +++ b/core/modules/comment/src/CommentLazyBuilders.php
   @@ -137,6 +137,12 @@ public function renderLinks($comment_entity_id, $view_mode, $langcode, $is_in_pr
   +      try {
   +        $entity = $entity->getTranslation($langcode);
   +      }
   

   Maybe if ($entity->hasTranslation($langcode)) {..} will be less expensive instead of exception

2. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +239,49 @@ protected function doTestTranslationEdit() {
   +      // Strip the CSRF token.
   +      $links['Approve'] = substr($links['Approve'], 0, strpos($links['Approve'], 'token='));
   

   Should tell why token is stripped

Known test failure, btw test-only should be uploaded before fixed one to not change status on failure

Rewrite for 9.1.x

This patch needs to be rerolled.

In #31, @alexpott mentioned the work going on in #2751269, that issue is still going on so I think we can still get this in when it is rerolled.

Rerolled patch #54 for 9.5.x, please review it.

Trying to fix failed tests of patch #60.

+++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
@@ -234,4 +235,49 @@ protected function doTestTranslationEdit() {
+      if (!$comment->hasTranslation($langcode)) {
+        $options = ['language' => \Drupal::languageManager()->getLanguage($default_langcode)];
+      }

I think we should also assert that $options['language'] doesn't equal the default langcode for all except $default_langcode and $not_translated_langcode

Re-roll the patch For 10.1

Rolled #65 into an MR, the test was failing on missing Reply links. I've removed that for now assuming that something has changed and this isn't expected anymore. #64 still needs actioning.