From 9fc51dc527ecf858853d70e342820cf0c23b8d9a Mon Sep 17 00:00:00 2001 From: Florian Thoma Date: Mon, 4 Jul 2016 16:55:35 +1000 Subject: [PATCH] add async and defer attributes to js requirements, replaces #4555 --- .../01_Templates/03_Requirements.md | 27 ++- tests/forms/RequirementsTest.php | 184 +++++++++++++++++- tests/view/SSViewerTest.php | 2 +- view/Requirements.php | 118 +++++++---- 4 files changed, 286 insertions(+), 45 deletions(-) diff --git a/docs/en/02_Developer_Guides/01_Templates/03_Requirements.md b/docs/en/02_Developer_Guides/01_Templates/03_Requirements.md index 5fccbdc7b..507b52df9 100644 --- a/docs/en/02_Developer_Guides/01_Templates/03_Requirements.md +++ b/docs/en/02_Developer_Guides/01_Templates/03_Requirements.md @@ -56,7 +56,7 @@ If you're using the CSS method a second argument can be used. This argument defi ### Javascript Files :::php - Requirements::javascript($path); + Requirements::javascript($path, $options); A variant on the inclusion of custom javascript is the inclusion of *templated* javascript. Here, you keep your JavaScript in a separate file and instead load, via search and replace, several PHP-generated variables into that code. @@ -75,7 +75,6 @@ that your included files provide these scripts. This will ensure that subsequent Requirement calls that rely on those included scripts will not double include those files. - :::php Requirements::javascript('/javascript/dist/bundle.js', ['provides' => [ '/javascript/jquery.js' @@ -84,6 +83,16 @@ files. ]]); Requirements::javascript('/javascript/jquery.js'); // Will will skip this file +You can also use the second argumet to add the 'async' and/or 'defer attributes to the script tag generated: + + :::php + Requirements::javascript( + "/javascript/some_file.js", + array( + "async" => true, + "defer" => true, + ) + ); ### Custom Inline CSS or Javascript @@ -218,6 +227,20 @@ When combining CSS files, take care of relative urls, as these will not be re-wr the destination location of the resulting combined CSS. +### Combined JS Files + +You can also add the 'async' and/or 'defer' attributes to combined Javascript files as you would with the +`Requirements::javascript` call - use the third paramter of the `combine_files` function: + + :::php + $scripts = array( + "$themeDir/javascript/some_script.js", + "$themeDir/javascript/some_other_script.js", + ); + + Requirements::combine_files('scripts.js', $scripts, array('async' => true, 'defer' => true)); + + ## Clearing assets :::php diff --git a/tests/forms/RequirementsTest.php b/tests/forms/RequirementsTest.php index e59446daf..554e37288 100644 --- a/tests/forms/RequirementsTest.php +++ b/tests/forms/RequirementsTest.php @@ -117,6 +117,29 @@ class RequirementsTest extends SapphireTest { ) ); } + + protected function setupCombinedRequirementsJavascriptAsyncDefer($backend, $async, $defer) { + $basePath = $this->getCurrentRelativePath(); + $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->combineFiles( + 'RequirementsTest_bc.js', + array( + $basePath . '/RequirementsTest_b.js', + $basePath . '/RequirementsTest_c.js' + ), + array( + 'async' => $async, + 'defer' => $defer, + ) + ); + } public function testCombinedJavascript() { /** @var Requirements_Backend $backend */ @@ -204,6 +227,152 @@ class RequirementsTest extends SapphireTest { 'combined files are not included twice' ); } + + public function testCombinedJavascriptAsyncDefer() { + /** @var Requirements_Backend $backend */ + $backend = Injector::inst()->create('Requirements_Backend'); + + $this->setupCombinedRequirementsJavascriptAsyncDefer($backend, true, false); + + $combinedFileName = '/_combinedfiles/RequirementsTest_bc-2a55d56.js'; + $combinedFilePath = AssetStoreTest_SpyStore::base_path() . $combinedFileName; + + $html = $backend->includeInHTML(false, self::$html_template); + + /* ASYNC IS INCLUDED IN SCRIPT TAG */ + $this->assertRegExp( + '/src=".*' . preg_quote($combinedFileName, '/') . '" async/', + $html, + 'async is included in script tag' + ); + + /* DEFER IS NOT INCLUDED IN SCRIPT TAG */ + $this->assertNotContains('defer', $html, 'defer is not included'); + + /* COMBINED JAVASCRIPT FILE EXISTS */ + clearstatcache(); // needed to get accurate file_exists() results + $this->assertFileExists($combinedFilePath, + 'combined javascript file exists'); + + /* COMBINED JAVASCRIPT HAS CORRECT CONTENT */ + $this->assertTrue((strpos(file_get_contents($combinedFilePath), "alert('b')") !== false), + 'combined javascript has correct content'); + $this->assertTrue((strpos(file_get_contents($combinedFilePath), "alert('c')") !== false), + 'combined javascript has correct content'); + + /* 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->assertRegExp('/src=".*\/RequirementsTest_a\.js/', $html, + 'normal requirements are still included'); + + /* NORMAL REQUIREMENTS DON'T HAVE ASYNC/DEFER */ + $this->assertNotRegExp('/src=".*\/RequirementsTest_a\.js\?m=\d+" async/', $html, + 'normal requirements don\'t have async'); + $this->assertNotRegExp('/src=".*\/RequirementsTest_a\.js\?m=\d+" defer/', $html, + 'normal requirements don\'t have defer'); + $this->assertNotRegExp('/src=".*\/RequirementsTest_a\.js\?m=\d+" async defer/', $html, + 'normal requirements don\'t have async/defer'); + + // setup again for testing defer + unlink($combinedFilePath); + /** @var Requirements_Backend $backend */ + $backend = Injector::inst()->create('Requirements_Backend'); + + $this->setupCombinedRequirementsJavascriptAsyncDefer($backend, false, true); + + $html = $backend->includeInHTML(self::$html_template); + + /* DEFER IS INCLUDED IN SCRIPT TAG */ + $this->assertRegExp( + '/src=".*' . preg_quote($combinedFileName, '/') . '" defer/', + $html, + 'defer is included in script tag' + ); + + /* ASYNC IS NOT INCLUDED IN SCRIPT TAG */ + $this->assertNotContains('async', $html, 'async is not included'); + + /* COMBINED JAVASCRIPT FILE EXISTS */ + clearstatcache(); // needed to get accurate file_exists() results + $this->assertFileExists($combinedFilePath, + 'combined javascript file exists'); + + /* COMBINED JAVASCRIPT HAS CORRECT CONTENT */ + $this->assertTrue((strpos(file_get_contents($combinedFilePath), "alert('b')") !== false), + 'combined javascript has correct content'); + $this->assertTrue((strpos(file_get_contents($combinedFilePath), "alert('c')") !== false), + 'combined javascript has correct content'); + + /* 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->assertRegExp('/src=".*\/RequirementsTest_a\.js/', $html, + 'normal requirements are still included'); + + /* NORMAL REQUIREMENTS DON'T HAVE ASYNC/DEFER */ + $this->assertNotRegExp('/src=".*\/RequirementsTest_a\.js\?m=\d+" async/', $html, + 'normal requirements don\'t have async'); + $this->assertNotRegExp('/src=".*\/RequirementsTest_a\.js\?m=\d+" defer/', $html, + 'normal requirements don\'t have defer'); + $this->assertNotRegExp('/src=".*\/RequirementsTest_a\.js\?m=\d+" async defer/', $html, + 'normal requirements don\'t have async/defer'); + + // setup again for testing async and defer + unlink($combinedFilePath); + /** @var Requirements_Backend $backend */ + $backend = Injector::inst()->create('Requirements_Backend'); + + $this->setupCombinedRequirementsJavascriptAsyncDefer($backend, true, true); + + $html = $backend->includeInHTML(self::$html_template); + + /* ASYNC/DEFER IS INCLUDED IN SCRIPT TAG */ + $this->assertRegExp( + '/src=".*' . preg_quote($combinedFileName, '/') . '" async defer/', + $html, + 'async and defer are included in script tag' + ); + + /* COMBINED JAVASCRIPT FILE EXISTS */ + clearstatcache(); // needed to get accurate file_exists() results + $this->assertFileExists($combinedFilePath, + 'combined javascript file exists'); + + /* COMBINED JAVASCRIPT HAS CORRECT CONTENT */ + $this->assertTrue((strpos(file_get_contents($combinedFilePath), "alert('b')") !== false), + 'combined javascript has correct content'); + $this->assertTrue((strpos(file_get_contents($combinedFilePath), "alert('c')") !== false), + 'combined javascript has correct content'); + + /* 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->assertRegExp('/src=".*\/RequirementsTest_a\.js/', $html, + 'normal requirements are still included'); + + /* NORMAL REQUIREMENTS DON'T HAVE ASYNC/DEFER */ + $this->assertNotRegExp('/src=".*\/RequirementsTest_a\.js\?m=\d+" async/', $html, + 'normal requirements don\'t have async'); + $this->assertNotRegExp('/src=".*\/RequirementsTest_a\.js\?m=\d+" defer/', $html, + 'normal requirements don\'t have defer'); + $this->assertNotRegExp('/src=".*\/RequirementsTest_a\.js\?m=\d+" async defer/', $html, + 'normal requirements don\'t have async/defer'); + + unlink($combinedFilePath); + } public function testCombinedCss() { $basePath = $this->getCurrentRelativePath(); @@ -217,7 +386,9 @@ class RequirementsTest extends SapphireTest { $basePath . '/RequirementsTest_print_a.css', $basePath . '/RequirementsTest_print_b.css' ), - 'print' + array( + 'media' => 'print' + ) ); $html = $backend->includeInHTML(self::$html_template); @@ -284,7 +455,7 @@ class RequirementsTest extends SapphireTest { /* BLOCKED UNCOMBINED FILES ARE NOT INCLUDED */ $this->setupCombinedRequirements($backend); $backend->block($basePath .'/RequirementsTest_b.js'); - $combinedFileName2 = '/_combinedfiles/RequirementsTest_bc-3748f67.js'; // SHA1 without file c included + $combinedFileName2 = '/_combinedfiles/RequirementsTest_bc-3748f67.js'; // SHA1 without file b included $combinedFilePath2 = AssetStoreTest_SpyStore::base_path() . $combinedFileName2; clearstatcache(); // needed to get accurate file_exists() results $html = $backend->includeInHTML(self::$html_template); @@ -353,7 +524,7 @@ class RequirementsTest extends SapphireTest { $this->assertTrue(count($backend->getJavascript()) == 1, "There should be only 1 file included in required javascript."); - $this->assertTrue(in_array($basePath . '/a.js', $backend->getJavascript()), + $this->assertArrayHasKey($basePath . '/a.js', $backend->getJavascript(), "a.js should be included in required javascript."); $backend->javascript($basePath . '/b.js'); @@ -363,9 +534,9 @@ class RequirementsTest extends SapphireTest { $backend->block($basePath . '/a.js'); $this->assertTrue(count($backend->getJavascript()) == 1, "There should be only 1 file included in required javascript."); - $this->assertFalse(in_array($basePath . '/a.js', $backend->getJavascript()), + $this->assertArrayNotHasKey($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->getJavascript()), + $this->assertArrayHasKey($basePath . '/b.js', $backend->getJavascript(), "b.js should be included in required javascript."); $backend->css($basePath . '/a.css'); @@ -675,8 +846,7 @@ EOS case 'js': case 'javascript': case 'script': - $scripts = $backend->getJavascript(); - return array_combine(array_values($scripts), array_values($scripts)); + return $backend->getJavascript(); } return array(); } diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php index b542e27c9..2f92f494e 100644 --- a/tests/view/SSViewerTest.php +++ b/tests/view/SSViewerTest.php @@ -183,7 +183,7 @@ class SSViewerTest extends SapphireTest { // secondly, make sure that requirements is generated, even though minification failed $testBackend->processCombinedFiles(); - $js = $testBackend->getJavascript(); + $js = array_keys($testBackend->getJavascript()); $combinedTestFilePath = BASE_PATH . reset($js); $this->assertContains('_combinedfiles/testRequirementsCombine-4c0e97a.js', $combinedTestFilePath); diff --git a/view/Requirements.php b/view/Requirements.php index 5f27dd2ce..a3bf19e96 100644 --- a/view/Requirements.php +++ b/view/Requirements.php @@ -112,6 +112,8 @@ class Requirements implements Flushable { * @param string $file Relative to docroot * @param array $options List of options. Available options include: * - 'provides' : List of scripts files included in this file + * - 'async' : Boolean value to set async attribute to script tag + * - 'defer' : Boolean value to set defer attribute to script tag */ public static function javascript($file, $options = array()) { self::backend()->javascript($file, $options); @@ -337,12 +339,19 @@ class Requirements implements Flushable { * * @param string $combinedFileName Filename of the combined file relative to docroot * @param array $files Array of filenames relative to docroot - * @param string $media + * @param array $options Array of options for combining files. Available options are: + * - 'media' : If including CSS Files, you can specify a media type + * - 'async' : If including JavaScript Files, boolean value to set async attribute to script tag + * - 'defer' : If including JavaScript Files, boolean value to set defer attribute to script tag * * @return bool|void */ - public static function combine_files($combinedFileName, $files, $media = null) { - self::backend()->combineFiles($combinedFileName, $files, $media); + public static function combine_files($combinedFileName, $files, $options = array()) { + if(is_string($options)) { + Deprecation::notice('4.0', 'Parameter media is deprecated. Use options array instead.'); + $options = array('media' => $options); + } + self::backend()->combineFiles($combinedFileName, $files, $options); } /** @@ -803,14 +812,37 @@ class Requirements_Backend * @param string $file Relative to docroot * @param array $options List of options. Available options include: * - 'provides' : List of scripts files included in this file + * - 'async' : Boolean value to set async attribute to script tag + * - 'defer' : Boolean value to set defer attribute to script tag */ public function javascript($file, $options = array()) { - $this->javascript[$file] = true; + // make sure that async/defer is set if it is set once even if file is included multiple times + $async = ( + isset($options['async']) && isset($options['async']) == true + || ( + isset($this->javascript[$file]) + && isset($this->javascript[$file]['async']) + && $this->javascript[$file]['async'] == true + ) + ); + $defer = ( + isset($options['defer']) && isset($options['defer']) == true + || ( + isset($this->javascript[$file]) + && isset($this->javascript[$file]['defer']) + && $this->javascript[$file]['defer'] == true + ) + ); + $this->javascript[$file] = array( + 'async' => $async, + 'defer' => $defer, + ); // Record scripts included in this file if(isset($options['provides'])) { $this->providedJavascript[$file] = array_values($options['provides']); - } + } + } /** @@ -841,7 +873,7 @@ class Requirements_Backend public function getProvidedScripts() { $providedScripts = array(); $includedScripts = array(); - foreach($this->javascript as $script => $flag) { + foreach($this->javascript as $script => $options) { // Ignore scripts that are explicitly blocked if(isset($this->blocked[$script])) { continue; @@ -869,11 +901,11 @@ class Requirements_Backend * @return array */ public function getJavascript() { - return array_keys(array_diff_key( + return array_diff_key( $this->javascript, $this->getBlocked(), $this->getProvidedScripts() - )); + ); } /** @@ -882,7 +914,7 @@ class Requirements_Backend * @return array Indexed array of javascript files */ protected function getAllJavascript() { - return array_keys($this->javascript); + return $this->javascript; } /** @@ -1130,10 +1162,12 @@ class Requirements_Backend // Combine files - updates $this->javascript and $this->css $this->processCombinedFiles(); - foreach($this->getJavascript() as $file) { + foreach($this->getJavascript() as $file => $attributes) { + $async = (isset($attributes['async']) && $attributes['async'] == true) ? " async" : ""; + $defer = (isset($attributes['defer']) && $attributes['defer'] == true) ? " defer" : ""; $path = Convert::raw2att($this->pathForFile($file)); if($path) { - $jsRequirements .= ""; + $jsRequirements .= ""; } } @@ -1267,7 +1301,7 @@ class Requirements_Backend $jsRequirements = array(); $cssRequirements = array(); - foreach($this->getJavascript() as $file) { + foreach($this->getJavascript() as $file => $attributes) { $path = $this->pathForFile($file); if($path) { $jsRequirements[] = str_replace(',', '%2C', $path); @@ -1399,10 +1433,14 @@ class Requirements_Backend * Example for combined JavaScript: * * Requirements::combine_files( - * 'foobar.js', - * array( + * 'foobar.js', + * array( * 'mysite/javascript/foo.js', * 'mysite/javascript/bar.js', + * ), + * array( + * 'async' => true, + * 'defer' => true, * ) * ); * @@ -1410,23 +1448,33 @@ class Requirements_Backend * Example for combined CSS: * * Requirements::combine_files( - * 'foobar.css', + * 'foobar.css', * array( * 'mysite/javascript/foo.css', * 'mysite/javascript/bar.css', + * ), + * array( + * 'media' => 'print', * ) * ); * * * @param string $combinedFileName Filename of the combined file relative to docroot * @param array $files Array of filenames relative to docroot - * @param string $media If including CSS Files, you can specify a media type + * @param array $options Array of options for combining files. Available options are: + * - 'media' : If including CSS Files, you can specify a media type + * - 'async' : If including JavaScript Files, boolean value to set async attribute to script tag + * - 'defer' : If including JavaScript Files, boolean value to set defer attribute to script tag */ - public function combineFiles($combinedFileName, $files, $media = null) { + public function combineFiles($combinedFileName, $files, $options = array()) { + if(is_string($options)) { + Deprecation::notice('4.0', 'Parameter media is deprecated. Use options array instead.'); + $options = array('media' => $options); + } // Skip this combined files if already included if(isset($this->combinedFiles[$combinedFileName])) { return; - } + } // Add all files to necessary type list $paths = array(); @@ -1436,25 +1484,25 @@ class Requirements_Backend 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); + case 'css': + $this->css($path, (isset($options['media']) ? $options['media'] : null)); break; case 'js': - $this->javascript($path); - break; - default: + $this->javascript($path, $options); + break; + default: throw new InvalidArgumentException("Invalid combined file type: {$type}"); } $combinedType = $type; $paths[] = $path; - } + } // Duplicate check foreach($this->combinedFiles as $existingCombinedFilename => $combinedItem) { @@ -1466,15 +1514,15 @@ class Requirements_Backend implode(',', $duplicates), $existingCombinedFilename )); - } - } + } + } $this->combinedFiles[$combinedFileName] = array( 'files' => $paths, 'type' => $combinedType, - 'media' => $media + 'options' => $options, ); - } + } /** * Return path and type of given combined file @@ -1559,7 +1607,7 @@ class Requirements_Backend foreach($this->getAllCombinedFiles() as $combinedFile => $combinedItem) { $fileList = $combinedItem['files']; $type = $combinedItem['type']; - $media = $combinedItem['media']; + $options = $combinedItem['options']; // Generate this file, unless blocked $combinedURL = null; @@ -1585,7 +1633,7 @@ class Requirements_Backend if(!in_array($css, $fileList)) { $newCSS[$css] = $spec; } elseif(!$included && $combinedURL) { - $newCSS[$combinedURL] = array('media' => $media); + $newCSS[$combinedURL] = array('media' => (isset($options['media']) ? $options['media'] : null)); $included = true; } // If already included, or otherwise blocked, then don't add into CSS @@ -1594,13 +1642,13 @@ class Requirements_Backend break; } case 'js': { - // Assoc array of file => true + // Assoc array of file => attributes $newJS = array(); - foreach($this->getAllJavascript() as $script) { + foreach($this->getAllJavascript() as $script => $attributes) { if(!in_array($script, $fileList)) { - $newJS[$script] = true; + $newJS[$script] = $attributes; } elseif(!$included && $combinedURL) { - $newJS[$combinedURL] = true; + $newJS[$combinedURL] = $options; $included = true; } // If already included, or otherwise blocked, then don't add into scripts