Make sure File_thumbnail->getPath() doesn't throw NoResultException

diff --git a/classes/File.php b/classes/File.php
index 0368153acda7a2acba235ef567774d69199061aa..9643b78f189ef6c8ed8f6a7389c6a23c0b4c52d3 100644 (file)
--- a/classes/File.php
+++ b/classes/File.php
@@ -260,11 +260,7 @@ class File extends Managed_DataObject
 
     public function getFilename()
     {
-        if (!self::validFilename($this->filename)) {
-            // TRANS: Client exception thrown if a file upload does not have a valid name.
-            throw new ClientException(_("Invalid filename."));
-        }
-        return $this->filename;
+        return self::tryFilename($this->filename);
     }
 
     // where should the file go?
@@ -349,15 +345,23 @@ class File extends Managed_DataObject
         return preg_match('/^[A-Za-z0-9._-]+$/', $filename);
     }
 
+    static function tryFilename($filename)
+    {
+        if (!self::validFilename($filename))
+        {
+            throw new InvalidFilenameException($filename);
+        }
+        // if successful, return the filename for easy if-statementing
+        return $filename;
+    }
+
     /**
      * @throws ClientException on invalid filename
      */
     static function path($filename)
     {
-        if (!self::validFilename($filename)) {
-            // TRANS: Client exception thrown if a file upload does not have a valid name.
-            throw new ClientException(_("Invalid filename."));
-        }
+        self::tryFilename($filename);
+
         $dir = common_config('attachments', 'dir');
 
         if (!in_array($dir[mb_strlen($dir)-1], ['/', '\\'])) {
@@ -369,10 +373,7 @@ class File extends Managed_DataObject
 
     static function url($filename)
     {
-        if (!self::validFilename($filename)) {
-            // TRANS: Client exception thrown if a file upload does not have a valid name.
-            throw new ClientException(_("Invalid filename."));
-        }
+        self::tryFilename($filename);
 
         if (common_config('site','private')) {
 

diff --git a/classes/File_thumbnail.php b/classes/File_thumbnail.php
index 4a4b25ff4d27becbba8917a127110dd060e7b516..f789a0d9a1637647efb13307c60be98f80d5fa7c 100644 (file)
--- a/classes/File_thumbnail.php
+++ b/classes/File_thumbnail.php
@@ -129,12 +129,9 @@ class File_thumbnail extends Managed_DataObject
 
     static function path($filename)
     {
-        if (!File::validFilename($filename)) {
-            // TRANS: Client exception thrown if a file upload does not have a valid name.
-            throw new ClientException(_('Invalid filename.'));
-        }
+        File::tryFilename($filename);
 
-        // NOTE: If this is empty, it will be set to File::path('thumb')
+        // NOTE: If this is left empty in default config, it will be set to File::path('thumb')
         $dir = common_config('thumbnail', 'dir');
 
         if (!in_array($dir[mb_strlen($dir)-1], ['/', '\\'])) {
@@ -146,10 +143,7 @@ class File_thumbnail extends Managed_DataObject
 
     static function url($filename)
     {
-        if (!File::validFilename($filename)) {
-            // TRANS: Client exception thrown if a file upload does not have a valid name.
-            throw new ClientException(_('Invalid filename.'));
-        }
+        File::tryFilename($filename);
 
         // FIXME: private site thumbnails?
 
@@ -173,21 +167,36 @@ class File_thumbnail extends Managed_DataObject
 
     public function getFilename()
     {
-        if (!File::validFilename($this->filename)) {
-            // TRANS: Client exception thrown if a file upload does not have a valid name.
-            throw new ClientException(_("Invalid filename."));
-        }
-        return $this->filename;
+        return File::tryFilename($this->filename);
     }
 
+    /**
+     *
+     * @return  string  full filesystem path to the locally stored thumbnail file
+     * @throws  
+     */
     public function getPath()
     {
         $oldpath = File::path($this->getFilename());
         $thumbpath = self::path($this->getFilename());
 
-        // If we have a file in our old thumbnail storage path, move it to the new one
+        // If we have a file in our old thumbnail storage path, move (or copy) it to the new one
+        // (if the if/elseif don't match, we have a $thumbpath just as we should and can return it)
         if (file_exists($oldpath) && !file_exists($thumbpath)) {
-            if ($this->getFilename() === $this->getFile()->filename) {
+            try {
+                // let's get the filename of the File, to check below if it happens to be identical
+                $file_filename = $this->getFile()->getFilename();
+            } catch (NoResultException $e) {
+                // reasonably the function calling us will handle the following as "File_thumbnail entry should be deleted"
+                throw FileNotFoundException($thumbpath);
+            } catch (InvalidFilenameException $e) {
+                // invalid filename in getFile()->getFilename(), just
+                // means the File object isn't stored locally and that
+                // means it's safe to move it below.
+                $file_filename = null;
+            }
+
+            if ($this->getFilename() === $file_filename) {
                 // special case where thumbnail file exactly matches stored File filename
                 common_debug('File filename and File_thumbnail filename match on '.$this->file_id.', copying instead');
                 copy($oldpath, $thumbpath);
@@ -200,6 +209,7 @@ class File_thumbnail extends Managed_DataObject
         } elseif (!file_exists($thumbpath)) {
             throw new FileNotFoundException($thumbpath);
         }
+
         return $thumbpath;
     }
 
@@ -243,15 +253,15 @@ class File_thumbnail extends Managed_DataObject
 
     public function delete($useWhere=false)
     {
-        if (!empty($this->filename)) {
-            try {
-                $deleted = @unlink($this->getPath());
-                if (!$deleted) {
-                    common_log(LOG_ERR, 'Could not unlink existing thumbnail file: '._ve($this->getPath()));
-                }
-            } catch (FileNotFoundException $e) {
-                common_log(LOG_INFO, 'Thumbnail already gone from '._ve($e->path));
+        try {
+            $thumbpath = self::path($this->getFilename());
+            // if file does not exist, try to delete it
+            $deleted = !file_exists($thumbpath) || @unlink($thumbpath);
+            if (!$deleted) {
+                common_log(LOG_ERR, 'Could not unlink existing thumbnail file: '._ve($thumbpath));
             }
+        } catch (InvalidFilenameException $e) {
+            common_log(LOG_ERR, 'Deleting object but not attempting deleting file: '._ve($e->getMessage()));
         }
 
         return parent::delete($useWhere);

diff --git a/scripts/upgrade.php b/scripts/upgrade.php
index c9fe4b081755ffd706c94259bb4fe259e2b5a436..fa55cfc206d9507367fafa5f7ba35efe7d00131d 100755 (executable)
--- a/scripts/upgrade.php
+++ b/scripts/upgrade.php
@@ -503,7 +503,7 @@ function setFilehashOnLocalFiles()
     printfnq('Ensuring all local files have the filehash field set...');
 
     $file = new File();
-    $file->whereAdd('filename IS NOT NULL');        // local files
+    $file->whereAdd('filename IS NOT NULL AND filename != ""');        // local files
     $file->whereAdd('filehash IS NULL', 'AND');     // without filehash value
 
     if ($file->find()) {