From f9892c628c8709333969acb3c8ace4cb820b9987 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 12 Oct 2015 17:34:34 +1300 Subject: [PATCH 1/2] API Generated files API API Refactor Requirements_Backend to use new APL --- _config/asset.yml | 4 + control/HTTPResponse.php | 5 +- docs/en/04_Changelogs/4.0.0.md | 4 + .../storage/CacheGeneratedAssetHandler.php | 167 ++++ filesystem/storage/GeneratedAssetHandler.php | 47 + logging/DebugViewFriendlyErrorFormatter.php | 125 ++- tests/forms/HtmlEditorConfigTest.php | 8 +- tests/forms/RequirementsTest.php | 399 ++++---- tests/view/SSViewerTest.php | 58 +- view/Requirements.php | 867 ++++++++++-------- 10 files changed, 1053 insertions(+), 631 deletions(-) create mode 100644 filesystem/storage/CacheGeneratedAssetHandler.php create mode 100644 filesystem/storage/GeneratedAssetHandler.php diff --git a/_config/asset.yml b/_config/asset.yml index a71ee605b..9fd3c7bcd 100644 --- a/_config/asset.yml +++ b/_config/asset.yml @@ -25,3 +25,7 @@ Injector: type: prototype # Image mechanism Image_Backend: GDBackend + GeneratedAssetHandler: + class: SilverStripe\Filesystem\Storage\CacheGeneratedAssetHandler + properties: + AssetStore: '%$AssetStore' diff --git a/control/HTTPResponse.php b/control/HTTPResponse.php index b06e21123..fcc2fdbea 100644 --- a/control/HTTPResponse.php +++ b/control/HTTPResponse.php @@ -272,8 +272,9 @@ EOT // a more specific error description. if(Director::isLive() && $this->isError() && !$this->body) { $formatter = Injector::get('FriendlyErrorFormatter'); - $formatter->setStatusCode($this->statusCode); - echo $formatter->format(array()); + echo $formatter->format(array( + 'code' => $this->statusCode + )); } else { echo $this->body; diff --git a/docs/en/04_Changelogs/4.0.0.md b/docs/en/04_Changelogs/4.0.0.md index 5ab4957c4..6290ceb3a 100644 --- a/docs/en/04_Changelogs/4.0.0.md +++ b/docs/en/04_Changelogs/4.0.0.md @@ -19,6 +19,8 @@ the `DataObject::ID` in a `data-fileid` property, or via shortcodes. This is necessary because file urls are no longer able to identify assets. * Extension point `HtmlEditorField::processImage` has been removed, and moved to `Image::regenerateImageHTML` + * `Requirements::combined_files` no longer does JS minification. Please ensure that JS files are optimised + before combining. ## New API @@ -26,6 +28,8 @@ * `ShortcodeHandler` interface to help generate standard handlers for HTML shortcodes in the editor. * `AssetNameGenerator` interface, including a `DefaultAssetNameGenerator` implementation, which is used to generate renaming suggestions based on an original given filename in order to resolve file duplication issues. + * `GeneratedAssetHandler` API now used to store and manage generated files (such as those used for error page + cache or combined files). ## Deprecated classes/methods diff --git a/filesystem/storage/CacheGeneratedAssetHandler.php b/filesystem/storage/CacheGeneratedAssetHandler.php new file mode 100644 index 000000000..439cab2a7 --- /dev/null +++ b/filesystem/storage/CacheGeneratedAssetHandler.php @@ -0,0 +1,167 @@ +assetStore = $store; + return $this; + } + + /** + * Get the asset backend + * + * @return AssetStore + */ + public function getAssetStore() { + return $this->assetStore; + } + + /** + * @return Zend_Cache_Core + */ + protected static function get_cache() { + $cache = SS_Cache::factory('CacheGeneratedAssetHandler'); + $cache->setLifetime(Config::inst()->get(__CLASS__, 'lifetime')); + return $cache; + } + + /** + * Flush the cache + */ + public static function flush() { + self::get_cache()->clean(); + } + + public function getGeneratedURL($filename, $entropy = 0, $callback = null) { + $result = $this->getGeneratedFile($filename, $entropy, $callback); + if($result) { + return $this + ->getAssetStore() + ->getAsURL($result['Filename'], $result['Hash'], $result['Variant']); + } + } + + public function getGeneratedContent($filename, $entropy = 0, $callback = null) { + $result = $this->getGeneratedFile($filename, $entropy, $callback); + if($result) { + return $this + ->getAssetStore() + ->getAsString($result['Filename'], $result['Hash'], $result['Variant']); + } + } + + /** + * Generate or return the tuple for the given file, optionally regenerating it if it + * doesn't exist + * + * @param string $filename + * @param mixed $entropy + * @param callable $callback + * @return array tuple array if available + * @throws Exception If the file isn't available and $callback fails to regenerate content + */ + protected function getGeneratedFile($filename, $entropy = 0, $callback = null) { + // Check if there is an existing asset + $cache = self::get_cache(); + $cacheID = $this->getCacheKey($filename, $entropy); + $data = $cache->load($cacheID); + if($data) { + $result = unserialize($data); + return $this->validateResult($result, $filename); + } + + // Regenerate + if($callback) { + // Invoke regeneration and save + $content = call_user_func($callback); + return $this->updateContent($filename, $entropy, $content); + } + } + + public function updateContent($filename, $entropy, $content) { + $cache = self::get_cache(); + $cacheID = $this->getCacheKey($filename, $entropy); + + // Store content + $result = $this + ->getAssetStore() + ->setFromString($content, $filename); + if($result) { + $cache->save(serialize($result), $cacheID); + } + + return $this->validateResult($result, $filename); + } + + /** + * Get cache key for the given generated asset + * + * @param string $filename + * @param mixed $entropy + * @return string + */ + protected function getCacheKey($filename, $entropy = 0) { + $cacheID = sha1($filename); + if($entropy) { + $cacheID .= '_' . sha1($entropy); + } + return $cacheID; + } + + /** + * Validate that the given result is valid + * + * @param mixed $result + * @param string $filename + * @return array The result + * @throws Exception + */ + protected function validateResult($result, $filename) { + // Retrieve URL from tuple + $store = $this->getAssetStore(); + if($result && $store->exists($result['Filename'], $result['Hash'], $result['Variant'])) { + return $result; + } + + throw new Exception("Error regenerating file \"{$filename}\""); + } + +} diff --git a/filesystem/storage/GeneratedAssetHandler.php b/filesystem/storage/GeneratedAssetHandler.php new file mode 100644 index 000000000..3cbc64192 --- /dev/null +++ b/filesystem/storage/GeneratedAssetHandler.php @@ -0,0 +1,47 @@ +statusCode; } + /** + * Set default status code + * + * @param int $statusCode + */ public function setStatusCode($statusCode) { $this->statusCode = $statusCode; } - public function getTitle($title) { + /** + * Get friendly title + * + * @return string + */ + public function getTitle() { return $this->friendlyErrorMessage; } + /** + * Set friendly title + * + * @param string $title + */ public function setTitle($title) { $this->friendlyErrorMessage = $title; } - public function getBody($title) { + /** + * Get default error body + * + * @return string + */ + public function getBody() { return $this->friendlyErrorDetail; } + /** + * Set default error body + * + * @param string $body + */ public function setBody($body) { $this->friendlyErrorDetail = $body; } - public function format(array $record) - { - - return $this->output(); + public function format(array $record) { + // Get error code + $code = empty($record['code']) ? $this->statusCode : $record['code']; + return $this->output($code); } public function formatBatch(array $records) { - return $this->output(); - } + $message = ''; + foreach ($records as $record) { + $message .= $this->format($record); + } + return $message; + } - public function output() { + /** + * Return the appropriate error content for the given status code + * + * @param int $statusCode + * @return string Content in an appropriate format for the current request + */ + public function output($statusCode) { // TODO: Refactor into a content-type option if(\Director::is_ajax()) { - return $this->friendlyErrorMessage; - - } else { - // TODO: Refactor this into CMS - if(class_exists('ErrorPage')){ - $errorFilePath = \ErrorPage::get_filepath_for_errorcode( - $this->statusCode, - class_exists('Translatable') ? \Translatable::get_current_locale() : null - ); - - if(file_exists($errorFilePath)) { - $content = file_get_contents($errorFilePath); - if(!headers_sent()) { - header('Content-Type: text/html'); - } - // $BaseURL is left dynamic in error-###.html, so that multi-domain sites don't get broken - return str_replace('$BaseURL', \Director::absoluteBaseURL(), $content); - } - } - - - $renderer = \Debug::create_debug_view(); - $output = $renderer->renderHeader(); - $output .= $renderer->renderInfo("Website Error", $this->friendlyErrorMessage, $this->friendlyErrorDetail); - - if(\Email::config()->admin_email) { - $mailto = \Email::obfuscate(\Email::config()->admin_email); - $output .= $renderer->renderParagraph('Contact an administrator: ' . $mailto . ''); - } - - $output .= $renderer->renderFooter(); - return $output; + return $this->getTitle(); } + + $renderer = \Debug::create_debug_view(); + $output = $renderer->renderHeader(); + $output .= $renderer->renderInfo("Website Error", $this->getTitle(), $this->getBody()); + + if(\Email::config()->admin_email) { + $mailto = \Email::obfuscate(\Email::config()->admin_email); + $output .= $renderer->renderParagraph('Contact an administrator: ' . $mailto . ''); + } + + $output .= $renderer->renderFooter(); + return $output; } } diff --git a/tests/forms/HtmlEditorConfigTest.php b/tests/forms/HtmlEditorConfigTest.php index 0c7a1754a..aeef05764 100644 --- a/tests/forms/HtmlEditorConfigTest.php +++ b/tests/forms/HtmlEditorConfigTest.php @@ -75,8 +75,8 @@ class HtmlEditorConfigTest extends SapphireTest { HtmlEditorConfig::require_js(); $js = Requirements::get_custom_scripts(); - $this->assertContains('tinymce.PluginManager.load("plugin1", "/mypath/plugin1");', $js); - $this->assertContains('tinymce.PluginManager.load("plugin2", "/mypath/plugin2");', $js); + $this->assertContains('tinymce.PluginManager.load("plugin1", "/mypath/plugin1");', $js['htmlEditorConfig']); + $this->assertContains('tinymce.PluginManager.load("plugin2", "/mypath/plugin2");', $js['htmlEditorConfig']); } public function testRequireJSIncludesAllConfigs() { @@ -86,7 +86,7 @@ class HtmlEditorConfigTest extends SapphireTest { HtmlEditorConfig::require_js(); $js = Requirements::get_custom_scripts(); - $this->assertContains('"configA":{', $js); - $this->assertContains('"configB":{', $js); + $this->assertContains('"configA":{', $js['htmlEditorConfig']); + $this->assertContains('"configB":{', $js['htmlEditorConfig']); } } diff --git a/tests/forms/RequirementsTest.php b/tests/forms/RequirementsTest.php index aa459766f..ce0ceef8a 100644 --- a/tests/forms/RequirementsTest.php +++ b/tests/forms/RequirementsTest.php @@ -1,4 +1,6 @@ '; - static $old_requirements = null; + public function setUp() { + parent::setUp(); + AssetStoreTest_SpyStore::activate('RequirementsTest'); // Set backend root to /RequirementsTest + } + + public function tearDown() { + AssetStoreTest_SpyStore::reset(); + parent::tearDown(); + } public function testExternalUrls() { $backend = new Requirements_Backend; - $backend->set_combined_files_enabled(true); + $backend->setCombinedFilesEnabled(true); $backend->javascript('http://www.mydomain.com/test.js'); $backend->javascript('https://www.mysecuredomain.com/test.js'); @@ -51,23 +61,35 @@ class RequirementsTest extends SapphireTest { ); } + /** + * Setup new backend + * + * @param Requirements_Backend $backend + */ + protected function setupRequirements($backend) { + // Flush requirements + $backend->clear(); + $backend->clearCombinedFiles(); + $backend->setCombinedFilesFolder('_combinedfiles'); + CacheGeneratedAssetHandler::flush(); + } + + /** + * Setup combined and non-combined js with the backend + * + * @param Requirements_Backend $backend + */ protected function setupCombinedRequirements($backend) { $basePath = $this->getCurrentRelativePath(); - - $backend->clear(); - $backend->setCombinedFilesFolder('assets'); - - // clearing all previously generated requirements (just in case) - $backend->clear_combined_files(); - $backend->delete_combined_files('RequirementsTest_bc.js'); - + $this->setupRequirements($backend); + // require files normally (e.g. called from a FormField instance) $backend->javascript($basePath . '/RequirementsTest_a.js'); $backend->javascript($basePath . '/RequirementsTest_b.js'); $backend->javascript($basePath . '/RequirementsTest_c.js'); // require two of those files as combined includes - $backend->combine_files( + $backend->combineFiles( 'RequirementsTest_bc.js', array( $basePath . '/RequirementsTest_b.js', @@ -76,44 +98,46 @@ class RequirementsTest extends SapphireTest { ); } + /** + * Setup combined files with the backend + * + * @param Requirements_Backend $backend + */ protected function setupCombinedNonrequiredRequirements($backend) { - $basePath = $this->getCurrentRelativePath(); + $basePath = $this->getCurrentRelativePath(); + $this->setupRequirements($backend); - $backend->clear(); - $backend->setCombinedFilesFolder('assets'); - - // clearing all previously generated requirements (just in case) - $backend->clear_combined_files(); - $backend->delete_combined_files('RequirementsTest_bc.js'); - - // require files as combined includes - $backend->combine_files( - 'RequirementsTest_bc.js', - array( - $basePath . '/RequirementsTest_b.js', - $basePath . '/RequirementsTest_c.js' - ) - ); - } + // require files as combined includes + $backend->combineFiles( + 'RequirementsTest_bc.js', + array( + $basePath . '/RequirementsTest_b.js', + $basePath . '/RequirementsTest_c.js' + ) + ); + } public function testCombinedJavascript() { - $backend = new Requirements_Backend; - $backend->set_combined_files_enabled(true); - $backend->setCombinedFilesFolder('assets'); - + $backend = new Requirements_Backend(); $this->setupCombinedRequirements($backend); - $combinedFilePath = Director::baseFolder() . '/assets/' . 'RequirementsTest_bc.js'; + $combinedFileName = '/_combinedfiles/b2a28d2463/RequirementsTest_bc.js'; + $combinedFilePath = AssetStoreTest_SpyStore::base_path() . $combinedFileName; $html = $backend->includeInHTML(false, self::$html_template); /* COMBINED JAVASCRIPT FILE IS INCLUDED IN HTML HEADER */ - $this->assertTrue((bool)preg_match('/src=".*\/RequirementsTest_bc\.js/', $html), - 'combined javascript file is included in html header'); + $this->assertRegExp( + '/src=".*' . preg_quote($combinedFileName, '/') . '/', + $html, + 'combined javascript file is included in html header' + ); /* COMBINED JAVASCRIPT FILE EXISTS */ - $this->assertTrue(file_exists($combinedFilePath), - 'combined javascript file exists'); + $this->assertTrue( + file_exists($combinedFilePath), + 'combined javascript file exists' + ); /* COMBINED JAVASCRIPT HAS CORRECT CONTENT */ $this->assertTrue((strpos(file_get_contents($combinedFilePath), "alert('b')") !== false), @@ -122,35 +146,42 @@ class RequirementsTest extends SapphireTest { 'combined javascript has correct content'); /* COMBINED FILES ARE NOT INCLUDED TWICE */ - $this->assertFalse((bool)preg_match('/src=".*\/RequirementsTest_b\.js/', $html), - 'combined files are not included twice'); - $this->assertFalse((bool)preg_match('/src=".*\/RequirementsTest_c\.js/', $html), - 'combined files are not included twice'); + $this->assertNotRegExp( + '/src=".*\/RequirementsTest_b\.js/', + $html, + 'combined files are not included twice' + ); + $this->assertNotRegExp( + '/src=".*\/RequirementsTest_c\.js/', + $html, + 'combined files are not included twice' + ); /* NORMAL REQUIREMENTS ARE STILL INCLUDED */ - $this->assertTrue((bool)preg_match('/src=".*\/RequirementsTest_a\.js/', $html), - 'normal requirements are still included'); - - $backend->delete_combined_files('RequirementsTest_bc.js'); + $this->assertRegExp( + '/src=".*\/RequirementsTest_a\.js/', + $html, + 'normal requirements are still included' + ); // Then do it again, this time not requiring the files beforehand - $backend = new Requirements_Backend; - $backend->set_combined_files_enabled(true); - $backend->setCombinedFilesFolder('assets'); - + unlink($combinedFilePath); + $backend = new Requirements_Backend(); $this->setupCombinedNonrequiredRequirements($backend); - - $combinedFilePath = Director::baseFolder() . '/assets/' . 'RequirementsTest_bc.js'; - $html = $backend->includeInHTML(false, self::$html_template); /* COMBINED JAVASCRIPT FILE IS INCLUDED IN HTML HEADER */ - $this->assertTrue((bool)preg_match('/src=".*\/RequirementsTest_bc\.js/', $html), - 'combined javascript file is included in html header'); + $this->assertRegExp( + '/src=".*' . preg_quote($combinedFileName, '/') . '/', + $html, + 'combined javascript file is included in html header' + ); /* COMBINED JAVASCRIPT FILE EXISTS */ - $this->assertTrue(file_exists($combinedFilePath), - 'combined javascript file exists'); + $this->assertTrue( + file_exists($combinedFilePath), + 'combined javascript file exists' + ); /* COMBINED JAVASCRIPT HAS CORRECT CONTENT */ $this->assertTrue((strpos(file_get_contents($combinedFilePath), "alert('b')") !== false), @@ -159,20 +190,24 @@ class RequirementsTest extends SapphireTest { 'combined javascript has correct content'); /* COMBINED FILES ARE NOT INCLUDED TWICE */ - $this->assertFalse((bool)preg_match('/src=".*\/RequirementsTest_b\.js/', $html), - 'combined files are not included twice'); - $this->assertFalse((bool)preg_match('/src=".*\/RequirementsTest_c\.js/', $html), - 'combined files are not included twice'); - - $backend->delete_combined_files('RequirementsTest_bc.js'); + $this->assertNotRegExp( + '/src=".*\/RequirementsTest_b\.js/', + $html, + 'combined files are not included twice' + ); + $this->assertNotRegExp( + '/src=".*\/RequirementsTest_c\.js/', + $html, + 'combined files are not included twice' + ); } public function testCombinedCss() { $basePath = $this->getCurrentRelativePath(); - $backend = new Requirements_Backend; - $backend->set_combined_files_enabled(true); + $backend = new Requirements_Backend(); + $this->setupRequirements($backend); - $backend->combine_files( + $backend->combineFiles( 'print.css', array( $basePath . '/RequirementsTest_print_a.css', @@ -183,120 +218,156 @@ class RequirementsTest extends SapphireTest { $html = $backend->includeInHTML(false, self::$html_template); - $this->assertTrue((bool)preg_match('/href=".*\/print\.css/', $html), 'Print stylesheets have been combined.'); - $this->assertTrue((bool)preg_match( - '/media="print/', $html), + $this->assertRegExp( + '/href=".*\/print\.css/', + $html, + 'Print stylesheets have been combined.' + ); + $this->assertRegExp( + '/media="print/', + $html, 'Combined print stylesheet retains the media parameter' ); + + // Test that combining a file multiple times doesn't trigger an error + $backend = new Requirements_Backend(); + $this->setupRequirements($backend); + $backend->combineFiles( + 'style.css', + array( + $basePath . '/RequirementsTest_b.css', + $basePath . '/RequirementsTest_c.css' + ) + ); + $backend->combineFiles( + 'style.css', + array( + $basePath . '/RequirementsTest_b.css', + $basePath . '/RequirementsTest_c.css' + ) + ); + + $html = $backend->includeInHTML(false, self::$html_template); + $this->assertRegExp( + '/href=".*\/style\.css/', + $html, + 'Stylesheets have been combined.' + ); } public function testBlockedCombinedJavascript() { $basePath = $this->getCurrentRelativePath(); - - $backend = new Requirements_Backend; - $backend->set_combined_files_enabled(true); - $backend->setCombinedFilesFolder('assets'); - $combinedFilePath = Director::baseFolder() . '/assets/' . 'RequirementsTest_bc.js'; + $backend = new Requirements_Backend(); + $this->setupCombinedRequirements($backend); + $combinedFileName = '/_combinedfiles/b2a28d2463/RequirementsTest_bc.js'; + $combinedFilePath = AssetStoreTest_SpyStore::base_path() . $combinedFileName; /* BLOCKED COMBINED FILES ARE NOT INCLUDED */ - $this->setupCombinedRequirements($backend); $backend->block('RequirementsTest_bc.js'); - $backend->delete_combined_files('RequirementsTest_bc.js'); clearstatcache(); // needed to get accurate file_exists() results $html = $backend->includeInHTML(false, self::$html_template); - - $this->assertFalse((bool)preg_match('/src=".*\/RequirementsTest_bc\.js/', $html), - 'blocked combined files are not included '); + $this->assertFileNotExists($combinedFilePath); + $this->assertNotRegExp( + '/src=".*\/RequirementsTest_bc\.js/', + $html, + 'blocked combined files are not included' + ); $backend->unblock('RequirementsTest_bc.js'); /* BLOCKED UNCOMBINED FILES ARE NOT INCLUDED */ $this->setupCombinedRequirements($backend); $backend->block($basePath .'/RequirementsTest_b.js'); - $backend->delete_combined_files('RequirementsTest_bc.js'); + $combinedFileName2 = '/_combinedfiles/37bd2d9dcb/RequirementsTest_bc.js'; // SHA1 without file c included + $combinedFilePath2 = AssetStoreTest_SpyStore::base_path() . $combinedFileName2; clearstatcache(); // needed to get accurate file_exists() results $html = $backend->includeInHTML(false, self::$html_template); - $this->assertFalse((strpos(file_get_contents($combinedFilePath), "alert('b')") !== false), - 'blocked uncombined files are not included'); - $backend->unblock('RequirementsTest_b.js'); + $this->assertFileExists($combinedFilePath2); + $this->assertTrue( + strpos(file_get_contents($combinedFilePath2), "alert('b')") === false, + 'blocked uncombined files are not included' + ); + $backend->unblock($basePath . '/RequirementsTest_b.js'); /* A SINGLE FILE CAN'T BE INCLUDED IN TWO COMBINED FILES */ $this->setupCombinedRequirements($backend); clearstatcache(); // needed to get accurate file_exists() results - // This throws a notice-level error, so we prefix with @ - @$backend->combine_files( + // Exception generated from including invalid file + $this->setExpectedException( + 'InvalidArgumentException', + sprintf( + "Requirements_Backend::combine_files(): Already included file(s) %s in combined file '%s'", + $basePath . '/RequirementsTest_c.js', + 'RequirementsTest_bc.js' + ) + ); + $backend->combineFiles( 'RequirementsTest_ac.js', array( $basePath . '/RequirementsTest_a.js', $basePath . '/RequirementsTest_c.js' ) ); - - $combinedFiles = $backend->get_combine_files(); - $this->assertEquals( - array_keys($combinedFiles), - array('RequirementsTest_bc.js'), - "A single file can't be included in two combined files" - ); - - $backend->delete_combined_files('RequirementsTest_bc.js'); } public function testArgsInUrls() { $basePath = $this->getCurrentRelativePath(); $backend = new Requirements_Backend; - $backend->set_combined_files_enabled(true); + $this->setupRequirements($backend); $backend->javascript($basePath . '/RequirementsTest_a.js?test=1&test=2&test=3'); $backend->css($basePath . '/RequirementsTest_a.css?test=1&test=2&test=3'); - $backend->delete_combined_files('RequirementsTest_bc.js'); - $html = $backend->includeInHTML(false, self::$html_template); /* Javascript has correct path */ - $this->assertTrue( - (bool)preg_match('/src=".*\/RequirementsTest_a\.js\?m=\d\d+&test=1&test=2&test=3/',$html), - 'javascript has correct path'); + $this->assertRegExp( + '/src=".*\/RequirementsTest_a\.js\?m=\d\d+&test=1&test=2&test=3/', + $html, + 'javascript has correct path' + ); /* CSS has correct path */ - $this->assertTrue( - (bool)preg_match('/href=".*\/RequirementsTest_a\.css\?m=\d\d+&test=1&test=2&test=3/',$html), - 'css has correct path'); + $this->assertRegExp( + '/href=".*\/RequirementsTest_a\.css\?m=\d\d+&test=1&test=2&test=3/', + $html, + 'css has correct path' + ); } public function testRequirementsBackend() { $basePath = $this->getCurrentRelativePath(); $backend = new Requirements_Backend(); + $this->setupRequirements($backend); $backend->javascript($basePath . '/a.js'); - $this->assertTrue(count($backend->get_javascript()) == 1, + $this->assertTrue(count($backend->getJavascript()) == 1, "There should be only 1 file included in required javascript."); - $this->assertTrue(in_array($basePath . '/a.js', $backend->get_javascript()), + $this->assertTrue(in_array($basePath . '/a.js', $backend->getJavascript()), "a.js should be included in required javascript."); $backend->javascript($basePath . '/b.js'); - $this->assertTrue(count($backend->get_javascript()) == 2, + $this->assertTrue(count($backend->getJavascript()) == 2, "There should be 2 files included in required javascript."); $backend->block($basePath . '/a.js'); - $this->assertTrue(count($backend->get_javascript()) == 1, + $this->assertTrue(count($backend->getJavascript()) == 1, "There should be only 1 file included in required javascript."); - $this->assertFalse(in_array($basePath . '/a.js', $backend->get_javascript()), + $this->assertFalse(in_array($basePath . '/a.js', $backend->getJavascript()), "a.js should not be included in required javascript after it has been blocked."); - $this->assertTrue(in_array($basePath . '/b.js', $backend->get_javascript()), + $this->assertTrue(in_array($basePath . '/b.js', $backend->getJavascript()), "b.js should be included in required javascript."); $backend->css($basePath . '/a.css'); - $this->assertTrue(count($backend->get_css()) == 1, + $this->assertTrue(count($backend->getCSS()) == 1, "There should be only 1 file included in required css."); - $this->assertArrayHasKey($basePath . '/a.css', $backend->get_css(), + $this->assertArrayHasKey($basePath . '/a.css', $backend->getCSS(), "a.css should be in required css."); $backend->block($basePath . '/a.css'); - $this->assertTrue(count($backend->get_css()) == 0, + $this->assertTrue(count($backend->getCSS()) == 0, "There should be nothing in required css after file has been blocked."); } @@ -307,6 +378,7 @@ class RequirementsTest extends SapphireTest { $basePath = 'framework' . substr($basePath, strlen(FRAMEWORK_DIR)); $backend = new Requirements_Backend(); + $this->setupRequirements($backend); $holder = Requirements::backend(); Requirements::set_backend($backend); $data = new ArrayData(array( @@ -335,16 +407,17 @@ class RequirementsTest extends SapphireTest { public function testJsWriteToBody() { $backend = new Requirements_Backend(); + $this->setupRequirements($backend); $backend->javascript('http://www.mydomain.com/test.js'); // Test matching with HTML5
tags as well $template = '
My header

Body

'; - $backend->set_write_js_to_body(false); + $backend->setWriteJavascriptToBody(false); $html = $backend->includeInHTML(false, $template); $this->assertContains('set_write_js_to_body(true); + $backend->setWriteJavascriptToBody(true); $html = $backend->includeInHTML(false, $template); $this->assertNotContains('assertContains('', $html); @@ -353,6 +426,7 @@ class RequirementsTest extends SapphireTest { public function testIncludedJsIsNotCommentedOut() { $template = ''; $backend = new Requirements_Backend(); + $this->setupRequirements($backend); $backend->javascript($this->getCurrentRelativePath() . '/RequirementsTest_a.js'); $html = $backend->includeInHTML(false, $template); //wiping out commented-out html @@ -364,9 +438,10 @@ class RequirementsTest extends SapphireTest { $template = '' . '

more content

'; $backend = new Requirements_Backend(); - $backend->set_suffix_requirements(false); + $this->setupRequirements($backend); + $backend->setSuffixRequirements(false); $src = $this->getCurrentRelativePath() . '/RequirementsTest_a.js'; - $urlSrc = Controller::join_links(Director::baseURL(), $src); + $urlSrc = ControllerTest_ContainerController::join_links(Director::baseURL(), $src); $backend->javascript($src); $html = $backend->includeInHTML(false, $template); $this->assertEquals('' @@ -375,6 +450,7 @@ class RequirementsTest extends SapphireTest { public function testForceJsToBottom() { $backend = new Requirements_Backend(); + $this->setupRequirements($backend); $backend->javascript('http://www.mydomain.com/test.js'); // Test matching with HTML5
tags as well @@ -391,8 +467,8 @@ class RequirementsTest extends SapphireTest { // Test if the script is before the head tag, not before the body. // Expected: $JsInHead - $backend->set_write_js_to_body(false); - $backend->set_force_js_to_bottom(false); + $backend->setWriteJavascriptToBody(false); + $backend->setForceJSToBottom(false); $html = $backend->includeInHTML(false, $template); $this->assertNotEquals($JsInBody, $html); $this->assertNotEquals($JsAtEnd, $html); @@ -400,16 +476,16 @@ class RequirementsTest extends SapphireTest { // Test if the script is before the first \n"; } } // Add all inline JavaScript *after* including external files they might rely on - if($this->customScript) { - foreach(array_diff_key($this->customScript,$this->blocked) as $script) { - $jsRequirements .= "\n"; - } + foreach($this->getCustomScripts() as $script) { + $jsRequirements .= "\n"; } - foreach(array_diff_key($this->css,$this->blocked) as $file => $params) { - $path = Convert::raw2xml($this->path_for_file($file)); + foreach($this->getCSS() as $file => $params) { + $path = Convert::raw2xml($this->pathForFile($file)); if($path) { $media = (isset($params['media']) && !empty($params['media'])) ? " media=\"{$params['media']}\"" : ""; @@ -824,15 +953,15 @@ class Requirements_Backend { } } - foreach(array_diff_key($this->customCSS, $this->blocked) as $css) { + foreach($this->getCustomCSS() as $css) { $requirements .= "\n"; } - foreach(array_diff_key($this->customHeadTags,$this->blocked) as $customHeadTag) { + foreach($this->getCustomHeadTags() as $customHeadTag) { $requirements .= "$customHeadTag\n"; } - if ($this->force_js_to_bottom) { + if ($this->getForceJSToBottom()) { // Remove all newlines from code to preserve layout $jsRequirements = preg_replace('/>\n*/', '>', $jsRequirements); @@ -842,7 +971,7 @@ class Requirements_Backend { // Put CSS at the bottom of the head $content = preg_replace("/(<\/head>)/i", $requirements . "\\1", $content); - } elseif($this->write_js_to_body) { + } elseif($this->getWriteJavascriptToBody()) { // Remove all newlines from code to preserve layout $jsRequirements = preg_replace('/>\n*/', '>', $jsRequirements); @@ -884,13 +1013,13 @@ class Requirements_Backend { * * @param SS_HTTPResponse $response */ - public function include_in_response(SS_HTTPResponse $response) { - $this->process_combined_files(); + public function includeInResponse(SS_HTTPResponse $response) { + $this->processCombinedFiles(); $jsRequirements = array(); $cssRequirements = array(); - foreach(array_diff_key($this->javascript, $this->blocked) as $file => $dummy) { - $path = $this->path_for_file($file); + foreach($this->getJavascript() as $file) { + $path = $this->pathForFile($file); if($path) { $jsRequirements[] = str_replace(',', '%2C', $path); } @@ -898,8 +1027,8 @@ class Requirements_Backend { $response->addHeader('X-Include-JS', implode(',', $jsRequirements)); - foreach(array_diff_key($this->css,$this->blocked) as $file => $params) { - $path = $this->path_for_file($file); + foreach($this->getCSS() as $file => $params) { + $path = $this->pathForFile($file); if($path) { $path = str_replace(',', '%2C', $path); $cssRequirements[] = isset($params['media']) ? "$path:##:$params[media]" : $path; @@ -948,13 +1077,17 @@ class Requirements_Backend { } } else { // Stub i18n implementation for when i18n is disabled. - if(!$langOnly) $files[] = FRAMEWORK_DIR . '/javascript/i18nx.js'; + if(!$langOnly) { + $files[] = FRAMEWORK_DIR . '/javascript/i18nx.js'; + } } if($return) { return $files; } else { - foreach($files as $file) $this->javascript($file); + foreach($files as $file) { + $this->javascript($file); + } } } @@ -964,15 +1097,16 @@ class Requirements_Backend { * @param string $fileOrUrl * @return string|bool */ - protected function path_for_file($fileOrUrl) { - if(preg_match('{^//|http[s]?}', $fileOrUrl)) { + protected function pathForFile($fileOrUrl) { + // Since combined urls could be root relative, treat them as urls here. + if(preg_match('{^(//)|(http[s]?:)}', $fileOrUrl) || Director::is_root_relative_url($fileOrUrl)) { return $fileOrUrl; } elseif(Director::fileExists($fileOrUrl)) { $filePath = preg_replace('/\?.*/', '', Director::baseFolder() . '/' . $fileOrUrl); $prefix = Director::baseURL(); $mtimesuffix = ""; $suffix = ''; - if($this->suffix_requirements) { + if($this->getSuffixRequirements()) { $mtimesuffix = "?m=" . filemtime($filePath); $suffix = '&'; } @@ -996,13 +1130,10 @@ class Requirements_Backend { * increases performance by fewer HTTP requests. * * The combined file is regenerated based on every file modification time. Optionally a - * rebuild can be triggered by appending ?flush=1 to the URL. If all files to be combined are - * JavaScript, we use the external JSMin library to minify the JavaScript. This can be - * controlled using {@link $combine_js_with_jsmin}. + * rebuild can be triggered by appending ?flush=1 to the URL. * * All combined files will have a comment on the start of each concatenated file denoting their - * original position. For easier debugging, we only minify JavaScript if not in development - * mode ({@link Director::isDev()}). + * original position. * * CAUTION: You're responsible for ensuring that the load order for combined files is * retained - otherwise combining JavaScript files can lead to functional errors in the @@ -1034,269 +1165,275 @@ class Requirements_Backend { * * * @param string $combinedFileName Filename of the combined file relative to docroot - * @param array $files Array of filenames relative to docroot - * @param string $media - * - * @return bool|void + * @param array $files Array of filenames relative to docroot + * @param string $media If including CSS Files, you can specify a media type */ - public function combine_files($combinedFileName, $files, $media = null) { - // duplicate check - foreach($this->combine_files as $_combinedFileName => $_files) { - $duplicates = array_intersect($_files, $files); - if($duplicates && $combinedFileName != $_combinedFileName) { - user_error("Requirements_Backend::combine_files(): Already included files " . implode(',', $duplicates) - . " in combined file '{$_combinedFileName}'", E_USER_NOTICE); - return false; + public function combineFiles($combinedFileName, $files, $media = null) { + // Skip this combined files if already included + if(isset($this->combinedFiles[$combinedFileName])) { + return; + } + + // Add all files to necessary type list + $paths = array(); + $combinedType = null; + foreach($files as $file) { + // Get file details + list($path, $type) = $this->parseCombinedFile($file); + if($type === 'javascript') { + $type = 'js'; + } + if($combinedType && $type && $combinedType !== $type) { + throw new InvalidArgumentException( + "Cannot mix js and css files in same combined file {$combinedFileName}" + ); + } + switch($type) { + case 'css': + $this->css($path, $media); + break; + case 'js': + $this->javascript($path); + break; + default: + throw new InvalidArgumentException("Invalid combined file type: {$type}"); + } + $combinedType = $type; + $paths[] = $path; + } + + // Duplicate check + foreach($this->combinedFiles as $existingCombinedFilename => $combinedItem) { + $existingFiles = $combinedItem['files']; + $duplicates = array_intersect($existingFiles, $paths); + if($duplicates) { + throw new InvalidArgumentException(sprintf( + "Requirements_Backend::combine_files(): Already included file(s) %s in combined file '%s'", + implode(',', $duplicates), + $existingCombinedFilename + )); } } - foreach($files as $index=>$file) { - if(is_array($file)) { - // Either associative array path=>path type=>type or numeric 0=>path 1=>type - // Otherwise, assume path is the first item - if (isset($file['type']) && in_array($file['type'], array('css', 'javascript', 'js'))) { - switch ($file['type']) { - case 'css': - $this->css($file['path'], $media); - break; - default: - $this->javascript($file['path']); - break; - } - $files[$index] = $file['path']; - } elseif (isset($file[1]) && in_array($file[1], array('css', 'javascript', 'js'))) { - switch ($file[1]) { - case 'css': - $this->css($file[0], $media); - break; - default: - $this->javascript($file[0]); - break; - } - $files[$index] = $file[0]; - } else { - $file = array_shift($file); - } - } - if (!is_array($file)) { - if(substr($file, -2) == 'js') { - $this->javascript($file); - } elseif(substr($file, -3) == 'css') { - $this->css($file, $media); - } else { - user_error("Requirements_Backend::combine_files(): Couldn't guess file type for file '$file', " - . "please specify by passing using an array instead.", E_USER_NOTICE); - } - } + + $this->combinedFiles[$combinedFileName] = array( + 'files' => $paths, + 'type' => $combinedType, + 'media' => $media + ); + } + + /** + * Return path and type of given combined file + * + * @param string|array $file Either a file path, or an array spec + * @return array array with two elements, path and type of file + */ + protected function parseCombinedFile($file) { + // Array with path and type keys + if(is_array($file) && isset($file['path']) && isset($file['type'])) { + return array($file['path'], $file['type']); } - $this->combine_files[$combinedFileName] = $files; + + // Extract value from indexed array + if(is_array($file)) { + $path = array_shift($file); + + // See if there's a type specifier + if($file) { + $type = array_shift($file); + return array($path, $type); + } + + // Otherwise convent to string + $file = $path; + } + + $type = File::get_file_extension($file); + return array($file, $type); } /** * Return all combined files; keys are the combined file names, values are lists of - * files being combined. + * associative arrays with 'files', 'type', and 'media' keys for details about this + * combined file. * * @return array */ - public function get_combine_files() { - return $this->combine_files; + public function getCombinedFiles() { + return array_diff_key($this->combinedFiles, $this->blocked); } /** - * Delete all dynamically generated combined files from the filesystem + * Includes all combined files, including blocked ones * - * @param string $combinedFileName If left blank, all combined files are deleted. + * @return type */ - public function delete_combined_files($combinedFileName = null) { - $combinedFiles = ($combinedFileName) ? array($combinedFileName => null) : $this->combine_files; - $combinedFolder = ($this->getCombinedFilesFolder()) ? - (Director::baseFolder() . '/' . $this->combinedFilesFolder) : Director::baseFolder(); - foreach($combinedFiles as $combinedFile => $sourceItems) { - $filePath = $combinedFolder . '/' . $combinedFile; - if(file_exists($filePath)) { - unlink($filePath); - } - } - } - - /** - * Deletes all generated combined files in the configured combined files directory, - * but doesn't delete the directory itself. - */ - public function delete_all_combined_files() { - $combinedFolder = $this->getCombinedFilesFolder(); - if(!$combinedFolder) return false; - - $path = Director::baseFolder() . '/' . $combinedFolder; - if(file_exists($path)) { - Filesystem::removeFolder($path, true); - } + protected function getAllCombinedFiles() { + return $this->combinedFiles; } /** * Clear all registered CSS and JavaScript file combinations */ - public function clear_combined_files() { - $this->combine_files = array(); + public function clearCombinedFiles() { + $this->combinedFiles = array(); } /** - * Do the heavy lifting involved in combining (and, in the case of JavaScript minifying) the - * combined files. + * Do the heavy lifting involved in combining the combined files. */ - public function process_combined_files() { - // The class_exists call prevents us loading SapphireTest.php (slow) just to know that - // SapphireTest isn't running :-) - if(class_exists('SapphireTest', false)) $runningTest = SapphireTest::is_running_test(); - else $runningTest = false; - - if((Director::isDev() && !$runningTest && !isset($_REQUEST['combine'])) || !$this->combined_files_enabled) { + public function processCombinedFiles() { + // Check if combining is enabled + if(!$this->enabledCombinedFiles()) { return; } - // Make a map of files that could be potentially combined - $combinerCheck = array(); - foreach($this->combine_files as $combinedFile => $sourceItems) { - foreach($sourceItems as $sourceItem) { - if(isset($combinerCheck[$sourceItem]) && $combinerCheck[$sourceItem] != $combinedFile){ - user_error("Requirements_Backend::process_combined_files - file '$sourceItem' appears in two " . - "combined files:" . " '{$combinerCheck[$sourceItem]}' and '$combinedFile'", E_USER_WARNING); - } - $combinerCheck[$sourceItem] = $combinedFile; + // Process each combined files + foreach($this->getAllCombinedFiles() as $combinedFile => $combinedItem) { + $fileList = $combinedItem['files']; + $type = $combinedItem['type']; + $media = $combinedItem['media']; - } - } - - // Work out the relative URL for the combined files from the base folder - $combinedFilesFolder = ($this->getCombinedFilesFolder()) ? ($this->getCombinedFilesFolder() . '/') : ''; - - // Figure out which ones apply to this request - $combinedFiles = array(); - $newJSRequirements = array(); - $newCSSRequirements = array(); - foreach($this->javascript as $file => $dummy) { - if(isset($combinerCheck[$file])) { - $newJSRequirements[$combinedFilesFolder . $combinerCheck[$file]] = true; - $combinedFiles[$combinerCheck[$file]] = true; - } else { - $newJSRequirements[$file] = true; - } - } - - foreach($this->css as $file => $params) { - if(isset($combinerCheck[$file])) { - // Inherit the parameters from the last file in the combine set. - $newCSSRequirements[$combinedFilesFolder . $combinerCheck[$file]] = $params; - $combinedFiles[$combinerCheck[$file]] = true; - } else { - $newCSSRequirements[$file] = $params; - } - } - - // Process the combined files - $base = Director::baseFolder() . '/'; - foreach(array_diff_key($combinedFiles, $this->blocked) as $combinedFile => $dummy) { - $fileList = $this->combine_files[$combinedFile]; - $combinedFilePath = $base . $combinedFilesFolder . '/' . $combinedFile; - - - // Make the folder if necessary - if(!file_exists(dirname($combinedFilePath))) { - Filesystem::makeFolder(dirname($combinedFilePath)); + // Generate this file, unless blocked + $combinedURL = null; + if(!isset($this->blocked[$combinedFile])) { + $combinedURL = $this->getCombinedFileURL($combinedFile, $fileList); } - // If the file isn't writeable, don't even bother trying to make the combined file and return. The - // files will be included individually instead. This is a complex test because is_writable fails - // if the file doesn't exist yet. - if((file_exists($combinedFilePath) && !is_writable($combinedFilePath)) - || (!file_exists($combinedFilePath) && !is_writable(dirname($combinedFilePath))) - ) { - user_error("Requirements_Backend::process_combined_files(): Couldn't create '$combinedFilePath'", - E_USER_WARNING); - return false; - } - - // Determine if we need to build the combined include - if(file_exists($combinedFilePath)) { - // file exists, check modification date of every contained file - $srcLastMod = 0; - foreach($fileList as $file) { - if(file_exists($base . $file)) { - $srcLastMod = max(filemtime($base . $file), $srcLastMod); + // Replace all existing files, injecting the combined file at the position of the first item + // in order to preserve inclusion order. + // Note that we iterate across blocked files in order to get the correct order, and validate + // that the file is included in the correct location (regardless of which files are blocked). + $included = false; + switch($type) { + case 'css': { + $newCSS = array(); // Assoc array of css file => spec + foreach($this->getAllCSS() as $css => $spec) { + if(!in_array($css, $fileList)) { + $newCSS[$css] = $spec; + } elseif(!$included && $combinedURL) { + $newCSS[$combinedURL] = array('media' => $media); + $included = true; + } + // If already included, or otherwise blocked, then don't add into CSS } + $this->css = $newCSS; + break; } - $refresh = $srcLastMod > filemtime($combinedFilePath); - } else { - // File doesn't exist, or refresh was explicitly required - $refresh = true; - } - - if(!$refresh) continue; - - $failedToMinify = false; - $combinedData = ""; - foreach(array_diff($fileList, $this->blocked) as $file) { - $fileContent = file_get_contents($base . $file); - - try{ - $fileContent = $this->minifyFile($file, $fileContent); - }catch(Exception $e){ - $failedToMinify = true; + case 'js': { + // Assoc array of file => true + $newJS = array(); + foreach($this->getAllJavascript() as $script) { + if(!in_array($script, $fileList)) { + $newJS[$script] = true; + } elseif(!$included && $combinedURL) { + $newJS[$combinedURL] = true; + $included = true; + } + // If already included, or otherwise blocked, then don't add into scripts + } + $this->javascript = $newJS; + break; } - - if ($this->write_header_comment) { - // Write a header comment for each file for easier identification and debugging. The semicolon between each file is required for jQuery to be combined properly and protects against unterminated statements. - $combinedData .= "/****** FILE: $file *****/\n"; - } - - $combinedData .= $fileContent . "\n"; } - - $successfulWrite = false; - $fh = fopen($combinedFilePath, 'wb'); - if($fh) { - if(fwrite($fh, $combinedData) == strlen($combinedData)) $successfulWrite = true; - fclose($fh); - unset($fh); - } - - if($failedToMinify){ - // Failed to minify, use unminified files instead. This warning is raised at the end to allow code execution - // to complete in case this warning is caught inside a try-catch block. - user_error('Failed to minify '.$file.', exception: '.$e->getMessage(), E_USER_WARNING); - } - - // Unsuccessful write - just include the regular JS files, rather than the combined one - if(!$successfulWrite) { - user_error("Requirements_Backend::process_combined_files(): Couldn't create '$combinedFilePath'", - E_USER_WARNING); - continue; + if($combinedURL && !$included) { + throw new Exception("Failed to merge combined file {$combinedFile} with existing requirements"); } } - - // Note: Alters the original information, which means you can't call this method repeatedly - it will behave - // differently on the subsequent calls - $this->javascript = $newJSRequirements; - $this->css = $newCSSRequirements; } /** - * Minify the given $content according to the file type indicated in $filename + * Given a set of files, combine them (as necessary) and return the url * - * @param string $filename - * @param string $content - * @return string + * @param string $combinedFile Filename for this combined file + * @param array $fileList List of files to combine + * @return string URL to this resource */ - protected function minifyFile($filename, $content) { - // if we have a javascript file and jsmin is enabled, minify the content - $isJS = stripos($filename, '.js'); - if($isJS && $this->combine_js_with_jsmin) { - require_once('thirdparty/jsmin/jsmin.php'); + protected function getCombinedFileURL($combinedFile, $fileList) { + // Generate path (Filename) + $combinedFileID = File::join_paths($this->getCombinedFilesFolder(), $combinedFile); - increase_time_limit_to(); - $content = JSMin::minify($content); + // Get entropy for this combined file (last modified date of most recent file) + $entropy = $this->getEntropyOfFiles($fileList); + + // Send file combination request to the backend, with an optional callback to perform regeneration + $combinedURL = $this + ->getAssetHandler() + ->getGeneratedURL( + $combinedFileID, + $entropy, + function() use ($fileList) { + $combinedData = ''; + $base = Director::baseFolder() . '/'; + foreach(array_diff($fileList, $this->getBlocked()) as $file) { + $fileContent = file_get_contents($base . $file); + if ($this->writeHeaderComment) { + // Write a header comment for each file for easier identification and debugging. + $combinedData .= "/****** FILE: $file *****/\n"; + } + $combinedData .= $fileContent . "\n"; + } + return $combinedData; + } + ); + + // Since url won't be automatically suffixed, add it in here + if($this->getSuffixRequirements()) { + $q = stripos($combinedURL, '?') === false ? '?' : '&'; + $combinedURL .= "{$q}m={$entropy}"; } - $content .= ($isJS ? ';' : '') . "\n"; - return $content; + + return $combinedURL; + } + + /** + * Check if combined files are enabled + * + * @return bool + */ + protected function enabledCombinedFiles() { + if(!$this->combinedFilesEnabled) { + return false; + } + + // Tests should be combined + if(class_exists('SapphireTest', false) && SapphireTest::is_running_test()) { + return true; + } + + // Check if specified via querystring + if(isset($_REQUEST['combine'])) { + return true; + } + + // Non-dev sites are always combined + if(!Director::isDev()) { + return true; + } + + // Fallback to default + return Config::inst()->get(__CLASS__, 'combine_in_dev'); + } + + /** + * For a given filelist, determine some discriminating value to determine if + * any of these files have changed. + * + * @param array $fileList List of files + * @return int Last modified timestamp of these files + */ + protected function getEntropyOfFiles($fileList) { + // file exists, check modification date of every contained file + $base = Director::baseFolder() . '/'; + $srcLastMod = 0; + foreach($fileList as $file) { + if(file_exists($base . $file)) { + $srcLastMod = max(filemtime($base . $file), $srcLastMod); + } else { + throw new InvalidArgumentException("Combined file {$file} does not exist"); + } + } + return $srcLastMod; } /** @@ -1340,7 +1477,7 @@ class Requirements_Backend { Debug::show($this->customCSS); Debug::show($this->customScript); Debug::show($this->customHeadTags); - Debug::show($this->combine_files); + Debug::show($this->combinedFiles); } } From fe3d23f0d4c381040f47d24c4dcc9b27768a06b8 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 23 Oct 2015 11:46:58 +1300 Subject: [PATCH 2/2] BUG Fix GeneratedAssetHandler crashing on expired resources --- .../storage/CacheGeneratedAssetHandler.php | 28 ++++++++++++------- tests/filesystem/AssetStoreTest.php | 11 ++++++++ 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/filesystem/storage/CacheGeneratedAssetHandler.php b/filesystem/storage/CacheGeneratedAssetHandler.php index 439cab2a7..2c6c47989 100644 --- a/filesystem/storage/CacheGeneratedAssetHandler.php +++ b/filesystem/storage/CacheGeneratedAssetHandler.php @@ -105,7 +105,10 @@ class CacheGeneratedAssetHandler implements GeneratedAssetHandler, Flushable { $data = $cache->load($cacheID); if($data) { $result = unserialize($data); - return $this->validateResult($result, $filename); + $valid = $this->validateResult($result, $filename); + if($valid) { + return $result; + } } // Regenerate @@ -127,8 +130,14 @@ class CacheGeneratedAssetHandler implements GeneratedAssetHandler, Flushable { if($result) { $cache->save(serialize($result), $cacheID); } - - return $this->validateResult($result, $filename); + + // Ensure this result is successfully saved + $valid = $this->validateResult($result, $filename); + if($valid) { + return $result; + } + + throw new Exception("Error regenerating file \"{$filename}\""); } /** @@ -151,17 +160,16 @@ class CacheGeneratedAssetHandler implements GeneratedAssetHandler, Flushable { * * @param mixed $result * @param string $filename - * @return array The result - * @throws Exception + * @return bool True if this $result is valid */ protected function validateResult($result, $filename) { + if(!$result) { + return false; + } + // Retrieve URL from tuple $store = $this->getAssetStore(); - if($result && $store->exists($result['Filename'], $result['Hash'], $result['Variant'])) { - return $result; - } - - throw new Exception("Error regenerating file \"{$filename}\""); + return $store->exists($result['Filename'], $result['Hash'], $result['Variant']); } } diff --git a/tests/filesystem/AssetStoreTest.php b/tests/filesystem/AssetStoreTest.php index bd4454a84..1d11606cf 100644 --- a/tests/filesystem/AssetStoreTest.php +++ b/tests/filesystem/AssetStoreTest.php @@ -7,6 +7,7 @@ use SilverStripe\Filesystem\Flysystem\FlysystemAssetStore; use SilverStripe\Filesystem\Flysystem\FlysystemUrlPlugin; use SilverStripe\Filesystem\Storage\AssetContainer; use SilverStripe\Filesystem\Storage\AssetStore; +use SilverStripe\Filesystem\Storage\CacheGeneratedAssetHandler; class AssetStoreTest extends SapphireTest { @@ -450,6 +451,10 @@ class AssetStoreTest_SpyStore extends FlysystemAssetStore { * Reset defaults for this store */ public static function reset() { + // Need flushing since it won't have any files left + CacheGeneratedAssetHandler::flush(); + + // Remove all files in this store if(self::$basedir) { $path = self::base_path(); if(file_exists($path)) { @@ -469,6 +474,12 @@ class AssetStoreTest_SpyStore extends FlysystemAssetStore { if($asset instanceof Folder) { return self::base_path() . '/' . $asset->getFilename(); } + if($asset instanceof File) { + $asset = $asset->File; + } + if($asset instanceof DBFile) { + return BASE_PATH . $asset->getSourceURL(); + } return BASE_PATH . $asset->getUrl(); }