Problem/Motivation

Drupal 6 comments for a given node type may or may not be configured to have subjects. If they have subjects, the usual 'comment' field name is used for them; if not, the field name 'comment_no_subject' is used. After performing a Drupal 6 (minimal profile) site upgrade to Drupal 8 in the latter case, loading the home page dies horribly:

( ! ) Fatal error: Call to a member function getSetting() on a non-object in /Users/mryan/Sites/d8/core/modules/comment/src/CommentStorage.php on line 140
Call Stack
#	Time	Memory	Function	Location
1	0.0062	134040	{main}( )	.../index.php:0
2	0.0412	1201264	Drupal\Core\DrupalKernel->handle( )	.../index.php:19
...
54	5.2376	36812624	comment_node_links_alter( )	.../ModuleHandler.php:492
55	5.2405	36853008	Drupal\comment\CommentLinkBuilder->buildCommentedEntityLinks( )	.../comment.module:204
56	5.2580	37016292	Drupal\comment\CommentStorage->getNewCommentPageNumber( )	.../CommentLinkBuilder.php:211

The reason is quite simple - buildCommentedEntityLinks() calls getNewCommentPageNumber() without passing the comment field name, and getNewCommentPageNumber() defaults it to 'comment', which does not exist in this case.

Proposed resolution

Pass the field name.

Remaining tasks

Write tests.

User interface changes

None.

API changes

comment field name no longer optional.

+++ 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);

Data model changes

None.

RC target triage

Without this fix, people doing Drupal 6 sites (with comments) upgrades to Drupal 8 minimal profile will end up with a broken D8 site.

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
850 bytes
Wim Leers’s picture

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

larowlan’s picture

Thanks

mikeryan’s picture

mikeryan’s picture

Ah, why do I only see copypasta cruft after uploading a patch...

The last submitted patch, 5: commentlinkbuilder-2588547-5-FAIL.patch, failed testing.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @mikeryan

The last submitted patch, 5: commentlinkbuilder-2588547-5-FAIL.patch, failed testing.

mikeryan’s picture

Issue tags: +rc target triage

We really want to get this in during RC - without this fix, most if not all people doing Drupal 6 upgrades for sites with comments will end up with a broken D8 site.

phenaproxima’s picture

Issue tags: +Migrate critical

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

mikeryan’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -rc target triage +rc target

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.

alexpott’s picture

Priority: Major » Critical
Issue tags: -rc target

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".

mikeryan’s picture

OK, I'll track down any other instances and fix them.

mikeryan’s picture

Do we need tests for every instance where getNewCommentPageNumber() is called?

Most of the calls have a handy comment field name to pass, views' NodeNewComments, not so much, working on that...

alexpott’s picture

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

dawehner’s picture

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?

mikeryan’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
7.9 KB
5.19 KB

OK, here it is - as I said, simple in most cases, but the NodeNewComments views field handler doesn't have a convenient comment field name. 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.

Oh, and fun fact - adding a "New comments" field to your view will blow up if the history module isn't enabled, given that it joins to the history table. I'll see if there's an existing issue for that and open one up if necessary.

mikeryan’s picture

mikeryan’s picture

Issue tags: -Needs change record

Draft change record added.

jhedstrom’s picture

+++ 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?

mikeryan’s picture

Fleshed out the comment some to make the motivation for this approach clear.

I think this particular spot is a little narrow for an @todo - dealing with multiple comment fields is generally problematic and probably could use a meta.

mikeryan’s picture

mikeryan’s picture

dawehner’s picture

+++ 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.

catch’s picture

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.

mikeryan’s picture

@todo added.

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?

One Drupal 6 node type might have had subjects turned on for its comments, while another might not have. But... I see what you're saying, the form mode setting is on the entity, not on the field, so we could have used the same field name regardless? It seems to have to do with the widget stuff in #2292821: Use widget for comment subject field, it's not clear to me if the distinct field name is technically necessary. Not really something I want to be changing at this stage anyway.

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.

Through the UI, add a new comment field with label "Comment" and you get a field named field_comment. If no comment field exists named 'comment', you'll get fatals. So, when I said "most if not all people doing Drupal 6 upgrades for sites with comments will end up with a broken D8 site", that was a bit of an exaggeration - if they start from the standard profile, which creates a field named 'comment', they won't fatal, but they will get incorrect results because getNewCommentPageNumber() will be looking at the wrong field. The reason I only discovered this now is I'm writing a couple of blog posts on performing D8 upgrades and I'm recommending using the minimal profile for the blankest slate to write on, and minimal does not create any comment fields (or even enable the module) - up to now I've been doing my own testing using the standard profile.

YesCT’s picture

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

lokapujya’s picture

+++ 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?

Wim Leers’s picture

Status: Needs review » Needs work

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);
mikeryan’s picture

Now with assertions! Only 9 levels deep into the array, hardly a Drupal record...

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: commentlinkbuilder-2588547-32.patch, failed testing.

catch’s picture

+++ 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 last submitted patch, 5: commentlinkbuilder-2588547-5-FAIL.patch, failed testing.

Berdir’s picture

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.

alexpott’s picture

  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.

The last submitted patch, 32: commentlinkbuilder-2588547-32.patch, failed testing.

mikeryan’s picture

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

Is there an advantage to this? A first pass of $this->drupalGet('node') didn't get what I needed from getDrupalSettings(), I think this would require some theme setup etc...

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.

What's the best way to do this? Figure out the subdirectory and prepend it to the expected string? Or can I simply look for '/node/2#new' as a suffix?

Thanks.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
8.72 KB
1.04 KB

alexpott gave me a hint on IRC on how to construct the URL, works in my environment, let's see how it does in Botland...

lokapujya’s picture

@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?

mikeryan’s picture

@lokapujya: When migrating from Drupal 6, the resulting nodes will have one comment field, which will be named either 'comment' or 'comment_no_subject'. The "pick-the-first-comment-field" part of the above patch really has nothing whatsoever to do with migration - it has to do with creating a node view where the node has multiple comment fields, and you are adding the "New comments" views field to the view. Because Views does not provide a way to select which of those comment fields to link to (see #2594201: No choice of comment field to use for "New comments" view field), the patch here picks one arbitrarily.

dawehner’s picture

Well, you can do pretty much just this:

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

Issue tags: +D8 Accelerate
FileSize
9.14 KB
2.64 KB

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
  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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 83054ca on 8.0.x
    Issue #2588547 by mikeryan, dawehner, YesCT: CommentLinkBuilder causes...

Status: Fixed » Needs work

The last submitted patch, 45: 2588547-45.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Fixed

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

The last submitted patch, 5: commentlinkbuilder-2588547-5-FAIL.patch, failed testing.

mikeryan’s picture

Thanks! Is there a "Needs change record published" tag;-)?

Wim Leers’s picture

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

mikeryan’s picture

OK, done, hadn't realized that was up to me...

dawehner’s picture

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

Status: Fixed » Closed (fixed)

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