Merge branch 'pr/4396' into 3.2

* pr/4396:
  FIX Image_Cached exists method doesnt check for positive ID FIX Files should only report as "existing" if the file actually exists

filesystem/File.php

@@ -221,6 +221,15 @@ class File extends DataObject {
 		}
 	}
 
+	/**
+	 * A file only exists if the file_exists() and is in the DB as a record
+	 *
+	 * @return bool
+	 */
+	public function exists() {
+		return parent::exists() && file_exists($this->getFullPath());
+	}
+
 	/**
 	 * Find a File object by the given filename.
 	 *
@@ -293,7 +302,7 @@ class File extends DataObject {
 		// ensure that the record is synced with the filesystem before deleting
 		$this->updateFilesystem();
 
-		if($this->Filename && $this->Name && file_exists($this->getFullPath()) && !is_dir($this->getFullPath())) {
+		if($this->exists() && !is_dir($this->getFullPath())) {
 			unlink($this->getFullPath());
 		}
 	}

model/Image.php

@@ -122,18 +122,6 @@ class Image extends File implements Flushable {
 		return $fields;
 	}
 
-	/**
-	 * An image exists if it has a filename.
-	 * Does not do any filesystem checks.
-	 *
-	 * @return boolean
-	 */
-	public function exists() {
-		if(isset($this->record["Filename"])) {
-			return true;
-		}
-	}
-
 	/**
 	 * Return an XHTML img tag for this Image,
 	 * or NULL if the image file doesn't exist on the filesystem.
@@ -141,7 +129,7 @@ class Image extends File implements Flushable {
 	 * @return string
 	 */
 	public function getTag() {
-		if(file_exists(Director::baseFolder() . '/' . $this->Filename)) {
+		if($this->exists()) {
 			$url = $this->getURL();
 			$title = ($this->Title) ? $this->Title : $this->Filename;
 			if($this->Title) {
@@ -229,7 +217,7 @@ class Image extends File implements Flushable {
 		// Check if image is already sized to the correct dimension
 		$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;
@@ -257,7 +245,7 @@ class Image extends File implements Flushable {
 	/**
 	 * Proportionally scale down this image if it is wider or taller than the specified dimensions.
 	 * Similar to Fit but without up-sampling. Use in templates with $FitMax.
-	 * 
+	 *
 	 * @uses Image::Fit()
 	 * @param integer $width The maximum width of the output image
 	 * @param integer $height The maximum height of the output image
@@ -266,7 +254,7 @@ class Image extends File implements Flushable {
 	public function FitMax($width, $height) {
 		// Temporary $force_resample support for 3.x, to be removed in 4.0
 		if (Config::inst()->get('Image', 'force_resample') && $this->getWidth() <= $width && $this->getHeight() <= $height) return $this->Fit($this->getWidth(),$this->getHeight());
-		
+
 		return $this->getWidth() > $width || $this->getHeight() > $height
 			? $this->Fit($width,$height)
 			: $this;
@@ -300,7 +288,7 @@ class Image extends File implements Flushable {
 	}
 
 	/**
-	 * Crop this image to the aspect ratio defined by the specified width and height, 
+	 * Crop this image to the aspect ratio defined by the specified width and height,
 	 * then scale down the image to those dimensions if it exceeds them.
 	 * Similar to Fill but without up-sampling. Use in templates with $FillMax.
 	 *
@@ -312,13 +300,13 @@ class Image extends File implements Flushable {
 	public function FillMax($width, $height) {
 		// Prevent divide by zero on missing/blank file
 		if(!$this->getWidth() || !$this->getHeight()) return null;
-		
+
 		// Temporary $force_resample support for 3.x, to be removed in 4.0
 		if (Config::inst()->get('Image', 'force_resample') && $this->isSize($width, $height)) return $this->Fill($width, $height);
-		
+
 		// Is the image already the correct size?
 		if ($this->isSize($width, $height)) return $this;
-		
+
 		// If not, make sure the image isn't upsampled
 		$imageRatio = $this->getWidth() / $this->getHeight();
 		$cropRatio = $width / $height;
@@ -326,7 +314,7 @@ class Image extends File implements Flushable {
 		if ($cropRatio < $imageRatio && $this->getHeight() < $height) return $this->Fill($this->getHeight()*$cropRatio, $this->getHeight());
 		// Otherwise we're cropping on the y axis (or not cropping at all) so compare widths
 		if ($this->getWidth() < $width) return $this->Fill($this->getWidth(), $this->getWidth()/$cropRatio);
-		
+
 		return $this->Fill($width, $height);
 	}
 
@@ -379,7 +367,7 @@ class Image extends File implements Flushable {
 	}
 
 	/**
-	 * Proportionally scale down this image if it is wider than the specified width. 
+	 * Proportionally scale down this image if it is wider than the specified width.
 	 * Similar to ScaleWidth but without up-sampling. Use in templates with $ScaleMaxWidth.
 	 *
 	 * @uses Image::ScaleWidth()
@@ -389,7 +377,7 @@ class Image extends File implements Flushable {
 	public function ScaleMaxWidth($width) {
 		// Temporary $force_resample support for 3.x, to be removed in 4.0
 		if (Config::inst()->get('Image', 'force_resample') && $this->getWidth() <= $width) return $this->ScaleWidth($this->getWidth());
-		
+
 		return $this->getWidth() > $width
 			? $this->ScaleWidth($width)
 			: $this;
@@ -419,7 +407,7 @@ class Image extends File implements Flushable {
 	}
 
 	/**
-	 * Proportionally scale down this image if it is taller than the specified height. 
+	 * Proportionally scale down this image if it is taller than the specified height.
 	 * Similar to ScaleHeight but without up-sampling. Use in templates with $ScaleMaxHeight.
 	 *
 	 * @uses Image::ScaleHeight()
@@ -429,7 +417,7 @@ class Image extends File implements Flushable {
 	public function ScaleMaxHeight($height) {
 		// Temporary $force_resample support for 3.x, to be removed in 4.0
 		if (Config::inst()->get('Image', 'force_resample') && $this->getHeight() <= $height) return $this->ScaleHeight($this->getHeight());
-		
+
 		return $this->getHeight() > $height
 			? $this->ScaleHeight($height)
 			: $this;
@@ -446,7 +434,7 @@ class Image extends File implements Flushable {
 	public function CropWidth($width) {
 		// Temporary $force_resample support for 3.x, to be removed in 4.0
 		if (Config::inst()->get('Image', 'force_resample') && $this->getWidth() <= $width) return $this->Fill($this->getWidth(), $this->getHeight());
-		
+
 		return $this->getWidth() > $width
 			? $this->Fill($width, $this->getHeight())
 			: $this;
@@ -463,7 +451,7 @@ class Image extends File implements Flushable {
 	public function CropHeight($height) {
 		// Temporary $force_resample support for 3.x, to be removed in 4.0
 		if (Config::inst()->get('Image', 'force_resample') && $this->getHeight() <= $height) return $this->Fill($this->getWidth(), $this->getHeight());
-		
+
 		return $this->getHeight() > $height
 			? $this->Fill($this->getWidth(), $height)
 			: $this;
@@ -684,7 +672,7 @@ class Image extends File implements Flushable {
 	public function getFormattedImage($format) {
 		$args = func_get_args();
 
-		if($this->ID && $this->Filename && Director::fileExists($this->Filename)) {
+		if($this->exists()) {
 			$cacheFile = call_user_func_array(array($this, "cacheFilename"), $args);
 
 			if(!file_exists(Director::baseFolder()."/".$cacheFile) || self::$flush) {
@@ -950,8 +938,8 @@ class Image extends File implements Flushable {
 	public function getDimensions($dim = "string") {
 		if($this->getField('Filename')) {
 
-			$imagefile = Director::baseFolder() . '/' . $this->getField('Filename');
-			if(file_exists($imagefile)) {
+			$imagefile = $this->getFullPath();
+			if($this->exists()) {
 				$size = getimagesize($imagefile);
 				return ($dim === "string") ? "$size[0]x$size[1]" : $size[$dim];
 			} else {
@@ -1029,6 +1017,16 @@ class Image_Cached extends Image {
 		$this->Filename = $filename;
 	}
 
+	/**
+	 * Override the parent's exists method becuase the ID is explicitly set to -1 on a cached image we can't use the
+	 * default check
+	 *
+	 * @return bool Whether the cached image exists
+	 */
+	public function exists() {
+		return file_exists($this->getFullPath());
+	}
+
 	public function getRelativePath() {
 		return $this->getField('Filename');
 	}

tests/forms/uploadfield/UploadFieldTest.php

@@ -253,23 +253,49 @@ class UploadFieldTest extends FunctionalTest {
 		// two should work and the third will fail.
 		$record = $this->objFromFixture('UploadFieldTest_Record', 'record1');
 		$record->HasManyFilesMaxTwo()->removeAll();
+		$this->assertCount(0, $record->HasManyFilesMaxTwo());
 
 		// Get references for each file to upload
 		$file1 = $this->objFromFixture('File', 'file1');
 		$file2 = $this->objFromFixture('File', 'file2');
 		$file3 = $this->objFromFixture('File', 'file3');
+		$this->assertTrue($file1->exists());
+		$this->assertTrue($file2->exists());
+		$this->assertTrue($file3->exists());
 
 		// Write the first element, should be okay.
 		$response = $this->mockUploadFileIDs('HasManyFilesMaxTwo', array($file1->ID));
 		$this->assertEmpty($response['errors']);
+		$this->assertCount(1, $record->HasManyFilesMaxTwo());
+		$this->assertContains($file1->ID, $record->HasManyFilesMaxTwo()->getIDList());
+
+
+		$record->HasManyFilesMaxTwo()->removeAll();
+		$this->assertCount(0, $record->HasManyFilesMaxTwo());
+		$this->assertTrue($file1->exists());
+		$this->assertTrue($file2->exists());
+		$this->assertTrue($file3->exists());
+
+
 
 		// Write the second element, should be okay.
 		$response = $this->mockUploadFileIDs('HasManyFilesMaxTwo', array($file1->ID, $file2->ID));
 		$this->assertEmpty($response['errors']);
+		$this->assertCount(2, $record->HasManyFilesMaxTwo());
+		$this->assertContains($file1->ID, $record->HasManyFilesMaxTwo()->getIDList());
+		$this->assertContains($file2->ID, $record->HasManyFilesMaxTwo()->getIDList());
+
+		$record->HasManyFilesMaxTwo()->removeAll();
+		$this->assertCount(0, $record->HasManyFilesMaxTwo());
+		$this->assertTrue($file1->exists());
+		$this->assertTrue($file2->exists());
+		$this->assertTrue($file3->exists());
+
 
 		// Write the third element, should result in error.
 		$response = $this->mockUploadFileIDs('HasManyFilesMaxTwo', array($file1->ID, $file2->ID, $file3->ID));
 		$this->assertNotEmpty($response['errors']);
+		$this->assertCount(0, $record->HasManyFilesMaxTwo());
 	}
 
 	/**
@@ -948,22 +974,21 @@ class UploadFieldTest extends FunctionalTest {
 	}
 
 	public function setUp() {
+		Config::inst()->update('File', 'update_filesystem', false);
 		parent::setUp();
 
 		if(!file_exists(ASSETS_PATH)) mkdir(ASSETS_PATH);
 
 		/* Create a test folders for each of the fixture references */
-		$folderIDs = $this->allFixtureIDs('Folder');
-		foreach($folderIDs as $folderID) {
-			$folder = DataObject::get_by_id('Folder', $folderID);
-			if(!file_exists(BASE_PATH."/$folder->Filename")) mkdir(BASE_PATH."/$folder->Filename");
+		$folders = Folder::get()->byIDs($this->allFixtureIDs('Folder'));
+		foreach($folders as $folder) {
+			if(!file_exists($folder->getFullPath())) mkdir($folder->getFullPath());
 		}
 
 		/* Create a test files for each of the fixture references */
-		$fileIDs = $this->allFixtureIDs('File');
-		foreach($fileIDs as $fileID) {
-			$file = DataObject::get_by_id('File', $fileID);
-			$fh = fopen(BASE_PATH."/$file->Filename", "w");
+		$files = File::get()->byIDs($this->allFixtureIDs('File'));
+		foreach($files as $file) {
+			$fh = fopen($file->getFullPath(), "w");
 			fwrite($fh, str_repeat('x',1000000));
 			fclose($fh);
 		}

tests/forms/uploadfield/UploadFieldTest.yml

@@ -3,48 +3,48 @@ Folder:
     Name: UploadFieldTest
   folder1-subfolder1:
     Name: subfolder1
-    ParentID: =>Folder.folder1
+    Parent: =>Folder.folder1
 File:
   file1:
     Title: File1
     Filename: assets/UploadFieldTest/file1.txt
-    ParentID: =>Folder.folder1
+    Parent: =>Folder.folder1
   file2:
     Title: File2
     Filename: assets/UploadFieldTest/file2.txt
-    ParentID: =>Folder.folder1
+    Parent: =>Folder.folder1
   file3:
     Title: File3
     Filename: assets/UploadFieldTest/file3.txt
-    ParentID: =>Folder.folder1
+    Parent: =>Folder.folder1
   file4:
     Title: File4
     Filename: assets/UploadFieldTest/file4.txt
-    ParentID: =>Folder.folder1
+    Parent: =>Folder.folder1
   file5:
     Title: File5
     Filename: assets/UploadFieldTest/file5.txt
-    ParentID: =>Folder.folder1
+    Parent: =>Folder.folder1
   file-noview:
     Title: noview.txt
     Name: noview.txt
     Filename: assets/UploadFieldTest/noview.txt
-    ParentID: =>Folder.folder1
+    Parent: =>Folder.folder1
   file-noedit:
     Title: noedit.txt
     Name: noedit.txt
     Filename: assets/UploadFieldTest/noedit.txt
-    ParentID: =>Folder.folder1
+    Parent: =>Folder.folder1
   file-nodelete:
     Title: nodelete.txt
     Name: nodelete.txt
     Filename: assets/UploadFieldTest/nodelete.txt
-    ParentID: =>Folder.folder1
+    Parent: =>Folder.folder1
   file-subfolder:
     Title: file-subfolder.txt
     Name: file-subfolder.txt
     Filename: assets/UploadFieldTest/subfolder1/file-subfolder.txt
-    ParentID: =>Folder.folder1-subfolder1
+    Parent: =>Folder.folder1-subfolder1
 UploadFieldTest_Record:
   record1:
     Title: Record 1