activity tracker cron calls \Drupal::service('comment.statistics')->read in a wrong way [#3232001]

Problem/Motivation

tracker.module has a function

function _tracker_calculate_changed($node) {
  $changed = $node->getChangedTime();
  $latest_comment = \Drupal::service('comment.statistics')->read([$node], 'node', FALSE);
  if ($latest_comment && $latest_comment->last_comment_timestamp > $changed) {
    $changed = $latest_comment->last_comment_timestamp;
  }
  return $changed;
}

See that it calls comment.statistics->read with [$node] .....
whereas inside the read function of that service, it expects a $key=>$value type of array.
Thus it never gets the $nid from this, and does not return last_comment_timestamp as against expected.

The call always fails to get the correct timestamp.

Since it never returns timestamp, the next bug is not exposed. If you fix this bug and make it receive valid return (by sending a proper assoc array), then the above function will still fail because it is expecting an object and the return from the read is not an object but an array.

Steps to reproduce

Create a node with changed property.
Create a comment on the node.
Run cron or invoke the above function:
- The comment created timestamp is not equal to _tracker_calculate_changed($node)

Run cron or run any test to invoke the above function.

Proposed resolution

Fixes the above function call with $nid => $node

Remaining tasks

Review the merge request

User interface changes

No.

API changes

No.

Data model changes

No.

Release notes snippet

Comment	File	Size	Author
#5	tracker-activity-update-cron-fix-3232001-5.patch	769 bytes	hardikpandya

Issue fork drupal-3232001

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

3232001-georob-drupalconportland compare
Check out this branch for the first time

Check out existing branch, if you already have it locally
3232001-karthikkumarbodu-drupalconportland compare
Check out this branch for the first time

Check out existing branch, if you already have it locally
3232001-lpeabody-drupalcon-portland compare
Check out this branch for the first time

Check out existing branch, if you already have it locally
9.4.x compare
Check out this branch for the first time

Check out existing branch, if you already have it locally

3 hidden branches

3232001-activity-tracker-cron

changes, plain diff MR !1212

Check out this branch for the first time

Check out existing branch, if you already have it locally

3232001-drakegreeott-drupalconPortland

changes, plain diff MR !2195

Check out this branch for the first time

Check out existing branch, if you already have it locally

3232001-vetchneons-drupalcon-portland

changes, plain diff MR !2193

Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

8 September 2021 at 19:49

mvelanka created an issue. See original summary.

Comment #2

mvelanka commented 8 September 2021 at 19:57

Issue summary:

View changes

Comment #3

mvelanka commented 8 September 2021 at 21:15

Issue summary:

View changes

Comment #4

larowlan

🇦🇺🏝.au GMT+10

commented 9 September 2021 at 23:19

Issue tags:

-Activity Tracker

+Bug Smash Initiative, +Needs tests, +Novice

Nice find

Comment #5

hardikpandya commented 14 September 2021 at 12:53

Status:

Active

» Needs review

Status	File	Size
new	tracker-activity-update-cron-fix-3232001-5.patch	769 bytes

I am attaching the patch for the issue linked in the description.

The issue mentions this can be found even in TrackerController class but post having a closer look, I find the value of $nodes come from loadMultiple here which returns array indexed by nid. So, this file won't need a change.

Comment #6

17 September 2021 at 18:26

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

Comment #7

mallezie

Dutch

Loenhout

commented 17 September 2021 at 20:14

Version:

9.2.x-dev

» 9.3.x-dev

Rebased to 9.3.x. Added test and created MR.

Comment #8

17 September 2021 at 20:16

mallezie opened merge request !1212

Comment #9

mvelanka commented 19 September 2021 at 12:48

the patch in comment #5 has this code

  $latest_comment_statistics = \Drupal::service('comment.statistics')->read([$node->id() => $node], 'node', FALSE);
  $latest_comment = reset($latest_comment_statistics);
   if ($latest_comment && $latest_comment->last_comment_timestamp > $changed) {
     $changed = $latest_comment->last_comment_timestamp;
   }

It seems the call to \Drupal::service('comment.statistics')->read is corrected in this patch... however if you see the return from that call, it returns an array .... not an object
so the code
$changed = $latest_comment->last_comment_timestamp;

is still going to fail. See the usage of \Drupal::service('comment.statistics')->read in comments module about how the return is handled.
I feel the the fix should be as follows.

  if ($latest_comment && $latest_comment[0]->last_comment_timestamp > $changed) {
    $changed = $latest_comment[0]->last_comment_timestamp;
  }

Correct me if wrong,

Comment #10

mvelanka commented 19 September 2021 at 13:26

duplicate

Comment #11

mallezie

Dutch

Loenhout

commented 20 September 2021 at 09:07

Adjusted the test. The setup of the test was incorrect to allow comments on the field. This time the test should succeed.
@mvelanka, your analysis is correct. however this was already taken into account with the reset() statement to take the first element of the returned array. I've changed the naming of the variables to make it a bit more clear.

Comment #12

mradcliffe

he/him

English

commented 28 September 2021 at 23:17

Issue summary:	View changes
Issue tags:	-Needs tests

1 file was hidden/shown/deleted

Status	File	Size
hidden	tracker-activity-update-cron-fix-3232001-5.patch	769 bytes

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because I think that this issue represents a good opportunity to review the merge request.

I removed Needs tests because we have a test in the merge request and I hid the patch as the merge request contains the latest changes.

Also I removed the note about TrackerController because in 9.2.x and 9.3.x, $nodes comes from EntityStorageInterface::loadMultiple, which should return an array keyed by entity id.

I attempted to add more verbose steps to reproduce.

Comment #13

volkswagenchick

she/her

English

San Francisco Bay Area

commented 5 October 2021 at 08:26

Issue tags:

+Europe2021

Tagging for DrupalCon Europe2021. Thanks!

Comment #14

5 October 2021 at 08:26

Version:

9.3.x-dev

» 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #15

mradcliffe

he/him

English

commented 25 April 2022 at 20:56

Issue tags:

+Portland2022

I added the Portland2022 tag to the issue.

+GiftofOpenSource

commented 4 May 2022 at 10:58

Status:	Needs review	» Needs work
Issue tags:	-Novice	+Needs tests

Left some comments on MR 1212

Closing the other MRs

9.4.x-dev

» 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now 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.

Comment #32

4 May 2022 at 10:58

Version:

9.5.x-dev

» 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now 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.

Comment #33

quietone commented 2 February 2023 at 03:05

Status:

Needs work

» Postponed

This extension is deprecated and scheduled for removal in Drupal 11.

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to the contributed extension once the Drupal 11 branch is open.

Comment #34

2 February 2023 at 03:05

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.

Comment #35

andypost

he/him

Russian

commented 22 April 2024 at 17:51

Project:	Drupal core	» Activity Tracker
Version:	11.x-dev	» 1.0.x-dev
Component:	tracker.module	» Code
Status:	Postponed	» Needs work
Related issues:		+#2821668: CommentManager::getCountNewComments() : PHP error on user activity and recent content view

now in contrib

activity tracker cron calls \Drupal::service('comment.statistics')->read in a wrong way

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3232001

Comments

Related issues

Referenced by