Merge pull request #4796 from open-sausages/pulls/4.0/fix-generated-files

API Replace CacheGeneratedAssetHandler

_config/asset.yml

@@ -25,9 +25,13 @@ Injector:
     type: prototype
   # Image mechanism
   Image_Backend: GDBackend
+  # Requirements config
   GeneratedAssetHandler:
-    class: SilverStripe\Filesystem\Storage\CacheGeneratedAssetHandler
+    class: SilverStripe\Filesystem\Storage\FlysystemGeneratedAssetHandler
     properties:
-      AssetStore: '%$AssetStore'
+      Filesystem: '%$FlysystemBackend'
   Requirements_Minifier:
     class: SilverStripe\View\JSMinifier
+  Requirements_Backend:
+    properties:
+      AssetHandler: '%$GeneratedAssetHandler'

control/Director.php

@@ -289,7 +289,7 @@ class Director implements TemplateGlobalProvider {
 		$existingRequirementsBackend = Requirements::backend();
 
 		Config::inst()->update('Cookie', 'report_errors', false);
-		Requirements::set_backend(new Requirements_Backend());
+		Requirements::set_backend(Injector::inst()->create('Requirements_Backend'));
 
 		// Set callback to invoke prior to return
 		$onCleanup = function() use(

docs/en/02_Developer_Guides/01_Templates/03_Requirements.md

@@ -102,22 +102,83 @@ by reducing HTTP requests.
 
 <div class="alert" markdown='1'>
 To make debugging easier in your local environment, combined files is disabled when running your application in `dev`
-mode.
+mode. You can re-enable dev combination by setting `Requirements_Backend.combine_in_dev` to true.
 </div>
 
-By default it stores the generated file in the assets/ folder, but you can configure this by pointing the 
+### Configuring combined file storage
-`Requirements.combined_files_folder` configuration setting to a specific folder.
 
-**mysite/_config/app.yml**
+In some situations or server configurations, it may be necessary to customise the behaviour of generated javascript
+files in order to ensure that current files are served in requests.
 
-	:::yml
+By default, files will be generated on demand in the format `assets/_combinedfiles/name-<hash>.js`,
-	Requirements:
+where `<hash>` represents the hash of the source files used to generate that content. The default flysystem backend,
-	  combined_files_folder: '_combined'
+as used by the `[api:AssetStore]` backend, is used for this storage, but it can be substituted for any
+other backend.
 
-<div class="info" markdown='1'>
+You can also use any of the below options in order to tweak this behaviour:
-If SilverStripe doesn't have permissions on your server to write these files it will default back to including them
+
-individually. SilverStripe **will not** rewrite your paths within the file.
+ * `Requirements.disable_flush_combined` - By default all combined files are deleted on flush.
-</div>
+   If combined files are stored in source control, and thus updated manually, you might want to
+   turn this on to disable this behaviour.
+ * `Requirements_Backend.combine_hash_querystring` - By default the `<hash>` of the source files is appended to
+   the end of the combined file (prior to the file extension). If combined files are versioned in source control,
+   or running in a distributed environment (such as one where the newest version of a file may not always be
+   immediately available) then it may sometimes be necessary to disable this. When this is set to true, the hash
+   will instead be appended via a querystring parameter to enable cache busting, but not in the
+   filename itself. I.e. `assets/_combinedfiles/name.js?m=<hash>`
+ * `Requirements_Backend.default_combined_files_folder` - This defaults to `_combinedfiles`, and is the folder
+   within the configured asset backend that combined files will be stored in. If using a backend shared with
+   other systems, it is usually necessary to distinguish combined files from other assets.
+ * `Requirements_Backend.combine_in_dev` - By default combined files will not be combined except in test
+   or live environments. Turning this on will allow for pre-combining of files in development mode.
+
+In some cases it may be necessary to create a new storage backend for combined files, if the default location
+is not appropriate. Normally a single backend is used for all site assets, so a number of objects must be
+replaced. For instance, the below will set a new set of dependencies to write to `mysite/javascript/combined`
+
+
+    :::yaml
+    ---
+    Name: myrequirements
+    ---
+    Requirements:
+      disable_flush_combined: true
+    Requirements_Backend:
+      combine_in_dev: true
+      combine_hash_querystring: true
+      default_combined_files_folder: 'combined'
+    Injector:
+      MySiteAdapter:
+        class: 'SilverStripe\Filesystem\Flysystem\AssetAdapter'
+        constructor:
+          Root: ./mysite/javascript 
+      # Define the default filesystem
+      MySiteBackend:
+        class: 'League\Flysystem\Filesystem'
+        constructor:
+          Adapter: '%$MySiteAdapter'
+        calls:
+          PublicURLPlugin: [ addPlugin, [ %$FlysystemUrlPlugin ] ]
+      # Requirements config
+      MySiteAssetHandler:
+        class: SilverStripe\Filesystem\Storage\FlysystemGeneratedAssetHandler
+        properties:
+          Filesystem: '%$MySiteBackend'
+      Requirements_Backend:
+        properties:
+          AssetHandler: '%$MySiteAssetHandler'
+
+In the above configuration, automatic expiry of generated files has been disabled, and it is necessary for
+the developer to maintain these files manually. This may be useful in environments where assets must
+be pre-cached, where scripts must be served alongside static files, or where no framework php request is
+guaranteed. Alternatively, files may be served from instances other than the one which generated the
+page response, and file synchronisation might not occur fast enough to propagate combined files to
+mirrored filesystems.
+
+In any case, care should be taken to determine the mechanism appropriate for your development
+and production environments.
+
+### Combined CSS Files
 
 You can also combine CSS files into a media-specific stylesheets as you would with the `Requirements::css` call - use
 the third paramter of the `combine_files` function:
@@ -130,6 +191,11 @@ the third paramter of the `combine_files` function:
 
 	Requirements::combine_files('print.css', $printStylesheets, 'print');
 
+<div class="alert" markdown='1'>
+When combining CSS files, take care of relative urls, as these will not be re-written to match
+the destination location of the resulting combined CSS.
+</div>
+
 ## Clearing assets
 
 	:::php

docs/en/04_Changelogs/4.0.0.md

@@ -38,6 +38,7 @@
 
 ### ErrorPage
 
+ * `ErrorPage.static_filepath` config has been removed.
  * `ErrorPage::get_filepath_for_errorcode` has been removed
  * `ErrorPage::alternateFilepathForErrorcode` extension point has been removed
 
@@ -123,7 +124,6 @@ New methods on `Requirements` are added to access these:
 And some methods on `Requirements` and `Requirements_Backend` have been removed as they are obsolete.
 
  * `delete_combined_files` (both classes)
- * `delete_all_combined_files` (both classes)
 
 A new config `Requirements_Backend.combine_in_dev` has been added in order to allow combined files to be
 forced on during development. If this is off, combined files is only enabled in live environments.
@@ -418,27 +418,13 @@ After:
 
 ### Update code that modifies the behaviour of ErrorPage
 
-Since ErrorPage writes statically cached files for each dataobject, in order to integrate it with both the
+ErrorPage has been updated to use a configurable asset backend, similar to the `AssetStore` described above.
-new asset backend, and ensure that .htaccess static references still works, these files are now cached
+This replaces the `ErrorPage.static_filepath` config that was used to write local files.
-potentially in two places:
 
- * When an error is generated within the live environment, by default the error handler will query the 
+As a result, error pages may be cached either to a local filesystem, or an external Flysystem store
-`GeneratedAssetHandler` for cached content, which is then served up in the same PHP request. This is the
+(which is configured via setting a new Flysystem backend with YAML).
-primary cache for error content, which does not involve database access, but is processed within the
-PHP process. Although, the file path for this cache is not predictable, as it uses the configured asset backend,
-and is not necessarily present on the same filesystem as the site code.
 
- * In order to ensure that the webserver has direct access an available cached error page, it can be necessary
+`ErrorPage::get_filepath_for_errorcode()` has been removed, because the local path for a specific code is
-to ensure a physical file is present locally. By setting the `ErrorPage.enable_static_file` config,
-the generation of this file can be controlled. When this disabled, the static file will only be cached
-via the configured backend. When this is enabled (as it is by default) then the error page will be generated
-in the same location as it were in framework version 3.x. E.g. `/assets/error-404.html`
-
-If your webserver relies on static paths encoded in `.htaccess` to these files, then it's preferable to leave
-this option on. If using a non-local filesystem, and another mechanism for intercepting webserver errors,
-then it may be preferable to leave this off, meaning that the local assets folder is unnecessary.
-
-`ErrorPage::get_filepath_for_errorcode` has been removed, because the local path for a specific code is
 no longer assumed. Instead you should use `ErrorPage::get_content_for_errorcode` which retrieves the
 appropriate content for that error using one of the methods above.
 

filesystem/flysystem/AssetAdapter.php

@@ -32,9 +32,22 @@ class AssetAdapter extends Local {
 	);
 
 	public function __construct($root = null, $writeFlags = LOCK_EX, $linkHandling = self::DISALLOW_LINKS) {
+		// Get root path
+		if (!$root) {
+			// Empty root will set the path to assets
+			$root = ASSETS_PATH;
+		} elseif(strpos($root, './') === 0) {
+			// Substitute leading ./ with BASE_PATH
+			$root = BASE_PATH . substr($root, 1);
+		} elseif(strpos($root, '../') === 0) {
+			// Substitute leading ./ with parent of BASE_PATH, in case storage is outside of the webroot.
+			$root = dirname(BASE_PATH) . substr($root, 2);
+		}
+
 		// Override permissions with config
 		$permissions = \Config::inst()->get(get_class($this), 'file_permissions');
-		parent::__construct($root ?: ASSETS_PATH, $writeFlags, $linkHandling, $permissions);
+
+		parent::__construct($root, $writeFlags, $linkHandling, $permissions);
 	}
 
 	/**

filesystem/flysystem/FlysystemGeneratedAssetHandler.php

@@ -0,0 +1,106 @@
+<?php
+
+namespace SilverStripe\Filesystem\Storage;
+
+use Config;
+use Exception;
+use League\Flysystem\Filesystem;
+
+/**
+ * Simple Flysystem implementation of GeneratedAssetHandler for storing generated content
+ * 
+ * @package framework
+ * @subpackage filesystem
+ */
+class FlysystemGeneratedAssetHandler implements GeneratedAssetHandler {
+
+	/**
+	 * Flysystem store for files
+	 *
+	 * @var Filesystem
+	 */
+	protected $assetStore = null;
+
+	/**
+	 * Assign the asset backend
+	 *
+	 * @param Filesystem $store
+	 * @return $this
+	 */
+	public function setFilesystem(Filesystem $store) {
+		$this->assetStore = $store;
+		return $this;
+	}
+
+	/**
+	 * Get the asset backend
+	 *
+	 * @return Filesystem
+	 */
+	public function getFilesystem() {
+		return $this->assetStore;
+	}
+
+    public function getContentURL($filename, $callback = null) {
+		$result = $this->checkOrCreate($filename, $callback);
+		if($result) {
+			return $this
+				->getFilesystem()
+				->getPublicUrl($filename);
+		}
+	}
+
+	public function getContent($filename, $callback = null) {
+		$result = $this->checkOrCreate($filename, $callback);
+		if($result) {
+			return $this
+				->getFilesystem()
+				->read($filename);
+		}
+	}
+
+	/**
+	 * Check if the file exists or that the $callback provided was able to regenerate it.
+	 *
+	 * @param string $filename
+	 * @param callable $callback
+	 * @return bool Whether or not the file exists
+	 * @throws Exception If an error has occurred during save
+	 */
+	protected function checkOrCreate($filename, $callback = null) {
+		// Check if there is an existing asset
+		if ($this->getFilesystem()->has($filename)) {
+			return true;
+		}
+
+		if (!$callback) {
+			return false;
+		}
+
+		// Invoke regeneration and save
+		$content = call_user_func($callback);
+		$this->setContent($filename, $content);
+		return true;
+	}
+
+	public function setContent($filename, $content) {
+		// Store content
+		$result = $this
+				->getFilesystem()
+				->put($filename, $content);
+
+		if(!$result) {
+			throw new Exception("Error regenerating file \"{$filename}\"");
+		}
+	}
+
+	public function removeContent($filename) {
+		if($this->getFilesystem()->has($filename)) {
+			$handler = $this->getFilesystem()->get($filename);
+			$handler->delete();
+		}
+
+	}
+
+
+}

filesystem/storage/CacheGeneratedAssetHandler.php

@@ -1,177 +0,0 @@
-<?php
-
-namespace SilverStripe\Filesystem\Storage;
-
-use Config;
-use Exception;
-use Flushable;
-use SS_Cache;
-use Zend_Cache_Core;
-
-/**
- * Handle references to generated files via cached tuples
- *
- * Important: If you are using the default FlysystemStore with legacy_filenames, you will need to ?flush
- * in order to refresh combined files.
- * 
- * @package framework
- * @subpackage filesystem
- */
-class CacheGeneratedAssetHandler implements GeneratedAssetHandler, Flushable {
-
-	/**
-	 * Lifetime of cache
-	 *
-	 * @config
-	 * @var int
-	 */
-	private static $lifetime = null;
-
-	/**
-	 * Backend for generated files
-	 *
-	 * @var AssetStore
-	 */
-	protected $assetStore = null;
-
-	/**
-	 * Assign the asset backend
-	 *
-	 * @param AssetStore $store
-	 * @return $this
-	 */
-	public function setAssetStore(AssetStore $store) {
-		$this->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');
-		$lifetime = Config::inst()->get(__CLASS__, 'lifetime') ?: null; // map falsey to null (indefinite)
-		$cache->setLifetime($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);
-			$valid = $this->validateResult($result, $filename);
-			if($valid) {
-				return $result;
-			}
-		}
-
-		// 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);
-		}
-
-		// Ensure this result is successfully saved
-		$valid = $this->validateResult($result, $filename);
-		if($valid) {
-			return $result;
-		}
-
-		throw new Exception("Error regenerating file \"{$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 bool True if this $result is valid
-	 */
-	protected function validateResult($result, $filename) {
-		if(!$result) {
-			return false;
-		}
-		
-		// Retrieve URL from tuple
-		$store = $this->getAssetStore();
-		return $store->exists($result['Filename'], $result['Hash'], $result['Variant']);
-	}
-
-}

filesystem/storage/GeneratedAssetHandler.php

@@ -11,37 +11,45 @@ namespace SilverStripe\Filesystem\Storage;
 interface GeneratedAssetHandler {
 
 	/**
-	 * Given a filename and entropy, determine if a pre-generated file is valid. If this file is invalid
+     * Returns a URL to a generated asset, if one is available.
-	 * or expired, invoke $callback to regenerate the content.
+     * 
-	 *
+	 * Given a filename, determine if a file is available. If the file is unavailable,
-	 * Returns a URL to the generated file
+     * and a callback is supplied, invoke it to regenerate the content.
 	 *
 	 * @param string $filename
-	 * @param mixed $entropy
 	 * @param callable $callback To generate content. If none provided, url will only be returned
 	 * if there is valid content.
 	 * @return string URL to generated file
 	 */
-	public function getGeneratedURL($filename, $entropy = 0, $callback = null);
+	public function getContentURL($filename, $callback = null);
 
 	/**
-	 * Given a filename and entropy, determine if a pre-generated file is valid. If this file is invalid
+	 * Returns the content for a generated asset, if one is available.
-	 * or expired, invoke $callback to regenerate the content.
+     * 
+	 * Given a filename, determine if a file is available. If the file is unavailable,
+     * and a callback is supplied, invoke it to regenerate the content.
 	 *
 	 * @param string $filename
-	 * @param mixed $entropy
 	 * @param callable $callback To generate content. If none provided, content will only be returned
 	 * if there is valid content.
 	 * @return string Content for this generated file
 	 */
-	public function getGeneratedContent($filename, $entropy = 0, $callback = null);
+	public function getContent($filename, $callback = null);
 
 	/**
 	 * Update content with new value
 	 *
 	 * @param string $filename
-	 * @param mixed $entropy
 	 * @param string $content Content to write to the backend
 	 */
-	public function updateContent($filename, $entropy, $content);
+	public function setContent($filename, $content);
+    
+    /**
+     * Remove any content under the given file.
+     * 
+     * If $filename is a folder, it should delete all files underneath it also.
+     * 
+     * @param string $filename
+     */
+    public function removeContent($filename);
 }

tests/filesystem/AssetStoreTest.php

@@ -7,7 +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;
+use SilverStripe\Filesystem\Storage\FlysystemGeneratedAssetHandler;
 
 class AssetStoreTest extends SapphireTest {
 
@@ -424,6 +424,12 @@ class AssetStoreTest_SpyStore extends FlysystemAssetStore {
 		$backend->setFilesystem($filesystem);
 		Injector::inst()->registerService($backend, 'AssetStore');
 
+		// Assign flysystem backend to generated asset handler at the same time
+		$generated = new FlysystemGeneratedAssetHandler();
+		$generated->setFilesystem($filesystem);
+		Injector::inst()->registerService($generated, 'GeneratedAssetHandler');
+		Requirements::backend()->setAssetHandler($generated);
+
 		// Disable legacy and set defaults
 		Config::inst()->remove(get_class(new FlysystemAssetStore()), 'legacy_filenames');
 		Config::inst()->update('Director', 'alternate_base_url', '/');
@@ -451,9 +457,6 @@ 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();

tests/forms/RequirementsTest.php

@@ -1,6 +1,5 @@
 <?php
 
-use SilverStripe\Filesystem\Storage\CacheGeneratedAssetHandler;
 /**
  * @package framework
  * @subpackage tests
@@ -23,7 +22,7 @@ class RequirementsTest extends SapphireTest {
 	}
 
 	public function testExternalUrls() {
-		$backend = new Requirements_Backend;
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$backend->setCombinedFilesEnabled(true);
 
 		$backend->javascript('http://www.mydomain.com/test.js');
@@ -72,7 +71,7 @@ class RequirementsTest extends SapphireTest {
 		$backend->clearCombinedFiles();
 		$backend->setCombinedFilesFolder('_combinedfiles');
 		$backend->setMinifyCombinedJSFiles(false);
-		CacheGeneratedAssetHandler::flush();
+		Requirements::flush();
 	}
 
 	/**
@@ -119,10 +118,10 @@ class RequirementsTest extends SapphireTest {
 	}
 
 	public function testCombinedJavascript() {
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupCombinedRequirements($backend);
 
-		$combinedFileName = '/_combinedfiles/b2a28d2463/RequirementsTest_bc.js';
+		$combinedFileName = '/_combinedfiles/RequirementsTest_bc-51622b5.js';
 		$combinedFilePath = AssetStoreTest_SpyStore::base_path() . $combinedFileName;
 
 		$html = $backend->includeInHTML(false, self::$html_template);
@@ -167,7 +166,7 @@ class RequirementsTest extends SapphireTest {
 
 		// Then do it again, this time not requiring the files beforehand
 		unlink($combinedFilePath);
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupCombinedNonrequiredRequirements($backend);
 		$html = $backend->includeInHTML(false, self::$html_template);
 
@@ -205,7 +204,7 @@ class RequirementsTest extends SapphireTest {
 
 	public function testCombinedCss() {
 		$basePath = $this->getCurrentRelativePath();
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 
 		$backend->combineFiles(
@@ -220,7 +219,7 @@ class RequirementsTest extends SapphireTest {
 		$html = $backend->includeInHTML(false, self::$html_template);
 
 		$this->assertRegExp(
-			'/href=".*\/print\.css/',
+			'/href=".*\/print\-94e723d\.css/',
 			$html,
 			'Print stylesheets have been combined.'
 		);
@@ -231,7 +230,7 @@ class RequirementsTest extends SapphireTest {
 		);
 
 		// Test that combining a file multiple times doesn't trigger an error
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 		$backend->combineFiles(
 			'style.css',
@@ -250,7 +249,7 @@ class RequirementsTest extends SapphireTest {
 
 		$html = $backend->includeInHTML(false, self::$html_template);
 		$this->assertRegExp(
-			'/href=".*\/style\.css/',
+			'/href=".*\/style\-2b3e4c9\.css/',
 			$html,
 			'Stylesheets have been combined.'
 		);
@@ -258,9 +257,9 @@ class RequirementsTest extends SapphireTest {
 
 	public function testBlockedCombinedJavascript() {
 		$basePath = $this->getCurrentRelativePath();
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupCombinedRequirements($backend);
-		$combinedFileName = '/_combinedfiles/b2a28d2463/RequirementsTest_bc.js';
+		$combinedFileName = '/_combinedfiles/RequirementsTest_bc-51622b5.js';
 		$combinedFilePath = AssetStoreTest_SpyStore::base_path() . $combinedFileName;
 
 		/* BLOCKED COMBINED FILES ARE NOT INCLUDED */
@@ -279,7 +278,7 @@ class RequirementsTest extends SapphireTest {
 		/* BLOCKED UNCOMBINED FILES ARE NOT INCLUDED */
 		$this->setupCombinedRequirements($backend);
 		$backend->block($basePath .'/RequirementsTest_b.js');
-		$combinedFileName2 = '/_combinedfiles/37bd2d9dcb/RequirementsTest_bc.js'; // SHA1 without file c included
+		$combinedFileName2 = '/_combinedfiles/RequirementsTest_bc-fc7468e.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);
@@ -315,7 +314,7 @@ class RequirementsTest extends SapphireTest {
 	public function testArgsInUrls() {
 		$basePath = $this->getCurrentRelativePath();
 
-		$backend = new Requirements_Backend;
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 
 		$backend->javascript($basePath . '/RequirementsTest_a.js?test=1&test=2&test=3');
@@ -340,7 +339,7 @@ class RequirementsTest extends SapphireTest {
 	public function testRequirementsBackend() {
 		$basePath = $this->getCurrentRelativePath();
 
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 		$backend->javascript($basePath . '/a.js');
 
@@ -378,7 +377,7 @@ class RequirementsTest extends SapphireTest {
 		// to something else
 		$basePath = 'framework' . substr($basePath, strlen(FRAMEWORK_DIR));
 
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 		$holder = Requirements::backend();
 		Requirements::set_backend($backend);
@@ -407,7 +406,7 @@ class RequirementsTest extends SapphireTest {
 	}
 
 	public function testJsWriteToBody() {
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 		$backend->javascript('http://www.mydomain.com/test.js');
 
@@ -426,7 +425,7 @@ class RequirementsTest extends SapphireTest {
 
 	public function testIncludedJsIsNotCommentedOut() {
 		$template = '<html><head></head><body><!--<script>alert("commented out");</script>--></body></html>';
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 		$backend->javascript($this->getCurrentRelativePath() . '/RequirementsTest_a.js');
 		$html = $backend->includeInHTML(false, $template);
@@ -438,7 +437,7 @@ class RequirementsTest extends SapphireTest {
 	public function testCommentedOutScriptTagIsIgnored() {
 		$template = '<html><head></head><body><!--<script>alert("commented out");</script>-->'
 			. '<h1>more content</h1></body></html>';
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 		$backend->setSuffixRequirements(false);
 		$src = $this->getCurrentRelativePath() . '/RequirementsTest_a.js';
@@ -450,7 +449,7 @@ class RequirementsTest extends SapphireTest {
 	}
 
 	public function testForceJsToBottom() {
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 		$backend->javascript('http://www.mydomain.com/test.js');
 
@@ -506,7 +505,7 @@ class RequirementsTest extends SapphireTest {
 		$template = '<html><head></head><body><header>My header</header><p>Body</p></body></html>';
 		$basePath = $this->getCurrentRelativePath();
 
-		$backend = new Requirements_Backend();
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$this->setupRequirements($backend);
 
 		$backend->javascript($basePath .'/RequirementsTest_a.js');

tests/view/SSViewerTest.php

@@ -146,7 +146,7 @@ class SSViewerTest extends SapphireTest {
 	}
 
 	public function testRequirementsCombine(){
-		$testBackend = new Requirements_Backend();
+		$testBackend = Injector::inst()->create('Requirements_Backend');
 		$testBackend->setSuffixRequirements(false);
 		//$combinedTestFilePath = BASE_PATH . '/' . $testBackend->getCombinedFilesFolder() . '/testRequirementsCombine.js';
 
@@ -167,7 +167,7 @@ class SSViewerTest extends SapphireTest {
 		$testBackend->processCombinedFiles();
 		$js = $testBackend->getJavascript();
 		$combinedTestFilePath = BASE_PATH . reset($js);
-		$this->assertContains('testRequirementsCombine.js', $combinedTestFilePath);
+		$this->assertContains('_combinedfiles/testRequirementsCombine-7c20750.js', $combinedTestFilePath);
 
 		// and make sure the combined content matches the input content, i.e. no loss of functionality
 		if(!file_exists($combinedTestFilePath)) {
@@ -1348,7 +1348,7 @@ after')
 		$template = new SSViewer(array('SSViewerTestProcess'));
 		$basePath = dirname($this->getCurrentRelativePath()) . '/forms';
 
-		$backend = new Requirements_Backend;
+		$backend = Injector::inst()->create('Requirements_Backend');
 		$backend->setCombinedFilesEnabled(false);
 		$backend->combineFiles(
 			'RequirementsTest_ab.css',

view/Requirements.php

@@ -8,7 +8,28 @@ use SilverStripe\Filesystem\Storage\GeneratedAssetHandler;
  * @package framework
  * @subpackage view
  */
-class Requirements {
+class Requirements implements Flushable {
+
+	/**
+	 * Flag whether combined files should be deleted on flush.
+	 *
+	 * By default all combined files are deleted on flush. If combined files are stored in source control,
+	 * and thus updated manually, you might want to turn this on to disable this behaviour.
+	 *
+	 * @config
+	 * @var bool
+	 */
+	private static $disable_flush_combined = false;
+
+	/**
+	 * Triggered early in the request when a flush is requested
+	 */
+	public static function flush() {
+		$disabled = Config::inst()->get(static::class, 'disable_flush_combined');
+		if(!$disabled) {
+			self::delete_all_combined_files();
+		}
+	}
 
 	/**
 	 * Enable combining of css/javascript files.
@@ -66,6 +87,9 @@ class Requirements {
 	 */
 	private static $backend = null;
 
+	/**
+	 * @return Requirements_Backend
+	 */
 	public static function backend() {
 		if(!self::$backend) {
 			self::$backend = Injector::inst()->create('Requirements_Backend');
@@ -323,6 +347,14 @@ class Requirements {
 		return self::backend()->getCombinedFiles();
 	}
 
+	/**
+	 * Deletes all generated combined files in the configured combined files directory,
+	 * but doesn't delete the directory itself
+	 */
+	public static function delete_all_combined_files() {
+		return self::backend()->deleteAllCombinedFiles();
+	}
+
 	/**
 	 * Re-sets the combined files definition. See {@link Requirements_Backend::clear_combined_files()}
 	 */
@@ -427,7 +459,8 @@ class Requirements {
  * @package framework
  * @subpackage view
  */
-class Requirements_Backend {
+class Requirements_Backend
+{
 
 	/**
 	 * Whether to add caching query params to the requests for file-based requirements.
@@ -447,8 +480,10 @@ class Requirements_Backend {
 	protected $combinedFilesEnabled = true;
 
 	/**
-	 * Determine if files should be combined automatically on dev mode
+	 * Determine if files should be combined automatically on dev mode.
-	 * By default, files will be left uncombined when developing.
+	 *
+	 * By default combined files will not be combined except in test or
+	 * live environments. Turning this on will allow for pre-combining of files in development mode.
 	 *
 	 * @config
 	 * @var bool
@@ -559,20 +594,53 @@ class Requirements_Backend {
 	protected $forceJSToBottom = false;
 
 	/**
-	 * Configures the default prefix for comined files
+	 * Configures the default prefix for combined files.
+	 *
+	 * This defaults to `_combinedfiles`, and is the folder within the configured asset backend that
+	 * combined files will be stored in. If using a backend shared with other systems, it is usually
+	 * necessary to distinguish combined files from other assets.
 	 *
 	 * @config
 	 * @var string
 	 */
 	private static $default_combined_files_folder = '_combinedfiles';
 
+	/**
+	 * Flag to include the hash in the querystring instead of the filename for combined files.
+	 *
+	 * By default the `<hash>` of the source files is appended to the end of the combined file
+	 * (prior to the file extension). If combined files are versioned in source control or running
+	 * in a distributed environment (such as one where the newest version of a file may not always be
+	 * immediately available) then it may sometimes be necessary to disable this. When this is set to true,
+	 * the hash will instead be appended via a querystring parameter to enable cache busting, but not in
+	 * the filename itself. I.e. `assets/_combinedfiles/name.js?m=<hash>`
+	 *
+	 * @config
+	 * @var bool
+	 */
+	private static $combine_hash_querystring = false;
+
+	/**
+	 * @var GeneratedAssetHandler
+	 */
+	protected $assetHandler = null;
+
 	/**
 	 * Gets the backend storage for generated files
 	 *
 	 * @return GeneratedAssetHandler
 	 */
-	protected function getAssetHandler() {
+	public function getAssetHandler() {
-		return Injector::inst()->get('GeneratedAssetHandler');
+		return $this->assetHandler;
+	}
+
+	/**
+	 * Set a new asset handler for this backend
+	 *
+	 * @param GeneratedAssetHandler $handler
+	 */
+	public function setAssetHandler(GeneratedAssetHandler $handler) {
+		$this->assetHandler = $handler;
 	}
 
 	/**
@@ -1328,6 +1396,16 @@ class Requirements_Backend {
 		return $this->combinedFiles;
 	}
 
+	/**
+	 * Clears all combined files
+	 */
+	public function deleteAllCombinedFiles() {
+		$combinedFolder = $this->getCombinedFilesFolder();
+		if($combinedFolder) {
+			$this->getAssetHandler()->removeContent($combinedFolder);
+		}
+	}
+
 	/**
 	 * Clear all registered CSS and JavaScript file combinations
 	 */
@@ -1404,25 +1482,28 @@ class Requirements_Backend {
 	 * @return string URL to this resource
 	 */
 	protected function getCombinedFileURL($combinedFile, $fileList, $type) {
-		// Generate path (Filename)
+		// Filter blocked files
-		$combinedFileID = File::join_paths($this->getCombinedFilesFolder(), $combinedFile);
+		$fileList = array_diff($fileList, $this->getBlocked());
 
-		// Get entropy for this combined file (last modified date of most recent file)
+		// Generate path (Filename)
-		$entropy = $this->getEntropyOfFiles($fileList);
+		$hashQuerystring = Config::inst()->get(static::class, 'combine_hash_querystring');
+		if(!$hashQuerystring) {
+			$combinedFile = $this->hashedCombinedFilename($combinedFile, $fileList);
+		}
+		$combinedFileID =  File::join_paths($this->getCombinedFilesFolder(), $combinedFile);
 
 		// Send file combination request to the backend, with an optional callback to perform regeneration
 		$minify = $this->getMinifyCombinedJSFiles();
 		$combinedURL = $this
 			->getAssetHandler()
-			->getGeneratedURL(
+			->getContentURL(
 				$combinedFileID,
-				$entropy,
 				function() use ($fileList, $minify, $type) {
 					// Physically combine all file content
 					$combinedData = '';
 					$base = Director::baseFolder() . '/';
 					$minifier = Injector::inst()->get('Requirements_Minifier');
-					foreach(array_diff($fileList, $this->getBlocked()) as $file) {
+					foreach($fileList as $file) {
 						$fileContent = file_get_contents($base . $file);
 						// Use configured minifier
 						if($minify) {
@@ -1439,15 +1520,31 @@ class Requirements_Backend {
 				}
 			);
 
+		// If the name isn't hashed, we will need to append the querystring m= parameter instead
 		// Since url won't be automatically suffixed, add it in here
-		if($this->getSuffixRequirements()) {
+		if($hashQuerystring && $this->getSuffixRequirements()) {
+			$hash = $this->hashOfFiles($fileList);
 			$q = stripos($combinedURL, '?') === false ? '?' : '&';
-			$combinedURL .= "{$q}m={$entropy}";
+			$combinedURL .= "{$q}m={$hash}";
 		}
 
 		return $combinedURL;
 	}
 
+	/**
+	 * Given a filename and list of files, generate a new filename unique to these files
+	 *
+	 * @param string $name
+	 * @param array $files
+	 * @return string
+	 */
+	protected function hashedCombinedFilename($combinedFile, $fileList) {
+		$name = pathinfo($combinedFile, PATHINFO_FILENAME);
+		$hash = $this->hashOfFiles($fileList);
+		$extension = File::get_file_extension($combinedFile);
+		return $name . '-' . substr($hash, 0, 7) . '.' . $extension;
+	}
+
 	/**
 	 * Check if combined files are enabled
 	 *
@@ -1482,20 +1579,20 @@ class Requirements_Backend {
 	 * any of these files have changed.
 	 *
 	 * @param array $fileList List of files
-	 * @return int Last modified timestamp of these files
+	 * @return string SHA1 bashed file hash
 	 */
-	protected function getEntropyOfFiles($fileList) {
+	protected function hashOfFiles($fileList) {
-		// file exists, check modification date of every contained file
+		// Get hash based on hash of each file
 		$base = Director::baseFolder() . '/';
-		$srcLastMod = 0;
+		$hash = '';
 		foreach($fileList as $file) {
 			if(file_exists($base . $file)) {
-				$srcLastMod = max(filemtime($base . $file), $srcLastMod);
+				$hash .= sha1_file($base . $file);
 			} else {
 				throw new InvalidArgumentException("Combined file {$file} does not exist");
 			}
 		}
-		return $srcLastMod;
+		return sha1($hash);
 	}
 
 	/**