FIX Image test cleanup

Ensured files referenced by name in tests are actually used, and fixed added support for Flushable interface in image regeneration. Also ensured getters methods are used for width and height.

model/Image.php

@@ -224,13 +224,12 @@ class Image extends File implements Flushable {
 	 * @return Image
 	 */
 	public function SetRatioSize($width, $height) {
-
 		// Prevent divide by zero on missing/blank file
-		if(empty($this->width) || empty($this->height)) return null;
+		if(!$this->getWidth() || !$this->getHeight()) return null;
 
 		// Check if image is already sized to the correct dimension
-		$widthRatio = $width / $this->width;
-		$heightRatio = $height / $this->height;
+		$widthRatio = $width / $this->getWidth();
+		$heightRatio = $height / $this->getHeight();
 		if( $widthRatio < $heightRatio ) {
 			// Target is higher aspect ratio than image, so check width
 			if($this->isWidth($width) && !Config::inst()->get('Image', 'force_resample')) return $this;
@@ -510,7 +509,8 @@ class Image extends File implements Flushable {
 
 	/**
 	 * Generate a resized copy of this image with the given width & height.
-	 * Use in templates with $ResizedImage.
+	 * This can be used in templates with $ResizedImage but should be avoided,
+	 * as it's the only image manipulation function which can skew an image.
 	 *
 	 * @param integer $width Width to resize to
 	 * @param integer $height Height to resize to
@@ -648,9 +648,9 @@ class Image extends File implements Flushable {
 	public function regenerateFormattedImages() {
 		if(!$this->Filename) return 0;
 
-			// Without this, not a single file would be written
-			// caused by a check in getFormattedImage()
-		$_GET['flush'] = 1;
+		// Without this, not a single file would be written
+		// caused by a check in getFormattedImage()
+		$this->flush();
 
 		$numGenerated = 0;
 		$generatedImages = $this->getGeneratedImages();

tests/model/DataDifferencerTest.php

@@ -55,8 +55,8 @@ class DataDifferencerTest extends SapphireTest {
 		$differ = new DataDifferencer($obj1v1, $obj1v2);
 		$obj1Diff = $differ->diffedData();
 
-		$this->assertContains($image1->Filename, $obj1Diff->getField('Image'));
-		$this->assertContains($image2->Filename, $obj1Diff->getField('Image'));
+		$this->assertContains($image1->Name, $obj1Diff->getField('Image'));
+		$this->assertContains($image2->Name, $obj1Diff->getField('Image'));
 		$this->assertContains('<ins>obj2</ins><del>obj1</del>',
 			str_replace(' ','',$obj1Diff->getField('HasOneRelationID')));
 	}

tests/model/GDImageTest.php

@@ -10,22 +10,9 @@ class GDImageTest extends ImageTest {
 			return;
 		}
 
-		parent::setUp();
-
 		Image::set_backend("GDBackend");
 
-		// Create a test files for each of the fixture references
-		$fileIDs = $this->allFixtureIDs('Image');
-		foreach($fileIDs as $fileID) {
-			$file = DataObject::get_by_id('Image', $fileID);
-
-			$image = imagecreatetruecolor(300,300);
-
-			imagepng($image, BASE_PATH."/{$file->Filename}");
-			imagedestroy($image);
-
-			$file->write();
-		}
+		parent::setUp();
 	}
 
 	public function tearDown() {

tests/model/ImageTest.php

@@ -30,19 +30,30 @@ class ImageTest extends SapphireTest {
 
 			if(!file_exists(BASE_PATH."/$folder->Filename")) mkdir(BASE_PATH."/$folder->Filename");
 		}
+
+		// Copy test images for each of the fixture references
+		$imageIDs = $this->allFixtureIDs('Image');
+		foreach($imageIDs as $imageID) {
+			$image = DataObject::get_by_id('Image', $imageID);
+			$filePath = BASE_PATH."/$image->Filename";
+			$sourcePath = str_replace('assets/ImageTest/', 'framework/tests/model/testimages/', $filePath);
+			if(!file_exists($filePath)) {
+				if (!copy($sourcePath, $filePath)) user_error('Failed to copy test images', E_USER_ERROR);
+			}
+		}
 	}
 
 	public function tearDown() {
 		if($this->origBackend) Image::set_backend($this->origBackend);
 
-		/* Remove the test files that we've created */
+		// Remove the test files that we've created
 		$fileIDs = $this->allFixtureIDs('Image');
 		foreach($fileIDs as $fileID) {
 			$file = DataObject::get_by_id('Image', $fileID);
 			if($file && file_exists(BASE_PATH."/$file->Filename")) unlink(BASE_PATH."/$file->Filename");
 		}
 
-		/* Remove the test folders that we've crated */
+		// Remove the test folders that we've created
 		$folderIDs = $this->allFixtureIDs('Folder');
 		foreach($folderIDs as $folderID) {
 			$folder = DataObject::get_by_id('Folder', $folderID);
@@ -298,6 +309,10 @@ class ImageTest extends SapphireTest {
 		$this->assertTrue(file_exists($p), 'Resized image exists after regeneration call');
 	}
 
+	/**
+	 * Tests that cached images are regenerated properly after a cached file is renamed with new arguments
+	 * ToDo: This doesn't seem like something that is worth testing - what is the point of this?
+	 */
 	public function testRegenerateImagesWithRenaming() {
 		$image = $this->objFromFixture('Image', 'imageWithMetacharacters');
 		$image_generated = $image->SetWidth(200);
@@ -311,8 +326,8 @@ class ImageTest extends SapphireTest {
 		$newPath = str_replace($oldArgumentString, $newArgumentString, $p);
 		$newRelative = str_replace($oldArgumentString, $newArgumentString, $image_generated->getFileName());
 		rename($p, $newPath);
-		$this->assertFalse(file_exists($p), 'Resized image does not exist after movement call under old name');
-		$this->assertTrue(file_exists($newPath), 'Resized image exists after movement call under new name');
+		$this->assertFalse(file_exists($p), 'Resized image does not exist at old path after renaming');
+		$this->assertTrue(file_exists($newPath), 'Resized image exists at new path after renaming');
 		$this->assertEquals(1, $image->regenerateFormattedImages(),
 			'Cached images were regenerated in the right number');
 

tests/model/ImageTest.yml

@@ -1,6 +1,6 @@
 Folder:
     folder1:
-        Filename: assets/ImageTest
+        Filename: assets/ImageTest/
 Image:
     imageWithTitle:
         Title: This is a image Title
@@ -16,3 +16,11 @@ Image:
         Title: This is a/an image Title
         Filename: assets/ImageTest/test_image.png
         Parent: =>Folder.folder1
+    lowQualityJPEG:
+        Title: This is a low quality JPEG
+        Filename: assets/ImageTest/test_image_low-quality.jpg
+        Parent: =>Folder.folder1
+    highQualityJPEG:
+        Title: This is a high quality JPEG
+        Filename: assets/ImageTest/test_image_high-quality.jpg
+        Parent: =>Folder.folder1

tests/model/ImagickImageTest.php

@@ -8,22 +8,8 @@ class ImagickImageTest extends ImageTest {
 			return;
 		}
 
-		parent::setUp();
-
 		Image::set_backend("ImagickBackend");
 
-		// Create a test files for each of the fixture references
-		$fileIDs = $this->allFixtureIDs('Image');
-		foreach($fileIDs as $fileID) {
-			$file = DataObject::get_by_id('Image', $fileID);
-
-			$image = new Imagick();
-
-			$image->newImage(300,300, new ImagickPixel("white"));
-			$image->setImageFormat("png");
-			$image->writeImage(BASE_PATH."/{$file->Filename}");
-
-			$file->write();
-		}
+		parent::setUp();
 	}
 }