Decouple Comment module from Tracker

The Tracker module requires Comment module to be enabled. Does it really need this dependency?

Views has a tracker-like view that (probably) can probably (be made to) work without comments enabled?

http://drupal.org/project/trackerlite is a clone of tracker but with the comment dependency ripped out.

+1

Tracker shouldn't depend on comment module, though rather enhance its own functionality if the latter is installed (i.e. when module_exists("comment") returns TRUE).

I've seen so many sites where tracking posts is a highly wanted feature, but post comments aren't needed or desirable.

This is so much simpler with the new DB API!

Patch attached.

+++ modules/tracker/tracker.pages.inc	10 Nov 2009 20:54:29 -0000
@@ -39,7 +41,17 @@ function tracker_page($account = NULL, $
+    $query->fields('n', array('nid', 'title', 'type', 'changed', 'uid'));
+    if ($comment_module) {
+      $query->join('node_comment_statistics', 'l', 'n.nid = l.nid');
+      $query->fields('l', array('comment_count'));
+    }
+    $query->join('users', 'u', 'n.uid = u.uid');
+    $query->fields('u', array('uid'));    
+    $query->condition('n.nid', array_keys($nodes), 'IN');    
+    $result = $query->execute();

You might be able to use the fluent interface to combine all non-conditional fields/condition calls into one. Something like that (after joins and conditional stuff):
$result = $query
->fields()
->fields()
->condition()
->execute();

Also, IN is the default when an array is passed to condition, so you can ommit that.

+++ modules/tracker/tracker.pages.inc	10 Nov 2009 20:54:29 -0000
@@ -74,7 +88,10 @@ function tracker_page($account = NULL, $
-    '#header' => array(t('Type'), t('Post'), t('Author'), t('Replies'), t('Last updated')),
+    '#header' => 
+      $comment_module ?
+      array(t('Type'), t('Post'), t('Author'), t('Replies'), t('Last updated')) :
+      array(t('Type'), t('Post'), t('Author'), t('Last updated')),

Not sure what our Coding Standard says about this, seems un-common :)

This review is powered by Dreditor.

For the conditional in the #header array -- seemed a bit easier on the eye than cutting the array assignment up key by key

What do other people think?

+++ modules/tracker/tracker.pages.inc	10 Nov 2009 20:54:29 -0000
@@ -11,6 +11,8 @@
+  $comment_module = module_exists('comment');

module_exists() does internal caching (through module_list()) so I'm not sure we need to keep a local variable. Pretty minor though.

I figured it saved us 3 or 4 function calls to the same thing -- though the flipside is storing the local variable.
Since I know next to nothing about code optimization, I'll change this when I reroll.

Updated patch with changes suggested above.

I've taken the ternary operator out of the array assignment but kept it like this:

  $page['tracker']['#header'] = module_exists('comment') ?
    array(t('Type'), t('Post'), t('Author'), t('Replies'), t('Last updated')) :
    array(t('Type'), t('Post'), t('Author'), t('Last updated'));

Like this the two different versions of the header are on consecutive lines, and so I think it's easier to see how they differ than with an if-else block.

Happened to spot these today in modules/node/content_types.inc:

  $field_ui = module_exists('field_ui');

which is later checked in conditionals.

and

  $header = array(t('Name'), array('data' => t('Operations'), 'colspan' => $field_ui ? '4' : '2'));

Should I maybe revert my changes back to the original style for consistency in core?

If it's not called dozens of times, then there's no real performance impact one way or the other, but assigning a variable can make things more concise if it's used several times, so it's really just a matter of taste here.

What if instead of having Tracker check if Comment module's data is available, we make the Comment module add the data to the Tracker page through a hook? Decoupling is a good thing, just not sure of the best way around it.

I don't think it would work -- for that to be a clean hierarchically, it would surely entail having comment module do things to tracker's tables, rather than with comment-related hooks in tracker as currently. Which sounds terribly complicated to me.
Also, which other modules would use this? I can't think of any in core. And why would a contrib module support this hook, for one list of stuff, when it can just supply a field to Views module?

This is a small fix I'd like to see get in for 7 -- please let's not creep it! :)

#10: 366511-2.drupal.tracker-without-comment.patch queued for re-testing.

@14: Well... so much for Drupal 7 ;) To give this a chance of perhaps making it into Drupal 8, I've updated the last patch to work on HEAD. Assuming that's even the relevant code-base now??

Actually, I started out doing a patch on my own (since I was looking in 7.x instead of 8.x for existing patches), then found this thread and was pleasantly surprised to see that what I'd done was remarkably similar to the existing patch. However, by starting with a fresh look at the code, I was able to identify a few additional changes, so I've merged those into the new patch. In particular, I found a few more places where a check for module_exists('comment') was needed (probably code added to HEAD since the last patch was written). Furthermore, I revamped the tests so that there are tests being done both with and without the comment module.

Is there anything else that needs to be done to get this moving forward again?

Oh cool!

I doubt this would get into 7... though I would contend that depending on comment is *kinda* a bug, given that the module describes itself as:

description = Enables tracking of recent content for users.

;)

Oops, not sure I meant to change the status!

Please try re-uploading the patch. Not sure why it's getting ignored for testing...

EDIT: nevermind, my comment triggered it

Adding the Platform Initiative tag as this helps decouple some of our sub-systems.

Wouldn't mind to see this as part of http://drupal.org/community-initiatives/drupal-core/clean-up. Especially with patch in the works it shouldn't be to much effort to bring this useful improvement into core.

Re-roll? Like tsvenson mentioned, I'm moving this to the Framework Initiative, as it involves resolving our dependency system.

Re-rolled again, with passing tests this time. The patch looks good to me but will let someone else to rtbc.

Edit: Improvements to this patch can be done in a sub-branch of http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/platfo...

Heh, that was silly :) We might not need Comment anymore but we still need Node.

I added issue summary

#29: 366511-29.patch queued for re-testing.

#29: 366511-29.patch queued for re-testing.

Alternatively, we could #1261120: Deprecate Tracker module in D10 and move to contrib in D11.

added issue summary

At least we should add dependency on node, because comment no more coupled to node
Also listings probably would be replaced with views #1941830: Convert tracker listings to a view

I don't understand this bit of the summary at all:

> Tracker requires an additional comment. By default two comments are added for every entry. Would like to remove the second comment entry.

Removed comment dependencies from tracker .

#45:decouple_comment_module-366511-45.patch queued for re-testing.

1. +++ b/core/modules/tracker/tracker.pages.inc
   @@ -103,18 +107,19 @@ function tracker_page($account = NULL) {
   +        $row['comments'] = array(
   

   lets use short array syntax

2. +++ b/core/modules/tracker/tracker.pages.inc
   @@ -103,18 +107,19 @@ function tracker_page($account = NULL) {
   +      $row['last updated'] = array(
   

   lets use short array syntax

3. +++ b/core/modules/tracker/tracker.pages.inc
   @@ -130,7 +135,7 @@ function tracker_page($account = NULL) {
   +    '#header' => $comment_module ? array(t('Type'), t('Title'), t('Author'), t('Comments'), t('Last updated')) : array(t('Type'), t('Title'), t('Author'), t('Last updated')),
   

   lets use short array syntax

Is this patch really blocked for monthes just because of the array syntax?

It looks like a random failure. Retesting.

Enabling Tracker without Comment crashes with this:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "comment.statistics". in Drupal\Component\DependencyInjection\Container->get() (line 157 of core/lib/Drupal/Component/DependencyInjection/Container.php).

Drupal::service('comment.statistics') (Line: 297)
_tracker_calculate_changed(Object) (Line: 61)
tracker_cron() (Line: 24)
tracker_install()
call_user_func_array('tracker_install', Array) (Line: 391)

That's really strange it does not get triggered by a drush install nor the testbot...
That's fixed anyway.

Looks like should work but makes complicated #1941830: Convert tracker listings to a view

All the tests in the tracker module install comment so atm there's no test coverage. I expect that we need additional test coverage to ensure everything makes sense as comment is both installed on an existing site and also uninstalled.

This is just a reroll on 8.4.x to start working on tests if someone is tempted

Wrong tag, sorry

Added tests

and second route - user's tracker

The latest patch looks good to me, still fixes the issue and add tests asked by @alexpott in #58
Congratulations!

+++ b/core/modules/tracker/tracker.module
@@ -93,26 +93,27 @@ function tracker_cron() {
+      if (\Drupal::moduleHandler()->moduleExists('comment')) {

@@ -293,9 +294,11 @@ function _tracker_add($nid, $uid, $changed) {
+  if (\Drupal::hasService('comment.statistics')) {

As far as I can see there's no test coverage of running cron or doing the calculation when comment is not installed.

Just a little change to a if condition to meet coding standards.

I cloned \Drupal\tracker\Tests\TrackerTest to make this new test by removing all comment related stuff. That's the best idea I had so I hope it's gonna be good. :/

Tests are passing.

Looks it covers lots more, should go now

+++ b/core/modules/tracker/src/Tests/TrackerWithoutCommentTest.php
@@ -0,0 +1,296 @@
+class TrackerWithoutCommentTest extends WebTestBase {

There is tonnes of duplication between TrackerWithoutCommentTest and TrackerTest. I think a few things.
1. This test should be called TrackerTest
2. The existing test should be renamed TrackerCommentTest
3. It would be great to reduce duplication. Maybe TrackerCommentTest cound extend TrackerTest and the tests be slightly refactored.

Let's try a new patch.
I have a failure locally but as I cannot explain it nor reproduce it, I'll let the testbot judge.
*crossing fingers*

Reduced code duplication and fixed test fail.

This one is the good one! #confidence

Thanks for the huge efforts @DuaelFr
I don't see any strong requirement of adding new WTB here. I'm working on another issue where I'm converting last one WTB tracker test to BTB. If this issue will add one more WTB we'll again need the efforts to convert it to BTB. So here is the patch which converted the WTB to BTB. If you have any strong reasons of using WTB please specify.

$ ../vendor/bin/phpunit modules/tracker/tests/src/Functional/TrackerWithoutCommentTest.php 
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\tracker\Functional\TrackerWithoutCommentTest
.....

Time: 1.51 minutes, Memory: 6.00MB

OK (5 tests, 74 assertions)

//Naveen

Hi @naveenvalecha!
Thanks for your review. I'm sorry but the test you changed wasn't supposed to exist anymore. I merged it into TrackerTest as asked in #73 but, I suppose I forgot to remove it from the codebase :/

#83,
Nice
Failures are expected.back to N/R

Commiter's feedback #73 addressed

Tested locally and it works

Rerolled after #2863318: Cache: Convert system functional tests to phpunit has been commited.
Let's hope testbot is happy again then move it back to RTBC and hope it can be commited inside the 8.4 alpha

Testbot is happy
Back to RTBC :)

Why are some hunks checking if comment module exists and others if the service exists?

Also this seems incompatible with #1941830: Convert tracker listings to a view.

I'm struggling a bit to see what's useful in tracker module when comment module isn't installed, that wouldn't be handled simply by creating views at this point.

Very interesting point @catch!
the Tracker is not maintainable
- it carry tracker_user & tracker_node tables (easy to convert to field)
- adds listings per user/node (mostly converted to views)
- use dirty hacks to access comment statistics and use them in listings

Makes sense to raise it in #2905741: *DRAFT* Proposed product goals for Drupal 8.5/8.6(+) core

@catch @andypost Is there something I can do to move this forward?

@DuaelFr I really no idea because tracker looks like used only to track commenting on nodes + history module

Maybe we need to open #1255674: [meta] Make core maintainable to decide about unmaintainable tracker module #1261120: Deprecate Tracker module in D10 and move to contrib in D11

Tracker is useful for allowing users to find content they have created -- that's why I created the Trackerlite module for sites that didn't have Comment installed.

Re-rolled patch #91 for 9.4.x-dev.

Re-rolled patch against #106 with interdiff

Updated patch against #107 with interdiff

Updated patch against #109 with interdiff

Fix missing access call on entity query

Will this be easier or harder if https://www.drupal.org/project/drupal/issues/1941830#comment-14634573 is completed? And since it's being removed in D10 is it worth it?

Tracker is not being removed from D10: https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsolete. That would be impossible without this issue being resolved first. If comment depends on it, we can't remove tracker.

@berdir don't follow. This ticket (if I follow) is trying to get comment out of the tracker module. But don't think the comment module needs tracker.

Sorry, misread the issue.

That doesn't change that tracker is AFAIK not being removed from D10. it's still there right now and also not mentioned in this week's D10 meeting about to-be-removed modules?

So I see it's listed as a recommended removal https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsol.... if that's the case should effort be made into this or https://www.drupal.org/project/drupal/issues/1941830#comment-14634573

Trying to fix failed tests of patch #113. Removed needs reroll tag as it's not needed anymore.

Nitpick:

+++ b/core/modules/tracker/tests/src/Functional/TrackerNodeAccessTest.php
@@ -109,6 +116,23 @@ public function testTrackerNodeAccess() {
+    $service = \Drupal::service('module_installer');

Use $this->container->get() to get a service in tests.

Also, $service isn't a very good variable name! You can just chain it here, as we don't call the uninstaller again.

Made changes as per comment #122.

Fixed build issue

Seems the trackerController is broken when comment is uninstalled. Added some checks.

+++ b/core/modules/tracker/src/Controller/TrackerController.php
@@ -68,7 +68,7 @@ class TrackerController extends ControllerBase {
-  public function __construct(Connection $database, Connection $databaseReplica, CommentStatisticsInterface $commentStatistics, DateFormatterInterface $dateFormatter, EntityTypeManagerInterface $entityTypeManager) {
+  public function __construct(Connection $database, Connection $databaseReplica, CommentStatisticsInterface $commentStatistics = NULL, DateFormatterInterface $dateFormatter, EntityTypeManagerInterface $entityTypeManager) {

Optional function params should go at the end.

We can rearrange the method signature, because route controllers are considered internal in the BC policy:

> Core modules contain many route controllers that are bound to individual routes, as well as form classes. These controllers are not part of the API of the module unless specifically marked with an @api tag on the method or class.

Also, I wonder whether it would be over-engineering to provide TWO controller classes for this route, and a Route Provider service which decides which controller to use based on whether comment module is present.

Patch #126 cant apply. so rerolled the patch.Thanks

Fixing Coding Standards

Removing credit for #128 and #129. Patch#126 applied to 10.1.x just fine so the followup was not needed.

Addressed the ordering from #127

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.

in contrib now, look it already decoupled enough in 10.3 core