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

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
This commit is contained in:
Brion Vibber 2011-01-24 12:22:47 -08:00
parent c309bbae93
commit 820dd293c9
4 changed files with 32 additions and 18 deletions

View File

@ -112,16 +112,17 @@ class ApiAccountUpdateProfileImageAction extends ApiAuthAction
return; return;
} }
$type = $imagefile->preferredType();
$filename = Avatar::filename( $filename = Avatar::filename(
$user->id, $user->id,
image_type_to_extension($imagefile->type), image_type_to_extension($type),
null, null,
'tmp'.common_timestamp() 'tmp'.common_timestamp()
); );
$filepath = Avatar::path($filename); $filepath = Avatar::path($filename);
move_uploaded_file($imagefile->filepath, $filepath); $imagefile->copyTo($filepath);
$profile = $this->user->getProfile(); $profile = $this->user->getProfile();

View File

@ -320,21 +320,20 @@ class AvatarsettingsAction extends AccountSettingsAction
} }
$cur = common_current_user(); $cur = common_current_user();
$type = $imagefile->preferredType();
$filename = Avatar::filename($cur->id, $filename = Avatar::filename($cur->id,
image_type_to_extension($imagefile->type), image_type_to_extension($type),
null, null,
'tmp'.common_timestamp()); 'tmp'.common_timestamp());
$filepath = Avatar::path($filename); $filepath = Avatar::path($filename);
$imagefile->copyTo($filepath);
move_uploaded_file($imagefile->filepath, $filepath);
$filedata = array('filename' => $filename, $filedata = array('filename' => $filename,
'filepath' => $filepath, 'filepath' => $filepath,
'width' => $imagefile->width, 'width' => $imagefile->width,
'height' => $imagefile->height, 'height' => $imagefile->height,
'type' => $imagefile->type); 'type' => $type);
$_SESSION['FILEDATA'] = $filedata; $_SESSION['FILEDATA'] = $filedata;

View File

@ -353,20 +353,21 @@ class GrouplogoAction extends GroupDesignAction
return; return;
} }
$type = $imagefile->preferredType();
$filename = Avatar::filename($this->group->id, $filename = Avatar::filename($this->group->id,
image_type_to_extension($imagefile->type), image_type_to_extension($type),
null, null,
'group-temp-'.common_timestamp()); 'group-temp-'.common_timestamp());
$filepath = Avatar::path($filename); $filepath = Avatar::path($filename);
move_uploaded_file($imagefile->filepath, $filepath); $imagefile->copyTo($filepath);
$filedata = array('filename' => $filename, $filedata = array('filename' => $filename,
'filepath' => $filepath, 'filepath' => $filepath,
'width' => $imagefile->width, 'width' => $imagefile->width,
'height' => $imagefile->height, 'height' => $imagefile->height,
'type' => $imagefile->type); 'type' => $type);
$_SESSION['FILEDATA'] = $filedata; $_SESSION['FILEDATA'] = $filedata;

View File

@ -128,7 +128,7 @@ class ImageFile
*/ */
function resize($size, $x = 0, $y = 0, $w = null, $h = null) function resize($size, $x = 0, $y = 0, $w = null, $h = null)
{ {
$targetType = $this->preferredType($this->type); $targetType = $this->preferredType();
$outname = Avatar::filename($this->id, $outname = Avatar::filename($this->id,
image_type_to_extension($targetType), image_type_to_extension($targetType),
$size, $size,
@ -138,6 +138,19 @@ class ImageFile
return $outname; 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. * Create and save a thumbnail image.
* *
@ -154,7 +167,7 @@ class ImageFile
{ {
$w = ($w === null) ? $this->width:$w; $w = ($w === null) ? $this->width:$w;
$h = ($h === null) ? $this->height:$h; $h = ($h === null) ? $this->height:$h;
$targetType = $this->preferredType($this->type); $targetType = $this->preferredType();
if (!file_exists($this->filepath)) { if (!file_exists($this->filepath)) {
throw new Exception(_('Lost our file.')); throw new Exception(_('Lost our file.'));
@ -247,25 +260,25 @@ class ImageFile
/** /**
* Several obscure file types should be normalized to PNG on resize. * 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 * @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 //we don't want to save BMP... it's an inefficient, rare, antiquated format
//save png instead //save png instead
return IMAGETYPE_PNG; 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 //we don't want to save WBMP... it's a rare format that we can't guarantee clients will support
//save png instead //save png instead
return IMAGETYPE_PNG; 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 //we don't want to save XBM... it's a rare format that we can't guarantee clients will support
//save png instead //save png instead
return IMAGETYPE_PNG; return IMAGETYPE_PNG;
} }
return $type; return $this->type;
} }
function unlink() function unlink()