I am getting this error when visiting Recent Content:

Type	php
Date	Sunday, November 22, 2015 - 23:15
Location	http://d8head:8888/comments/render_new_comments_node_links
Referrer	http://d8head:8888/activity/1
Message	Recoverable fatal error: Argument 1 passed to Drupal\comment\CommentManager::getCountNewComments() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in ~/drupal/core/modules/comment/src/Controller/CommentController.php on line 338 and defined in Drupal\comment\CommentManager->getCountNewComments() (line 195 of ~/drupal/core/modules/comment/src/CommentManager.php).
Severity	Error
Hostname	::1

Comments

Jeff Burnz created an issue. See original summary.

larowlan’s picture

What do you mean by Recent Content?

Jeff Burnz’s picture

The URL is in the error message: http://d8head:8888/activity/1 this is page titled "Recent Content" that displays a table with recently added content, you need the Activity Tracker module enabled and the link is in the Tools menu.

dawehner’s picture

Version: 8.1.x-dev » 8.0.x-dev

Bugfixes should be fixed against 8.0.x, IMHO

larowlan’s picture

Assigned: Unassigned » larowlan

digging in

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Active » Needs review
Issue tags: +JavaScript
FileSize
2.08 KB

Not sure we can test this without JavaScript testing.
This bring tracker-history.js into line with node-new-comment-links.js

larowlan’s picture

Component: comment.module » tracker.module

The bug is in tracker module code

andypost’s picture

Issue tags: +Needs manual testing

Also history module related

catch’s picture

Priority: Normal » Major
Status: Needs review » Needs work

I think the history callback should validate the arguments it gets too. Can't find it right now but there's a similar issue open for the nid being out of range.

You could manually trigger the PHP error here which isn't great (especially if load balancers take 5* errors as a sign to take nodes out of rotation).

andypost’s picture

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing +Needs tests
FileSize
776 bytes

I can't reproduce that on /activity/1 anymore
maybe something was fixed already? or that was some caching issue

Anyway something like the patch is needed with test coverage

Berdir’s picture

Status: Needs review » Needs work

Needs work for test coverage then.

Also if you convert this to do a loadMultiple($nids) as $node then you've solved the problem too. A node that can't be loaded will never go into the loop. And performance is better.

andypost’s picture

@Berdir I wanna to convert that to load multiple long ago, but each time that out of scope ;)

There's actually 3 problems here:
1) JS passed only node Ids - manager call will be broken (no idea how reproduce) so @larowlan patch makes sense but that's a front side
2) Nodes could have different comment fields (names of fields) so $node->{wrong_field_name} will throw this exception too
3) no check for access/existence of nodes (my patch) should be refactored to load multiple call.

+++ b/core/modules/tracker/js/tracker-history.js
@@ -105,7 +107,7 @@
-      data: {'node_ids[]': nodeIDs},
+      data: {'node_ids[]': nodeIDs, 'field_name': 'comment'},

request should carry field name for each node or group them

Shefarik’s picture

I think we need replace 'comment' with name of last comment field, but we have empty value last_comment_name in $node in tracker.pages.inc

andypost’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: 2620748-13.patch, failed testing.

andypost’s picture

+++ b/core/modules/comment/src/Controller/CommentController.php
@@ -325,7 +325,11 @@ public function renderNewCommentsNodeLinks(Request $request) {
     $field_name = $request->request->get('field_name');
+
     if (!isset($nids)) {
       throw new NotFoundHttpException();

+++ b/core/modules/tracker/js/tracker-history.js
@@ -105,7 +105,7 @@
+      data: {'node_ids[]': nodeIDs,'field_name': fieldName},

the field name could be different for each node and not only one

andypost’s picture

Status: Needs work » Needs review
FileSize
2 KB

The proper controller code, JS side should be fixed like #14

  1. +++ b/core/modules/tracker/js/tracker-history.js
    @@ -105,7 +105,7 @@
    -      data: {'node_ids[]': nodeIDs},
    +      data: {'node_ids[]': nodeIDs,'field_name': fieldName},
    

    pass a field_name is a requirement

  2. +++ b/core/modules/tracker/tracker.pages.inc
    @@ -104,6 +105,7 @@ function tracker_page($account = NULL) {
    +          'data-history-node-last-comment-name' => 'comment',
    

    yep! but needs to check a views plugins as well, to properly fix all over

Status: Needs review » Needs work

The last submitted patch, 18: 2620748-18.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
859 bytes
2 KB

Status: Needs review » Needs work

The last submitted patch, 20: 2620748-20.patch, failed testing.

xjm’s picture

The comments on this issue confuse me because they almost seem to be about different bugs; can we update the issue summary with steps to reproduce this issue?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.