From e7466be9b88edb39f75c670ebbdd8bdb4e8039af Mon Sep 17 00:00:00 2001
From: Bob Vincent <bobvin@pillars.net>
Date: Thu, 19 May 2011 21:28:50 -0400
Subject: [PATCH] Issue #361071 by MauMau, Sutharsan, Frans, BrockBoland, miro_dietiker, Fabianx, Vasudeva, pillarsdotnet: Use locking to eliminate duplicate emails from concurrent simplenews cron runs.

---
 includes/simplenews.mail.inc |  238 +++++++++++++++++++++++++++++-------------
 simplenews.module            |  226 +++++++++++++++++++++++-----------------
 2 files changed, 292 insertions(+), 172 deletions(-)

diff --git a/includes/simplenews.mail.inc b/includes/simplenews.mail.inc
index e9675e62968becc7a7e9eb66367f745c42deb9ef..b60bcda3a8dd108f92ef063cbb87fe20792a7613 100644
--- a/includes/simplenews.mail.inc
+++ b/includes/simplenews.mail.inc
@@ -10,17 +10,19 @@
  /**
  * Send newsletter node to subscribers.
  *
- * @param integer or object $node Newsletter node to be sent. integer = nid; object = node object
- * @param array $accounts  account objects to send the newsletter to.
- *   account = object (
- *     snid     = subscription id. 0 if no subscription record exists
- *     tid     = newsletter category ID
- *     uid      = user id. 0 if subscriber is anonymous user.
- *     mail     = user email address.
- *     name     = <empty>. Added for compatibility with user account object
- *     language = language object. User preferred of default language
- *   )
- *   NOTE: either snid, mail or uid is required.
+ * @param mixed $node
+ *   The newsletter node to be sent.  If an integer, the node nid; if an object,
+ *   the node object.
+ * @param array $accounts
+ *   An array of recipient accounts for this newsletter.  Each account may
+ *   have the following properties, but either snid, mail, or uid must be
+ *   defined:
+ *   - snid: The subscription id, or 0 if no subscription record exists.
+ *   - tid: The newsletter category id.
+ *   - uid: The subscriber's uid, or 0 if the subscriber has no user account.
+ *   - mail: The subscriber's email address.
+ *   - name: Emtpy; added for compatibility with user account object.
+ *   - language: The language object of the subscriber's preferred language.
  */
 function simplenews_send_node($node, $accounts = array()) {
   if (is_numeric($node)) {
@@ -57,7 +59,7 @@ function simplenews_send_node($node, $accounts = array()) {
       }
       $recipients = $temp_recipients;
     }
-    
+
     // To send the newsletter, the node id and target email addresses
     // are stored in the spool.
     // Only subscribed recipients are stored in the spool (status = 1).
@@ -97,7 +99,9 @@ function simplenews_send_node($node, $accounts = array()) {
 /**
  * Send test version of newsletter.
  *
- * @param integer or object $node Newsletter node to be sent. Integer = nid; Object = node object
+ * @param mixed $node
+ *   The newsletter node to be sent. If an integer, the node nid; if an object,
+ *   the node object.
  */
 function simplenews_send_test($node, $test_addresses) {
   if (is_numeric($node)) {
@@ -159,14 +163,17 @@ function simplenews_send_test($node, $test_addresses) {
  * Send a node to an email address.
  *
  * @param object $msgbase
- *   Mail message object as returned by simplenews_load_spool():
- *     $msgbase->nid
- *     $msgbase->vid
- *     $msgbase->tid
- *     $msgbase->data
- * @param $key email key [node|test]
+ *   The mail message object as returned by simplenews_load_spool(), containing
+ *   the following properties:
+ *   - nid
+ *   - vid
+ *   - tid
+ *   - data
+ * @param $key
+ *   The email key, either 'node' or 'test'.
  *
- * @return TRUE if email is successfully delivered by php mail()
+ * @return boolean
+ *   TRUE if the email was successfully delivered; otherwise FALSE.
  */
 function simplenews_mail_mail($msgbase, $key = 'node') {
   static $cache;
@@ -174,7 +181,7 @@ function simplenews_mail_mail($msgbase, $key = 'node') {
   $nid = $msgbase->nid;
   $vid = $msgbase->vid;
   $tid = $msgbase->tid;
-  
+
   $subscriber = $msgbase->data;
   if (!$subscriber) {
     $subscriber = simplenews_get_subscription((object)array('mail' => $msgbase->mail));
@@ -270,7 +277,7 @@ function simplenews_mail_spool($nid = NULL, $vid = NULL, $limit = NULL) {
   // Send pending messages from database cache
   // A limited number of mails is retrieved from the spool
   $limit = isset($limit) ? $limit : variable_get('simplenews_throttle', 20);
-  if ($spool_list = simplenews_get_spool(SIMPLENEWS_SPOOL_PENDING, $nid, $vid, $limit)) {
+  if ($spool_list = simplenews_get_spool(array(SIMPLENEWS_SPOOL_PENDING, SIMPLENEWS_SPOOL_IN_PROGRESS), $limit)) {
     // Prevent session information from being saved while sending.
     if ($original_session = drupal_save_session()) {
       drupal_save_session(FALSE);
@@ -337,16 +344,14 @@ function simplenews_mail_spool($nid = NULL, $vid = NULL, $limit = NULL) {
  * Save mail message in mail cache table.
  *
  * @param array $spool
- *   Data array to be stored in the spool table.
- *  $spool['mail']
- *  $spool['nid']
- *  $spool['vid']
- *  $spool['tid']
- *  $spool['status']  (Default: 1 = pending)
- *  $spool['time']    (default: current unix timestamp)
-
- * @param array $spool Mail message array
- * @todo Replace time(): http://drupal.org/node/224333#time
+ *   The message to be stored in the spool table, as an array containing the
+ *   following keys:
+ *   - mail
+ *   - nid
+ *   - vid
+ *   - tid
+ *   - status: (optional) Defaults to SIMPLENEWS_SPOOL_PENDING
+ *   - time: (optional) Defaults to REQUEST_TIME.
  */
 function simplenews_save_spool($spool) {
   $status = isset($spool['status']) ? $spool['status'] : SIMPLENEWS_SPOOL_PENDING;
@@ -366,43 +371,102 @@ function simplenews_save_spool($spool) {
     ->execute();
 }
 
+/*
+ * Returns the expiration time for IN_PROGRESS status.
+ *
+ * @return int
+ *   A unix timestamp. Any IN_PROGRESS messages with a timestamp older than
+ *   this will be re-allocated and re-sent.
+ */
+function simplenews_get_expiration_time() {
+  $timeout = variable_get('simplenews_spool_progress_expiration', 3600);
+  $expiration_time = REQUEST_TIME - $timeout;
+  return $expiration_time;
+}
+
 /**
- * Retrieve data from mail spool
+ * This function allocates messages to be sent in current run.
  *
- * @param string  $status Status of data to be retrieved (0 = hold, 1 = pending, 2 = send)
- * @param integer $nid node id
- * @param integer $vid node version id
- * @param integer $limit The maximum number of mails to load from the spool
+ * Drupal acquire_lock guarantees that no concurrency issue happened.
+ * If the message status is SIMPLENEWS_SPOOL_IN_PROGRESS but the maximum send
+ * time has expired, the message id will be returned as a message which is not
+ * allocated to another process.
  *
- * @return array Spool data
- *  $spool['msid']
- *  $spool['mail']
- *  $spool['nid']
- *  $spool['tid']
- *  $spool['status']
- *  $spool['time']
- * @todo Convert output to array of objects.
+ * @param string  $status
+ *   The status of data to be retrieved.  Must be one of:
+ *   - 0: hold
+ *   - 1: pending
+ *   - 2: send
+ *   - 3: in progress
+ * @param integer $limit
+ *   The maximum number of mails to load from the spool.
+ *
+ * @return array
+ *   An array of message ids to be sent in the current run.
  */
-function simplenews_get_spool($status, $nid = NULL, $vid = NULL, $limit = 0) {
-  $spool = array();
+function simplenews_get_spool($status, $limit = NULL) {
+  $messages = array();
+  $clauses = array();
+  $params = array();
+
+  if (!is_array($status)) {
+    $status = array($status);
+  }
+
+  foreach ($status as $s) {
+    if ($s == SIMPLENEWS_SPOOL_IN_PROGRESS) {
+      // Select messages which are allocated by another process, but whose
+      // maximum send time has expired.
+      $clauses[] = '(s.status = :status AND s.timestamp < :expiration)';
+      $params[':status'] = $s;
+      $params[':expiration'] = simplenews_get_expiration_time();
+    }
+    else {
+      $clauses[] = 's.status = :status';
+      $params[':status'] = $s;
+    }
+  }
 
   $query = db_select('simplenews_mail_spool', 's')
     ->fields('s')
-    ->condition('s.status', $status)
+    ->where(implode(' OR ', $clauses), $params)
     ->orderBy('s.timestamp', 'ASC');
-  if ($limit) {
-    $query->range(0, $limit);
-  }
-  foreach ($query->execute() as $row) {
-    if (strlen($row->data)) {
-      $row->data = unserialize($row->data);
+
+  /* BEGIN CRITICAL SECTION */
+  // The semaphore ensures that multiple processes get different message ID's,
+  // so that duplicate messages are not sent.
+
+  if (lock_acquire('simplenews_acquire_mail')) {
+    // Get message id's
+    // Allocate messages
+    if (is_numeric($limit)) {
+      $query->range(0, $limit);
+    }
+    foreach ($query->execute() as $message) {
+      if (strlen($message->data)) {
+        $message->data = unserialize($message->data);
+      }
+      else {
+        $mail = (object)array('mail' => $message->mail);
+        $message->data = simplenews_get_subscription($mail);
+      }
+      $messages[$message->msid] = $message;
     }
-    else {
-      $row->data = simplenews_get_subscription((object)array('mail' => $row->mail));
+    if (count($messages) > 0) {
+      // Set the state and the timestamp of the messages
+      simplenews_update_spool(
+        array_keys($messages),
+        array('status' => SIMPLENEWS_SPOOL_IN_PROGRESS)
+      );
     }
-    $spool[$row->msid] = $row;
+
+    lock_release('simplenews_acquire_mail');
   }
-  return $spool;
+
+  /* END CRITICAL SECTION */
+
+
+  return $messages;
 }
 
 /**
@@ -413,9 +477,13 @@ function simplenews_get_spool($status, $nid = NULL, $vid = NULL, $limit = 0) {
  * @param array $msids
  *   Array of Mail spool ids to be updated
  * @param array $data
- *   Array containing email sent results
- *    'status' => (0 = hold, 1 = pending, 2 = send)
- *    'error' => error id (optional; defaults to '')
+ *   Array containing email sent results, with the following keys:
+ *   - status: An integer indicating the updated status. Must be one of:
+ *     - 0: hold
+ *     - 1: pending
+ *     - 2: send
+ *     - 3: in progress
+ *   - error: (optional) The error id.  Defaults to 0 (no error).
  */
 function simplenews_update_spool($msids, $data) {
   db_update('simplenews_mail_spool')
@@ -431,17 +499,32 @@ function simplenews_update_spool($msids, $data) {
 /**
  * Count data in mail spool table.
  *
- * @param integer $nid newsletter node id
- * @param integer $vid newsletter revision id
- * @param string $status email sent status
+ * @param integer $nid
+ *   Newsletter node id.
+ * @param integer $vid
+ *   Newsletter revision id.
+ * @param array $status
+ *   Array of status flags.
  *
- * @return array Mail message array
+ * @return integer
+ *   Count of mail spool elements which own the attributes passed in as params.
  */
-function simplenews_count_spool($nid, $vid, $status = SIMPLENEWS_SPOOL_PENDING) {
+function simplenews_count_spool($nid, $vid, $status = array(SIMPLENEWS_SPOOL_PENDING, SIMPLENEWS_SPOOL_IN_PROGRESS)) {
+  $clauses = array();
+  $params = array();
+
+  if (!is_array($status)) {
+    $status = array($status);
+  }
+
+  foreach ($status as $s) {
+    $clauses[] = "s.status = $s";
+  }
+
   $query = db_select('simplenews_mail_spool')
-   ->condition('nid', $nid)
-   ->condition('vid', $vid)
-   ->condition('status', $status);
+    ->condition('nid', $nid)
+    ->condition('vid', $vid)
+    ->where(implode(' OR ', $clauses));
   return $query->countQuery()->execute()->fetchField();
 }
 
@@ -484,7 +567,7 @@ function simplenews_send_status_update() {
   $send = array(); // nodes with the status 'send'
 
   // For each pending newsletter count the number of pending emails in the spool.
-  $query = db_select('simplenews_newsletter', 's'); 
+  $query = db_select('simplenews_newsletter', 's');
   $query->innerJoin('node', 'n', 's.nid = n.nid AND s.vid = n.vid');
   $query->fields('s', array('nid', 'vid', 'tid'))
     ->fields('n', array('tnid'))
@@ -619,11 +702,13 @@ function _simplenews_set_from($category = NULL) {
  *
  * Converts some special HTML characters in addition to drupal_html_to_text()
  *
- * @param string $text Source text with HTML and special characters
+ * @param string $text
+ *   The source text with HTML and special characters.
  * @param boolean $inline_hyperlinks
  *   TRUE: URLs will be placed inline.
  *   FALSE: URLs will be converted to numbered reference list.
- * @return string Target text with HTML and special characters replaced
+ * @return string
+ *   The target text with HTML and special characters replaced.
  */
 function simplenews_html_to_text($text, $inline_hyperlinks = TRUE) {
   // By replacing <a> tag by only its URL the URLs will be placed inline
@@ -706,8 +791,11 @@ function _simplenews_html_replace() {
 /**
  * Helper function to measure PHP execution time in microseconds.
  *
- * @param bool $start TRUE reset the time and start counting.
- * @return float: elapsed PHP execution time since start.
+ * @param bool $start
+ *   If TRUE, reset the time and start counting.
+ *
+ * @return float
+ *   The elapsed PHP execution time since the last start.
  */
 function _simplenews_measure_usec($start = FALSE) {
   // Windows systems don't implement getrusage(). There is no alternative.
@@ -717,7 +805,7 @@ function _simplenews_measure_usec($start = FALSE) {
 
   static $start_time;
   $usage = getrusage();
-  $now = (float)($usage['ru_stime.tv_sec'] . '.' . $usage['ru_stime.tv_usec']) + (float)($usage['ru_utime.tv_sec'] . '.' . $usage['ru_utime.tv_usec']); 
+  $now = (float)($usage['ru_stime.tv_sec'] . '.' . $usage['ru_stime.tv_usec']) + (float)($usage['ru_utime.tv_sec'] . '.' . $usage['ru_utime.tv_usec']);
 
   if ($start) {
     $start_time = $now;
diff --git a/simplenews.module b/simplenews.module
index bf33baf3325dd246338a2eaae23b0d788b0bf70b..769b361e64d0b7f4ae271fad091d1d9a0b9ea10b 100644
--- a/simplenews.module
+++ b/simplenews.module
@@ -58,6 +58,7 @@ define('SIMPLENEWS_STATUS_SEND_READY', 2);
 define('SIMPLENEWS_SPOOL_HOLD', 0);
 define('SIMPLENEWS_SPOOL_PENDING', 1);
 define('SIMPLENEWS_SPOOL_DONE', 2);
+define('SIMPLENEWS_SPOOL_IN_PROGRESS', 3);
 
 /**
  * AFTER EACH 100 NEWSLETTERS
@@ -300,7 +301,7 @@ function simplenews_menu() {
     'access arguments' => array('subscribe to newsletters'),
     'file' => 'includes/simplenews.subscription.inc',
   );
-  
+
   $items['node/%node/simplenews'] = array(
     'title' => 'Newsletter',
     'type' => MENU_LOCAL_TASK,
@@ -428,7 +429,7 @@ function simplenews_node_presave($node) {
   if (!simplenews_check_node_types($node->type)) {
     return;
   }
-  
+
   // Note that $node is NO full node object, see node_form_validate().
   // Complete $node!
   $newsletter = simplenews_newsletter_load($node->nid);
@@ -471,14 +472,14 @@ function simplenews_newsletter_defaults($node = NULL) {
     'status' => SIMPLENEWS_STATUS_SEND_NOT,
     'sent_subscriber_count' => 0,
   );
-  
+
   if ($node) {
     $newsletter['nid'] = $node->nid;
     $newsletter['vid'] = $node->vid;
     $terms = simplenews_get_term_values($node);
     $newsletter['tid'] = $terms[0]['tid'];
   }
-  
+
   return $newsletter;
 }
 
@@ -496,7 +497,7 @@ function simplenews_node_update($node) {
   if (!$newsletter) {
     $newsletter = (object)simplenews_newsletter_defaults($node);
   }
-  
+
   simplenews_newsletter_save($newsletter);
 }
 
@@ -765,7 +766,7 @@ function _simplenews_node_form(&$form, $form_state) {
     '#collapsed' => TRUE,
     '#description' => t('These tokens can be used in all text fields and will be replaced on-screen and in the email. Note that receiver-* tokens are not suitable for on-screen use.'),
   );
-  
+
   $form['simplenews_token_help']['help'] = array(
     '#markup' => _simplenews_get_token_info(array('simplenews-newsletter', 'simplenews-category', 'site')),
   );
@@ -840,7 +841,7 @@ function simplenews_simplenews_category_delete($category) {
  * Implementation of hook_form_FORM_ID_alter().
  *
  * Add simplenews subscription fields to user register form.
- * @todo: mode this function to another place in the module.
+ * @todo mode this function to another place in the module.
  */
 function simplenews_form_user_register_form_alter(&$form, &$form_state) {
   $options = $default_value = $hidden = array();
@@ -861,7 +862,7 @@ function simplenews_form_user_register_form_alter(&$form, &$form_state) {
       }
     }
   }
-  
+
   if (count($options)) {
     // TODO Change this text: use less words;
     $form['simplenews'] = array(
@@ -903,7 +904,7 @@ function simplenews_user_insert(&$edit, $account, $category) {  // Use the email
     $subscriber->snid = $subscription->snid;
     simplenews_subscriber_save($subscriber);
   }
-  
+
   // Process subscription check boxes.
   if (isset($edit['newsletters'])) {
     $nl_tids = array_keys(array_filter($edit['newsletters']));
@@ -921,7 +922,7 @@ function simplenews_user_insert(&$edit, $account, $category) {  // Use the email
     }
     unset($edit['simplenews_hidden']);
   }
-  
+
   // set inactive if not created by an administrator.
   // @todo This needs a cleaner API.
   if (!user_access('administer users')) {
@@ -1007,7 +1008,7 @@ function simplenews_user_presave(&$edit, $account, $category) {
  */
 function simplenews_user_cancel($edit, $account, $method) {
  // Deactivate subscriber when account is disabled via cancel user.
-  
+
   if ($account) {
     $query = db_update('simplenews_subscriber')
       ->fields(array('activated' => SIMPLENEWS_SUBSCRIPTION_INACTIVE))
@@ -1025,7 +1026,7 @@ function simplenews_user_delete($account) {
   // deleted from the {user} table.
   $subscribers = simplenews_subscribers_load_multiple(array(), array('mail' => $account->mail));
   $subscriber = $subscribers ? reset($subscribers) : FALSE;
-  
+
   if ($subscriber) {
     simplenews_subscription_delete(array('snid' => $subscriber->snid));
     simplenews_subscriber_delete($subscriber);
@@ -1352,15 +1353,18 @@ function simplenews_subscribe_user($mail, $tid, $confirm = TRUE, $source = 'unkn
  *   FALSE = The user is unsubscribed
  *   TRUE  = User receives an email to verify the address and complete the subscription cancellation
  *
- * @param string $mail The email address to unsubscribe from the mailing list.
- * @param integer $tid The term ID of the list.
- * @param boolean $confirm TRUE = send confirmation mail; FALSE = unsubscribe immediate from the list
+ * @param string $mail
+ *   The email address to unsubscribe from the mailing list.
+ * @param integer $tid
+ *   The term ID of the list.
+ * @param boolean $confirm
+ *   If TRUE, send a confirmation mail; if FALSE, unsubscribe immediately.
  * @param string $source
- *   Indication of the unsubscribe source. Simplenews uses these sources:
- *    website: via any website form (with or without confirmation email)
- *    mass subscribe: mass admin UI
- *    mass unsubscribe: mass admin UI
- *    action: Drupal actions
+ *   Indicates the unsubscribe source. Simplenews uses these sources:
+ *   - website: Via any website form (with or without confirmation email).
+ *   - mass subscribe: Mass admin UI.
+ *   - mass unsubscribe: Mass admin UI.
+ *   - action: Drupal actions.
  */
 function simplenews_unsubscribe_user($mail, $tid, $confirm = TRUE, $source = 'unknown') {
   $account = (object) array(
@@ -1407,13 +1411,17 @@ function simplenews_unsubscribe_user($mail, $tid, $confirm = TRUE, $source = 'un
 /**
  * Check if the email address is subscribed to the given mailing list.
  *
- * @param string $mail email address
- * @param integer $tid mailing list id
+ * @param string $mail
+ *   The email address to be checkd.
+ * @param integer $tid
+ *   The mailing list id.
  *
- * @return boolean TRUE = email address is subscribed to given mailing list id
+ * @return boolean
+ *   TRUE if the email address is subscribed; otherwise false.
+ *
+ * @todo Only return active subscriptions.
+ * @todo Caching should be done in simplenews_get_subscription().
  */
-// TODO only return active subscriptions.
-// TODO This function can do without caching. simplenews_get_subscription() should cache.
 function simplenews_user_is_subscribed($mail, $tid, $reset = FALSE) {
   static $subscribed = array();
 
@@ -1462,36 +1470,40 @@ function simplenews_subscriber_defaults($account = NULL) {
  * If the account is not subscribed a default subscription object is returned
  * containing all available account info.
  *
- * @param object $account account details. Containing one or none of these items:
- *  object(
- *    snid :  subscription id
- *    mail :  email address
- *    uid  :  user id
- *  )
+ * @param object $account
+ *   The account details, which may contain the following properties:
+ *   - snid: The subscription id.
+ *   - mail: The email address.
+ *   - uid: The user uid.
  *
- * @return subscription object
- *   object(
- *     snid :  subscription id. 0 if account is not subscribed
- *     tids :  array of tid's of active subscriptions
- *     newsletter_subscriptions : array of newsletter subscription objects
- *     uid  :  user id. 0 if account is anonymous user
- *     mail :  user email address. empty if email is unknown
- *     name :  always empty. Added for compatibility with user account object
- *     language : language object. User preferred or default language
- *   )
+ * @return object
+ *   The subscription object, which contains the following properties:
+ *   - snid :  subscription id. 0 if account is not subscribed
+ *   - tids :  array of tid's of active subscriptions
+ *   - newsletter_subscriptions : array of newsletter subscription objects
+ *   - uid  :  user id. 0 if account is anonymous user
+ *   - mail :  user email address. empty if email is unknown
+ *   - name :  always empty. Added for compatibility with user account object
+ *   - language : language object. User preferred or default language
  *
- * TODO: Consider changing the subscription object:
- *   subscribed : array of subscription objects of newsletters the user is subscribed to
- *   unsubscribed : array of subscription objects of newsletters the user is unsubscribed from
- *   Both arrays have newsletter ID (tid) as key.
- *   Drop 'tids' and 'newsletter_subscriptions'
- * TODO: Cache the $subscription
- * TODO: Combine the two queries into one.
- * TODO: Load subscription object into tids[].
+ * @todo Consider adding the following properties to the return object:
+ * following properties:
+ * - subscribed: An array of subscription objects of newsletters to which the
+ *   user is subscribed.
+ * - unsubscribed: An array of subscription objects of newsletters to which the
+ *   user is not subscribed.
+ * Both arrays should have newsletter ID (tid) as the key.  Then the 'tids' and
+ * 'newsletter_subscriptions' properties could be dropped.
+ *
+ * @todo Cache the $subscription object.
+ * @todo Combine the two queries into one.
+ * @todo Load the subscription object into the 'tids' array.
  */
 function simplenews_get_subscription($account) {
-  // Load subscription data based on available account information
-  // NOTE that the order of checking for snid, mail and uid is critical. mail must be checked *before* uid. See simplenews_subscribe_user()
+  // Load subscription data based on available account information.
+  // NOTE that the order of checking for 'snid', 'mail' and 'uid' is critical.
+  // The 'mail' property must be checked *before* 'uid'. See
+  // simplenews_subscribe_user()
   $query = db_select('simplenews_subscriber', 's');
   $query->leftJoin('users', 'u', 'u.uid = s.uid');
   $query->fields('s')
@@ -1528,7 +1540,8 @@ function simplenews_get_subscription($account) {
     $subscription->language = user_preferred_language($subscription)->language;
   }
   else {
-    // Account is unknown in subscription table. Create default subscription object
+    // The account was not found in the subscription table.
+    // Create and return a default subscription object.
     return simplenews_subscriber_defaults($account);
   }
 
@@ -1589,7 +1602,7 @@ function simplenews_subscription_save($subscription) {
  * Delete subscriptions.
  *
  * @param $conditions
- *   Associative array of conditions matching the records to be delete.
+ *   An associative array of conditions matching the records to be delete.
  *   Example: array('tid' => 5, 'snid' => 12)
  *   Delete the subscription of subscriber 12 to newsletter tid 5.
  */
@@ -1696,14 +1709,15 @@ function simplenews_subscriber_delete($subscriber) {
 /**
  * Build subscription manager form.
  *
- * @param object $subscription subscription object
+ * @param object $subscription
+ *   The subscription object.
  */
 function _simplenews_subscription_manager_form($subscription) {
   $form = array();
   $options = array();
   $default_value = array();
   global $language;
-  
+
   $form['subscriptions'] = array(
     '#type' => 'fieldset',
     '#description' => t('Select the newsletter(s) to which you want to subscribe or unsubscribe.'),
@@ -1762,11 +1776,12 @@ function _simplenews_subscription_manager_form($subscription) {
  * Create a list of recent newsletters.
  *
  * @param integer $tid
- *   Newsletter category id
+ *   The newsletter category id.
  * @param integer $count
- *   Number of newsletters
- * @todo Replace 'class' string by array: http://drupal.org/node/224333#class_attribute_array
- * TODO Replace this list by a View.
+ *   The number of newsletters.
+ * @todo Replace 'class' string by array.
+ * @see http://drupal.org/node/224333#class_attribute_array
+ * @todo Replace this list by a View.
  */
 function simplenews_recent_newsletters($tid, $count = 5) {
   $titles = '';
@@ -1795,7 +1810,8 @@ function simplenews_recent_newsletters($tid, $count = 5) {
  *
  * @see simplenews_block_form_validate()
  * @see simplenews_block_form_submit()
- * @todo Add $form to drupal_get_form() callback functions: http://drupal.org/node/224333#hook_forms_signature
+ * @todo Add $form to drupal_get_form() callback functions.
+ * @see http://drupal.org/node/224333#hook_forms_signature
  */
 function simplenews_block_form($form, &$form_state, $tid) {
   global $user;
@@ -1899,20 +1915,33 @@ function simplenews_block_form_submit($form, &$form_state) {
  * Send simplenews mails using drupal mail API
  * @see drupal_mail()
  *
- * @param $key: node | test | subscribe | unsubscribe
- * @param array $message message array
- *          [from]
- *          [headers][From]
- *          [language]            : preferred message language
- * @param array $params parameter array
- *          [context][node]       : node object of message to be sent
- *          [context][snid]       : used for $key = subscribe or unsubscribe
- *          [context][from_name]  : name of mail sender or site name (optional)
- *          [context][account]    : account details of recipient
- *          [from]                : array('address' => 'noreply@example.org', 'formatted' =>  'site name <noreply@example.org>')
- *          [newsletter]          : newsletter object (tid, name)
- *          [tokens]              : tokens for variable replacement. Defaults to: user_mail_tokens()
- * @todo Replace drupal_clone: http://drupal.org/node/224333#drupal_clone
+ * @param string $key
+ *   Must be one of:
+ *   - node
+ *   - test
+ *   - subscribe
+ *   - unsubscribe
+ * @param array $message
+ *   The message array, containing at least the following keys:
+ *   - from
+ *   - headers: An array containing at least a 'From' key.
+ *   - language: The preferred message language.
+ * @param array $params
+ *   The parameter array, containing the following key:
+ *   - context: An array containing the following keys:
+ *     - node: The node object of the message to be sent.
+ *     - snid: Used for $key = subscribe or unsubscribe.
+ *     - from_name: (optional) The mail sender or site name.
+ *     - account: The account details of the recipient.
+ *   - from: An array containing the following keys:
+ *     - address: The sender email address, e.g., 'noreply@example.com'.
+ *     - formatted: The formatted email address, e.g.,
+ *       '"site name" <noreply@example.com>'.
+ *   - newsletter: The newsletter object, containing the following properties:
+ *     - tid
+ *     - name
+ *   - tokens: (optional) An array of tokens for variable replacement. Defaults
+ *     to user_mail_tokens().
  */
 function simplenews_mail($key, &$message, $params) {
   $params['key'] = $key;
@@ -2029,11 +2058,11 @@ function simplenews_build_node_mail(&$message, $params) {
     // Set the active language to the node's language.
     // This needs to be done as otherwise the language used to send the mail
     // is the language of the user logged in.
-    // TODO rewrite this code to drupal 7 
+    // TODO rewrite this code to drupal 7
 //    if (module_exists('i18n')) {
 //         i18n_selection_mode('node', $node->language);
 //    }
-    
+
     // Build message body
 // TODO move this code to a function. We need to do this twice for HTML and Plain text Alternative.
     // TODO: restore the format selection.
@@ -2057,7 +2086,7 @@ function simplenews_build_node_mail(&$message, $params) {
     //      In the future this will also depend on whether the receiver is subscribed to a list or not a list member at all.
     if ($category->opt_inout != SIMPLENEWS_OPT_INOUT_HIDDEN) {
       // Build and buffer message footer
-      $footer = theme('simplenews_newsletter_footer', 
+      $footer = theme('simplenews_newsletter_footer',
         array(
           'build' => $build,
           'category' => $category,
@@ -2068,13 +2097,13 @@ function simplenews_build_node_mail(&$message, $params) {
       );
       $messages[$nid][$langcode]['footer'] = $footer;
     }
-    
+
     // Reset the language to the original settings.
-    // TODO rewrite this code to drupal 7 
+    // TODO rewrite this code to drupal 7
 //    if (module_exists('i18n')) {
 //      i18n_selection_mode('reset');
 //    }
-    
+
     // Restore the custom theme.
     $custom_theme = $org_custom_theme;
   }
@@ -2432,9 +2461,10 @@ function simplenews_newsletter_delete($newsletter) {
  *   Array of module names
  * @return string
  *   Formatted Token information
- * TODO Add Token types description
- * TODO Use Theme function
- * TODO Is this available in Core??
+ *
+ * @todo Add Token types description.
+ * @todo Use Theme function.
+ * @todo Is this available in Core?
  */
 function _simplenews_get_token_info($modules) {
   $tokens = array();
@@ -2724,14 +2754,16 @@ function simplenews_help($path, $arg) {
 /**
  * Helper function to translate a newsletter name if required.
  *
- * @param <object> $newsletter
- *   Newsletter category object. Typically from simplenews_category_load().
- *    $newsletter -> tid    newsletter category id
- *    $newsletter -> name   newsletter name
- * @param <string> $langcode
- *   Optional language code (defaults to current global $language);
+ * @param object $newsletter
+ *   Newsletter category object, typically from simplenews_category_load().
+ *   Contains at least the following properties:
+ *   - tid: The newsletter category id.
+ *   - name: The newsletter name.
+ * @param string $langcode
+ *   (optional) The language code.  Defaults to $GLOBALS['language'].
  *
- * @return <string> translated newsletter name.
+ * @return string
+ *   The translated newsletter name.
  */
 function _simplenews_tt_newsletter_name($newsletter, $langcode = NULL) {
   // TODO Translate newsletter name
@@ -3130,7 +3162,7 @@ function _simplenews_checked($value) {
 
 /**
  * Following 5 functions are for separated newsletter tab
- * 
+ *
  */
 
 /**
@@ -3152,7 +3184,7 @@ function simplenews_node_tab_send_form($form, &$form_state){
   // Prepare
   $node = $form_state['build_info']['args'][0];
   $simplenews_values = (object)_simplenews_get_node_form_defaults();
-  
+
   if (isset($node->simplenews) && !empty($node->simplenews)) {
     $simplenews_values = $node->simplenews;
   }
@@ -3172,7 +3204,7 @@ function simplenews_node_tab_send_form($form, &$form_state){
     '#collapsed' => FALSE,
     '#tree' => TRUE,
   );
-  
+
   // Translations of newsletters don't have the 'send' option. Only the
   // translation source (and non translated) newsletters will get these options.
   if (module_exists('translation') && translation_supported_type($node->type)
@@ -3189,12 +3221,12 @@ function simplenews_node_tab_send_form($form, &$form_state){
       // TODO move add_js to form theme function. It is not (re)loaded on validation error.
       // Add dynamic text for send button.
       drupal_add_js(drupal_get_path('module', 'simplenews') . '/simplenews.js', 'file');
-      
+
       $options = array(
         SIMPLENEWS_COMMAND_SEND_TEST => t('Send one test newsletter to the test address'),
         SIMPLENEWS_COMMAND_SEND_NOW => t('Send newsletter'),
       );
-      
+
       $send_default = variable_get('simplenews_send', SIMPLENEWS_COMMAND_SEND_TEST);
       $form['simplenews']['send'] = array(
         '#type' => 'radios',
@@ -3274,7 +3306,7 @@ function simplenews_node_tab_send_form_validate($form, &$form_state) {
 function simplenews_node_tab_send_form_submit($form, &$form_state) {
   $values = $form_state['values'];
   $node = node_load($values['nid']);
-  
+
   // When this node is selected for translation
   // all translation of this node will be sent too.
   if (module_exists('translation') && translation_supported_type($node->type) && $node->tnid > 0) {
@@ -3302,7 +3334,7 @@ function simplenews_node_tab_send_form_submit($form, &$form_state) {
       simplenews_newsletter_save($newsletter);
     }
   }
-  
+
   // Send newsletter to all subscribers or send test newsletter
   module_load_include('inc', 'simplenews', 'includes/simplenews.mail');
   if ($values['simplenews']['send'] == SIMPLENEWS_COMMAND_SEND_NOW) {
-- 
1.7.4.1

