Variable name mismatch?

$total_replies = variable_get('advanced_forum_stats_replies', 0);
...
variable_set('advanced_forum_stats_posts', $total_replies);

Second line should be: variable_set('advanced_forum_stats_replies', $total_replies);

This affects 7.x too.

Comments

troky’s picture

troky commented

Sub-question: why not use built-in cache mechanism and avoid variable cleanup?

My proposal:

function advanced_forum_statistics_replies($refresh = FALSE) {
  // If there's no cache or we need to refresh the cache
  if ($refresh || !($cached = cache_get('advanced_forum_stats_replies'))) {
    if (module_exists('nodecomment')) {
      $total_replies = db_query('SELECT COUNT(cid) FROM {node_comments} c INNER JOIN {forum} f ON (f.nid = c.nid)')->fetchField();
    }
    else {
      $total_replies = db_query('SELECT SUM(s.comment_count) FROM {node_comment_statistics} s INNER JOIN {forum} f ON (s.nid = f.nid)')->fetchField();
    }
    cache_set('advanced_forum_stats_replies', $total_replies);
  }
  else {
    $total_replies = $cached->data;
  }

  return $total_replies;
}
michelle’s picture

michelle
she/her
Primary language English
Location Wisconsin, USA
commented

It's a little fuzzy, now, and I can't find an issue on it but I either didn't write this bit or did it based on someone's advice. I know I didn't come up with the idea but can't remember who did, now.

At any rate, that variable does look wrong and needs to be fixed if we keep it. I don't know why it's not using the normal cache. The variable isn't used any place else that I can see. I wish I could figure out who came up with this to begin with so I could ask... I'm always nervous changing code when I don't know why it was done a certain way.

Let me think on this a bit before changing anything. Maybe I can figure out why it was this way.

Michelle

michelle’s picture

michelle
she/her
Primary language English
Location Wisconsin, USA
commented

I found the commit that added it:

http://drupalcode.org/viewvc/drupal/contributions/modules/advanced_forum...

Unfortunately, my changelog entry isn't terribly useful:

#473336 by Michelle: Made post count in statistics work with nodecomment and also added caching.

There's a chance it was merlinofchaos that advised me on how to do it... Asked him on IRC to take a peek.

Michelle

troky’s picture

troky commented

Well, that part of code is pretty straigthforward... Calculates total_replies and stores value in drupal's persistent storage (variable_set). Variable name mismatch is obviuos bug and my proposal - using cache - just seems more reasonable to me. Nothing is wrong with functionality.

michelle’s picture

michelle
she/her
Primary language English
Location Wisconsin, USA
commented
Version: 6.x-2.x-dev » 7.x-2.x-dev
Status: Active » Patch (to be ported)

I talked to merlinofchaos. While he didn't remember the code, which isn't surprising since it was 2 years ago and I'm not even positive it was him that helped me, he did say this: "Well, the variable saves a query. For a tiny number like that I would consider doing it that way." So I'm going to go ahead and leave it as it is, aside for fixing the variable, which I just committed.

Thanks!

Michelle

troky’s picture

troky commented
Status: Patch (to be ported) » Fixed

Committed to D7.

Status: Fixed » Closed (fixed)

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