Fix for ticket #3007: .bmp avatar uploads weren't being properly converted to PNG...

Part of the reported issue was previuosly fixed by dc497ed0 (smaller size images being blanked).
This commit fixes the remaining bug with original-size avatars being left as BMP (which could include the 96px size for instance, which could cause problems in browsers not supporting BMP natively)

Added ImageFile::copyTo() as a convenient alias for resizeTo() when not resizing; this performs the BMP/XPM/XBM->PNG conversion if needed, or copies the original file.
Copying instead of using move_uploaded_file() is fine here since:
a) the files are cleaned up on script completion anyway (vs moving to remove it)
b) we're already performing getimagesize() and possibly load/resize on the file before this point (vs needing to move the file into a usable area to work with open_basedir restrictions that prevent working directly with uploaded files in the temp dir; since this would fail anyway, we lose nothing)

ImageFile::preferredType() now works on $this->type instead of asking for one, to make it handier to use from outside. (This is still needed in order for calling code to generate a target filename.)

Recommended for future:
* additional consolidation between the various ways of uploading avatars (touched avatarsettings, grouplogo, and apiaccountupdateprofileimage with similar minor changes)
* consolidate type checks and file naming into Avatar class

diff --git a/actions/apiaccountupdateprofileimage.php b/actions/apiaccountupdateprofileimage.php
index f2886509d7cf4338ba994ab22e84c7ad90add934..6c53e2cbc1a2d0e98ee09ab379c586761134ee5a 100644 (file)
--- a/actions/apiaccountupdateprofileimage.php
+++ b/actions/apiaccountupdateprofileimage.php
@@ -112,16 +112,17 @@ class ApiAccountUpdateProfileImageAction extends ApiAuthAction
             return;
         }
 
+        $type = $imagefile->preferredType();
         $filename = Avatar::filename(
             $user->id,
-            image_type_to_extension($imagefile->type),
+            image_type_to_extension($type),
             null,
             'tmp'.common_timestamp()
         );
 
         $filepath = Avatar::path($filename);
 
-        move_uploaded_file($imagefile->filepath, $filepath);
+        $imagefile->copyTo($filepath);
 
         $profile = $this->user->getProfile();
 

diff --git a/actions/avatarsettings.php b/actions/avatarsettings.php
index 375420c5c9d5fc656f20b80054333438e28e1bb1..10c82ebfd72d722d644e5860428d33854a59b35f 100644 (file)
--- a/actions/avatarsettings.php
+++ b/actions/avatarsettings.php
@@ -320,21 +320,20 @@ class AvatarsettingsAction extends AccountSettingsAction
         }
 
         $cur = common_current_user();
-
+        $type = $imagefile->preferredType();
         $filename = Avatar::filename($cur->id,
-                                     image_type_to_extension($imagefile->type),
+                                     image_type_to_extension($type),
                                      null,
                                      'tmp'.common_timestamp());
 
         $filepath = Avatar::path($filename);
-
-        move_uploaded_file($imagefile->filepath, $filepath);
+        $imagefile->copyTo($filepath);
 
         $filedata = array('filename' => $filename,
                           'filepath' => $filepath,
                           'width' => $imagefile->width,
                           'height' => $imagefile->height,
-                          'type' => $imagefile->type);
+                          'type' => $type);
 
         $_SESSION['FILEDATA'] = $filedata;
 

diff --git a/actions/grouplogo.php b/actions/grouplogo.php
index 7b7c01b23b9fc7425d63585971669e713e86207d..d2e8fd0e91bf290dda890e154ee238b1add0d759 100644 (file)
--- a/actions/grouplogo.php
+++ b/actions/grouplogo.php
@@ -353,20 +353,21 @@ class GrouplogoAction extends GroupDesignAction
             return;
         }
 
+        $type = $imagefile->preferredType();
         $filename = Avatar::filename($this->group->id,
-                                     image_type_to_extension($imagefile->type),
+                                     image_type_to_extension($type),
                                      null,
                                      'group-temp-'.common_timestamp());
 
         $filepath = Avatar::path($filename);
 
-        move_uploaded_file($imagefile->filepath, $filepath);
+        $imagefile->copyTo($filepath);
 
         $filedata = array('filename' => $filename,
                           'filepath' => $filepath,
                           'width' => $imagefile->width,
                           'height' => $imagefile->height,
-                          'type' => $imagefile->type);
+                          'type' => $type);
 
         $_SESSION['FILEDATA'] = $filedata;
 

diff --git a/lib/imagefile.php b/lib/imagefile.php
index 159deead61130a283e306c72c72a0c0469a38aba..56a7b7d4a20ad21f19dfdaf1edcbedab3c70891d 100644 (file)
--- a/lib/imagefile.php
+++ b/lib/imagefile.php
@@ -128,7 +128,7 @@ class ImageFile
      */
     function resize($size, $x = 0, $y = 0, $w = null, $h = null)
     {
-        $targetType = $this->preferredType($this->type);
+        $targetType = $this->preferredType();
         $outname = Avatar::filename($this->id,
                                     image_type_to_extension($targetType),
                                     $size,
@@ -138,6 +138,19 @@ class ImageFile
         return $outname;
     }
 
+    /**
+     * Copy the image file to the given destination.
+     * For obscure formats, this will automatically convert to PNG;
+     * otherwise the original file will be copied as-is.
+     *
+     * @param string $outpath
+     * @return string filename
+     */
+    function copyTo($outpath)
+    {
+        return $this->resizeTo($outpath, $this->width, $this->height);
+    }
+
     /**
      * Create and save a thumbnail image.
      *
@@ -154,7 +167,7 @@ class ImageFile
     {
         $w = ($w === null) ? $this->width:$w;
         $h = ($h === null) ? $this->height:$h;
-        $targetType = $this->preferredType($this->type);
+        $targetType = $this->preferredType();
 
         if (!file_exists($this->filepath)) {
             throw new Exception(_('Lost our file.'));
@@ -247,25 +260,25 @@ class ImageFile
     /**
      * Several obscure file types should be normalized to PNG on resize.
      *
-     * @param int $type
+     * @fixme consider flattening anything not GIF or JPEG to PNG
      * @return int
      */
-    function preferredType($type)
+    function preferredType()
     {
-        if($type == IMAGETYPE_BMP) {
+        if($this->type == IMAGETYPE_BMP) {
             //we don't want to save BMP... it's an inefficient, rare, antiquated format
             //save png instead
             return IMAGETYPE_PNG;
-        } else if($type == IMAGETYPE_WBMP) {
+        } else if($this->type == IMAGETYPE_WBMP) {
             //we don't want to save WBMP... it's a rare format that we can't guarantee clients will support
             //save png instead
             return IMAGETYPE_PNG;
-        } else if($type == IMAGETYPE_XBM) {
+        } else if($this->type == IMAGETYPE_XBM) {
             //we don't want to save XBM... it's a rare format that we can't guarantee clients will support
             //save png instead
             return IMAGETYPE_PNG;
         }
-        return $type;
+        return $this->type;
     }
 
     function unlink()