I like to throw exceptions instead of using if statements.

diff --git a/classes/File.php b/classes/File.php
index b0da3f09f3e418548f6e9827707d61f5f7251702..9cfdb94c564fe0db38ada239bc2e4f2df4793755 100644 (file)
--- a/classes/File.php
+++ b/classes/File.php
@@ -194,10 +194,14 @@ class File extends Managed_DataObject
         }
 
         $redir = File_redirection::where($given_url);
-        $file = $redir->getFile();
-
-        if (!$file instanceof File || empty($file->id)) {
+        try {
+            $file = $redir->getFile();
+        } catch (EmptyPkeyValueException $e) {
+            common_log(LOG_ERR, 'File_redirection::where gave object with empty file_id for given_url '._ve($given_url));
+            throw new ServerException('URL processing failed without new File object');
+        } catch (NoResultException $e) {
             // This should not happen
+            common_log(LOG_ERR, 'File_redirection after discovery could still not return a File object.');
             throw new ServerException('URL processing failed without new File object');
         }
 

diff --git a/classes/File_redirection.php b/classes/File_redirection.php
index d1b266c90b4a89f96625ece8e3b81084040519b0..742a6143cc43a93278c579accf90faf891914861 100644 (file)
--- a/classes/File_redirection.php
+++ b/classes/File_redirection.php
@@ -445,8 +445,8 @@ class File_redirection extends Managed_DataObject
     }
 
     public function getFile() {
-        if(empty($this->file) && $this->file_id) {
-            $this->file = File::getKV('id', $this->file_id);
+        if (!$this->file instanceof File) {
+            $this->file = File::getByID($this->file_id);
         }
 
         return $this->file;