The patch in #2041287: Convert $node->nid to $node->id() and $node->isNew() was a bit to zealous and converted a $node->nid too many.

In \Drupal\comment\Plugin\views\field\NodeNewComments::preRender() the $node variable is a simple object from a databse result with only an nid and num_comments property.

CommentFileSizeAuthor
#13 interdiff.txt707 bytesborisson_
#13 call_to_undefined-2505879-13.patch6.16 KBborisson_
#9 call_to_undefined-2505879-9-complete.patch6.17 KBgeertvd
#9 call_to_undefined-2505879-9-test.patch5.54 KBgeertvd
#6 2505879-call-to-undefined-with-test.patch1.59 KBRavindraSingh
#4 2505879-call-to-undefined.patch646 bytesRavindraSingh
#1 fix_nid_access_in_prerender-2505879-1.patch632 bytesJvE
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JvE’s picture

JvE CreditAttribution: JvE at One Shoe commented
FileSize
fix_nid_access_in_prerender-2505879-1.patch632 bytes
olli’s picture

olli CreditAttribution: olli commented
Component: views.module » comment.module
Status: Active » Needs review
Issue tags: +VDC, +Needs tests

Nice find. I think we need a test here.

Status: Needs review » Needs work

The last submitted patch, 1: fix_nid_access_in_prerender-2505879-1.patch, failed testing.

RavindraSingh’s picture

RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commented
FileSize
2505879-call-to-undefined.patch646 bytes

Without tests.

RavindraSingh’s picture

RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commented
Status: Needs work » Needs review
RavindraSingh’s picture

RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commented
FileSize
2505879-call-to-undefined-with-test.patch1.59 KB

Here, I am adding a patch which includes tests in DefaultViewRecentCommentsTest.php file. I am not sure how to write the tests here, Adding the tests in existing file.

/**
 * @file
 * Contains \Drupal\comment\Tests\Views\DefaultViewRecentCommentsTest.
 * Contains \Drupal\comment\Tests\Views\NodeNewComments.
*/

Here I am not sure if we can add more tests into single file.

RavindraSingh’s picture

RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commented
Status: Needs review » Needs work

Moving back to needs work.

Needs guidelines for writing tests.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented 
+++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
@@ -141,7 +141,7 @@ public function preRender(&$values) {
-        foreach ($ids[$node->id()] as $id) {
+        foreach ($ids[$node->nid()] as $id) {

should be $node->nid as patch #1 does

geertvd’s picture

geertvd CreditAttribution: geertvd at XIO commented
Status: Needs work » Needs review
FileSize
call_to_undefined-2505879-9-test.patch5.54 KB
call_to_undefined-2505879-9-complete.patch6.17 KB

Added some tests for this

The last submitted patch, 9: call_to_undefined-2505879-9-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: call_to_undefined-2505879-9-complete.patch, failed testing.

Status: Needs work » Needs review

geertvd queued 9: call_to_undefined-2505879-9-complete.patch for re-testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Issue tags: -Needs tests
FileSize
call_to_undefined-2505879-13.patch6.16 KB
interdiff.txt707 bytes

Fixed a small nitpick in the patch. (Short array syntax).

This has tests so removing the tag.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Since I didn't really change anything with the patch I added and test still pass, I feel I can RTBC this issue.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented

+1 rtbc

alexpott’s picture

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

@borisson_ the short array syntax is a recommendation rather than a standard.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes.Committed f85c0c1 and pushed to 8.0.x. Thanks!

  • alexpott committed f85c0c1 on 8.0.x 
    Issue #2505879 by RavindraSingh, geertvd, borisson_, JvE: Call to...

Status: Fixed » Closed (fixed)

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