API Restore JS Minification

BUG Fix incorrect cache on CacheGeneratedAssetHandler

_config/asset.yml

@@ -29,3 +29,5 @@ Injector:
     class: SilverStripe\Filesystem\Storage\CacheGeneratedAssetHandler
     properties:
       AssetStore: '%$AssetStore'
+  Requirements_Minifier:
+    class: SilverStripe\View\JSMinifier

docs/en/04_Changelogs/4.0.0.md

@@ -20,8 +20,6 @@
    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
 
@@ -31,9 +29,20 @@
    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).
+ * `Requirements_Minifier` API can be used to declare any new mechanism for minifying combined required files.
+   By default this api is provided by the `JSMinifier` class, but user code can substitute their own.
 
 ## Deprecated classes/methods
 
+### ErrorPage
+
+ * `ErrorPage::get_filepath_for_errorcode` has been removed
+ * `ErrorPage::alternateFilepathForErrorcode` extension point has been removed
+
+See notes below on upgrading extensions to the ErrorPage class
+
+### Assets and Filesystem
+
 The following image manipulations previously deprecated has been removed:
 
  * `Image::SetRatioSize` superceded by `Fit`
@@ -79,6 +88,44 @@ The following filesystem synchronisation methods are also removed
 
 The Spyc YAML library has been removed from /thirdparty. Please load it yourself, or use the Symfony YAML component that’s automatically installed by composer.
 
+### Requirements
+
+The following methods and properties on `Requirements_Backend` have been renamed:
+
+ * `Requirements_Backund::$combine_files` made protected and renamed `$combinedFiles`
+ * `Requirements_Backend::$combine_js_with_min` made protected and renamed `$minifyCombinedFiles`
+ * `Requirements_Backend::$write_header_comments` made protected and renamed `$writeHeaderComment`
+ * `Requirements_Backend::$write_js_to_body` made protected and renamed to `$writeJavascriptToBody`
+ * `Requirements_Backend::$force_js_to_bottom` renamed to `$forceJSToBottom`
+ * `get_combined_files_enabled` renamed to `getCombinedFilesEnabled`
+ * `set_combined_files_enabled` renamed to `setCombinedFilesEnabled`
+ * `get_suffix_requirements` renamed to `getSuffixRequirements`
+ * `set_suffix_requirements` renamed to `setSuffixRequirements`
+ * `get_custom_scripts` renamed to `getCustomScripts`
+ * `unblock_all` renamed to `unblockAll`
+ * `include_in_response` renamed to `includeInResponse`
+ * `combine_files` renamed to `combineFiles`
+ * `get_combine_files` renamed to `getCombinedFiles`
+ * `clear_combined_files` renamed to `clearCombinedFiles`
+ * `process_combined_files` renamed to `processCombinedFiles`
+ * `set_write_js_to_body` renamed to `setWriteJavascriptToBody`
+ * `set_force_js_to_bottom` renamed to `setForceJSToBottom`
+
+New methods on `Requirements` are added to access these:
+
+ * `get_minify_combined_js_files`
+ * `set_minify_combined_js_files`
+ * `get_force_js_to_bottom`
+ * `get_write_js_to_body`
+
+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.
+
 ## Upgrading
 
 ### New asset storage mechanism
@@ -359,3 +406,60 @@ 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
+new asset backend, and ensure that .htaccess static references still works, these files are now cached
+potentially in two places:
+
+ * When an error is generated within the live environment, by default the error handler will query the 
+`GeneratedAssetHandler` for cached content, which is then served up in the same PHP request. This is the
+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
+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.
+
+In order to retrieve the actual filename (which is used to identify an error page regardless of base
+path), you can use `ErrorPage::get_error_filename()` instead. Unlike the old `get_filepath_for_errorcode`
+method, there is no $locale parameter.
+
+In case that user code must customise this filename, such as for extensions which provide a locale value
+for any error page, the extension point `updateErrorFilename` can be used. This extension point should
+also be used to replace any `alternateFilepathForErrorcode` used.
+
+E.g.
+
+
+	:::php
+	class MyErrorPageExtension extends SiteTreeExtension {
+		public function updateErrorFilename(&$name, &$statuscode) {
+			if($this->owner->exists()) {
+				$locale = $this->Locale;
+			} else {
+				$locale = Translatable::get_current_locale();
+			}
+			$name = "error-{$statusCode}-{$locale}.html";
+		}
+	}
+
+
+	:::yaml
+	ErrorPage:
+	  extensions:
+	    - MyErrorPageExtension
+

filesystem/storage/CacheGeneratedAssetHandler.php

@@ -25,7 +25,7 @@ class CacheGeneratedAssetHandler implements GeneratedAssetHandler, Flushable {
 	 * @config
 	 * @var int
 	 */
-	private static $lifetime = 0;
+	private static $lifetime = null;
 
 	/**
 	 * Backend for generated files
@@ -59,7 +59,8 @@ class CacheGeneratedAssetHandler implements GeneratedAssetHandler, Flushable {
 	 */
 	protected static function get_cache() {
 		$cache = SS_Cache::factory('CacheGeneratedAssetHandler');
-		$cache->setLifetime(Config::inst()->get(__CLASS__, 'lifetime'));
+		$lifetime = Config::inst()->get(__CLASS__, 'lifetime') ?: null; // map falsey to null (indefinite)
+		$cache->setLifetime($lifetime);
 		return $cache;
 	}
 
@@ -140,6 +141,7 @@ class CacheGeneratedAssetHandler implements GeneratedAssetHandler, Flushable {
 		throw new Exception("Error regenerating file \"{$filename}\"");
 	}
 
+
 	/**
 	 * Get cache key for the given generated asset
 	 *

tests/forms/RequirementsTest.php

@@ -71,6 +71,7 @@ class RequirementsTest extends SapphireTest {
 		$backend->clear();
 		$backend->clearCombinedFiles();
 		$backend->setCombinedFilesFolder('_combinedfiles');
+		$backend->setMinifyCombinedJSFiles(false);
 		CacheGeneratedAssetHandler::flush();
 	}
 

tests/forms/uploadfield/UploadFieldTest.php

@@ -1,7 +1,5 @@
 <?php
 
-use Filesystem as SS_Filesystem;
-
 /**
  * @package framework
  * @subpackage tests
@@ -25,14 +23,14 @@ class UploadFieldTest extends FunctionalTest {
 		// Create a test folders for each of the fixture references
 		foreach(Folder::get() as $folder) {
 			$path = AssetStoreTest_SpyStore::getLocalPath($folder);
-			SS_Filesystem::makeFolder($path);
+			Filesystem::makeFolder($path);
 		}
 
 		// Create a test files for each of the fixture references
 		$files = File::get()->exclude('ClassName', 'Folder');
 		foreach($files as $file) {
 			$path = AssetStoreTest_SpyStore::getLocalPath($file);
-			SS_Filesystem::makeFolder(dirname($path));
+			Filesystem::makeFolder(dirname($path));
 			$fh = fopen($path, "w+");
 			fwrite($fh, str_repeat('x', 1000000));
 			fclose($fh);
@@ -845,7 +843,7 @@ class UploadFieldTest extends FunctionalTest {
 		// Check that uploaded files can be detected
 		$response = $this->mockFileUpload('NoRelationField', $tmpFileName);
 		$this->assertFalse($response->isError());
-		$this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/UploadFieldTest/315ae4c3d4/$tmpFileName");
+		$this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/UploadedFiles/315ae4c3d4/$tmpFileName");
 		$responseExists = $this->mockFileExists('NoRelationField', $tmpFileName);
 		$responseExistsData = json_decode($responseExists->getBody());
 		$this->assertFalse($responseExists->isError());
@@ -857,7 +855,7 @@ class UploadFieldTest extends FunctionalTest {
 		$tmpFileNameExpected = 'test-Upload-Bad.txt';
 		$response = $this->mockFileUpload('NoRelationField', $tmpFileName);
 		$this->assertFalse($response->isError());
-		$this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/UploadFieldTest/315ae4c3d4/$tmpFileNameExpected");
+		$this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/UploadedFiles/315ae4c3d4/$tmpFileNameExpected");
 		// With original file
 		$responseExists = $this->mockFileExists('NoRelationField', $tmpFileName);
 		$responseExistsData = json_decode($responseExists->getBody());
@@ -1086,49 +1084,49 @@ class UploadFieldTestForm extends Form implements TestOnly {
 			->setFolderName('/');
 
 		$fieldNoRelation = UploadField::create('NoRelationField')
-			->setFolderName('UploadFieldTest');
+			->setFolderName('UploadedFiles');
 
 		$fieldHasOne = UploadField::create('HasOneFile')
-			->setFolderName('UploadFieldTest');
+			->setFolderName('UploadedFiles');
 
 		$fieldHasOneExtendedFile = UploadField::create('HasOneExtendedFile')
-			->setFolderName('UploadFieldTest');
+			->setFolderName('UploadedFiles');
 
 		$fieldHasOneMaxOne = UploadField::create('HasOneFileMaxOne')
-			->setFolderName('UploadFieldTest')
+			->setFolderName('UploadedFiles')
 			->setAllowedMaxFileNumber(1);
 
 		$fieldHasOneMaxTwo = UploadField::create('HasOneFileMaxTwo')
-			->setFolderName('UploadFieldTest')
+			->setFolderName('UploadedFiles')
 			->setAllowedMaxFileNumber(2);
 
 		$fieldHasMany = UploadField::create('HasManyFiles')
-			->setFolderName('UploadFieldTest');
+			->setFolderName('UploadedFiles');
 
 		$fieldHasManyMaxTwo = UploadField::create('HasManyFilesMaxTwo')
-			->setFolderName('UploadFieldTest')
+			->setFolderName('UploadedFiles')
 			->setAllowedMaxFileNumber(2);
 
 		$fieldManyMany = UploadField::create('ManyManyFiles')
-			->setFolderName('UploadFieldTest');
+			->setFolderName('UploadedFiles');
 
 		$fieldHasManyNoView = UploadField::create('HasManyNoViewFiles')
-			->setFolderName('UploadFieldTest');
+			->setFolderName('UploadedFiles');
 
 		$fieldHasManyDisplayFolder = UploadField::create('HasManyDisplayFolder')
-			->setFolderName('UploadFieldTest')
+			->setFolderName('UploadedFiles')
 			->setDisplayFolderName('UploadFieldTest');
 
 		$fieldReadonly = UploadField::create('ReadonlyField')
-			->setFolderName('UploadFieldTest')
+			->setFolderName('UploadedFiles')
 			->performReadonlyTransformation();
 
 		$fieldDisabled = UploadField::create('DisabledField')
-			->setFolderName('UploadFieldTest')
+			->setFolderName('UploadedFiles')
 			->performDisabledTransformation();
 
 		$fieldSubfolder = UploadField::create('SubfolderField')
-			->setFolderName('UploadFieldTest/subfolder1');
+			->setFolderName('UploadedFiles/subfolder1');
 
 		$fieldCanUploadFalse = UploadField::create('CanUploadFalseField')
 			->setCanUpload(false);

tests/forms/uploadfield/UploadFieldTest.yml

@@ -7,7 +7,7 @@ Folder:
 File:
   file1:
     Title: File1
-    FileFilename: assets/UploadFieldTest/file1.txt
+    FileFilename: UploadFieldTest/file1.txt
     FileHash: 55b443b60176235ef09801153cca4e6da7494a0c
     Name: file1.txt
     Parent: =>Folder.folder1

tests/view/SSViewerTest.php

@@ -145,6 +145,40 @@ class SSViewerTest extends SapphireTest {
 		$this->assertFalse((bool)trim($template), "Should be no content in this return.");
 	}
 
+	public function testRequirementsCombine(){
+		$testBackend = new Requirements_Backend();
+		$testBackend->setSuffixRequirements(false);
+		//$combinedTestFilePath = BASE_PATH . '/' . $testBackend->getCombinedFilesFolder() . '/testRequirementsCombine.js';
+
+		$jsFile = FRAMEWORK_DIR . '/tests/view/themes/javascript/bad.js';
+		$jsFileContents = file_get_contents(BASE_PATH . '/' . $jsFile);
+		$testBackend->combineFiles('testRequirementsCombine.js', array($jsFile));
+		
+		// first make sure that our test js file causes an exception to be thrown
+		try{
+			require_once('thirdparty/jsmin/jsmin.php');
+			$content = JSMin::minify($content);
+			$this->fail('JSMin did not throw exception on minify bad file: ');
+		} catch(Exception $e) {
+			// exception thrown... good
+		}
+
+		// secondly, make sure that requirements is generated, even though minification failed
+		$testBackend->processCombinedFiles();
+		$js = $testBackend->getJavascript();
+		$combinedTestFilePath = BASE_PATH . reset($js);
+		$this->assertContains('testRequirementsCombine.js', $combinedTestFilePath);
+
+		// and make sure the combined content matches the input content, i.e. no loss of functionality
+		if(!file_exists($combinedTestFilePath)) {
+			$this->fail('No combined file was created at expected path: '.$combinedTestFilePath);
+		}
+		$combinedTestFileContents = file_get_contents($combinedTestFilePath);
+		$this->assertContains($jsFileContents, $combinedTestFileContents);
+	}
+	
+
+
 	public function testComments() {
 		$output = $this->render(<<<SS
 This is my template<%-- this is a comment --%>This is some content<%-- this is another comment --%>Final content

view/JSMinifier.php

@@ -0,0 +1,32 @@
+<?php
+
+namespace SilverStripe\View;
+
+use JSMin;
+use Requirements_Minifier;
+
+/**
+ * @package framework
+ * @subpackage view
+ */
+class JSMinifier implements Requirements_Minifier {
+	
+	public function minify($content, $type, $filename) {
+		// Non-js files aren't minified
+		if($type !== 'js') {
+			return $content . "\n";
+		}
+
+		// Combine JS
+		try {
+			require_once('thirdparty/jsmin/jsmin.php');
+			increase_time_limit_to();
+			$content = JSMin::minify($content);
+		} catch(Exception $e) {
+			$message = $e->getMessage();
+			user_error("Failed to minify {$filename}, exception: {$message}", E_USER_WARNING);
+		} finally {
+			return $content . ";\n";
+		}
+	}
+}

view/Requirements.php

@@ -337,6 +337,16 @@ class Requirements {
 		return self::backend()->processCombinedFiles();
 	}
 
+	/**
+	 * Set whether you want to write the JS to the body of the page rather than at the end of the
+	 * head tag.
+	 *
+	 * @return bool
+	 */
+	public static function get_write_js_to_body() {
+		return self::backend()->getWriteJavascriptToBody();
+	}
+
 	/**
 	 * Set whether you want to write the JS to the body of the page rather than at the end of the
 	 * head tag.
@@ -347,16 +357,44 @@ class Requirements {
 		self::backend()->setWriteJavascriptToBody($var);
 	}
 
+	/**
+	 * Get whether to force the JavaScript to end of the body. Useful if you use inline script tags
+	 * that don't rely on scripts included via {@link Requirements::javascript()).
+	 *
+	 * @return bool
+	 */
+	public static function get_force_js_to_bottom() {
+		return self::backend()->getForceJSToBottom();
+	}
+
 	/**
 	 * Set whether to force the JavaScript to end of the body. Useful if you use inline script tags
 	 * that don't rely on scripts included via {@link Requirements::javascript()).
 	 *
-	 * @param boolean $var If true, force the JavaScript to be included at the bottom of the page
+	 * @param bool $var If true, force the JavaScript to be included at the bottom of the page
 	 */
 	public static function set_force_js_to_bottom($var) {
 		self::backend()->setForceJSToBottom($var);
 	}
 
+	/**
+	 * Check if JS minification is enabled
+	 *
+	 * @return bool
+	 */
+	public static function get_minify_combined_js_files() {
+		return self::backend()->getMinifyCombinedJSFiles();
+	}
+
+	/**
+	 * Enable or disable js minification
+	 *
+	 * @param bool $minify
+	 */
+	public static function set_minify_combined_js_files($minify) {
+		self::backend()->setMinifyCombinedJSFiles($minify);
+	}
+
 	/**
 	 * Check if header comments are written
 	 *
@@ -481,12 +519,19 @@ class Requirements_Backend {
 	 */
 	protected $combinedFiles = array();
 
+	/**
+	 * Use the JSMin library to minify any javascript file passed to {@link combine_files()}.
+	 *
+	 * @var bool
+	 */
+	protected $minifyCombinedJSFiles = true;
+
 	/**
 	 * Whether or not file headers should be written when combining files
 	 *
 	 * @var boolean
 	 */
-	public $writeHeaderComment = true;
+	protected $writeHeaderComment = true;
 
 	/**
 	 * Where to save combined files. By default they're placed in assets/_combinedfiles, however
@@ -539,15 +584,6 @@ class Requirements_Backend {
 		$this->combinedFilesEnabled = (bool) $enable;
 	}
 
-	/**
-	 * Check whether file combination is enabled.
-	 *
-	 * @return bool
-	 */
-	public function getCombinedFilesEnabled() {
-		return $this->combinedFilesEnabled;
-	}
-
 	/**
 	 * Check if header comments are written
 	 *
@@ -656,6 +692,26 @@ class Requirements_Backend {
 		return $this->forceJSToBottom;
 	}
 
+	/**
+	 * Check if minify js files should be combined
+	 *
+	 * @return bool
+	 */
+	public function getMinifyCombinedJSFiles() {
+		return $this->minifyCombinedJSFiles;
+	}
+
+	/**
+	 * Set if combined js files should be minified
+	 *
+	 * @param bool $minify
+	 * @return $this
+	 */
+	public function setMinifyCombinedJSFiles($minify) {
+		$this->minifyCombinedJSFiles = $minify;
+		return $this;
+	}
+
 	/**
 	 * Register the given JavaScript file as required.
 	 *
@@ -1284,7 +1340,7 @@ class Requirements_Backend {
 	 */
 	public function processCombinedFiles() {
 		// Check if combining is enabled
-		if(!$this->enabledCombinedFiles()) {
+		if(!$this->getCombinedFilesEnabled()) {
 			return;
 		}
 
@@ -1297,7 +1353,7 @@ class Requirements_Backend {
 			// Generate this file, unless blocked
 			$combinedURL = null;
 			if(!isset($this->blocked[$combinedFile])) {
-				$combinedURL = $this->getCombinedFileURL($combinedFile, $fileList);
+				$combinedURL = $this->getCombinedFileURL($combinedFile, $fileList, $type);
 			}
 
 			// Replace all existing files, injecting the combined file at the position of the first item
@@ -1347,9 +1403,10 @@ class Requirements_Backend {
 	 *
 	 * @param string $combinedFile Filename for this combined file
 	 * @param array $fileList List of files to combine
+	 * @param string $type Either 'js' or 'css'
 	 * @return string URL to this resource
 	 */
-	protected function getCombinedFileURL($combinedFile, $fileList) {
+	protected function getCombinedFileURL($combinedFile, $fileList, $type) {
 		// Generate path (Filename)
 		$combinedFileID = File::join_paths($this->getCombinedFilesFolder(), $combinedFile);
 
@@ -1357,16 +1414,24 @@ class Requirements_Backend {
 		$entropy = $this->getEntropyOfFiles($fileList);
 
 		// Send file combination request to the backend, with an optional callback to perform regeneration
+		$minify = $this->getMinifyCombinedJSFiles();
 		$combinedURL = $this
 			->getAssetHandler()
 			->getGeneratedURL(
 				$combinedFileID,
 				$entropy,
-				function() use ($fileList) {
+				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) {
 						$fileContent = file_get_contents($base . $file);
+						// Use configured minifier
+						if($minify) {
+							$fileContent = $minifier->minify($fileContent, $type, $file);
+						}
+					
 						if ($this->writeHeaderComment) {
 							// Write a header comment for each file for easier identification and debugging.
 							$combinedData .= "/****** FILE: $file *****/\n";
@@ -1391,7 +1456,7 @@ class Requirements_Backend {
 	 *
 	 * @return bool
 	 */
-	protected function enabledCombinedFiles() {
+	public function getCombinedFilesEnabled() {
 		if(!$this->combinedFilesEnabled) {
 			return false;
 		}
@@ -1481,3 +1546,19 @@ class Requirements_Backend {
 	}
 
 }
+
+/**
+ * Provides an abstract interface for minifying content
+ */
+interface Requirements_Minifier {
+
+	/**
+	 * Minify the given content
+	 *
+	 * @param string $content
+	 * @param string $type Either js or css
+	 * @param string $filename Name of file to display in case of error
+	 * @return string minified content
+	 */
+	public function minify($content, $type, $filename);
+}