CommentLinkBuilder causes fatal error when comment field not named 'comment'

Yep, looks like a good fix :) Now just for the test coverage!

Thanks

Thank you @mikeryan

If that's the case, this should be Migrate critical.

This seems like a sensible RC target. Looking at the code I think we should remove the default value and fix this everywhere it is called.

I think this is a real critical - we need to support comment migration in Drupal 8 and without this apparently it is very broken. Therefore this is critical and not just an "rc target".

Well once the default is removed hopefully existing test coverage is around to show we've broken nothing.

Looking at \Drupal\comment\Controller\CommentController::renderNewCommentsNodeLinks this should indeed also pass along the fieldname ... maybe it was even a bad decision to make the field name optional in the first place?

+++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
@@ -161,12 +161,25 @@ public function preRender(&$values) {
+      // Identify the "first" comment field available.
+      $entity_manager = \Drupal::entityManager();
+      $field_map = $entity_manager->getFieldMapByFieldType('comment');
+      $comment_field_name = 'comment';
+      foreach ($field_map['node'] as $field_name => $field_data) {
+        foreach ($field_data['bundles'] as $bundle_name) {
+          if ($node_type == $bundle_name) {
+            $comment_field_name = $field_name;
+            break 2;
+          }
+        }
+      }

I found this bit of code a bit confusing, so I think the code comments could use a bit of the info/reasoning from #19:

What I've done in this case is simply look for the first comment field on the node bundle, which will work for 99.99% of cases. Given that the handler doesn't as far as I can see provide a way to select which comment field you want the "New comments" field to reference, that seems to be the best we can do here. I kind of feel that multiple comment fields is an area that needs significant attention if we really want to support it, but this isn't the time or place.

and possibly even a follow-up issue/@todo here?

+++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
@@ -161,12 +161,26 @@ public function preRender(&$values) {
+      // Because there is no support for selecting a specific comment field to
+      // reference, we arbitrarily use the first such field name we find.
+      $entity_manager = \Drupal::entityManager();
+      $field_map = $entity_manager->getFieldMapByFieldType('comment');
+      $comment_field_name = 'comment';
+      foreach ($field_map['node'] as $field_name => $field_data) {
+        foreach ($field_data['bundles'] as $bundle_name) {
+          if ($node_type == $bundle_name) {
+            $comment_field_name = $field_name;
+            break 2;
+          }
+        }
+      }
+      $page_number = $entity_manager->getStorage('comment')
+        ->getNewCommentPageNumber($this->getValue($values, 'comment_count'), $this->getValue($values), $node, $comment_field_name);

What about adding a todo + a follow up to make that configurable? I'm curios whether we need a test for this particular bit.

I'm a bit confused by the issue summary.

Whether the subject displays or not would be a form mode setting, I'm not seeing why we'd need a different field name to represent that?

However if it's possible to configure an entity to have a comment field not named comment (and no comment field named comment), and this produces fatal errors, that seems critical by itself - especially with the API change to comment storage.

noting the api change in the issue summary.

tried to correct the rc target triage summary... but not sure if
"Without this fix, people doing Drupal 6 sites (with comments) upgrades to Drupal 8 minimal profile will end up with a broken D8 site."
is accurate.

-----------------

reviewed this, looked at the whole patch, read each change carefully.
looked at it in context to see if the @param needed to change to say not optional (and it already did not say optional), so no change needed to the @param.

+++ b/core/modules/comment/src/Tests/CommentFieldsTest.php
@@ -95,6 +95,27 @@ public function testCommentFieldDelete() {
+   * Tests link building with non-default comment field names.
+   */
+  public function testCommentFieldLinksNonDefaultName() {
...
+    $this->postComment($node, 'Here is a comment', '', NULL, 'comment2');
+    $context = ['view_mode' => 'teaser'];
+    \Drupal::service('comment.link_builder')->buildCommentedEntityLinks($node, $context);
+  }

this tests it, in that it is making a call, and the test passes if there is no fatal error, correct?

maybe to be a bit more robust, we should actually assert something.

---------

The change record looks fine.

---------

fixed a spacing thing with the @todo. https://www.drupal.org/node/1354#todo

+++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
@@ -161,12 +161,28 @@ public function preRender(&$values) {
+      // Because there is no support for selecting a specific comment field to
+      // reference, we arbitrarily use the first such field name we find.

does the subject get migrated into a comment field? If so, would the subject end up being the first?

maybe to be a bit more robust, we should actually assert something.

Agreed, we should assert the result of

+++ b/core/modules/comment/src/Tests/CommentFieldsTest.php
@@ -95,6 +95,27 @@ public function testCommentFieldDelete() {
+    \Drupal::service('comment.link_builder')->buildCommentedEntityLinks($node, $context);

+++ b/core/modules/comment/src/Tests/CommentFieldsTest.php
@@ -95,6 +95,30 @@ public function testCommentFieldDelete() {
+    $link_info = $links['comment__comment2']['#attached']['drupalSettings']['comment']['newCommentsLinks']['node']['comment2']['2'];

Note the comment2 in there.

+++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
@@ -161,12 +161,28 @@ public function preRender(&$values) {
+      $field_map = $entity_manager->getFieldMapByFieldType('comment');

Does this have a performance cost?

The field map list is cached based on information in state. We never have to load all the fields to collect the field map cache anymore. So that shouldn't be a problem I think.

1. +++ b/core/modules/comment/src/Tests/CommentFieldsTest.php
   @@ -95,6 +95,30 @@ public function testCommentFieldDelete() {
   +    $link_info = $links['comment__comment2']['#attached']['drupalSettings']['comment']['newCommentsLinks']['node']['comment2']['2'];
   

   If you can get a page with this in drupalSettings you can use $this->getDrupalSettings() instead.

2. +++ b/core/modules/comment/src/Tests/CommentFieldsTest.php
   @@ -95,6 +95,30 @@ public function testCommentFieldDelete() {
   +    $this->assertIdentical($link_info['first_new_comment_link'], '/node/2#new');
   

   The fail is due to the fact that the testbot runs drupal in a sub directory. So this assert needs to be adjusted to work in both types of environment.

@lokapujya: The issue here does not affect the migration process at all - migration of comments does migrate subjects if they are present.

I guess that if the D6 comment field is comment_no_subject, then no subject will get migrated (because there isn't one.) The reason I brought it up is: if there is a subject and we migrate comment_no_subject and subject (and subject ends up being the first comment field in D8) then the subject will get used as the comment. So, before using the first field should we check for a comment_no_subject field?

Well, you can do pretty much just this:

$this->assertIdentical($link_info['first_new_comment_link'], $node->url('canonical', ['fragment' => 'new']));

Just took the time to do a proper test for the drupalGetSettings

1. +++ b/core/modules/comment/src/CommentStorageInterface.php
   @@ -54,7 +54,7 @@ public function getMaxThreadPerThread(CommentInterface $comment);
   -  public function getNewCommentPageNumber($total_comments, $new_comments, FieldableEntityInterface $entity, $field_name = 'comment');
   +  public function getNewCommentPageNumber($total_comments, $new_comments, FieldableEntityInterface $entity, $field_name);
   

   I think this is the correct change, thanks for whoever pushed to fix the interface.

2. +++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
   @@ -161,12 +161,28 @@ public function preRender(&$values) {
   +      $field_map = $entity_manager->getFieldMapByFieldType('comment');
   ...
   +      foreach ($field_map['node'] as $field_name => $field_data) {
   +        foreach ($field_data['bundles'] as $bundle_name) {
   ...
   +            break 2;
   

   Always fun traversing nested arrays, feels so familiar :)

The test code looks good.

Committed 83054ca and pushed to 8.0.x. Thanks!

Looks like the retest couldn't apply the patch because it was already committed.

@mikeryan: no, just publish the CR draft as soon as it's committed.

I usually just forget about it and then someone else fixes it for me, Thanks!