Problem/Motivation

Found this bug while review #2318875-20: Redo CommentStatisticsInterface
This broken at least from #2101183: Move {comment_entity_statistics} to proper service

Proposed resolution

Fix and cover with tests

Remaining tasks

Write tests, probably unittest because:
1) entity without owner interface - will always get 0 as author now (bug)
2) needs test if owner on entity is not set
both cases could mock the current user service with [0-1-2] permutations

User interface changes

no

API changes

no

Evaluation

Issue category Bug because broken
Issue priority no Major because ... data will regenerate on next comment
Disruption No
Unfrozen/prioritized changes The issue fixes a bug in 8.x new code
Target Given above, should be good for 8.0..x
CommentFileSizeAuthor
#40 2422443-40.patch15.62 KBameymudras
#37 interdiff-2422443-35-37.txt5.6 KBmohit_aghera
#37 2422443-37.patch14.98 KBmohit_aghera
#35 interdiff-2422443-33-35.txt4.87 KBmohit_aghera
#35 2422443-35.patch12.28 KBmohit_aghera
#33 interdiff-2422443-29-33.txt3.55 KBmohit_aghera
#33 2422443-33.patch14.92 KBmohit_aghera
#31 2422443-30.patch635 bytesMadhu Kumar M E
#29 interdiff_28-29.txt573 bytesranjith_kumar_k_u
#29 2422443-29.patch14.21 KBranjith_kumar_k_u
#28 reroll_diff_2422443_15-28.txt11.09 KBankithashetty
#28 2422443-28.patch14.15 KBankithashetty
#15 2422443-11-15.txt8.59 KBroderik
#15 2422443-15.patch13.79 KBroderik
#11 2422443-11.patch9.22 KBroderik
#11 2422443-11-test-only.patch8.64 KBroderik
#10 interdiff-2422443-8-10.txt1.25 KBroderik
#10 2422443-10.patch9.5 KBroderik
#8 2422443-8.patch9.05 KBroderik
#7 2422443-7.patch596 bytesandypost
#4 interdiff-2422443-0-4.patch832 bytesroderik
#4 commentstats-create-2422443-4.patch629 bytesroderik
cs-author.patch829 bytesandypost

Issue fork drupal-2422443

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » larowlan
Issue summary: View changes
mgifford’s picture

mgifford
he/him
Primary language English
commented

What's the easiest way to test this? Code looks fine.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs tests

Step to reproduce:
1) add comment field to entity (user, term, entity_test) that does not implements EntityOwnerInterface
2) create entity and check comment statistics last_comment_uid should be 0, but current user is expected

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
StatusFileSize
new commentstats-create-2422443-4.patch629 bytes
new interdiff-2422443-0-4.patch832 bytes

I my first thought "...if it's buggy but not failing, don't we need a test?"

But no, I don't think so. Just reading those lines of code makes clear that something is off and should be fixed. We don't need automated tests to see whether code is bad.

--- BUT -

The current patch is not consistent with the code comments around it (which are visible in the patch context). The comments indicate that getOwnerId() could return NULL.
(I got confused by the interface docs but Just checked with fago: it can return NULL.)

So the patch attached here in #4 actually does what the comments say.

P.S. I did test the steps in #3 manually, for completeness. #3 is correct.

Status: Needs review » Needs work

The last submitted patch, 4: interdiff-2422443-0-4.patch, failed testing.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs work » Needs review
Issue tags: -Needs tests

So that was just a wrong interdiff name.

(Taking tag away to match my comment #4.)

andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: larowlan » Unassigned
Issue summary: View changes
Issue tags: +Needs tests
StatusFileSize
new 2422443-7.patch596 bytes

We need tests, probably unittest because:
1) entity without owner interface - will always get 0 as author now (bug)
2) needs test if owner on entity is not set
3) permutation on current user ID [0-1]
Updated summary

+++ b/core/modules/comment/src/CommentStatistics.php
@@ -115,7 +115,7 @@ public function create(FieldableEntityInterface $entity, $fields) {
-      $last_comment_uid = 0;
+      $last_comment_uid = NULL;

;) define as undefined

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
StatusFileSize
new 2422443-8.patch9.05 KB

I'm just going to post this for review if the test covers what you want, in the way you want it. I still don't have a 'natural' feeling for unit tests. I probably have not done yet, what was mentioned about permutation on the user ID.

(I started writing a unit test in april, but then decided to try a simpletest based test instead, for checking the values on 'real life' fields. But I don't remember why I decided to do that anymore... so will leave the simpletest out of the patch. It's not completely finished anyway.)

(No interdiff because #7 was so small.)

Status: Needs review » Needs work

The last submitted patch, 8: 2422443-8.patch, failed testing.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs work » Needs review
StatusFileSize
new 2422443-10.patch9.5 KB
new interdiff-2422443-8-10.txt1.25 KB

OK that still needed cleanup.

We are allowed to use PHP superglobals like $_SERVER?

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
StatusFileSize
new 2422443-11-test-only.patch8.64 KB
new 2422443-11.patch9.22 KB

In hindsight, I don't know what I was doing in #10. Never mind, let's start again. Uploading a new patch without interdiff - one test-only, one including code change.

The comments from #8 / about #7 still stand, I guess: (edit: I turned my brain on and it could parse what is clearly in the issue description)

The last submitted patch, 11: 2422443-11-test-only.patch, failed testing.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Issue summary: View changes

Re-summarized prioritization: 8.0.x. (I don't know of a specified template for that; just used what makes sense to me.)

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/modules/comment/tests/src/Unit/CommentStatisticsUnitTest.php
@@ -10,6 +10,12 @@
 
+// Drupal's automated testing has REQUEST_TIME defined but we would also
+// like to be able to run PHPUnit standalone to test CommentStatistics.
+if (!defined('REQUEST_TIME')) {
+  define('REQUEST_TIME', $_SERVER['REQUEST_TIME']);
+}
+

    Instead of doing that just use $request->server->get('REQUEST_TIME') in the actual code. 

  2. +++ b/core/modules/comment/tests/src/Unit/CommentStatisticsUnitTest.php
@@ -79,15 +127,39 @@ protected function setUp() {
+    $this->currentUser = $this->getMock('Drupal\Core\Session\AccountInterface');

    Note: You can use AccountInterface::class

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
StatusFileSize
new 2422443-15.patch13.79 KB
new 2422443-11-15.txt8.59 KB

re 14.1. OK so that's why I couldn't find references to $SERVER['REQUEST_TIME'] in the source. I think this is OK now.

1a. As a result of turning my brain on and reading the summary, now we're testing several entity creations with different UIDs.

re 14.2. I cannot grok this.

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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.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.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.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.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Issue tags: +Bug Smash Initiative, +Needs reroll

This appears to be still applicable.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Issue tags: -Needs reroll
StatusFileSize
new 2422443-28.patch14.15 KB
new reroll_diff_2422443_15-28.txt11.09 KB

Here is the rerolled patch. Noticed a few long array syntax issues in the old patch, and handled them in the new patch.

Thanks!

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new 2422443-29.patch14.21 KB
new interdiff_28-29.txt573 bytes

Status: Needs review » Needs work

The last submitted patch, 29: 2422443-29.patch, failed testing. View results

Madhu Kumar M E’s picture

Madhu Kumar M E commented
Status: Needs work » Needs review
StatusFileSize
new 2422443-30.patch635 bytes

Status: Needs review » Needs work

The last submitted patch, 31: 2422443-30.patch, failed testing. View results

mohit_aghera’s picture

mohit_aghera commented
Status: Needs work » Needs review
StatusFileSize
new 2422443-33.patch14.92 KB
new interdiff-2422443-29-33.txt3.55 KB

Looks like patch in #31 is removing the changes by mistake.
I've put back the original code and changes.
- Fixing the test case failures.
- Interdiff is taken against patch from #29
- Tests seem to be passing on local now.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2086125: Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column

I got some collision in #2318875-53: Redo CommentStatisticsInterface

+++ b/core/modules/comment/comment.services.yml
@@ -11,7 +11,7 @@ services:
-    arguments: ['@database', '@current_user', '@entity_type.manager', '@state', '@database.replica']
+    arguments: ['@database', '@current_user', '@entity_type.manager', '@state', '@database.replica', '@request_stack']

+++ b/core/modules/comment/src/CommentStatistics.php
@@ -129,13 +139,17 @@ public function create(FieldableEntityInterface $entity, $fields) {
+        // Default to REQUEST_TIME when entity does not have a changed property.
+        $last_comment_timestamp = $this->requestStack->getCurrentRequest()
+          ->server->get('REQUEST_TIME');

@@ -246,13 +260,22 @@ public function update(CommentInterface $comment) {
+        // Default to REQUEST_TIME when entity does not have a changed property.
+        $last_comment_timestamp = $this->requestStack->getCurrentRequest()
+          ->server->get('REQUEST_TIME');

it should be a method with Request $request argument, no reason to inject request stack here if there's only a request time here required or request object

mohit_aghera’s picture

mohit_aghera commented
StatusFileSize
new 2422443-35.patch12.28 KB
new interdiff-2422443-33-35.txt4.87 KB

Thanks @andypost
That totally makes sense.
I have updated the patch and removed the service.

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/modules/comment/src/CommentStatistics.php
@@ -147,8 +136,7 @@ public function create(FieldableEntityInterface $entity, $fields) {
+        $last_comment_timestamp = $_SERVER['REQUEST_TIME'];

@@ -266,8 +254,7 @@ public function update(CommentInterface $comment) {
+        $last_comment_timestamp = $_SERVER['REQUEST_TIME'];

I thought it will be picked from request not from global, see #2902895: [meta][no patch] Replace uses of REQUEST_TIME and time() with time service

mohit_aghera’s picture

mohit_aghera commented
StatusFileSize
new 2422443-37.patch14.98 KB
new interdiff-2422443-35-37.txt5.6 KB

Thanks @andypost
I didn't realised about that service.
Fixed it.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs tests +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Removing the needs tests tag as that seemed to be resolved.

See we have a change record but that still needs work. Description is currently TBD

#37 doesn't apply to 10.1.x either.

Moving to NW for the change record

ameymudras’s picture

ameymudras commented
Version: 9.5.x-dev » 10.1.x-dev
StatusFileSize
new 2422443-40.patch15.62 KB

Re rolling to 10.1.x

ameymudras’s picture

ameymudras commented
Status: Needs work » Needs review

Bhanu951 made their first commit to this issue’s fork.

Bhanu951 opened merge request !3200

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs change record updates

Change record is still TBD.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.