Found some unreachable code in Favorite

The portion after StartAtomPubNewActivity would never be reached since
Favorite handles that activity through ActivityHandlerPlugin nowadays.
So I cleaned it up and followed a couple of paths, making stuff prettier.

lib/activityhandlerplugin.php

@@ -430,12 +430,13 @@ abstract class ActivityHandlerPlugin extends Plugin
      * Handle object posted via AtomPub
      *
      * @param Activity &$activity Activity that was posted
-     * @param User     $user      User that posted it
+     * @param Profile   $scoped   Profile of user posting
      * @param Notice   &$notice   Resulting notice
      *
      * @return boolean hook value
      */
-    function onStartAtomPubNewActivity(Activity &$activity, $user, &$notice)
+    // FIXME: Make sure we can really do strong Notice typing with a $notice===null without having =null here
+    public function onStartAtomPubNewActivity(Activity &$activity, Profile $scoped, Notice &$notice)
     {
         if (!$this->isMyActivity($activity)) {
             return true;
@@ -443,10 +444,9 @@ abstract class ActivityHandlerPlugin extends Plugin
 
         $options = array('source' => 'atompub');
 
-        // $user->getProfile() is a Profile
+        $notice = $this->saveNoticeFromActivity($activity, $scoped, $options);
-        $notice = $this->saveNoticeFromActivity($activity,
+
-                                                $user->getProfile(),
+        Event::handle('EndAtomPubNewActivity', array($activity, $scoped, $notice));
-                                                $options);
 
         return false;
     }

lib/mail.php

@@ -675,6 +675,7 @@ function mail_notify_message($message, $from=null, $to=null)
  */
 function mail_notify_fave(User $rcpt, Profile $sender, Notice $notice)
 {
+    // This test is actually "if the sender is sandboxed"
     if (!$sender->hasRight(Right::EMAILONFAVE)) {
         return;
     }

plugins/Favorite/actions/atompubfavoritefeed.php

@@ -28,11 +28,7 @@
  * @link      http://status.net/
  */
 
-if (!defined('STATUSNET')) {
+if (!defined('GNUSOCIAL') && !defined('STATUSNET')) { exit(1); }
-    // This check helps protect against security problems;
-    // your code file can't be executed directly from the web.
-    exit(1);
-}
 
 /**
  * Feed of ActivityStreams 'favorite' actions
@@ -49,20 +45,13 @@ class AtompubfavoritefeedAction extends ApiAuthAction
     private $_profile = null;
     private $_faves   = null;
 
-    /**
+    protected function prepare(array $args=array())
-     * For initializing members of the class.
-     *
-     * @param array $argarray misc. arguments
-     *
-     * @return boolean true
-     */
-    function prepare($argarray)
     {
-        parent::prepare($argarray);
+        parent::prepare($args);
 
         $this->_profile = Profile::getKV('id', $this->trimmed('profile'));
 
-        if (empty($this->_profile)) {
+        if (!$this->_profile instanceof Profile) {
             // TRANS: Client exception thrown when requesting a favorite feed for a non-existing profile.
             throw new ClientException(_('No such profile.'), 404);
         }
@@ -77,16 +66,9 @@ class AtompubfavoritefeedAction extends ApiAuthAction
         return true;
     }
 
-    /**
+    protected function handle()
-     * Handler method
-     *
-     * @param array $argarray is ignored since it's now passed in in prepare()
-     *
-     * @return void
-     */
-    function handle($argarray=null)
     {
-        parent::handle($argarray);
+        parent::handle();
 
         switch ($_SERVER['REQUEST_METHOD']) {
         case 'HEAD':
@@ -99,10 +81,9 @@ class AtompubfavoritefeedAction extends ApiAuthAction
         default:
             // TRANS: Client exception thrown when using an unsupported HTTP method.
             throw new ClientException(_('HTTP method not supported.'), 405);
-            return;
         }
 
-        return;
+        return true;
     }
 
     /**
@@ -209,11 +190,10 @@ class AtompubfavoritefeedAction extends ApiAuthAction
     {
         // XXX: Refactor this; all the same for atompub
 
-        if (empty($this->auth_user) ||
+        if (!$this->scoped instanceof Profile ||
-            $this->auth_user->id != $this->_profile->id) {
+            $this->scoped->id != $this->_profile->id) {
             // TRANS: Client exception thrown when trying to set a favorite for another user.
-            throw new ClientException(_("Cannot add someone else's".
+            throw new ClientException(_("Cannot add someone else's subscription."), 403);
-                                        " subscription."), 403);
         }
 
         $xml = file_get_contents('php://input');
@@ -224,61 +204,17 @@ class AtompubfavoritefeedAction extends ApiAuthAction
             $dom->documentElement->localName != 'entry') {
             // TRANS: Client error displayed when not using an Atom entry.
             throw new ClientException(_('Atom post must be an Atom entry.'));
-            return;
         }
 
         $activity = new Activity($dom->documentElement);
+        $notice = null;
 
-        $fave = null;
+        // Favorite plugin handles these as ActivityHandlerPlugin through Notice->saveActivity
+        // which in turn uses "StoreActivityObject" event.
+        Event::handle('StartAtomPubNewActivity', array(&$activity, $this->scoped, &$notice));
+        assert($notice instanceof Notice);
 
-        if (Event::handle('StartAtomPubNewActivity', array(&$activity))) {
+        $act = $notice->asActivity();
-
-            if ($activity->verb != ActivityVerb::FAVORITE) {
-                // TRANS: Client exception thrown when trying use an incorrect activity verb for the Atom pub method.
-                throw new ClientException(_('Can only handle favorite activities.'));
-                return;
-            }
-
-            $note = $activity->objects[0];
-
-            if (!in_array($note->type, array(ActivityObject::NOTE,
-                                             ActivityObject::BLOGENTRY,
-                                             ActivityObject::STATUS))) {
-                // TRANS: Client exception thrown when trying favorite an object that is not a notice.
-                throw new ClientException(_('Can only fave notices.'));
-                return;
-            }
-
-            $notice = Notice::getKV('uri', $note->id);
-
-            if (empty($notice)) {
-                // XXX: import from listed URL or something
-                // TRANS: Client exception thrown when trying favorite a notice without content.
-                throw new ClientException(_('Unknown notice.'));
-            }
-
-            $old = Fave::pkeyGet(array('user_id' => $this->auth_user->id,
-                                       'notice_id' => $notice->id));
-
-            if (!empty($old)) {
-                // TRANS: Client exception thrown when trying favorite an already favorited notice.
-                throw new ClientException(_('Already a favorite.'));
-            }
-
-            $profile = $this->auth_user->getProfile();
-
-            $fave = Fave::addNew($profile, $notice);
-
-            if ($fave instanceof Fave) {
-                Fave::blowCacheForProfileId($this->_profile->id);
-                $this->notify($fave, $notice, $this->auth_user);
-            }
-
-            Event::handle('EndAtomPubNewActivity', array($activity, $fave));
-        }
-
-        if (!empty($fave)) {
-            $act = $fave->asActivity();
 
         header('Content-Type: application/atom+xml; charset=utf-8');
         header('Content-Location: ' . $act->selfLink);
@@ -287,7 +223,6 @@ class AtompubfavoritefeedAction extends ApiAuthAction
         $this->raw($act->asString(true, true, true));
         $this->endXML();
     }
-    }
 
     /**
      * Return true if read only.
@@ -348,25 +283,4 @@ class AtompubfavoritefeedAction extends ApiAuthAction
             return true;
         }
     }
-
-    /**
-     * Notify the author of the favorite that the user likes their notice
-     *
-     * @param Favorite $fave   the favorite in question
-     * @param Notice   $notice the notice that's been faved
-     * @param User     $user   the user doing the favoriting
-     *
-     * @return void
-     */
-    function notify($fave, $notice, $user)
-    {
-        $other = User::getKV('id', $notice->profile_id);
-        if ($other && $other->id != $user->id && !empty($other->email)) {
-            require_once INSTALLDIR.'/lib/mail.php';
-
-            mail_notify_fave($other, $user->getProfile(), $notice);
-            // XXX: notify by IM
-            // XXX: notify by SMS
-        }
-    }
 }

plugins/Favorite/classes/Fave.php

@@ -44,7 +44,8 @@ class Fave extends Managed_DataObject
      *
      * @param Profile $profile the local or remote user who likes
      * @param Notice $notice the notice that is liked
-     * @return mixed false on failure, or Fave record on success
+     * @return Fave record on success
+     * @throws Exception on failure
      */
     static function addNew(Profile $profile, Notice $notice) {
 
@@ -66,7 +67,7 @@ class Fave extends Managed_DataObject
                 $fave->insert();
             } catch (ServerException $e) {
                 common_log_db_error($fave, 'INSERT', __FILE__);
-                return false;
+                throw $e;
             }
             self::blowCacheForProfileId($fave->user_id);
             self::blowCacheForNoticeId($fave->notice_id);
@@ -158,7 +159,7 @@ class Fave extends Managed_DataObject
         return $act;
     }
 
-    static function existsForProfile($notice, Profile $scoped)
+    static function existsForProfile(Notice $notice, Profile $scoped)
     {
         $fave = self::pkeyGet(array('user_id'=>$scoped->id, 'notice_id'=>$notice->id));
 

plugins/Favorite/lib/favcommand.php

@@ -25,11 +25,10 @@ class FavCommand extends Command
             return; 
         } 
  
+        try {
             $fave = Fave::addNew($this->user->getProfile(), $notice); 
- 
+        } catch (Exception $e) {
-        if (!$fave) { 
+            $channel->error($this->user, $e->getMessage());
-            // TRANS: Error message text shown when a favorite could not be set. 
-            $channel->error($this->user, _('Could not create favorite.')); 
             return;
         }