IM cleanup on 1.0.x branch:

* Fake_XMPP back to Queued_XMPP, refactor how we use it and don't create objects and load classes until we need them.
* fix fatal error in IM settings while waiting for a Jabber confirmation.
* Caching fix for user_im_prefs
* fix for saving multiple transport settings
* some fixes for AIM & using normalized addresses for lookups

8 files changed, 89 insertions, 37 deletions

diff --git a/actions/imsettings.php b/actions/imsettings.php
index 2c2606b76..662b1063e 100644
--- a/actions/imsettings.php
+++ b/actions/imsettings.php
@@ -133,8 +133,7 @@ class ImsettingsAction extends ConnectSettingsAction
                                              'message with further instructions. '.
                                              '(Did you add %s to your buddy list?)'),
                                              $transport_info['display'],
-                                             $transport_info['daemon_screenname'],
-                                             jabber_daemon_address()));
+                                             $transport_info['daemon_screenname']));
                     $this->hidden('screenname', $confirm->address);
                     // TRANS: Button label to cancel an IM address confirmation procedure.
                     $this->submit('cancel', _m('BUTTON','Cancel'));
@@ -163,12 +162,11 @@ class ImsettingsAction extends ConnectSettingsAction
                                               'action' =>
                                               common_local_url('imsettings')));
             $this->elementStart('fieldset', array('id' => 'settings_im_preferences'));
-            $this->element('legend', null, _('Preferences'));
+            // TRANS: Header for IM preferences form.
+            $this->element('legend', null, _('IM Preferences'));
             $this->hidden('token', common_session_token());
             $this->elementStart('table');
             $this->elementStart('tr');
-            // TRANS: Header for IM preferences form.
-            $this->element('th', null, _('IM Preferences'));
             foreach($user_im_prefs_by_transport as $transport=>$user_im_prefs)
             {
                 $this->element('th', null, $transports[$transport]['display']);
@@ -278,19 +276,20 @@ class ImsettingsAction extends ConnectSettingsAction
         $user = common_current_user();
 
         $user_im_prefs = new User_im_prefs();
+        $user_im_prefs->query('BEGIN');
         $user_im_prefs->user_id = $user->id;
         if($user_im_prefs->find() && $user_im_prefs->fetch())
         {
             $preferences = array('notify', 'updatefrompresence', 'replies', 'microid');
-            $user_im_prefs->query('BEGIN');
             do
             {
                 $original = clone($user_im_prefs);
+                $new = clone($user_im_prefs);
                 foreach($preferences as $preference)
                 {
-                    $user_im_prefs->$preference = $this->boolean($user_im_prefs->transport . '_' . $preference);
+                    $new->$preference = $this->boolean($new->transport . '_' . $preference);
                 }
-                $result = $user_im_prefs->update($original);
+                $result = $new->update($original);
 
                 if ($result === false) {
                     common_log_db_error($user, 'UPDATE', __FILE__);
@@ -299,8 +298,8 @@ class ImsettingsAction extends ConnectSettingsAction
                     return;
                 }
             }while($user_im_prefs->fetch());
-            $user_im_prefs->query('COMMIT');
         }
+        $user_im_prefs->query('COMMIT');
         // TRANS: Confirmation message for successful IM preferences save.
         $this->showForm(_('Preferences saved.'), true);
     }
diff --git a/classes/User_im_prefs.php b/classes/User_im_prefs.php
index 8ecdfe9fa..75be8969e 100644
--- a/classes/User_im_prefs.php
+++ b/classes/User_im_prefs.php
@@ -68,4 +68,27 @@ class User_im_prefs extends Memcached_DataObject
     {
         return array(false,false);
     }
+
+    /**
+     * We have two compound keys with unique constraints:
+     * (transport, user_id) which is our primary key, and
+     * (transport, screenname) which is an additional constraint.
+     * 
+     * Currently there's not a way to represent that second key
+     * in the general keys list, so we're adding it here to the
+     * list of keys to use for caching, ensuring that it gets
+     * cleared as well when we change.
+     * 
+     * @return array of cache keys
+     */
+    function _allCacheKeys()
+    {
+        $ukeys = 'transport,screenname';
+        $uvals = $this->transport . ',' . $this->screenname;
+
+        $ckeys = parent::_allCacheKeys();
+        $ckeys[] = $this->cacheKey($this->tableName(), $ukeys, $uvals);
+        return $ckeys;
+    }
+
 }
diff --git a/classes/statusnet.ini b/classes/statusnet.ini
index d13fdfa52..b57d86226 100644
--- a/classes/statusnet.ini
+++ b/classes/statusnet.ini
@@ -647,8 +647,10 @@ modified = 384
 [user_im_prefs__keys]
 user_id = K
 transport = K
-transport = U
-screenname = U
+; There's another unique index on (transport, screenname)
+; but we have no way to represent a compound index other than
+; the primary key in here. To ensure proper cache purging,
+; we need to tweak the class.
 
 [user_urlshortener_prefs]
 user_id = 129
diff --git a/lib/implugin.php b/lib/implugin.php
index 7302859a4..7125aaee8 100644
--- a/lib/implugin.php
+++ b/lib/implugin.php
@@ -107,10 +107,15 @@ abstract class ImPlugin extends Plugin
      * receive a raw message
      * Raw IM data is taken from the incoming queue, and passed to this function.
      * It should parse the raw message and call handle_incoming()
+     * 
+     * Returning false may CAUSE REPROCESSING OF THE QUEUE ITEM, and should
+     * be used for temporary failures only. For permanent failures such as
+     * unrecognized addresses, return true to indicate your processing has
+     * completed.
      *
      * @param object $data raw IM data
      *
-     * @return boolean success value
+     * @return boolean true if processing completed, false for temporary failures
      */
     abstract function receive_raw_message($data);
 
@@ -185,9 +190,12 @@ abstract class ImPlugin extends Plugin
      */
     function get_user_im_prefs_from_screenname($screenname)
     {
-        if($user_im_prefs = User_im_prefs::pkeyGet( array('transport' => $this->transport, 'screenname' => $screenname) )){
+        $user_im_prefs = User_im_prefs::pkeyGet(
+            array('transport' => $this->transport,
+                  'screenname' => $this->normalize($screenname)));
+        if ($user_im_prefs) {
             return $user_im_prefs;
-        }else{
+        } else {
             return false;
         }
     }
@@ -203,9 +211,9 @@ abstract class ImPlugin extends Plugin
     function get_screenname($user)
     {
         $user_im_prefs = $this->get_user_im_prefs_from_user($user);
-        if($user_im_prefs){
+        if ($user_im_prefs) {
             return $user_im_prefs->screenname;
-        }else{
+        } else {
             return false;
         }
     }
@@ -220,9 +228,12 @@ abstract class ImPlugin extends Plugin
      */
     function get_user_im_prefs_from_user($user)
     {
-        if($user_im_prefs = User_im_prefs::pkeyGet( array('transport' => $this->transport, 'user_id' => $user->id) )){
+        $user_im_prefs = User_im_prefs::pkeyGet(
+            array('transport' => $this->transport,
+                  'user_id' => $user->id));
+        if ($user_im_prefs){
             return $user_im_prefs;
-        }else{
+        } else {
             return false;
         }
     }
diff --git a/plugins/Aim/AimPlugin.php b/plugins/Aim/AimPlugin.php
index 3855d1fb0..30da1dbc7 100644
--- a/plugins/Aim/AimPlugin.php
+++ b/plugins/Aim/AimPlugin.php
@@ -126,6 +126,11 @@ class AimPlugin extends ImPlugin
         return true;
     }
 
+    /**
+     * Accept a queued input message.
+     *
+     * @return true if processing completed, false if message should be reprocessed
+     */
     function receive_raw_message($message)
     {
         $info=Aim::getMessageInfo($message);
@@ -133,7 +138,9 @@ class AimPlugin extends ImPlugin
         $user = $this->get_user($from);
         $notice_text = $info['message'];
 
-        return $this->handle_incoming($from, $notice_text);
+        $this->handle_incoming($from, $notice_text);
+
+        return true;
     }
 
     function initialize(){
diff --git a/plugins/Aim/README b/plugins/Aim/README
index 046591738..7d486a036 100644
--- a/plugins/Aim/README
+++ b/plugins/Aim/README
@@ -6,7 +6,7 @@ add "addPlugin('aim',
     array('setting'=>'value', 'setting2'=>'value2', ...);"
 to the bottom of your config.php
 
-The daemon included with this plugin must be running. It will be started by
+scripts/imdaemon.php included with StatusNet must be running. It will be started by
 the plugin along with their other daemons when you run scripts/startdaemons.sh.
 See the StatusNet README for more about queuing and daemons.
 
diff --git a/plugins/Xmpp/Fake_XMPP.php b/plugins/Xmpp/Queued_XMPP.php
index 0f7cfd3b4..73eff2246 100644
--- a/plugins/Xmpp/Fake_XMPP.php
+++ b/plugins/Xmpp/Queued_XMPP.php
@@ -2,7 +2,7 @@
 /**
  * StatusNet, the distributed open-source microblogging tool
  *
- * Instead of sending XMPP messages, retrieve the raw XML that would be sent
+ * Queue-mediated proxy class for outgoing XMPP messages.
  *
  * PHP version 5
  *
@@ -31,13 +31,17 @@ if (!defined('STATUSNET') && !defined('LACONICA')) {
     exit(1);
 }
 
-class Fake_XMPP extends XMPPHP_XMPP
+class Queued_XMPP extends XMPPHP_XMPP
 {
-    public $would_be_sent = null;
+    /**
+     * Reference to the XmppPlugin object we're hooked up to.
+     */
+    public $plugin;
 
 	/**
 	 * Constructor
 	 *
+     * @param XmppPlugin $plugin
 	 * @param string  $host
 	 * @param integer $port
 	 * @param string  $user
@@ -47,8 +51,10 @@ class Fake_XMPP extends XMPPHP_XMPP
 	 * @param boolean $printlog
 	 * @param string  $loglevel
 	 */
-	public function __construct($host, $port, $user, $password, $resource, $server = null, $printlog = false, $loglevel = null)
+	public function __construct($plugin, $host, $port, $user, $password, $resource, $server = null, $printlog = false, $loglevel = null)
 	{
+        $this->plugin = $plugin;
+
         parent::__construct($host, $port, $user, $password, $resource, $server, $printlog, $loglevel);
 
         // We use $host to connect, but $server to build JIDs if specified.
@@ -73,7 +79,7 @@ class Fake_XMPP extends XMPPHP_XMPP
      */
     public function send($msg, $timeout=NULL)
     {
-        $this->would_be_sent = $msg;
+        $this->plugin->enqueue_outgoing_raw($msg);
     }
 
     //@{
@@ -110,5 +116,6 @@ class Fake_XMPP extends XMPPHP_XMPP
         throw new Exception("Can't read stream from fake XMPP.");
     }
     //@}
+
 }
 
diff --git a/plugins/Xmpp/XmppPlugin.php b/plugins/Xmpp/XmppPlugin.php
index 03bf47fea..a2521536b 100644
--- a/plugins/Xmpp/XmppPlugin.php
+++ b/plugins/Xmpp/XmppPlugin.php
@@ -60,8 +60,6 @@ class XmppPlugin extends ImPlugin
 
     public $transport = 'xmpp';
 
-    protected $fake_xmpp;
-
     function getDisplayName(){
         return _m('XMPP/Jabber/GTalk');
     }
@@ -292,7 +290,7 @@ class XmppPlugin extends ImPlugin
             require_once 'XMPP.php';
             return false;
         case 'Sharing_XMPP':
-        case 'Fake_XMPP':
+        case 'Queued_XMPP':
             require_once $dir . '/'.$cls.'.php';
             return false;
         case 'XmppManager':
@@ -317,9 +315,7 @@ class XmppPlugin extends ImPlugin
 
     function send_message($screenname, $body)
     {
-        $this->fake_xmpp->message($screenname, $body, 'chat');
-        $this->enqueue_outgoing_raw($this->fake_xmpp->would_be_sent);
-        return true;
+        $this->queuedConnection()->message($screenname, $body, 'chat');
     }
 
     function send_notice($screenname, $notice)
@@ -327,8 +323,7 @@ class XmppPlugin extends ImPlugin
         $msg   = $this->format_notice($notice);
         $entry = $this->format_entry($notice);
         
-        $this->fake_xmpp->message($screenname, $msg, 'chat', null, $entry);
-        $this->enqueue_outgoing_raw($this->fake_xmpp->would_be_sent);
+        $this->queuedConnection()->message($screenname, $msg, 'chat', null, $entry);
         return true;
     }
 
@@ -385,10 +380,19 @@ class XmppPlugin extends ImPlugin
             return true;
         }
 
-        return $this->handle_incoming($from, $pl['body']);
+        $this->handle_incoming($from, $pl['body']);
+        
+        return true;
     }
 
-    function initialize(){
+    /**
+     * Build a queue-proxied XMPP interface object. Any outgoing messages
+     * will be run back through us for enqueing rather than sent directly.
+     * 
+     * @return Queued_XMPP
+     * @throws Exception if server settings are invalid.
+     */
+    function queuedConnection(){
         if(!isset($this->server)){
             throw new Exception("must specify a server");
         }
@@ -402,7 +406,7 @@ class XmppPlugin extends ImPlugin
             throw new Exception("must specify a password");
         }
 
-        $this->fake_xmpp = new Fake_XMPP($this->host ?
+        return new Queued_XMPP($this, $this->host ?
                                     $this->host :
                                     $this->server,
                                     $this->port,
@@ -415,7 +419,6 @@ class XmppPlugin extends ImPlugin
                                     $this->debug ?
                                     XMPPHP_Log::LEVEL_VERBOSE :  null
                                     );
-        return true;
     }
 
     function onPluginVersion(&$versions)