From 0096ab0d3d1ba07dffbc025eb1677b299180bad7 Mon Sep 17 00:00:00 2001
From: Devlin <support@layon.de>
Date: Tue, 4 Mar 2014 13:43:08 +0100
Subject: [PATCH] Upload: refactor file versioning

---
 filesystem/Upload.php           | 38 ++++++++++++++---------
 tests/filesystem/UploadTest.php | 55 ++++++++++++++++++++++++++++++++-
 2 files changed, 77 insertions(+), 16 deletions(-)

diff --git a/filesystem/Upload.php b/filesystem/Upload.php
index ec8b7ca12..6b0003b9c 100644
--- a/filesystem/Upload.php
+++ b/filesystem/Upload.php
@@ -134,9 +134,10 @@ class Upload extends Controller {
 		$file = $nameFilter->filter($tmpFile['name']);
 		$fileName = basename($file);
 
-		$relativeFilePath = $parentFolder
-			? $parentFolder->getRelativePath() . "$fileName" 
-			: ASSETS_DIR . "/" . $fileName;
+		$relativeFolderPath = $parentFolder
+				? $parentFolder->getRelativePath()
+				: ASSETS_DIR . '/';
+		$relativeFilePath = $relativeFolderPath . $fileName;
 		
 		// Create a new file record (or try to retrieve an existing one)
 		if(!$this->file) {
@@ -156,26 +157,33 @@ class Upload extends Controller {
 			}
 		}
 		
-		// if filename already exists, version the filename (e.g. test.gif to test1.gif)
+		// if filename already exists, version the filename (e.g. test.gif to test2.gif, test2.gif to test3.gif)
 		if(!$this->replaceFile) {
+			$fileSuffixArray = explode('.', $fileName);
+			$fileTitle = array_shift($fileSuffixArray);
+			$fileSuffix = !empty($fileSuffixArray)
+					? '.' . implode('.', $fileSuffixArray)
+					: null;
+
+			// make sure files retain valid extensions
+			$oldFilePath = $relativeFilePath;
+			$relativeFilePath = $relativeFolderPath . $fileTitle . $fileSuffix;
+			if($oldFilePath !== $relativeFilePath) {
+				user_error("Couldn't fix $relativeFilePath", E_USER_ERROR);
+			}
 			while(file_exists("$base/$relativeFilePath")) {
 				$i = isset($i) ? ($i+1) : 2;
-				$oldFilePath = $relativeFilePath;
-				// make sure archives retain valid extensions
-				if(substr($relativeFilePath, strlen($relativeFilePath) - strlen('.tar.gz')) == '.tar.gz' ||
-					substr($relativeFilePath, strlen($relativeFilePath) - strlen('.tar.bz2')) == '.tar.bz2') {
-						$relativeFilePath = preg_replace('/[0-9]*(\.tar\.[^.]+$)/', $i . '\\1', $relativeFilePath);
-				} else if (strpos($relativeFilePath, '.') !== false) {
-					$relativeFilePath = preg_replace('/[0-9]*(\.[^.]+$)/', $i . '\\1', $relativeFilePath);
-				} else if (strpos($relativeFilePath, '_') !== false) {
-					$relativeFilePath = preg_replace('/_([^_]+$)/', '_'.$i, $relativeFilePath);
+				$pattern = '/([0-9]+$)/';
+				if(preg_match($pattern, $fileTitle)) {
+					$fileTitle = preg_replace($pattern, $i, $fileTitle);
 				} else {
-					$relativeFilePath .= '_'.$i;
+					$fileTitle .= $i;
 				}
+				$relativeFilePath = $relativeFolderPath . $fileTitle . $fileSuffix;
 				if($oldFilePath == $relativeFilePath && $i > 2) {
 					user_error("Couldn't fix $relativeFilePath with $i tries", E_USER_ERROR);
 				}
-			}	
+			}
 		} else {
 			//reset the ownerID to the current member when replacing files
 			$this->file->OwnerID = (Member::currentUser() ? Member::currentUser()->ID : 0);
diff --git a/tests/filesystem/UploadTest.php b/tests/filesystem/UploadTest.php
index 48a833027..aa825948c 100644
--- a/tests/filesystem/UploadTest.php
+++ b/tests/filesystem/UploadTest.php
@@ -258,6 +258,10 @@ class UploadTest extends SapphireTest {
 			$file->Name,
 			'File has a name without a number because it\'s not a duplicate'
 		);
+		$this->assertFileExists(
+			BASE_PATH . '/'  . $file->getRelativePath(),
+			'File exists'
+		);
 		
 		$u = new Upload();
 		$u->load($tmpFile);
@@ -267,9 +271,37 @@ class UploadTest extends SapphireTest {
 			$file2->Name,
 			'File receives a number attached to the end before the extension'
 		);
+		$this->assertFileExists(
+			BASE_PATH . '/'  . $file2->getRelativePath(),
+			'File exists'
+		);
+		$this->assertGreaterThan(
+			$file->ID,
+			$file2->ID,
+			'File database record is not the same'
+		);
+		
+		$u = new Upload();
+		$u->load($tmpFile);
+		$file3 = $u->getFile();
+		$this->assertEquals(
+			'UploadTest-testUpload3.tar.gz',
+			$file3->Name,
+			'File receives a number attached to the end before the extension'
+		);
+		$this->assertFileExists(
+			BASE_PATH . '/'  . $file3->getRelativePath(),
+			'File exists'
+		);
+		$this->assertGreaterThan(
+			$file2->ID,
+			$file3->ID,
+			'File database record is not the same'
+		);
 		
 		$file->delete();
 		$file2->delete();
+		$file3->delete();
 	}
 	
 	public function testUploadFileWithNoExtensionTwiceAppendsNumber() {
@@ -307,16 +339,29 @@ class UploadTest extends SapphireTest {
 			$file->Name,
 			'File is uploaded without extension'
 		);
+		$this->assertFileExists(
+			BASE_PATH . '/'  . $file->getRelativePath(),
+			'File exists'
+		);
 		
 		$u = new Upload();
 		$u->setValidator($v);
 		$u->load($tmpFile);
 		$file2 = $u->getFile();
 		$this->assertEquals(
-			'UploadTest-testUpload-2',
+			'UploadTest-testUpload2',
 			$file2->Name,
 			'File receives a number attached to the end'
 		);
+		$this->assertFileExists(
+			BASE_PATH . '/'  . $file2->getRelativePath(),
+			'File exists'
+		);
+		$this->assertGreaterThan(
+			$file->ID,
+			$file2->ID,
+			'File database record is not the same'
+		);
 		
 		$file->delete();
 		$file2->delete();
@@ -357,6 +402,10 @@ class UploadTest extends SapphireTest {
 			$file->Name,
 			'File is uploaded without extension'
 		);
+		$this->assertFileExists(
+			BASE_PATH . '/'  . $file->getRelativePath(),
+			'File exists'
+		);
 		
 		$u = new Upload();
 		$u->setValidator($v);
@@ -368,6 +417,10 @@ class UploadTest extends SapphireTest {
 			$file2->Name,
 			'File does not receive new name'
 		);
+		$this->assertFileExists(
+			BASE_PATH . '/'  . $file2->getRelativePath(),
+			'File exists'
+		);
 		$this->assertEquals(
 			$file->ID,
 			$file2->ID,