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

This commit is contained in:
Mikael Nordfeldth 2014-06-02 02:08:48 +02:00
parent d6f52f5939
commit db443e9374
5 changed files with 68 additions and 41 deletions

View File

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

View File

@ -822,7 +822,11 @@ class Notice extends Managed_DataObject
if (common_config('attachments', 'process_links')) { if (common_config('attachments', 'process_links')) {
// @fixme validation? // @fixme validation?
foreach (array_unique($urls) as $url) { foreach (array_unique($urls) as $url) {
try {
File::processNew($url, $this->id); 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 * @private callback
*/ */
function saveUrl($url, $notice_id) { function saveUrl($url, $notice_id) {
try {
File::processNew($url, $notice_id); File::processNew($url, $notice_id);
} catch (ServerException $e) {
// Could not save URL. Log it?
}
} }
static function checkDupes($profile_id, $content) { static function checkDupes($profile_id, $content) {

View File

@ -992,7 +992,11 @@ function common_linkify($url) {
if (!$f instanceof File) { if (!$f instanceof File) {
if (common_config('attachments', 'process_links')) { if (common_config('attachments', 'process_links')) {
// XXX: this writes to the database. :< // XXX: this writes to the database. :<
try {
$f = File::processNew($longurl); $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', if (Event::handle('StartShortenUrl',
array($long_url, $shortenerName, &$shortenedUrl))) { array($long_url, $shortenerName, &$shortenedUrl))) {
if ($shortenerName == 'internal') { if ($shortenerName == 'internal') {
try {
$f = File::processNew($long_url); $f = File::processNew($long_url);
if (empty($f)) { $shortenedUrl = common_local_url('redirecturl', array('id' => $f->id));
return $long_url;
} else {
$shortenedUrl = common_local_url('redirecturl',
array('id' => $f->id));
if ((mb_strlen($shortenedUrl) < mb_strlen($long_url)) || $force) { if ((mb_strlen($shortenedUrl) < mb_strlen($long_url)) || $force) {
return $shortenedUrl; return $shortenedUrl;
} else { } else {
return $long_url; return $long_url;
} }
} catch (ServerException $e) {
return $long_url;
} }
} else { } else {
return $long_url; return $long_url;

View File

@ -78,17 +78,19 @@ class BookmarkforurlAction extends Action
throw new ClientException(_('Invalid URL.'), 400); throw new ClientException(_('Invalid URL.'), 400);
} }
$f = File::getKV('url', $this->url); try {
// processNew will first try to fetch a locally stored File entry
if (empty($url)) {
$f = File::processNew($this->url); $f = File::processNew($this->url);
} catch (ServerException $e) {
$f = null;
} }
// How about now? // 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); $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->title = $this->oembed->title;
} }
$this->thumbnail = File_thumbnail::getKV('file_id', $f->id); $this->thumbnail = File_thumbnail::getKV('file_id', $f->id);

View File

@ -569,7 +569,11 @@ class TwitterImport
if (common_config('attachments', 'process_links')) { if (common_config('attachments', 'process_links')) {
if (!empty($status->entities) && !empty($status->entities->urls)) { if (!empty($status->entities) && !empty($status->entities->urls)) {
foreach ($status->entities->urls as $url) { foreach ($status->entities->urls as $url) {
try {
File::processNew($url->url, $notice->id); File::processNew($url->url, $notice->id);
} catch (ServerException $e) {
// Could not process attached URL
}
} }
} }
} }