Make Tracker not depend on Comment
jwells - January 30, 2009 - 03:52
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | tracker.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Description
What a slick program - thank you for bringing this over to us.
I think I have it working correctly, but I just can't determine one thing.
In order to stop seeing two Comment add sections (Disqus & core comment) I had to disable Comment in the modules section. Even though I disabled Comment at the CCK Node config area.
But by disabling Core Comment - I also lose Tracker, which in turn causes a loss the the Recent Posts feature.
Is this true or did I miss a setting?
#1
The Tracker module requires Comment module to be enabled. Does it really need this dependency?
#2
Views has a tracker-like view that (probably) can probably (be made to) work without comments enabled?
#3
http://drupal.org/project/trackerlite is a clone of tracker but with the comment dependency ripped out.
#4
+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.
#5
This is so much simpler with the new DB API!
Patch attached.
#6
+++ 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.
#7
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?
#8
+++ 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.
#9
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.
#10
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.
#11
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?
#12
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.
#13
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.
#14
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! :)