Fix for failure edge case in TwitterBridge outgoing repeat/retweets.

When the retweet failed with a 403 error (say due to it being a private tweet, which can't be retweeted) we would end up mishandling the return value from our internal error handling.
Instead of correctly discarding the message and closing out the queue item, we ended up trying to save a bogus twitter<->local ID mapping, which threw another exception and lead the queue system to re-run it.

- Fixed the logic check and return values for the retweet case in broadcast_twitter().
- Added doc comments explaining the return values on some functions in twitter.php
- Added check on Notice_to_status::saveNew() for empty input -- throw an exception before we try to actually insert into db. :)

plugins/TwitterBridge/Notice_to_status.php

@@ -144,6 +144,7 @@ class Notice_to_status extends Memcached_DataObject
 
     /**
      * Save a mapping between a notice and a status
+     * Warning: status_id values may not fit in 32-bit integers.
      *
      * @param integer $notice_id ID of the notice in StatusNet
      * @param integer $status_id ID of the status in Twitter
@@ -153,12 +154,18 @@ class Notice_to_status extends Memcached_DataObject
 
     static function saveNew($notice_id, $status_id)
     {
+        if (empty($notice_id)) {
+            throw new Exception("Invalid notice_id $notice_id");
+        }
         $n2s = Notice_to_status::staticGet('notice_id', $notice_id);
 
         if (!empty($n2s)) {
             return $n2s;
         }
 
+        if (empty($status_id)) {
+            throw new Exception("Invalid status_id $status_id");
+        }
         $n2s = Notice_to_status::staticGet('status_id', $status_id);
 
         if (!empty($n2s)) {

plugins/TwitterBridge/twitter.php

@@ -128,6 +128,16 @@ function is_twitter_notice($id)
     return (!empty($n2s));
 }
 
+/**
+ * Check if we need to broadcast a notice over the Twitter bridge, and
+ * do so if necessary. Will determine whether to do a straight post or
+ * a repeat/retweet
+ *
+ * This function is meant to be called directly from TwitterQueueHandler.
+ *
+ * @param Notice $notice
+ * @return boolean true if complete or successful, false if we should retry
+ */
 function broadcast_twitter($notice)
 {
     $flink = Foreign_link::getByUserID($notice->profile_id,
@@ -137,8 +147,13 @@ function broadcast_twitter($notice)
     if (!empty($flink) && TwitterOAuthClient::isPackedToken($flink->credentials)) {
         if (!empty($notice->repeat_of) && is_twitter_notice($notice->repeat_of)) {
             $retweet = retweet_notice($flink, Notice::staticGet('id', $notice->repeat_of));
-            if (!empty($retweet)) {
+            if (is_object($retweet)) {
                 Notice_to_status::saveNew($notice->id, $retweet->id);
+                return true;
+            } else {
+                // Our error processing will have decided if we need to requeue
+                // this or can discard safely.
+                return $retweet;
             }
         } else if (is_twitter_bound($notice, $flink)) {
             return broadcast_oauth($notice, $flink);
@@ -148,6 +163,21 @@ function broadcast_twitter($notice)
     return true;
 }
 
+/**
+ * Send a retweet to Twitter for a notice that has been previously bridged
+ * in or out.
+ *
+ * Warning: the return value is not guaranteed to be an object; some error
+ * conditions will return a 'true' which should be passed on to a calling
+ * queue handler.
+ *
+ * No local information about the resulting retweet is saved: it's up to
+ * caller to save new mappings etc if appropriate.
+ *
+ * @param Foreign_link $flink
+ * @param Notice $notice
+ * @return mixed object with resulting Twitter status data on success, or true/false/null on error conditions.
+ */
 function retweet_notice($flink, $notice)
 {
     $token = TwitterOAuthClient::unpackToken($flink->credentials);