From db443e9374ebfe7f098ef1b236cac6e8f4b82fbf Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Mon, 2 Jun 2014 02:08:48 +0200 Subject: [PATCH] File::processNew now static and always throws exception on failure --- classes/File.php | 60 ++++++++++++--------- classes/Notice.php | 12 ++++- lib/util.php | 17 +++--- plugins/Bookmark/actions/bookmarkforurl.php | 14 ++--- plugins/TwitterBridge/lib/twitterimport.php | 6 ++- 5 files changed, 68 insertions(+), 41 deletions(-) diff --git a/classes/File.php b/classes/File.php index 5acf6a7b50..9b703aa425 100644 --- 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 25422aa332..f055096c33 100644 --- 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 97c0a5aa57..ec0d7c6a25 100644 --- 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 5eac33b11b..c4cc4a8487 100644 --- 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 124d9e2cfa..41d8ac9d46 100644 --- 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 + } } } }