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'
          );
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

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?

R.Muilwijk’s picture

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.

nielsvm’s picture

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

Yay less code!

R.Muilwijk’s picture

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()'

R.Muilwijk’s picture

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.....

catch’s picture

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.

R.Muilwijk’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.34 KB

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?

catch’s picture

Status: Needs review » Reviewed & tested by the community

Yep, this is fine.

R.Muilwijk’s picture

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)

Dries’s picture

Status: Reviewed & tested by the community » Needs work

+ $this->assertRaw('2 comments', t('Link to the 2 comments exist. %s'));

What is the %s for?

R.Muilwijk’s picture

@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.

R.Muilwijk’s picture

Status: Needs work » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Thanks R. Committed to CVS HEAD.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.