Week counter does not reset [#2211099]

I've been testing this module for two weeks now and I noticed that it never resets week counter. I guess that the same may be true for month counter or year counter.

This happens because counters are reset when variable statistics_day_timestamp shows different week (or month or year) than current week (or month or year). The problem is that this variable is updated by the statistics module as soon as statistics module updates its core counters. Function statistics_counter_cron is set to run after statistics_cron, so it seems that inside statistics_counter_cron value of statistics_day_timestamp always shows current timestamp, hence week (or month or year) change is never detected.

The solution would be to make sure that statistics_day_timestamp is run before statistics_cron.

So instead of this

/**
 * Implements hook_module_implements_alter().
 */
function statistics_counter_module_implements_alter(&$implementations, $hook) {
  $module_name = 'statistics_counter';

  switch ($hook) {
    case 'cron':
    case 'exit':
      if (isset($implementations[$module_name])) {
        unset($implementations[$module_name]);
        $implementations[$module_name] = $hook;
      }

      break;
  }
}

we need this:

/**
 * Implements hook_module_implements_alter().
 */
function statistics_counter_module_implements_alter(&$implementations, $hook) {
  $module_name = 'statistics_counter';

  switch ($hook) {
    case 'cron':
    case 'exit':
      if (isset($implementations[$module_name])) {
        $new = array($module_name => $implementations[$module_name]);
        unset($implementations[$module_name]);
        $implementations = $new + $implementations;
      }

      break;
  }
}

Comment	File	Size	Author
#1	statistics_counter-fix-counter-reset-2211099-1.patch	593 bytes	SiliconMind

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

SiliconMind CreditAttribution: SiliconMind commented 5 March 2014 at 11:41

Issue summary:	View changes
Status:	Active	» Needs review

File	Size
statistics_counter-fix-counter-reset-2211099-1.patch	593 bytes

I guess I was right. Patch included.

Comment #2

Proteo CreditAttribution: Proteo commented 7 April 2014 at 05:15

I'm having this exact issue. Applied the patch, but it didn't make any difference, then realized that... according to the readme file, the module keeps a record of "views this week". Just to be sure: what's exactly "this week"? The last 7 days, the days from the last monday, or the days from the first day of the week set in Drupal?

If "this week" means the last 7 days, the counter shouldn't be reset to begin with, I think.

Comment #3

SiliconMind CreditAttribution: SiliconMind commented 7 April 2014 at 08:20

Proteo, the original code does not change week counters at all. They keep adding and adding for weeks, just like normal page counter provided by the statistics module. If you take a look at my issue description and the module's source code, then you should understand what I'm talking about. The source code resets week counter if current week is different than the one stored by the statistics module. The problem is that the module runs after statistics module, so when this module runs, the current week variable created by statistics module is already updated. Hence this module "thinks", that week did not change and there is no need to reset week counters.

Take a look at statistics_counter_cron():

/**
 * Implements hook_cron().
 */
function statistics_counter_cron() {
  // Get timestamps.
  $statistics_timestamp = variable_get('statistics_day_timestamp', '');

  if (empty($statistics_timestamp)) {
    return;
  }

  // Get date.
  $week  = date('W') | 0;
  $month = date('n') | 0;
  $year  = date('Y') | 0;
  
  /*** THE BUG:
   * Then this code runs AFTER statistics_cron,
   * the $statistics_timestamp will be set to current day!
   */
  $statistics_week  = date('W', $statistics_timestamp) | 0;
  $statistics_month = date('n', $statistics_timestamp) | 0;
  $statistics_year  = date('Y', $statistics_timestamp) | 0;

  $fields = array();

  if ($week != $statistics_week || $year != $statistics_year) {
    // Reset week counts.
    $fields['weekcount'] = 0;
  }

  if ($month != $statistics_month || $year != $statistics_year) {
    // Reset month counts.
    $fields['monthcount'] = 0;
  }

  if ($year != $statistics_year) {
    // Reset year counts.
    $fields['yearcount'] = 0;
  }

  if (!empty($fields)) {
    // Reset year counts.
    db_update('node_counter')
      ->fields($fields)
      ->execute();
  }
}

Comment #4

Proteo CreditAttribution: Proteo commented 7 April 2014 at 13:50

I see, thanks for the clarification. As I mentioned before, the patch had no effect in the week count of a live site. I'm using ultimate cron to run the regular statistics job at 12:05, then statistics counter at 12:10. I tried it just a few minutes after today's midnight (which is monday).

Do you think it's necessary to use ISO-8601 weeks in /admin/config/regional/settings?

Best regards.

Comment #5

SiliconMind CreditAttribution: SiliconMind commented 11 April 2014 at 12:38

Proteo, if you specifically make regular statistics cron to run BEFORE statistics counter, then it wont work. The point of this patch is to make regular statistics to run AFTER statistics counter. It will never work the other way around.

In other words, you have to make sure that statistics counter cron job runs first, possibly just after midnight.

Comment #6

stephencross CreditAttribution: stephencross commented 23 August 2014 at 16:38

I'm considering installing this module. It's not clear to me what needs to be done to make sure the counting is done properly. Do I need this patch, change cron or what?
I would like to make sure this works properly from install and not wait a week to see the results.

Thank you.
-S

Comment #7

JordanMagnuson CreditAttribution: JordanMagnuson commented 13 March 2015 at 00:11

Seems that it would be simpler and more foolproof to just implement our own timestamp lookups separate from the statistics module.

See hook_cron() from the similar statistics_granularity module:

/**
 * Implementation of hook_cron().
 */
function statistics_granularity_cron() {
  // get the last cleared variables
  $last_week = variable_get('statistics_granularity_last_week', '');
  $last_month = variable_get('statistics_granularity_last_month', '');
  $last_year = variable_get('statistics_granularity_last_year', '');
  
  // check to see if it's the first of the week and we haven't reset the count
  // yet
  if (date('w') == 0 && $last_week != date('W')) {
    db_query('UPDATE {node_counter} SET weekcount = 0');
    variable_set('statistics_granularity_last_week', date('W'));
  } // end if we need to clear the week counts
  
  // check to see if it's the first of the month and we haven't reset the count
  // yet
  if (date('d') == '01' && $last_month != date('m')) {
    db_query('UPDATE {node_counter} SET monthcount = 0');
    variable_set('statistics_granularity_last_month', date('m'));
  } // end if we need to clear the month counts
  
  // check to see if it's the first of the year and we haven't reset the count
  // yet
  if (date('z') == 0 && $last_year != date('Y')) {
    db_query('UPDATE {node_counter} SET yearcount = 0');
    variable_set('statistics_granularity_last_year', date('Y'));
  } // end if we need to clear the year counts
} // end function statistics_granularity_cron()

Comment #8

JordanMagnuson CreditAttribution: JordanMagnuson commented 13 March 2015 at 00:12

Status:

Needs review

» Needs work

There's really no reason that resetting the aggregated counts should be tied to the core statistics module.

Comment #9

thmnhat CreditAttribution: thmnhat commented 30 April 2015 at 14:24

Hi, sorry for late reply. I agree that we should use different timestamp for this module. I've pushed a fix, please help to review and test the dev version