Posted by R.Muilwijk on May 7, 2008 at 8:04pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
In the hook_link the comment_count is retrieved by calling function 'comment_num_all()':
function comment_link($type, $node = NULL, $teaser = FALSE) {
$links = array();
if ($type == 'node' && $node->comment) {
if ($teaser) {
// Main page: display the number of comments that have been posted.
if (user_access('access comments')) {
$all = comment_num_all($node->nid);
if ($all) {function comment_num_all($nid) {
static $cache;
if (!isset($cache[$nid])) {
$cache[$nid] = db_result(db_query('SELECT comment_count FROM {node_comment_statistics} WHERE nid = %d', $nid));
}
return $cache[$nid];
}However the comment_count is already retrieved in the nodeapi load which is always runned before hook_link.
function comment_nodeapi(&$node, $op, $arg = 0) {
switch ($op) {
case 'load':
if ($node->comment != COMMENT_NODE_DISABLED) {
return db_fetch_array(db_query("SELECT last_comment_timestamp, last_comment_name, comment_count FROM {node_comment_statistics} WHERE nid = %d", $node->nid));
}
break;New code:
function comment_link($type, $node = NULL, $teaser = FALSE) {
$links = array();
if ($type == 'node' && $node->comment) {
if ($teaser) {
// Main page: display the number of comments that have been posted.
if (user_access('access comments')) {
if ($node->comment_count) {
$links['comment_comments'] = array(
'title' => format_plural($node->comment_count, '1 comment', '@count comments'),
'href' => "node/$node->nid",
'attributes' => array('title' => t('Jump to the first comment of this posting.')),
'fragment' => 'comments'
);| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| comment_hook_link.patch | 1.35 KB | Ignored: Check issue status. | None | None |
Comments
#1
Not tested this yet but this looks sane to me. Since this is the only place that comment_num_all() is used I'm wondering if there's any need for it at all?
#2
I don't know whether or not comment_num_all() should stay. After a grep I saw only comment_link was using the function.
The patch added to this comment removes the function. I hope someone who is more familiar with the comment module can make the decision what patch to apply.
#3
The patch looks sane to me as well, I've just ran simpletest on a CVS up from an hour ago, pre-patch and post patch:
"297 passes, 98 fails and 6 exceptions"
niels@gazprom:~/cvs/drupal$ patch -p0 < comment_hook_link_del_func.patch
patching file modules/comment/comment.module
Hunk #3 succeeded at 1076 (offset -2 lines).
"297 passes, 98 fails and 6 exceptions."
So the patch doesn't have any influence here, seems valid to me.
+1
#4
Yay less code!
#5
The comment module was cleaned up (http://drupal.org/node/258171). Patches were broken.
This are the fixed patches. Somebody still has to make a decision whether or not to delete 'comment_num_all()'
#6
I've been looking into the comment_num_all().
It was added Sun Dec 30 16:16:38 2001
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/comment/comment.mo...
Added "x new comments" feature. Requires a SQL update.
As of then it is used in core only for that. When nodeapi was introduced with it loading the comment_count it never got deleted. There for I think it is save to delete the function and use the comment_hook_link_del_func.patch
Looks RTBC to me.....
#7
In that case it looks like we can definitely delete it which means applying: http://drupal.org/files/issues/comment_hook_link_del_func_0.patch
Once that's in, we should add a note in the update modules instructions, just in case, since it's officially a public function.
#8
Patch applied with huge offsets. Patch now applies to correct offsets and adds a test for hook_link. I tested them and they pass after applying patch. Can someone confirm?
#9
Yep, this is fine.
#10
Because of another commit to comment this patch only applied with huge offsets. New patch is up which has been tested and is still correct. (no functional changes)
#11
+ $this->assertRaw('2 comments', t('Link to the 2 comments exist. %s'));What is the %s for?
#12
@Dries,
Normal output is like this:
Link to the 2 comments exist. at [/var/www/head/d7/drupal/modules/comment/comment.test line 74]%s adds this part:
Link to the 2 comments exist. Expected false, got [Boolean: false] at [/var/www/head/d7/drupal/modules/comment/comment.test line 74]Patch up without %s and with %s. Status back to reviewed and tested because it has no functional changes.
#13
#14
Thanks R. Committed to CVS HEAD.
#15
Automatically closed -- issue fixed for two weeks with no activity.