The present version of the Notify module uses the following expression in hook_cron() to compute whether to send out a notification:

if (
   ($send_start - $send_last > $send_interval)
   && (date('G', $send_start) >= $send_hour || $send_interval < 86400)
   && ($send_interval != -1)
   )
   {
     _notify_send($send_start);
     variable_set('notify_send_last', $send_start);
   }

Transforming to pseudo-code, we get.

if (
   (time_elapsed > interval)
   && (hour_of_last_send >= send_hour || interval < 24_hours)
   && (not_never)
   ) send_notification();

The problem is with this construct: hour_of_last_send >= send_hour.
Let's say interval is 24 hours and you want notifications at hour 23 (send_hour). Let's say that for some reason hour_of_last_send is 1 (i.e. the last notification was sent around hour 1).
Then hour_of_last_send >= send_hour will never be true (and the or clause will also be false if the interval is 24 hours or longer). Even a week later, nothing will have been be sent. (As a workaround until this is fixed set send_hour to zero or your interval to less than 24 hours.)

I am proposing to replace this expression with a simple function to compute when the next notification should be sent. Below is a function that does exactly that:

/**
 * Compute the next time a notification should be sent.
 * 
 * @param int $send_last
 *   timestamp of last notification
 *
 * @return int
 *   -1 never, 0 send instantly, else next time to notify.
 */
function _notify_next_notificaton($send_last) {
  $send_interval = variable_get('notify_send', 86400);
  if ($send_interval < 0) {
    return(-1);
  }
  $send_hour  = variable_get('notify_send_hour',  0);

  $next_last = $next_frst = $send_last + $send_interval;
  if (REQUEST_TIME >= $next_frst) {
    return(0);
  }

  if ($send_interval >= 86400) {
    $next_last = strtotime(date('Y-m-d ', $next_frst) . $send_hour . ':00:00');
    if ($next_last < REQUEST_TIME) $next_last += 86400;
  }
  return($next_last);
}

Then all we need to do in hook_cron() is to compare the present with this timestamp, and notify when the present passes this time. Then we send the notification.

Comments

gisle’s picture

Issue summary: View changes

typo

gisle’s picture

Issue summary: View changes

Workaround suggestion.

gisle’s picture

Title: Bad logic in hook_cron() may result in notifivations never being sent. » Bad logic in hook_cron() may result in notifications never being sent.

This bug is in the version for Drupal 6 as well.

gisle’s picture

Issue summary: View changes

workaround

gisle’s picture

Issue summary: View changes

markup

gisle’s picture

Version: 7.x-1.x-dev » 6.x-1.2
Issue tags: -needs backport to 6.x

I just commited a development snapshot for the Drupal 7 version that I think fixes this.
This bugfix need to be backported to Drupal 6, so I'm changing version.

gisle’s picture

Assigned: gisle » Unassigned

And reverting to unassigned (I am only working on the D7 port).

gisle’s picture

Issue summary: View changes

markup

AdamPS’s picture

So if I understand correctly, the bug is fixed in D7 and D6 is no longer supported. In which case can this issue safely be marked as "fixed"?

gisle’s picture

Status: Active » Closed (outdated)

So if I understand correctly, the bug is fixed in D7 and D6 is no longer supported. In which case can this issue safely be marked as "fixed"?

Yes. Or rather: "outdated".