Merge pull request #3432 from IgorNadj/3.1-fix-minify

FIX making minify javascript fail-safe

tests/view/SSViewerTest.php

@@ -104,6 +104,54 @@ class SSViewerTest extends SapphireTest {
 		$this->assertFalse((bool)trim($template), "Should be no content in this return.");
 	}
 	
+	public function testRequirementsCombine(){
+		$oldBackend = Requirements::backend();
+		$testBackend = new Requirements_Backend();
+		Requirements::set_backend($testBackend);
+		$combinedTestFilePath = BASE_PATH . '/' . $testBackend->getCombinedFilesFolder() . '/testRequirementsCombine.js';
+
+		$jsFile = FRAMEWORK_DIR . '/tests/view/themes/javascript/bad.js';
+		$jsFileContents = file_get_contents(BASE_PATH . '/' . $jsFile);
+		Requirements::combine_files('testRequirementsCombine.js', array($jsFile));
+		require_once('thirdparty/jsmin/jsmin.php');
+		
+		// first make sure that our test js file causes an exception to be thrown
+		try{
+			$content = JSMin::minify($content);
+			Requirements::set_backend($oldBackend);
+			$this->fail('JSMin did not throw exception on minify bad file: ');
+		}catch(Exception $e){
+			// exception thrown... good
+		}
+
+		// secondly, make sure that requirements combine throws the correct warning, and only that warning
+		@unlink($combinedTestFilePath);
+		try{
+			Requirements::process_combined_files();
+		}catch(PHPUnit_Framework_Error_Warning $e){
+			if(strstr($e->getMessage(), 'Failed to minify') === false){
+				Requirements::set_backend($oldBackend);
+				$this->fail('Requirements::process_combined_files raised a warning, which is good, but this is not the expected warning ("Failed to minify..."): '.$e);
+			}
+		}catch(Exception $e){
+			Requirements::set_backend($oldBackend);
+			$this->fail('Requirements::process_combined_files did not catch exception caused by minifying bad js file: '.$e);
+		}
+		
+		// and make sure the combined content matches the input content, i.e. no loss of functionality
+		if(!file_exists($combinedTestFilePath)){
+			Requirements::set_backend($oldBackend);
+			$this->fail('No combined file was created at expected path: '.$combinedTestFilePath);
+		}
+		$combinedTestFileContents = file_get_contents($combinedTestFilePath);
+		$this->assertContains($jsFileContents, $combinedTestFileContents);
+
+		// reset
+		Requirements::set_backend($oldBackend);
+	}
+	
+
+
 	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

tests/view/themes/javascript/bad.js

@@ -0,0 +1 @@
+this is a javascript file which should cause an exception to be thrown when minified by jsmin '

view/Requirements.php

@@ -1139,10 +1139,16 @@ class Requirements_Backend {
 
 			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;
+				}
 
 				if ($this->write_header_comment) {
 					// write a header comment for each file for easier identification and debugging
@@ -1161,6 +1167,12 @@ class Requirements_Backend {
 				unset($fh);
 			}
 			
+			if($failedToMinify){
+				// Failed to minify, use unminified. 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'",