From 5414396a2ee9f1401d69b60969e04a1941e24e21 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Fri, 30 Apr 2010 14:41:54 -0700 Subject: [PATCH] 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 --- actions/imsettings.php | 17 ++++++------ classes/User_im_prefs.php | 23 ++++++++++++++++ classes/statusnet.ini | 6 +++-- lib/implugin.php | 25 ++++++++++++----- plugins/Aim/AimPlugin.php | 9 ++++++- plugins/Aim/README | 2 +- .../Xmpp/{Fake_XMPP.php => Queued_XMPP.php} | 17 ++++++++---- plugins/Xmpp/XmppPlugin.php | 27 ++++++++++--------- 8 files changed, 89 insertions(+), 37 deletions(-) rename plugins/Xmpp/{Fake_XMPP.php => Queued_XMPP.php} (87%) diff --git a/actions/imsettings.php b/actions/imsettings.php index 2c2606b76c..662b1063e7 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 8ecdfe9fab..75be8969e0 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 d13fdfa526..b57d862263 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 7302859a47..7125aaee8d 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 3855d1fb05..30da1dbc79 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 0465917383..7d486a0366 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 similarity index 87% rename from plugins/Xmpp/Fake_XMPP.php rename to plugins/Xmpp/Queued_XMPP.php index 0f7cfd3b4d..73eff22467 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 03bf47feac..a2521536bc 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)