Merge pull request #4507 from JorisDebonnet/resampled-images-in-folders

Save resampled images into a folder structure indicating transformations

filesystem/File.php

@@ -238,7 +238,7 @@ class File extends DataObject {
 	 */
 	public static function find($filename) {
 		// Get the base file if $filename points to a resampled file
-		$filename = preg_replace('/_resampled\/[^-]+-/', '', $filename);
+		$filename = Image::strip_resampled_prefix($filename);
 
 		// Split to folders and the actual filename, and traverse the structure.
 		$parts = explode("/", $filename);

filesystem/Filesystem.php

@@ -78,6 +78,29 @@ class Filesystem extends Object {
 		}
 	}
 
+	/**
+	 * Remove a directory, but only if it is empty.
+	 *
+	 * @param string $folder Absolute folder path
+	 * @param boolean $recursive Remove contained empty folders before attempting to remove this one
+	 * @return boolean True on success, false on failure.
+	 */
+	public static function remove_folder_if_empty($folder, $recursive = true) {
+		if (!is_readable($folder)) return false;
+		$handle = opendir($folder);
+		while (false !== ($entry = readdir($handle))) {
+			if ($entry != "." && $entry != "..") {
+				// if an empty folder is detected, remove that one first and move on
+				if($recursive && is_dir($entry) && self::remove_folder_if_empty($entry)) continue;
+				// if a file was encountered, or a subdirectory was not empty, return false.
+				return false;
+			}
+		}
+		// if we are still here, the folder is empty. 
+		rmdir($folder);
+		return true;
+	}
+
 	/**
 	 * Cleanup function to reset all the Filename fields.  Visit File/fixfiles to call.
 	 */

forms/HtmlEditorField.php

@@ -458,14 +458,13 @@ class HtmlEditorField_Toolbar extends RequestHandler {
 		// but GridField doesn't allow for this kind of metadata customization at the moment.
 		if($url = $request->getVar('FileURL')) {
 			if(Director::is_absolute_url($url) && !Director::is_site_url($url)) {
-				$url = $url;
 				$file = new File(array(
 					'Title' => basename($url),
 					'Filename' => $url
 				));
 			} else {
 				$url = Director::makeRelative($request->getVar('FileURL'));
-				$url = preg_replace('/_resampled\/[^-]+-/', '', $url);
+				$url = Image::strip_resampled_prefix($url);
 				$file = File::get()->filter('Filename', $url)->first();
 				if(!$file) $file = new File(array(
 					'Title' => basename($url),

model/Image.php

@@ -93,6 +93,17 @@ class Image extends File implements Flushable {
 		return self::config()->backend;
 	}
 
+	/**
+	 * Retrieve the original filename from the path of a transformed image.
+	 * Any other filenames pass through unchanged. 
+	 * 
+	 * @param string $path
+	 * @return string
+	 */
+	public static function strip_resampled_prefix($path) {
+		return preg_replace('/_resampled\/(.+\/|[^-]+-)/', '', $path);
+	}
+
 	/**
 	 * Set up template methods to access the transformations generated by 'generate' methods.
 	 */
@@ -114,6 +125,7 @@ class Image extends File implements Flushable {
 		$urlLink .= "<label class='left'>"._t('AssetTableField.URL','URL')."</label>";
 		$urlLink .= "<span class='readonly'><a href='{$this->Link()}'>{$this->RelativeLink()}</a></span>";
 		$urlLink .= "</div>";
+		// todo: check why the above code is here, since $urlLink is not used?
 
 		//attach the addition file information for an image to the existing FieldGroup create in the parent class
 		$fileAttributes = $fields->fieldByName('Root.Main.FilePreview')->fieldByName('FilePreviewData');
@@ -697,12 +709,25 @@ class Image extends File implements Flushable {
 	public function cacheFilename($format) {
 		$args = func_get_args();
 		array_shift($args);
+
+		// Note: $folder holds the *original* file, while the Image we're working with
+		// may be a formatted image in a child directory (this happens when we're chaining formats)
 		$folder = $this->ParentID ? $this->Parent()->Filename : ASSETS_DIR . "/";
 
 		$format = $format . Convert::base64url_encode($args);
-		$filename = $format . "-" . $this->Name;
+		$filename = $format . "/" . $this->Name;
-		$patterns = $this->getFilenamePatterns($this->Name);
+
-		if (!preg_match($patterns['FullPattern'], $filename)) {
+		$pattern = $this->getFilenamePatterns($this->Name);
+
+		// Any previous formats need to be derived from this Image's directory, and prepended to the new filename
+		$prepend = array();
+		preg_match_all($pattern['GeneratorPattern'], $this->Filename, $matches, PREG_SET_ORDER);
+		foreach($matches as $formatdir) {
+			$prepend[] = $formatdir[0];
+		}
+		$filename = implode($prepend) . $filename;
+
+		if (!preg_match($pattern['FullPattern'], $filename)) {
 			throw new InvalidArgumentException('Filename ' . $filename
 				. ' that should be used to cache a resized image is invalid');
 		}
@@ -826,9 +851,9 @@ class Image extends File implements Flushable {
 		$generateFuncs = implode('|', $generateFuncs);
 		$base64url_match = "[a-zA-Z0-9_~]*={0,2}";
 		return array(
-				'FullPattern' => "/^((?P<Generator>{$generateFuncs})(?P<Args>" . $base64url_match . ")\-)+"
+				'FullPattern' => "/^((?P<Generator>{$generateFuncs})(?P<Args>" . $base64url_match . ")\/)+"
 									. preg_quote($filename) . "$/i",
-				'GeneratorPattern' => "/(?P<Generator>{$generateFuncs})(?P<Args>" . $base64url_match . ")\-/i"
+				'GeneratorPattern' => "/(?P<Generator>{$generateFuncs})(?P<Args>" . $base64url_match . ")\//i"
 		);
 	}
 
@@ -842,40 +867,35 @@ class Image extends File implements Flushable {
 		$folder = $this->ParentID ? $this->Parent()->Filename : ASSETS_DIR . '/';
 		$cacheDir = Director::getAbsFile($folder . '_resampled/');
 
+		// Find all paths with the same filename as this Image (the path contains the transformation info)
 		if(is_dir($cacheDir)) {
-			if($handle = opendir($cacheDir)) {
+			$files = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($cacheDir));
-				while(($file = readdir($handle)) !== false) {
+			foreach($files as $path => $file){
-					// ignore all entries starting with a dot
+				if ($file->getFilename() == $this->Name) {
-					if(substr($file, 0, 1) != '.' && is_file($cacheDir . $file)) {
+					$cachedFiles[] = $path;
-						$cachedFiles[] = $file;
-					}
 				}
-				closedir($handle);
 			}
 		}
 
 		$pattern = $this->getFilenamePatterns($this->Name);
 
-		foreach($cachedFiles as $cfile) {
+		// Reconstruct the image transformation(s) from the format-folder(s) in the path
-			if(preg_match($pattern['FullPattern'], $cfile, $matches)) {
+		// (if chained, they contain the transformations in the correct order)
-				if(Director::fileExists($cacheDir . $cfile)) {
+		foreach($cachedFiles as $cf_path) {
-					$subFilename = substr($cfile, 0, -1 * strlen($this->Name));
+			preg_match_all($pattern['GeneratorPattern'], $cf_path, $matches, PREG_SET_ORDER);
-					preg_match_all($pattern['GeneratorPattern'], $subFilename, $subMatches, PREG_SET_ORDER);
+			
-
+			$generatorArray = array();
-					$generatorArray = array();
+			foreach ($matches as $singleMatch) {
-					foreach ($subMatches as $singleMatch) {
+				$generatorArray[] = array(
-						$generatorArray[] = array('Generator' => $singleMatch['Generator'],
+					'Generator' => $singleMatch['Generator'],
-						'Args' => Convert::base64url_decode($singleMatch['Args']));
+					'Args' => Convert::base64url_decode($singleMatch['Args'])
-					}
+				);
-
-						// Using array_reverse is important, as a cached image will
-						// have the generators settings in the filename in reversed
-						// order: the last generator given in the filename is the
-						// first that was used. Later resizements are prepended
-					$generatedImages[] = array ( 'FileName' => $cacheDir . $cfile,
-							'Generators' => array_reverse($generatorArray) );
-				}
 			}
+
+			$generatedImages[] = array(
+				'FileName' => $cf_path,
+				'Generators' => $generatorArray
+			);
 		}
 
 		return $generatedImages;
@@ -922,8 +942,14 @@ class Image extends File implements Flushable {
 		$numDeleted = 0;
 		$generatedImages = $this->getGeneratedImages();
 		foreach($generatedImages as $singleImage) {
-			unlink($singleImage['FileName']);
+			$path = $singleImage['FileName'];
+			unlink($path);
 			$numDeleted++;
+			do {
+				$path = dirname($path);
+			}
+			// remove the folder if it's empty (and it's not the assets folder)
+			while(!preg_match('/assets$/', $path) && Filesystem::remove_folder_if_empty($path));
 		}
 
 		return $numDeleted;

tests/forms/HtmlEditorFieldTest.php

@@ -89,7 +89,7 @@ class HtmlEditorFieldTest extends FunctionalTest {
 		$this->assertEquals(20, (int)$xml[0]['height'], 'Height tag of resized image is set.');
 
 		$neededFilename = 'assets/_resampled/ResizedImage' . Convert::base64url_encode(array(10,20)) .
-			'-HTMLEditorFieldTest_example.jpg';
+			'/HTMLEditorFieldTest_example.jpg';
 
 		$this->assertEquals($neededFilename, (string)$xml[0]['src'], 'Correct URL of resized image is set.');
 		$this->assertTrue(file_exists(BASE_PATH.DIRECTORY_SEPARATOR.$neededFilename), 'File for resized image exists');

tests/model/ImageTest.php

@@ -119,7 +119,6 @@ class ImageTest extends SapphireTest {
 	 * of the output image do not resample the file.
 	 */
 	public function testReluctanceToResampling() {
-
 		$image = $this->objFromFixture('Image', 'imageWithoutTitle');
 		$this->assertTrue($image->isSize(300, 300));
 
@@ -170,7 +169,6 @@ class ImageTest extends SapphireTest {
 	 * of the output image resample the file when force_resample is set to true.
 	 */
 	public function testForceResample() {
-
 		$image = $this->objFromFixture('Image', 'imageWithoutTitle');
 		$this->assertTrue($image->isSize(300, 300));
 
@@ -315,23 +313,24 @@ class ImageTest extends SapphireTest {
 		$this->assertContains($argumentString, $imageThird->getFullPath(),
 			'Image contains background color for padded resizement');
 
-		$imageThirdPath = $imageThird->getFullPath();
+		$resampledFolder = dirname($image->getFullPath()) . "/_resampled";
-		$filesInFolder = $folder->find(dirname($imageThirdPath));
+		$filesInFolder = $folder->find($resampledFolder);
 		$this->assertEquals(3, count($filesInFolder),
 			'Image folder contains only the expected number of images before regeneration');
 
+		$imageThirdPath = $imageThird->getFullPath();
 		$hash = md5_file($imageThirdPath);
 		$this->assertEquals(3, $image->regenerateFormattedImages(),
 			'Cached images were regenerated in the right number');
 		$this->assertEquals($hash, md5_file($imageThirdPath), 'Regeneration of third image is correct');
 
 		/* Check that no other images exist, to ensure that the regeneration did not create other images */
-		$this->assertEquals($filesInFolder, $folder->find(dirname($imageThirdPath)),
+		$this->assertEquals($filesInFolder, $folder->find($resampledFolder),
 			'Image folder contains only the expected image files after regeneration');
 	}
 
 	public function testRegenerateImages() {
-		$image = $this->objFromFixture('Image', 'imageWithMetacharacters');
+		$image = $this->objFromFixture('Image', 'imageWithoutTitle');
 		$image_generated = $image->ScaleWidth(200);
 		$p = $image_generated->getFullPath();
 		$this->assertTrue(file_exists($p), 'Resized image exists after creation call');
@@ -346,7 +345,7 @@ class ImageTest extends SapphireTest {
 	 * 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 = $this->objFromFixture('Image', 'imageWithoutTitle');
 		$image_generated = $image->ScaleWidth(200);
 		$p = $image_generated->getFullPath();
 		$this->assertTrue(file_exists($p), 'Resized image exists after creation call');
@@ -356,6 +355,7 @@ class ImageTest extends SapphireTest {
 		$newArgumentString = Convert::base64url_encode(array(300));
 
 		$newPath = str_replace($oldArgumentString, $newArgumentString, $p);
+		if(!file_exists(dirname($newPath))) mkdir(dirname($newPath));
 		$newRelative = str_replace($oldArgumentString, $newArgumentString, $image_generated->getFileName());
 		rename($p, $newPath);
 		$this->assertFalse(file_exists($p), 'Resized image does not exist at old path after renaming');
@@ -368,7 +368,7 @@ class ImageTest extends SapphireTest {
 	}
 
 	public function testGeneratedImageDeletion() {
-		$image = $this->objFromFixture('Image', 'imageWithMetacharacters');
+		$image = $this->objFromFixture('Image', 'imageWithoutTitle');
 		$image_generated = $image->ScaleWidth(200);
 		$p = $image_generated->getFullPath();
 		$this->assertTrue(file_exists($p), 'Resized image exists after creation call');
@@ -381,7 +381,7 @@ class ImageTest extends SapphireTest {
 	 * Tests that generated images with multiple image manipulations are all deleted
 	 */
 	public function testMultipleGenerateManipulationCallsImageDeletion() {
-		$image = $this->objFromFixture('Image', 'imageWithMetacharacters');
+		$image = $this->objFromFixture('Image', 'imageWithoutTitle');
 
 		$firstImage = $image->ScaleWidth(200);
 		$firstImagePath = $firstImage->getFullPath();
@@ -400,7 +400,7 @@ class ImageTest extends SapphireTest {
 	 * Tests path properties of cached images with multiple image manipulations
 	 */
 	public function testPathPropertiesCachedImage() {
-		$image = $this->objFromFixture('Image', 'imageWithMetacharacters');
+		$image = $this->objFromFixture('Image', 'imageWithoutTitle');
 		$firstImage = $image->ScaleWidth(200);
 		$firstImagePath = $firstImage->getRelativePath();
 		$this->assertEquals($firstImagePath, $firstImage->Filename);
@@ -410,6 +410,33 @@ class ImageTest extends SapphireTest {
 		$this->assertEquals($secondImagePath, $secondImage->Filename);
 	}
 
+	/**
+	 * Tests the static function Image::strip_resampled_prefix, to ensure that
+	 * the original filename can be extracted from the path of transformed images, 
+	 * both in current and previous formats 
+	 */
+	public function testStripResampledPrefix() {
+		$orig_image = $this->objFromFixture('Image', 'imageWithoutTitleContainingDots');
+
+		// current format (3.3+). Example:
+		// assets/ImageTest/_resampled/ScaleHeightWzIwMF0=/ScaleWidthWzQwMF0=/test.image.with.dots.png;
+		$firstImage = $orig_image->ScaleWidth(200);
+		$secondImage = $firstImage->ScaleHeight(200);
+		$paths_1 = $firstImage->Filename;
+		$paths_2 = $secondImage->Filename;
+
+		// 3.2 format  (did not work for multiple transformations)
+		$paths_3 = 'assets/ImageTest/_resampled/ScaleHeightWzIwMF0=-test.image.with.dots.png';
+
+		// 3.1 (and earlier) format  (did not work for multiple transformations)
+		$paths_4 = 'assets/ImageTest/_resampled/ScaleHeight200-test.image.with.dots.png';
+
+		$this->assertEquals($orig_image->Filename, Image::strip_resampled_prefix($paths_1));
+		$this->assertEquals($orig_image->Filename, Image::strip_resampled_prefix($paths_2));
+		$this->assertEquals($orig_image->Filename, Image::strip_resampled_prefix($paths_3));
+		$this->assertEquals($orig_image->Filename, Image::strip_resampled_prefix($paths_4));
+	}
+
 	/**
 	 * Test all generate methods
 	 */