Relatively experimental change to store thumbnails in 'file/thumb/' (by default)

actions/attachment.php

@@ -96,7 +96,7 @@ class AttachmentAction extends ManagedAction
     {
         if (empty($this->attachment->filename)) {
             // if it's not a local file, gtfo
-            common_redirect($this->attachment->url, 303);
+            common_redirect($this->attachment->getUrl(), 303);
         }
 
         parent::showPage();

actions/attachment_thumbnail.php

@@ -62,6 +62,6 @@ class Attachment_thumbnailAction extends AttachmentAction
             common_redirect($e->file->getUrl(), 302);
         }
 
-        common_redirect(File_thumbnail::url($thumbnail->filename), 302);
+        common_redirect(File_thumbnail::url($thumbnail->getFilename()), 302);
     }
 }

classes/File.php

@@ -500,7 +500,7 @@ class File extends Managed_DataObject
     {
         if ($prefer_local && !empty($this->filename)) {
             // A locally stored file, so let's generate a URL for our instance.
-            return self::url($this->filename);
+            return self::url($this->getFilename());
         }
 
         // No local filename available, return the URL we have stored

classes/File_thumbnail.php

@@ -129,23 +129,76 @@ class File_thumbnail extends Managed_DataObject
 
     static function path($filename)
     {
-        // TODO: Store thumbnails in their own directory and don't use File::path here
+        if (!File::validFilename($filename)) {
-        return File::path($filename);
+            // TRANS: Client exception thrown if a file upload does not have a valid name.
+            throw new ClientException(_('Invalid filename.'));
+        }
+
+        $dir = common_config('thumbnail', 'dir') ?: File::path('thumb');
+
+        if (!in_array($dir[mb_strlen($dir)-1], ['/', '\\'])) {
+            $dir .= DIRECTORY_SEPARATOR;
+        }
+
+        return $dir . $filename;
     }
 
     static function url($filename)
     {
-        // TODO: Store thumbnails in their own directory and don't use File::url here
+        if (!File::validFilename($filename)) {
-        return File::url($filename);
+            // TRANS: Client exception thrown if a file upload does not have a valid name.
+            throw new ClientException(_('Invalid filename.'));
+        }
+
+        // FIXME: private site thumbnails?
+
+        $path = common_config('thumbnail', 'path');
+        if (empty($path)) {
+            return File::url('thumb')."/{$filename}";
+        }
+
+        $protocol = (GNUsocial::useHTTPS() ? 'https' : 'http');
+        $server = common_config('thumbnail', 'server') ?: common_config('site', 'server');
+
+        if ($path[mb_strlen($path)-1] != '/') {
+            $path .= '/';
+        }
+        if ($path[0] != '/') {
+            $path = '/'.$path;
+        }
+
+        return $protocol.'://'.$server.$path.$filename;
+    }
+
+    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;
     }
 
     public function getPath()
     {
-        $filepath = self::path($this->filename);
+        $oldpath = File::path($this->getFilename());
-        if (!file_exists($filepath)) {
+        $thumbpath = self::path($this->getFilename());
-            throw new FileNotFoundException($filepath);
+
+        // If we have a file in our old thumbnail storage path, move it to the new one
+        if (file_exists($oldpath) && !file_exists($thumbpath)) {
+            if ($this->getFilename() === $this->getFile()->filename) {
+                // special case where thumbnail file exactly matches stored File
+                common_debug('File filename and File_thumbnail filename match on '.$this->file_id);
+            } elseif (!rename($oldpath, $thumbpath)) {
+                common_log(LOG_ERR, 'Could not move thumbnail from '._ve($oldpath).' to '._ve($thumbpath));
+                throw new ServerException('Could not move thumbnail from old path to new path.');
+            } else {
+                common_log(LOG_DEBUG, 'Moved thumbnail '.$this->file_id.' from '._ve($oldpath).' to '._ve($thumbpath));
             }
-        return $filepath;
+        } elseif (!file_exists($thumbpath)) {
+            throw new FileNotFoundException($thumbpath);
+        }
+        return $thumbpath;
     }
 
     public function getUrl()
@@ -188,10 +241,14 @@ class File_thumbnail extends Managed_DataObject
 
     public function delete($useWhere=false)
     {
-        if (!empty($this->filename) && file_exists(File_thumbnail::path($this->filename))) {
+        if (!empty($this->filename)) {
-            $deleted = @unlink(self::path($this->filename));
+            try {
+                $deleted = @unlink($this->getPath());
                 if (!$deleted) {
-                common_log(LOG_ERR, sprintf('Could not unlink existing file: "%s"', self::path($this->filename)));
+                    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));
             }
         }
 

lib/attachmentlistitem.php

@@ -204,11 +204,7 @@ class AttachmentListItem extends Widget
      */
     protected function scrubHtmlFile(File $attachment)
     {
-        $path = File::path($attachment->filename);
+        $path = $attachment->getPath();
-        if (!file_exists($path) || !is_readable($path)) {
-            common_log(LOG_ERR, "Missing local HTML attachment $path");
-            return false;
-        }
         $raw = file_get_contents($path);
 
         // Normalize...

lib/default.php

@@ -271,13 +271,18 @@ $default =
                     'exe' => false,  // this would deny any uploads to keep the "exe" file extension
                 ],
               ),
-        'thumbnail' =>
+        'thumbnail' => [
-        array('crop' => false,      // overridden to true if thumb height === null
+              'dir' => null,    // falls back to File::path('thumb') (equivalent to ['attachments']['dir'] .  '/thumb/')
+              'path' => null,   // falls back to generating a URL with File::url('thumb/$filename') (equivalent to ['attachments']['path'] . '/thumb/')
+              'server' => null, // Only used if ['thumbnail']['path'] is NOT empty, and then it falls back to ['site']['server'], schema is decided from GNUsocial::useHTTPS()
+
+              'crop' => false,      // overridden to true if thumb height === null
               'maxsize' => 1000,     // thumbs with an edge larger than this will not be generated
               'width' => 450,
               'height' => 600,
               'upscale' => false,
-              'animated' => false), // null="UseFileAsThumbnail", false="can use still frame". true requires ImageMagickPlugin
+              'animated' => false, // null="UseFileAsThumbnail", false="can use still frame". true requires ImageMagickPlugin
+            ],
         'application' =>
         array('desclimit' => null),
         'group' =>

lib/gnusocial.php

@@ -429,10 +429,28 @@ class GNUsocial
      */
     static function verifyLoadedConfig()
     {
+        $mkdirs = [];
+
         if (common_config('htmlpurifier', 'Cache.DefinitionImpl') === 'Serializer'
                 && !is_dir(common_config('htmlpurifier', 'Cache.SerializerPath'))) {
-            if (!mkdir(common_config('htmlpurifier', 'Cache.SerializerPath'))) {
+            $mkdirs[common_config('htmlpurifier', 'Cache.SerializerPath')] = 'HTMLPurifier Serializer cache';
-                throw new ConfigException('Could not create HTMLPurifier cache dir: '._ve(common_config('htmlpurifier', 'Cache.SerializerPath')));
+        }
+
+        // go through our configurable storage directories
+        foreach (['attachments', 'thumbnail'] as $dirtype) {
+            $dir = common_config($dirtype, 'dir');
+            if (!empty($dir) && !is_dir($dir)) {
+                $mkdirs[$dir] = $dirtype;
+            }
+        }
+
+        // try to create those that are not directories
+        foreach ($mkdirs as $dir=>$description) {
+            if (is_file($dir)) {
+                throw new ConfigException('Expected directory for '._ve($description).' is a file!');
+            }
+            if (!mkdir($dir)) {
+                throw new ConfigException('Could not create directory for '._ve($description).': '._ve($dir));
             }
         }
 

lib/imagefile.php

@@ -153,8 +153,14 @@ class ImageFile
             $image = new ImageFile($file->getID(), $imgPath);
         } catch (UnsupportedMediaException $e) {
             // Avoid deleting the original
-            if ($imgPath != $file->getPath()) {
+            try {
-                unlink($imgPath);
+                if ($imgPath !== $file->getPath()) {
+                    @unlink($imgPath);
+                }
+            } catch (FileNotFoundException $e) {
+                // File reported (via getPath) that the original file
+                // doesn't exist anyway, so it's safe to delete $imgPath
+                @unlink($imgPath);
             }
             throw $e;
         }
@@ -607,10 +613,15 @@ class ImageFile
         // Perform resize and store into file
         $this->resizeTo($outpath, $box);
 
+        try {
             // Avoid deleting the original
-        if ($this->getPath() != File_thumbnail::path($this->filename)) {
+            if (!in_array($this->getPath(), [File::path($this->filename), File_thumbnail::path($this->filename)])) {
                 $this->unlink();
             }
+        } catch (FileNotFoundException $e) {
+            // $this->getPath() says the file doesn't exist anyway, so no point in trying to delete it!
+        }
+
         return File_thumbnail::saveThumbnail($this->fileRecord->getID(),
                                       null, // no url since we generated it ourselves and can dynamically generate the url
                                       $width, $height,

plugins/Oembed/OembedPlugin.php

@@ -328,7 +328,7 @@ class OembedPlugin extends Plugin
         $ext = File::guessMimeExtension($info['mime']);
 
         // We'll trust sha256 (File::FILEHASH_ALG) not to have collision issues any time soon :)
-        $filename = hash(File::FILEHASH_ALG, $imgData) . ".{$ext}";
+        $filename = 'oembed-'.hash(File::FILEHASH_ALG, $imgData) . ".{$ext}";
         $fullpath = File_thumbnail::path($filename);
         // Write the file to disk. Throw Exception on failure
         if (!file_exists($fullpath) && file_put_contents($fullpath, $imgData) === false) {