File::processNew now static and always throws exception on failure

diff --git a/classes/File.php b/classes/File.php
index 5acf6a7b50bbacfff11dd4e96a96f578eceaaa34..9b703aa425ae25d9fc3b6e4c976b8fd778897151 100644 (file)
--- a/classes/File.php
+++ b/classes/File.php
@@ -91,6 +91,7 @@ class File extends Managed_DataObject
         }
 
         Event::handle('EndFileSaveNew', array($file, $redir_data, $given_url));
+        assert ($file instanceof File);
         return $file;
     }
 
@@ -109,16 +110,32 @@ class File extends Managed_DataObject
      *
      * @return mixed File on success, -1 on some errors
      *
-     * @throws ServerException on some errors
+     * @throws ServerException on failure
      */
-    public function processNew($given_url, $notice_id=null, $followRedirects=true) {
-        if (empty($given_url)) return -1;   // error, no url to process
+    public static function processNew($given_url, $notice_id=null, $followRedirects=true) {
+        if (empty($given_url)) {
+            throw new ServerException('No given URL to process');
+        }
+
         $given_url = File_redirection::_canonUrl($given_url);
-        if (empty($given_url)) return -1;   // error, no url to process
+        if (empty($given_url)) {
+            throw new ServerException('No canonical URL from given URL to process');
+        }
+
         $file = File::getKV('url', $given_url);
-        if (empty($file)) {
+        if (!$file instanceof File) {
+            // First check if we have a lookup trace for this URL already
             $file_redir = File_redirection::getKV('url', $given_url);
-            if (empty($file_redir)) {
+            if ($file_redir instanceof File_redirection) {
+                $file = File::getKV('id', $file_redir->file_id);
+                if (!$file instanceof File) {
+                    // File did not exist, let's clean up the File_redirection entry
+                    $file_redir->delete();
+                }
+            }
+
+            // If we still don't have a File object, let's create one now!
+            if (!$file instanceof File) {
                 // @fixme for new URLs this also looks up non-redirect data
                 // such as target content type, size, etc, which we need
                 // for File::saveNew(); so we call it even if not following
@@ -133,10 +150,11 @@ class File extends Managed_DataObject
                     // TRANS: Server exception thrown when a URL cannot be processed.
                     throw new ServerException(sprintf(_("Cannot process URL '%s'"), $given_url));
                 }
+
                 // TODO: max field length
                 if ($redir_url === $given_url || strlen($redir_url) > 255 || !$followRedirects) {
-                    $x = File::saveNew($redir_data, $given_url);
-                    $file_id = $x->id;
+                    // Save the File object based on our lookup trace
+                    $file = File::saveNew($redir_data, $given_url);
                 } else {
                     // This seems kind of messed up... for now skipping this part
                     // if we're already under a redirect, so we don't go into
@@ -146,31 +164,23 @@ class File extends Managed_DataObject
                     //
                     // Seen in the wild with clojure.org, which redirects through
                     // wikispaces for auth and appends session data in the URL params.
-                    $x = File::processNew($redir_url, $notice_id, /*followRedirects*/false);
-                    $file_id = $x->id;
-                    File_redirection::saveNew($redir_data, $file_id, $given_url);
+                    $file = self::processNew($redir_url, $notice_id, /*followRedirects*/false);
+                    File_redirection::saveNew($redir_data, $file->id, $given_url);
                 }
-            } else {
-                $file_id = $file_redir->file_id;
             }
-        } else {
-            $file_id = $file->id;
-            $x = $file;
-        }
 
-        if (empty($x)) {
-            $x = File::getKV('id', $file_id);
-            if (empty($x)) {
-                // @todo FIXME: This could possibly be a clearer message :)
-                // TRANS: Server exception thrown when... Robin thinks something is impossible!
-                throw new ServerException(_('Robin thinks something is impossible.'));
+            if (!$file instanceof File) {
+                // This should only happen if File::saveNew somehow did not return a File object,
+                // though we have an assert for that in case the event there might've gone wrong.
+                // If anything else goes wrong, there should've been an exception thrown.
+                throw new ServerException('URL processing failed without new File object');
             }
         }
 
         if (!empty($notice_id)) {
-            File_to_post::processNew($file_id, $notice_id);
+            File_to_post::processNew($file->id, $notice_id);
         }
-        return $x;
+        return $file;
     }
 
     public static function respectsQuota(Profile $scoped, $fileSize) {

diff --git a/classes/Notice.php b/classes/Notice.php
index 25422aa332d5b8ef6b421075f8e505aeaeff3899..f055096c33f4f22c7cc2c4c5cc5ea6465487c33e 100644 (file)
--- a/classes/Notice.php
+++ b/classes/Notice.php
@@ -822,7 +822,11 @@ class Notice extends Managed_DataObject
         if (common_config('attachments', 'process_links')) {
             // @fixme validation?
             foreach (array_unique($urls) as $url) {
-                File::processNew($url, $this->id);
+                try {
+                    File::processNew($url, $this->id);
+                } catch (ServerException $e) {
+                    // Could not save URL. Log it?
+                }
             }
         }
     }
@@ -831,7 +835,11 @@ class Notice extends Managed_DataObject
      * @private callback
      */
     function saveUrl($url, $notice_id) {
-        File::processNew($url, $notice_id);
+        try {
+            File::processNew($url, $notice_id);
+        } catch (ServerException $e) {
+            // Could not save URL. Log it?
+        }
     }
 
     static function checkDupes($profile_id, $content) {

diff --git a/lib/util.php b/lib/util.php
index 97c0a5aa57517c75752241224324a067d1f17f5b..ec0d7c6a251913a26cd7dbea53760c5024578fd7 100644 (file)
--- a/lib/util.php
+++ b/lib/util.php
@@ -992,7 +992,11 @@ function common_linkify($url) {
     if (!$f instanceof File) {
         if (common_config('attachments', 'process_links')) {
             // XXX: this writes to the database. :<
-            $f = File::processNew($longurl);
+            try {
+                $f = File::processNew($longurl);
+            } catch (ServerException $e) {
+                $f = null;
+            }
         }
     }
 
@@ -2170,17 +2174,16 @@ function common_shorten_url($long_url, User $user=null, $force = false)
     if (Event::handle('StartShortenUrl',
                       array($long_url, $shortenerName, &$shortenedUrl))) {
         if ($shortenerName == 'internal') {
-            $f = File::processNew($long_url);
-            if (empty($f)) {
-                return $long_url;
-            } else {
-                $shortenedUrl = common_local_url('redirecturl',
-                                                 array('id' => $f->id));
+            try {
+                $f = File::processNew($long_url);
+                $shortenedUrl = common_local_url('redirecturl', array('id' => $f->id));
                 if ((mb_strlen($shortenedUrl) < mb_strlen($long_url)) || $force) {
                     return $shortenedUrl;
                 } else {
                     return $long_url;
                 }
+            } catch (ServerException $e) {
+                return $long_url;
             }
         } else {
             return $long_url;

diff --git a/plugins/Bookmark/actions/bookmarkforurl.php b/plugins/Bookmark/actions/bookmarkforurl.php
index 5eac33b11b73b7f32530c06812386093694ca135..c4cc4a8487cd81ed8c00f73036ca1b24c9d527ed 100644 (file)
--- a/plugins/Bookmark/actions/bookmarkforurl.php
+++ b/plugins/Bookmark/actions/bookmarkforurl.php
@@ -78,17 +78,19 @@ class BookmarkforurlAction extends Action
             throw new ClientException(_('Invalid URL.'), 400);
         }
 
-        $f = File::getKV('url', $this->url);
-
-        if (empty($url)) { 
-           $f = File::processNew($this->url);
+        try {
+            // processNew will first try to fetch a locally stored File entry
+            $f = File::processNew($this->url);
+        } catch (ServerException $e) {
+            $f = null;
         }
 
         // How about now?
 
-        if (!empty($f)) {
+        if ($f instanceof File) {
+            // FIXME: Use some File metadata Event instead
             $this->oembed    = File_oembed::getKV('file_id', $f->id);
-            if (!empty($this->oembed)) {
+            if ($this->oembed instanceof File_oembed) {
                 $this->title = $this->oembed->title;
             }
             $this->thumbnail = File_thumbnail::getKV('file_id', $f->id);

diff --git a/plugins/TwitterBridge/lib/twitterimport.php b/plugins/TwitterBridge/lib/twitterimport.php
index 124d9e2cfaebcd59a63a5ac13273a697bb2e86a2..41d8ac9d469831490498fd1b00af399928027f59 100644 (file)
--- a/plugins/TwitterBridge/lib/twitterimport.php
+++ b/plugins/TwitterBridge/lib/twitterimport.php
@@ -569,7 +569,11 @@ class TwitterImport
         if (common_config('attachments', 'process_links')) {
             if (!empty($status->entities) && !empty($status->entities->urls)) {
                 foreach ($status->entities->urls as $url) {
-                    File::processNew($url->url, $notice->id);
+                    try {
+                        File::processNew($url->url, $notice->id);
+                    } catch (ServerException $e) {
+                        // Could not process attached URL
+                    }
                 }
             }
         }