From 5fbb3da661248d277af78073c12adae1ddf93808 Mon Sep 17 00:00:00 2001 From: Nico Haase Date: Mon, 15 Apr 2013 12:17:00 +0200 Subject: [PATCH] Enhanced tests for regeneration of images and a bugfix for images that were cached in multiple steps --- forms/HtmlEditorField.php | 4 +- model/Image.php | 84 ++++++++++++++++++++++------- tests/forms/HtmlEditorFieldTest.php | 7 ++- tests/model/ImageTest.php | 84 +++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 24 deletions(-) diff --git a/forms/HtmlEditorField.php b/forms/HtmlEditorField.php index f75e05367..c3a02dd8a 100644 --- a/forms/HtmlEditorField.php +++ b/forms/HtmlEditorField.php @@ -151,8 +151,8 @@ class HtmlEditorField extends TextareaField { } // Resample the images if the width & height have changed. - $width = $img->getAttribute('width'); - $height = $img->getAttribute('height'); + $width = (int)$img->getAttribute('width'); + $height = (int)$img->getAttribute('height'); if($image){ if($width && $height && ($width != $image->getWidth() || $height != $image->getHeight())) { diff --git a/model/Image.php b/model/Image.php index ecdf33079..980a484de 100644 --- a/model/Image.php +++ b/model/Image.php @@ -428,17 +428,23 @@ class Image extends File { * Return the filename for the cached image, given it's format name and arguments. * @param string $format The format name. * @return string + * @throws InvalidArgumentException */ public function cacheFilename($format) { $args = func_get_args(); array_shift($args); $folder = $this->ParentID ? $this->Parent()->Filename : ASSETS_DIR . "/"; - $format = $format.implode('x', $args); + $format = $format . base64_encode(json_encode($args)); + $filename = $format . "-" . $this->Name; + $patterns = $this->getFilenamePatterns($this->Name); + if (!preg_match($patterns['FullPattern'], $filename)) { + throw new InvalidArgumentException('Filename ' . $filename . ' that should be used to cache a resized image is invalid'); + } - return $folder . "_resampled/$format-" . $this->Name; + return $folder . "_resampled/" . $filename; } - + /** * Generate an image on the specified format. It will save the image * at the location specified by cacheFilename(). The image will be generated @@ -532,14 +538,36 @@ class Image extends File { public function generateCroppedImage(Image_Backend $backend, $width, $height) { return $backend->croppedResize($width, $height); } + + /** + * Generate patterns that will help to match filenames of cached images + * + * @param string $filename Filename of source image + * @return array + */ + private function getFilenamePatterns($filename) { + $methodNames = $this->allMethodNames(true); + $generateFuncs = array(); + foreach($methodNames as $methodName) { + if(substr($methodName, 0, 8) == 'generate') { + $format = substr($methodName, 8); + $generateFuncs[] = preg_quote($format); + } + } + // All generate functions may appear any number of times in the image cache name. + $generateFuncs = implode('|', $generateFuncs); + $base64Match = "[a-zA-Z0-9\/\r\n+]*={0,2}"; + return array( + 'FullPattern' => "/^((?P{$generateFuncs})(?P" . $base64Match . ")\-)+" . preg_quote($filename) . "$/i", + 'GeneratorPattern' => "/(?P{$generateFuncs})(?P" . $base64Match . ")\-/i" + ); + } /** * Generate a list of images that were generated from this image */ private function getGeneratedImages() { $generatedImages = array(); - - $methodNames = $this->allMethodNames(true); $cachedFiles = array(); $folder = $this->ParentID ? $this->Parent()->Filename : ASSETS_DIR . '/'; @@ -557,22 +585,26 @@ class Image extends File { } } - $generateFuncs = array(); - foreach($methodNames as $methodName) { - if(substr($methodName, 0, 8) == 'generate') { - $format = substr($methodName, 8); - $generateFuncs[] = preg_quote($format); - } - } - // All generate functions may appear any number of times in the image cache name. - $generateFuncs = implode('|', $generateFuncs); - $pattern = "/^((?P{$generateFuncs})(?P\d*)x(?P\d*)\-)+" . preg_quote($this->Name) . "$/i"; + $pattern = $this->getFilenamePatterns($this->Name); foreach($cachedFiles as $cfile) { - if(preg_match($pattern, $cfile, $matches)) { + if(preg_match($pattern['FullPattern'], $cfile, $matches)) { if(Director::fileExists($cacheDir . $cfile)) { - $generatedImages[] = array ( 'FileName' => $cacheDir . $cfile, 'Generator' => $matches['Generator'], - 'Arg1' => $matches['Arg1'], 'Arg2' => $matches['Arg2'] ); + $subFilename = substr($cfile, 0, -1 * strlen($this->Name)); + preg_match_all($pattern['GeneratorPattern'], $subFilename, $subMatches, PREG_SET_ORDER); + + $generatorArray = array(); + foreach ($subMatches as $singleMatch) { + $generatorArray[] = array('Generator' => $singleMatch['Generator'], + 'Args' => json_decode(base64_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) ); } } } @@ -588,11 +620,23 @@ class Image extends File { 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; + $numGenerated = 0; $generatedImages = $this->getGeneratedImages(); + $doneList = array(); foreach($generatedImages as $singleImage) { - $this->generateFormattedImage($singleImage['Generator'], $singleImage['Arg1'], - $singleImage['Arg2']); + $cachedImage = $this; + if (in_array($singleImage['FileName'], $doneList) ) continue; + + foreach($singleImage['Generators'] as $singleGenerator) { + $args = array_merge(array($singleGenerator['Generator']), $singleGenerator['Args']); + $cachedImage = call_user_func_array(array($cachedImage, "getFormattedImage"), $args); + } + $doneList[] = $singleImage['FileName']; + $numGenerated++; } return $numGenerated; diff --git a/tests/forms/HtmlEditorFieldTest.php b/tests/forms/HtmlEditorFieldTest.php index 8beb5f708..454870a4e 100644 --- a/tests/forms/HtmlEditorFieldTest.php +++ b/tests/forms/HtmlEditorFieldTest.php @@ -87,8 +87,11 @@ class HtmlEditorFieldTest extends FunctionalTest { $this->assertEquals('', (string)$xml[0]['title'], 'Title tags are added by default.'); $this->assertEquals(10, (int)$xml[0]['width'], 'Width tag of resized image is set.'); $this->assertEquals(20, (int)$xml[0]['height'], 'Height tag of resized image is set.'); - $this->assertEquals('assets/_resampled/ResizedImage10x20-HTMLEditorFieldTest_example.jpg', (string)$xml[0]['src'], 'Correct URL of resized image is set.'); - $this->assertTrue(file_exists('assets/_resampled/ResizedImage10x20-HTMLEditorFieldTest_example.jpg'), 'File for resized image exists'); + + $neededFilename = 'assets/_resampled/ResizedImage' . base64_encode(json_encode(array(10,20))) . '-HTMLEditorFieldTest_example.jpg'; + + $this->assertEquals($neededFilename, (string)$xml[0]['src'], 'Correct URL of resized image is set.'); + $this->assertTrue(file_exists($neededFilename), 'File for resized image exists'); $this->assertEquals(false, $obj->HasBrokenFile, 'Referenced image file exists.'); } diff --git a/tests/model/ImageTest.php b/tests/model/ImageTest.php index b728df789..7bb56e26e 100644 --- a/tests/model/ImageTest.php +++ b/tests/model/ImageTest.php @@ -175,6 +175,90 @@ class ImageTest extends SapphireTest { $ratio = $image->SetRatioSize(80, 160); $this->assertTrue($ratio->isSize(80, 80)); } + + /** + * @expectedException PHPUnit_Framework_Error + */ + public function testGenerateImageWithInvalidParameters() { + $image = $this->objFromFixture('Image', 'imageWithoutTitle'); + $image->setHeight('String'); + $image->PaddedImage(600,600,'XXXXXX'); + } + + public function testCacheFilename() { + $image = $this->objFromFixture('Image', 'imageWithoutTitle'); + $imageFirst = $image->SetSize(200,200); + $imageFilename = $imageFirst->getFullPath(); + // Encoding of the arguments is duplicated from cacheFilename + $neededPart = 'SetSize' . base64_encode(json_encode(array(200,200))); + $this->assertContains($neededPart, $imageFilename, 'Filename for cached image is correctly generated'); + } + + public function testMultipleGenerateManipulationCalls_Regeneration() { + $image = $this->objFromFixture('Image', 'imageWithoutTitle'); + $folder = new SS_FileFinder(); + + $imageFirst = $image->SetSize(200,200); + $this->assertNotNull($imageFirst); + $expected = 200; + $actual = $imageFirst->getWidth(); + + $this->assertEquals($expected, $actual); + + $imageSecond = $imageFirst->setHeight(100); + $this->assertNotNull($imageSecond); + $expected = 100; + $actual = $imageSecond->getHeight(); + $this->assertEquals($expected, $actual); + + $imageThird = $imageSecond->PaddedImage(600,600,'0F0F0F'); + // Encoding of the arguments is duplicated from cacheFilename + $argumentString = base64_encode(json_encode(array(600,600,'0F0F0F'))); + $this->assertNotNull($imageThird); + $this->assertContains($argumentString, $imageThird->getFullPath(), 'Image contains background color for padded resizement'); + + $imageThirdPath = $imageThird->getFullPath(); + $filesInFolder = $folder->find(dirname($imageThirdPath)); + $this->assertEquals(3, count($filesInFolder), 'Image folder contains only the expected number of images before regeneration'); + + $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)), 'Image folder contains only the expected image files after regeneration'); + } + + public function testRegenerateImages() { + $image = $this->objFromFixture('Image', 'imageWithMetacharacters'); + $image_generated = $image->SetWidth(200); + $p = $image_generated->getFullPath(); + $this->assertTrue(file_exists($p), 'Resized image exists after creation call'); + $this->assertEquals(1, $image->regenerateFormattedImages(), 'Cached images were regenerated correct'); + $this->assertEquals($image_generated->getWidth(), 200, 'Resized image has correct width after regeneration call'); + $this->assertTrue(file_exists($p), 'Resized image exists after regeneration call'); + } + + public function testRegenerateImagesWithRenaming() { + $image = $this->objFromFixture('Image', 'imageWithMetacharacters'); + $image_generated = $image->SetWidth(200); + $p = $image_generated->getFullPath(); + $this->assertTrue(file_exists($p), 'Resized image exists after creation call'); + + // Encoding of the arguments is duplicated from cacheFilename + $oldArgumentString = base64_encode(json_encode(array(200))); + $newArgumentString = base64_encode(json_encode(array(300))); + + $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->assertEquals(1, $image->regenerateFormattedImages(), 'Cached images were regenerated in the right number'); + + $image_generated_2 = new Image_Cached($newRelative); + $this->assertEquals(300, $image_generated_2->getWidth(), 'Cached image was regenerated with correct width'); + } public function testGeneratedImageDeletion() { $image = $this->objFromFixture('Image', 'imageWithMetacharacters');