From 0242686a7a32b45022602d839e133ac969219b06 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Tue, 18 Dec 2012 00:58:09 +0100 Subject: [PATCH] Requirements acces to files with query strings (fixes #7735) Originally authored by florian.thoma, tests added by Ingo Schommer. Also removed query params from file paths before calling mtime() on it. See https://github.com/silverstripe/sapphire/pull/1023 --- tests/forms/RequirementsTest.php | 28 +++ view/Requirements.php | 281 ++++++++++++++++--------------- 2 files changed, 171 insertions(+), 138 deletions(-) diff --git a/tests/forms/RequirementsTest.php b/tests/forms/RequirementsTest.php index 9c53e0ec9..af54ce2a7 100644 --- a/tests/forms/RequirementsTest.php +++ b/tests/forms/RequirementsTest.php @@ -325,6 +325,34 @@ class RequirementsTest extends SapphireTest { $this->assertContains('', $html); } + public function testSuffix() { + $template = '
My header

Body

'; + $basePath = $this->getCurrentRelativePath(); + $basePath = 'framework' . substr($basePath, strlen(FRAMEWORK_DIR)); + + $backend = new Requirements_Backend; + + $backend->javascript($basePath .'/RequirementsTest_a.js'); + $backend->javascript($basePath .'/RequirementsTest_b.js?foo=bar&bla=blubb'); + $backend->css($basePath .'/RequirementsTest_a.css'); + $backend->css($basePath .'/RequirementsTest_b.css?foo=bar&bla=blubb'); + + $backend->set_suffix_requirements(true); + $html = $backend->includeInHTML(false, $template); + $this->assertRegexp('/RequirementsTest_a\.js\?m=[\d]*/', $html); + $this->assertRegexp('/RequirementsTest_b\.js\?m=[\d]*&foo=bar&bla=blubb/', $html); + $this->assertRegexp('/RequirementsTest_a\.css\?m=[\d]*/', $html); + $this->assertRegexp('/RequirementsTest_b\.css\?m=[\d]*&foo=bar&bla=blubb/', $html); + + $backend->set_suffix_requirements(false); + $html = $backend->includeInHTML(false, $template); + $this->assertNotContains('RequirementsTest_a.js=', $html); + $this->assertNotRegexp('/RequirementsTest_a\.js\?m=[\d]*/', $html); + $this->assertNotRegexp('/RequirementsTest_b\.js\?m=[\d]*&foo=bar&bla=blubb/', $html); + $this->assertNotRegexp('/RequirementsTest_a\.css\?m=[\d]*/', $html); + $this->assertNotRegexp('/RequirementsTest_b\.css\?m=[\d]*&foo=bar&bla=blubb/', $html); + } + public function assertFileIncluded($backend, $type, $files) { $type = strtolower($type); switch (strtolower($type)) { diff --git a/view/Requirements.php b/view/Requirements.php index c39a1cfd8..14b38969a 100644 --- a/view/Requirements.php +++ b/view/Requirements.php @@ -3,12 +3,12 @@ /** * Requirements tracker, for javascript and css. * @todo Document the requirements tracker, and discuss it with the others. - * + * * @package framework * @subpackage view */ class Requirements { - + /** * Enable combining of css/javascript files. * @param boolean $enable @@ -34,38 +34,38 @@ class Requirements { } /** - * Set whether we want to suffix requirements with the time / + * Set whether we want to suffix requirements with the time / * location on to the requirements - * + * * @param bool */ public static function set_suffix_requirements($var) { self::backend()->set_suffix_requirements($var); } - + /** * Return whether we want to suffix requirements - * + * * @return bool */ public static function get_suffix_requirements() { return self::backend()->get_suffix_requirements(); } - + /** * Instance of requirements for storage * * @var Requirements */ private static $backend = null; - + public static function backend() { if(!self::$backend) { self::$backend = new Requirements_Backend(); } return self::$backend; } - + /** * Setter method for changing the Requirements backend * @@ -74,20 +74,20 @@ class Requirements { public static function set_backend(Requirements_Backend $backend) { self::$backend = $backend; } - + /** * Register the given javascript file as required. - * + * * See {@link Requirements_Backend::javascript()} for more info - * + * */ public static function javascript($file) { self::backend()->javascript($file); } - + /** * Add the javascript code to the header of the page - * + * * See {@link Requirements_Backend::customScript()} for more info * @param script The script content * @param uniquenessID Use this to ensure that pieces of code only get added once. @@ -98,50 +98,50 @@ class Requirements { /** * Include custom CSS styling to the header of the page. - * + * * See {@link Requirements_Backend::customCSS()} - * + * * @param string $script CSS selectors as a string (without \n"; } - - foreach(array_diff_key($this->customHeadTags,$this->blocked) as $customHeadTag) { - $requirements .= "$customHeadTag\n"; + + foreach(array_diff_key($this->customHeadTags,$this->blocked) as $customHeadTag) { + $requirements .= "$customHeadTag\n"; } - + if($this->write_js_to_body) { // Remove all newlines from code to preserve layout $jsRequirements = preg_replace('/>\n*/', '>', $jsRequirements); - + // We put script tags into the body, for performance. - // If your template already has script tags in the body, then we put our script + // If your template already has script tags in the body, then we put our script // tags just before those. Otherwise, we put it at the bottom. $p1 = strripos($content, ']*>)/i", $jsRequirements . "\\1", $content); } - - // Put CSS at the bottom of the head + + // Put CSS at the bottom of the head $content = preg_replace("/(<\/head>)/i", $requirements . "\\1", $content); } else { $content = preg_replace("/(<\/head>)/i", $requirements . "\\1", $content); $content = preg_replace("/(<\/head>)/i", $jsRequirements . "\\1", $content); } - } - + } + if(isset($_GET['debug_profile'])) Profiler::unmark("Requirements::includeInHTML"); return $content; @@ -724,20 +724,20 @@ class Requirements_Backend { * Attach requirements inclusion to X-Include-JS and X-Include-CSS headers on the HTTP response */ public function include_in_response(SS_HTTPResponse $response) { - $this->process_combined_files(); + $this->process_combined_files(); $jsRequirements = array(); $cssRequirements = array(); - foreach(array_diff_key($this->javascript, $this->blocked) as $file => $dummy) { + foreach(array_diff_key($this->javascript, $this->blocked) as $file => $dummy) { $path = $this->path_for_file($file); if($path) { $jsRequirements[] = str_replace(',', '%2C', $path); } } - + $response->addHeader('X-Include-JS', implode(',', $jsRequirements)); - foreach(array_diff_key($this->css,$this->blocked) as $file => $params) { + foreach(array_diff_key($this->css,$this->blocked) as $file => $params) { $path = $this->path_for_file($file); if($path) { $path = str_replace(',', '%2C', $path); @@ -747,11 +747,11 @@ class Requirements_Backend { $response->addHeader('X-Include-CSS', implode(',', $cssRequirements)); } - + /** * Add i18n files from the given javascript directory. SilverStripe expects that the given directory * will contain a number of java script files named by language: en_US.js, de_DE.js, etc. - * + * * @param String The javascript lang directory, relative to the site root, e.g., 'framework/javascript/lang' * @param Boolean Return all relative file paths rather than including them in requirements * @param Boolean Only include language files, not the base libraries @@ -765,10 +765,10 @@ class Requirements_Backend { if(!$langOnly) $files[] = FRAMEWORK_DIR . '/javascript/i18n.js'; if(substr($langDir,-1) != '/') $langDir .= '/'; - + $files[] = $langDir . i18n::default_locale() . '.js'; $files[] = $langDir . i18n::get_locale() . '.js'; - + // Stub i18n implementation for when i18n is disabled. } else { if(!$langOnly) $files[] = FRAMEWORK_DIR . '/javascript/i18nx.js'; @@ -779,49 +779,54 @@ class Requirements_Backend { } else { foreach($files as $file) $this->javascript($file); } - } - + } + /** * Finds the path for specified file. * * @param string $fileOrUrl - * @return string|boolean + * @return string|boolean */ protected function path_for_file($fileOrUrl) { if(preg_match('{^//|http[s]?}', $fileOrUrl)) { return $fileOrUrl; } elseif(Director::fileExists($fileOrUrl)) { + $filePath = preg_replace('/\?.*/', '', Director::baseFolder() . '/' . $fileOrUrl); $prefix = Director::baseURL(); $mtimesuffix = ""; $suffix = ''; - if(strpos($fileOrUrl, '?') !== false) { - $suffix = '&' . substr($fileOrUrl, strpos($fileOrUrl, '?')+1); - $fileOrUrl = substr($fileOrUrl, 0, strpos($fileOrUrl, '?')); - } if($this->suffix_requirements) { - $mtimesuffix = "?m=" . filemtime(Director::baseFolder() . '/' . $fileOrUrl); + $mtimesuffix = "?m=" . filemtime($filePath); + $suffix = '&'; + } + if(strpos($fileOrUrl, '?') !== false) { + if (strlen($suffix) == 0) { + $suffix = '?'; + } + $suffix .= substr($fileOrUrl, strpos($fileOrUrl, '?')+1); + $fileOrUrl = substr($fileOrUrl, 0, strpos($fileOrUrl, '?')); } return "{$prefix}{$fileOrUrl}{$mtimesuffix}{$suffix}"; } else { return false; } } - + /** * Concatenate several css or javascript files into a single dynamically generated * file (stored in {@link Director::baseFolder()}). This 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 by {@link $combine_js_with_jsmin}. - * + * * All combined files will have a comment on the start of each concatenated file * denoting their original position. For easier debugging, we recommend to only * minify javascript if not in development mode ({@link Director::isDev()}). - * + * * 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 javascript logic, and combining css can lead to wrong styling inheritance. @@ -829,7 +834,7 @@ class Requirements_Backend { * in more than one combine_files() call. * Best practice is to include every javascript file in exactly *one* combine_files() * directive to avoid the issues mentioned above - this is enforced by this function. - * + * * CAUTION: Combining CSS Files discards any "media" information. * * Example for combined JavaScript: @@ -855,10 +860,10 @@ class Requirements_Backend { * * * @see http://code.google.com/p/jsmin-php/ - * + * * @todo Should we enforce unique inclusion of files, or leave it to the developer? Can auto-detection cause * breaks? - * + * * @param string $combinedFileName Filename of the combined file (will be stored in {@link Director::baseFolder()} * by default) * @param array $files Array of filenames relative to the webroot @@ -914,7 +919,7 @@ class Requirements_Backend { } $this->combine_files[$combinedFileName] = $files; } - + /** * Returns all combined files. * @return array @@ -922,15 +927,15 @@ class Requirements_Backend { public function get_combine_files() { return $this->combine_files; } - + /** - * Deletes all dynamically generated combined files from the filesystem. - * + * Deletes all dynamically generated combined files from the filesystem. + * * @param string $combinedFileName If left blank, all combined files are deleted. */ public function delete_combined_files($combinedFileName = null) { $combinedFiles = ($combinedFileName) ? array($combinedFileName => null) : $this->combine_files; - $combinedFolder = ($this->getCombinedFilesFolder()) ? + $combinedFolder = ($this->getCombinedFilesFolder()) ? (Director::baseFolder() . '/' . $this->combinedFilesFolder) : Director::baseFolder(); foreach($combinedFiles as $combinedFile => $sourceItems) { $filePath = $combinedFolder . '/' . $combinedFile; @@ -939,7 +944,7 @@ class Requirements_Backend { } } } - + public function clear_combined_files() { $this->combine_files = array(); } @@ -953,7 +958,7 @@ class Requirements_Backend { // 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) { return; } @@ -962,12 +967,12 @@ class Requirements_Backend { $combinerCheck = array(); foreach($this->combine_files as $combinedFile => $sourceItems) { foreach($sourceItems as $sourceItem) { - if(isset($combinerCheck[$sourceItem]) && $combinerCheck[$sourceItem] != $combinedFile){ + 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; - + } } @@ -986,7 +991,7 @@ class Requirements_Backend { $newJSRequirements[$file] = true; } } - + foreach($this->css as $file => $params) { if(isset($combinerCheck[$file])) { $newCSSRequirements[$combinedFilesFolder . $combinerCheck[$file]] = true; @@ -1042,7 +1047,7 @@ class Requirements_Backend { $isJS = stripos($file, '.js'); if($isJS && $this->combine_js_with_jsmin) { require_once('thirdparty/jsmin/jsmin.php'); - + increase_time_limit_to(); $fileContent = JSMin::minify($fileContent); } @@ -1075,13 +1080,13 @@ class Requirements_Backend { public function get_custom_scripts() { $requirements = ""; - + if($this->customScript) { foreach($this->customScript as $script) { $requirements .= "$script\n"; } } - + return $requirements; } @@ -1103,7 +1108,7 @@ class Requirements_Backend { $this->css($module.$css, $media); } } - + public function debug() { Debug::show($this->javascript); Debug::show($this->css); @@ -1112,5 +1117,5 @@ class Requirements_Backend { Debug::show($this->customHeadTags); Debug::show($this->combine_files); } - + }