Group discovery from text functions polished

Also removed the entirely unused saveGroups function.

Now avoiding multiGet and using listFind in Profile->getGroups()
so we don't have to deal with ArrayWrapper.

actions/usergroups.php

@@ -137,13 +137,12 @@ class UsergroupsAction extends ProfileAction
 
             $groups = $this->user->getGroups($offset, $limit);
 
-            if ($groups) {
+            if ($groups instanceof User_group) {
                 $gl = new GroupList($groups, $this->user, $this);
                 $cnt = $gl->show();
-                if (0 == $cnt) {
+            } else {
                 $this->showEmptyListMessage();
             }
-            }
 
             $this->pagination($this->page > 1, $cnt > GROUPS_PER_PAGE,
                               $this->page, 'usergroups',

classes/Notice.php

@@ -410,9 +410,8 @@ class Notice extends Managed_DataObject
         $notice->url = $url;
 
         // Get the groups here so we can figure out replies and such
-
         if (!isset($groups)) {
-            $groups = self::groupsFromText($notice->content, $profile);
+            $groups = User_group::idsFromText($notice->content, $profile);
         }
 
         $reply = null;
@@ -1154,7 +1153,7 @@ class Notice extends Managed_DataObject
         $groups = array();
         foreach (array_unique($group_ids) as $id) {
             $group = User_group::getKV('id', $id);
-            if ($group) {
+            if ($group instanceof User_group) {
                 common_log(LOG_ERR, "Local delivery to group id $id, $group->nickname");
                 $result = $this->addToGroupInbox($group);
                 if (!$result) {
@@ -1181,48 +1180,7 @@ class Notice extends Managed_DataObject
         return $groups;
     }
 
-    /**
+    function addToGroupInbox(User_group $group)
-     * Parse !group delivery and record targets into group_inbox.
-     * @return array of Group objects
-     */
-    function saveGroups()
-    {
-        // Don't save groups for repeats
-
-        if (!empty($this->repeat_of)) {
-            return array();
-        }
-
-        $profile = $this->getProfile();
-
-        $groups = self::groupsFromText($this->content, $profile);
-
-        /* Add them to the database */
-
-        foreach ($groups as $group) {
-            /* XXX: remote groups. */
-
-            if (empty($group)) {
-                continue;
-            }
-
-
-            if ($profile->isMember($group)) {
-
-                $result = $this->addToGroupInbox($group);
-
-                if (!$result) {
-                    common_log_db_error($gi, 'INSERT', __FILE__);
-                }
-
-                $groups[] = clone($group);
-            }
-        }
-
-        return $groups;
-    }
-
-    function addToGroupInbox($group)
     {
         $gi = Group_inbox::pkeyGet(array('group_id' => $group->id,
                                          'notice_id' => $this->id));
@@ -1445,7 +1403,7 @@ class Notice extends Managed_DataObject
 
     /**
      * Pull list of groups this notice needs to be delivered to,
-     * as previously recorded by saveGroups() or saveKnownGroups().
+     * as previously recorded by saveKnownGroups().
      *
      * @return array of Group objects
      */
@@ -2500,29 +2458,6 @@ class Notice extends Managed_DataObject
         return false;
     }
 
-    static function groupsFromText($text, $profile)
-    {
-        $groups = array();
-
-        /* extract all !group */
-        $count = preg_match_all('/(?:^|\s)!(' . Nickname::DISPLAY_FMT . ')/',
-                                strtolower($text),
-                                $match);
-
-        if (!$count) {
-            return $groups;
-        }
-
-        foreach (array_unique($match[1]) as $nickname) {
-            $group = User_group::getForNickname($nickname, $profile);
-            if ($group instanceof User_group && $profile->isMember($group)) {
-                $groups[] = $group->id;
-            }
-        }
-
-        return $groups;
-    }
-
     public function getParent()
     {
         $parent = Notice::getKV('id', $this->reply_to);

classes/Profile.php

@@ -221,9 +221,8 @@ class Profile extends Managed_DataObject
     function isMember($group)
     {
     	$groups = $this->getGroups(0, null);
-    	$gs = $groups->fetchAll();
+        while ($groups instanceof User_group && $groups->fetch()) {
-    	foreach ($gs as $g) {
+    	    if ($groups->id == $group->id) {
-    	    if ($group->id == $g->id) {
     	        return true;
     	    }
     	}
@@ -272,7 +271,18 @@ class Profile extends Managed_DataObject
             $ids = array_slice($ids, $offset, $limit);
         }
 
-        return User_group::multiGet('id', $ids);
+        try {
+            return User_group::listFind('id', $ids);
+        } catch (NoResultException $e) {
+            return null;    // throw exception when we handle it everywhere
+        }
+    }
+
+    function getGroupCount() {
+        $groups = $this->getGroups(0, null);
+        return $groups instanceof User_group
+                ? $groups->N
+                : 0;
     }
 
     function isTagged($peopletag)

classes/User_group.php

@@ -418,14 +418,14 @@ class User_group extends Managed_DataObject
         return true;
     }
 
-    static function getForNickname($nickname, $profile=null)
+    static function getForNickname($nickname, Profile $profile=null)
     {
         $nickname = Nickname::normalize($nickname);
 
         // Are there any matching remote groups this profile's in?
-        if ($profile) {
+        if ($profile instanceof Profile) {
             $group = $profile->getGroups(0, null);
-            while ($group->fetch()) {
+            while ($group instanceof User_group && $group->fetch()) {
                 if ($group->nickname == $nickname) {
                     // @fixme is this the best way?
                     return clone($group);
@@ -434,13 +434,12 @@ class User_group extends Managed_DataObject
         }
 
         // If not, check local groups.
-
         $group = Local_group::getKV('nickname', $nickname);
-        if (!empty($group)) {
+        if ($group instanceof Local_group) {
             return User_group::getKV('id', $group->group_id);
         }
         $alias = Group_alias::getKV('alias', $nickname);
-        if (!empty($alias)) {
+        if ($alias instanceof Group_alias) {
             return User_group::getKV('id', $alias->group_id);
         }
         return null;
@@ -822,4 +821,37 @@ class User_group extends Managed_DataObject
         return ($this->join_policy == self::JOIN_POLICY_MODERATE &&
                 $this->force_scope == 1);
     }
+
+    static function groupsFromText($text, Profile $profile)
+    {
+        $groups = array();
+
+        /* extract all !group */
+        $count = preg_match_all('/(?:^|\s)!(' . Nickname::DISPLAY_FMT . ')/',
+                                strtolower($text),
+                                $match);
+
+        if (!$count) {
+            return $groups;
+        }
+
+        foreach (array_unique($match[1]) as $nickname) {
+            $group = self::getForNickname($nickname, $profile);
+            if ($group instanceof User_group && $profile->isMember($group)) {
+                $groups[] = clone($group);
+            }
+        }
+
+        return $groups;
+    }
+
+    static function idsFromText($text, Profile $profile)
+    {
+        $ids = array();
+        $groups = self::groupsFromText($text, $profile);
+        foreach ($groups as $group) {
+            $ids[$group->id] = true;
+        }
+        return array_keys($ids);
+    }
 }

lib/apiaction.php

@@ -225,7 +225,7 @@ class ApiAction extends Action
         }
         $twitter_user['profile_image_url_original'] = $origurl;
 
-        $twitter_user['groups_count'] = $profile->getGroups(0, null)->N;
+        $twitter_user['groups_count'] = $profile->getGroupCount();
         foreach (array('linkcolor', 'backgroundcolor') as $key) {
             $twitter_user[$key] = Profile_prefs::getConfigData($profile, 'theme', $key);
         }

lib/command.php

@@ -980,7 +980,7 @@ class GroupsCommand extends Command
     {
         $group = $this->user->getGroups();
         $groups=array();
-        while ($group->fetch()) {
+        while ($group instanceof User_group && $group->fetch()) {
             $groups[]=$group->nickname;
         }
         if(count($groups)==0){

lib/groupsnav.php

@@ -58,7 +58,7 @@ class GroupsNav extends MoreMenu
 
     function haveGroups()
     {
-        return (!empty($this->groups) && ($this->groups->N > 0));
+        return ($this->groups instanceof User_group && $this->groups->N > 0);
     }
 
     function tag()
@@ -70,7 +70,7 @@ class GroupsNav extends MoreMenu
     {
         $items = array();
 
-        while ($this->groups->fetch()) {
+        while ($this->groups instanceof User_group && $this->groups->fetch()) {
             $items[] = array('placeholder',
                              array('nickname' => $this->groups->nickname,
                                    'mainpage' => $this->groups->homeUrl()),

lib/profileaction.php

@@ -270,14 +270,13 @@ class ProfileAction extends Action
             $this->text($this->profile->getGroups(0, null)->N);
             $this->elementEnd('h2');
 
-            if ($groups) {
+            if ($groups instanceof User_group) {
                 $gml = new GroupMiniList($groups, $this->profile, $this);
                 $cnt = $gml->show();
-                if ($cnt == 0) {
+            } else {
                 // TRANS: Text for user user group membership statistics if user is not a member of any group.
                 $this->element('p', null, _('(None)'));
             }
-            }
 
             Event::handle('EndShowGroupsMiniList', array($this));
         }

lib/toselector.php

@@ -94,7 +94,7 @@ class ToSelector extends Widget
 
         $groups = $this->user->getGroups();
 
-        while ($groups->fetch()) {
+        while ($groups instanceof User_group && $groups->fetch()) {
             $value = 'group:'.$groups->id;
             if (($this->to instanceof User_group) && $this->to->id == $groups->id) {
                 $default = $value;

lib/util.php

@@ -320,7 +320,7 @@ function common_set_user($user)
     } else if (is_string($user)) {
         $nickname = $user;
         $user = User::getKV('nickname', $nickname);
-    } else if (!($user instanceof User)) {
+    } else if (!$user instanceof User) {
         return false;
     }
 
@@ -433,7 +433,7 @@ function common_remembered_user()
 
     $user = User::getKV('id', $rm->user_id);
 
-    if (!$user) {
+    if (!$user instanceof User) {
         common_log(LOG_WARNING, 'No such user for rememberme: ' . $rm->user_id);
         common_forgetme();
         return null;
@@ -488,8 +488,8 @@ function common_current_user()
             common_ensure_session();
             $id = isset($_SESSION['userid']) ? $_SESSION['userid'] : false;
             if ($id) {
-                $user = User::getKV($id);
+                $user = User::getKV('id', $id);
-                if ($user) {
+                if ($user instanceof User) {
                 	$_cur = $user;
                 	return $_cur;
                 }
@@ -584,13 +584,10 @@ function common_canonical_email($email)
  * @param Notice $notice in whose context we're working
  * @return string partially rendered HTML
  */
-function common_render_content($text, $notice)
+function common_render_content($text, Notice $notice)
 {
     $r = common_render_text($text);
-    $id = $notice->profile_id;
     $r = common_linkify_mentions($r, $notice);
-    $r = preg_replace_callback('/(^|[\s\.\,\:\;]+)!(' . Nickname::DISPLAY_FMT . ')/',
-                      function ($m) { return "{$m[1]}!".common_group_link($id, $m[2]); }, $r);
     return $r;
 }
 
@@ -677,35 +674,39 @@ function common_linkify_mention($mention)
  */
 function common_find_mentions($text, $notice)
 {
-    $mentions = array();
+    try {
-
         $sender = Profile::getKV('id', $notice->profile_id);
-
+    } catch (NoProfileException $e) {
-    if (empty($sender)) {
+        return array();
-        return $mentions;
     }
 
+    $mentions = array();
+
     if (Event::handle('StartFindMentions', array($sender, $text, &$mentions))) {
         // Get the context of the original notice, if any
-        $originalAuthor   = null;
+        $origAuthor   = null;
-        $originalNotice   = null;
+        $origNotice   = null;
-        $originalMentions = array();
+        $origMentions = array();
 
         // Is it a reply?
 
-        if (!empty($notice) && !empty($notice->reply_to)) {
+        if ($notice instanceof Notice) {
-            $originalNotice = Notice::getKV('id', $notice->reply_to);
+            try {
-            if (!empty($originalNotice)) {
+                $origNotice = $notice->getParent();
-                $originalAuthor = Profile::getKV('id', $originalNotice->profile_id);
+                $origAuthor = $origNotice->getProfile();
 
-                $ids = $originalNotice->getReplies();
+                $ids = $origNotice->getReplies();
 
                 foreach ($ids as $id) {
                     $repliedTo = Profile::getKV('id', $id);
-                    if (!empty($repliedTo)) {
+                    if ($repliedTo instanceof Profile) {
-                        $originalMentions[$repliedTo->nickname] = $repliedTo;
+                        $origMentions[$repliedTo->nickname] = $repliedTo;
                     }
                 }
+            } catch (NoProfileException $e) {
+                common_log(LOG_WARNING, sprintf('Notice %d author profile id %d does not exist', $origNotice->id, $origNotice->profile_id));
+            } catch (ServerException $e) {
+                common_log(LOG_WARNING, __METHOD__ . ' got exception: ' . $e->getMessage());
             }
         }
 
@@ -723,19 +724,19 @@ function common_find_mentions($text, $notice)
             // Start with conversation context, then go to
             // sender context.
 
-            if (!empty($originalAuthor) && $originalAuthor->nickname == $nickname) {
+            if ($origAuthor instanceof Profile && $origAuthor->nickname == $nickname) {
-                $mentioned = $originalAuthor;
+                $mentioned = $origAuthor;
-            } else if (!empty($originalMentions) &&
+            } else if (!empty($origMentions) &&
-                       array_key_exists($nickname, $originalMentions)) {
+                       array_key_exists($nickname, $origMentions)) {
-                $mentioned = $originalMentions[$nickname];
+                $mentioned = $origMentions[$nickname];
             } else {
                 $mentioned = common_relative_profile($sender, $nickname);
             }
 
-            if (!empty($mentioned)) {
+            if ($mentioned instanceof Profile) {
                 $user = User::getKV('id', $mentioned->id);
 
-                if ($user) {
+                if ($user instanceof User) {
                     $url = common_local_url('userbyid', array('id' => $user->id));
                 } else {
                     $url = $mentioned->profileurl;
@@ -757,15 +758,13 @@ function common_find_mentions($text, $notice)
         // @#tag => mention of all subscriptions tagged 'tag'
 
         preg_match_all('/(?:^|[\s\.\,\:\;]+)@#([\pL\pN_\-\.]{1,64})/',
-                       $text,
+                       $text, $hmatches, PREG_OFFSET_CAPTURE);
-                       $hmatches,
-                       PREG_OFFSET_CAPTURE);
-
         foreach ($hmatches[1] as $hmatch) {
-
             $tag = common_canonical_tag($hmatch[0]);
             $plist = Profile_list::getByTaggerAndTag($sender->id, $tag);
-            if (!empty($plist) && !$plist->private) {
+            if (!$plist instanceof Profile_list || $plist->private) {
+                continue;
+            }
             $tagged = $sender->getTaggedSubscribers($tag);
 
             $url = common_local_url('showprofiletag',
@@ -777,6 +776,24 @@ function common_find_mentions($text, $notice)
                                 'position' => $hmatch[1],
                                 'url' => $url);
         }
+
+        preg_match_all('/(?:^|[\s\.\,\:\;]+)!(' . Nickname::DISPLAY_FMT . ')/',
+                       $text, $hmatches, PREG_OFFSET_CAPTURE);
+        foreach ($hmatches[1] as $hmatch) {
+            $nickname = Nickname::normalize($hmatch[0]);
+            $group = User_group::getForNickname($nickname, $sender);
+
+            if (!$group instanceof User_group || !$sender->isMember($group)) {
+                continue;
+            }
+
+            $profile = $group->getProfile();
+
+            $mentions[] = array('mentioned' => $profile,
+                                'text'      => $hmatch[0],
+                                'position'  => $hmatch[1],
+                                'url'       => $group->permalink,
+                                'title'     => $group->getFancyName());
         }
 
         Event::handle('EndFindMentions', array($sender, $text, &$mentions));
@@ -1146,35 +1163,6 @@ function common_valid_profile_tag($str)
     return preg_match('/^[A-Za-z0-9_\-\.]{1,64}$/', $str);
 }
 
-/**
- *
- * @param <type> $sender_id
- * @param <type> $nickname
- * @return <type>
- * @access private
- */
-function common_group_link($sender_id, $nickname)
-{
-    $sender = Profile::getKV($sender_id);
-    $group = User_group::getForNickname($nickname, $sender);
-    if ($sender && $group && $sender->isMember($group)) {
-        $attrs = array('href' => $group->permalink(),
-                       'class' => 'url');
-        if (!empty($group->fullname)) {
-            $attrs['title'] = $group->getFancyName();
-        }
-        $xs = new XMLStringer();
-        $xs->elementStart('span', 'vcard');
-        $xs->elementStart('a', $attrs);
-        $xs->element('span', 'fn nickname group', $nickname);
-        $xs->elementEnd('a');
-        $xs->elementEnd('span');
-        return $xs->getString();
-    } else {
-        return $nickname;
-    }
-}
-
 /**
  * Resolve an ambiguous profile nickname reference, checking in following order:
  * - profiles that $sender subscribes to
@@ -1222,7 +1210,7 @@ function common_relative_profile($sender, $nickname, $dt=null)
         return $recipient;
     }
     // If this is a local user, try to find a local user with that nickname.
-    $sender = User::getKV($sender->id);
+    $sender = User::getKV('id', $sender->id);
     if ($sender instanceof User) {
         $recipient_user = User::getKV('nickname', $nickname);
         if ($recipient_user instanceof User) {
@@ -2020,8 +2008,8 @@ function common_profile_uri($profile)
 
     if (!empty($profile)) {
         if (Event::handle('StartCommonProfileURI', array($profile, &$uri))) {
-            $user = User::getKV($profile->id);
+            $user = User::getKV('id', $profile->id);
-            if (!empty($user)) {
+            if ($user instanceof User) {
                 $uri = $user->uri;
             }
             Event::handle('EndCommonProfileURI', array($profile, &$uri));

plugins/OStatus/OStatusPlugin.php

@@ -337,7 +337,7 @@ class OStatusPlugin extends Plugin
      * @param array &$mention in/out param: set of found mentions
      * @return boolean hook return value
      */
-    function onEndFindMentions($sender, $text, &$mentions)
+    function onEndFindMentions(Profile $sender, $text, &$mentions)
     {
         $matches = array();
 

scripts/createsim.php

@@ -163,7 +163,7 @@ function newNotice($i, $tagmax)
 
     if ($in_group == 0) {
         $groups = $user->getGroups();
-        if ($groups->N > 0) {
+        if ($groups instanceof User_group) {
             $gval = rand(0, $groups->N - 1);
             $groups->fetch(); // go to 0th
             for ($i = 0; $i < $gval; $i++) {