BUG Fix issue with Requirements mangling custom scripts (#5337)

API Remove deprecated $template parameter from Requirements::includeInHTML
This commit is contained in:
Damian Mooyman 2016-04-19 17:20:30 +12:00 committed by Sam Minnée
parent 6ccfbb7c52
commit 0bd62735bc
4 changed files with 341 additions and 265 deletions

View File

@ -224,6 +224,10 @@ New methods on `Requirements` are added to access these:
* `get_force_js_to_bottom`
* `get_write_js_to_body`
Some methods on `Requirements` have had their method signatures changed:
* `includeInHTML` has had the first parameter $template removed as it was previously deprecated.
And some methods on `Requirements` and `Requirements_Backend` have been removed as they are obsolete.
* `delete_combined_files` (both classes)

View File

@ -33,7 +33,7 @@ class RequirementsTest extends SapphireTest {
$backend->css('https://www.mysecuredomain.com/test.css');
$backend->css('//scheme-relative.example.com/test.css');
$html = $backend->includeInHTML(false, self::$html_template);
$html = $backend->includeInHTML(self::$html_template);
$this->assertTrue(
(strpos($html, 'http://www.mydomain.com/test.js') !== false),
@ -126,7 +126,7 @@ class RequirementsTest extends SapphireTest {
$combinedFileName = '/_combinedfiles/RequirementsTest_bc-2a55d56.js';
$combinedFilePath = AssetStoreTest_SpyStore::base_path() . $combinedFileName;
$html = $backend->includeInHTML(false, self::$html_template);
$html = $backend->includeInHTML(self::$html_template);
/* COMBINED JAVASCRIPT FILE IS INCLUDED IN HTML HEADER */
$this->assertRegExp(
@ -171,7 +171,7 @@ class RequirementsTest extends SapphireTest {
/** @var Requirements_Backend $backend */
$backend = Injector::inst()->create('Requirements_Backend');
$this->setupCombinedNonrequiredRequirements($backend);
$html = $backend->includeInHTML(false, self::$html_template);
$html = $backend->includeInHTML(self::$html_template);
/* COMBINED JAVASCRIPT FILE IS INCLUDED IN HTML HEADER */
$this->assertRegExp(
@ -220,7 +220,7 @@ class RequirementsTest extends SapphireTest {
'print'
);
$html = $backend->includeInHTML(false, self::$html_template);
$html = $backend->includeInHTML(self::$html_template);
$this->assertRegExp(
'/href=".*\/print\-94e723d\.css/',
@ -252,7 +252,7 @@ class RequirementsTest extends SapphireTest {
)
);
$html = $backend->includeInHTML(false, self::$html_template);
$html = $backend->includeInHTML(self::$html_template);
$this->assertRegExp(
'/href=".*\/style\-bcd90f5\.css/',
$html,
@ -272,7 +272,7 @@ class RequirementsTest extends SapphireTest {
$backend->block('RequirementsTest_bc.js');
clearstatcache(); // needed to get accurate file_exists() results
$html = $backend->includeInHTML(false, self::$html_template);
$html = $backend->includeInHTML(self::$html_template);
$this->assertFileNotExists($combinedFilePath);
$this->assertNotRegExp(
'/src=".*\/RequirementsTest_bc\.js/',
@ -287,7 +287,7 @@ class RequirementsTest extends SapphireTest {
$combinedFileName2 = '/_combinedfiles/RequirementsTest_bc-3748f67.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);
$html = $backend->includeInHTML(self::$html_template);
$this->assertFileExists($combinedFilePath2);
$this->assertTrue(
strpos(file_get_contents($combinedFilePath2), "alert('b')") === false,
@ -326,7 +326,7 @@ class RequirementsTest extends SapphireTest {
$backend->javascript($basePath . '/RequirementsTest_a.js?test=1&test=2&test=3');
$backend->css($basePath . '/RequirementsTest_a.css?test=1&test=2&test=3');
$html = $backend->includeInHTML(false, self::$html_template);
$html = $backend->includeInHTML(self::$html_template);
/* Javascript has correct path */
$this->assertRegExp(
@ -424,11 +424,11 @@ class RequirementsTest extends SapphireTest {
$template = '<html><head></head><body><header>My header</header><p>Body</p></body></html>';
$backend->setWriteJavascriptToBody(false);
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertContains('<head><script', $html);
$backend->setWriteJavascriptToBody(true);
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertNotContains('<head><script', $html);
$this->assertContains('</script></body>', $html);
}
@ -439,7 +439,7 @@ class RequirementsTest extends SapphireTest {
$backend = Injector::inst()->create('Requirements_Backend');
$this->setupRequirements($backend);
$backend->javascript($this->getCurrentRelativePath() . '/RequirementsTest_a.js');
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
//wiping out commented-out html
$html = preg_replace('/<!--(.*)-->/Uis', '', $html);
$this->assertContains("RequirementsTest_a.js", $html);
@ -455,7 +455,7 @@ class RequirementsTest extends SapphireTest {
$src = $this->getCurrentRelativePath() . '/RequirementsTest_a.js';
$urlSrc = ControllerTest_ContainerController::join_links(Director::baseURL(), $src);
$backend->javascript($src);
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertEquals('<html><head></head><body><!--<script>alert("commented out");</script>-->'
. '<h1>more content</h1><script type="application/javascript" src="' . $urlSrc . '"></script></body></html>', $html);
}
@ -465,24 +465,32 @@ class RequirementsTest extends SapphireTest {
$backend = Injector::inst()->create('Requirements_Backend');
$this->setupRequirements($backend);
$backend->javascript('http://www.mydomain.com/test.js');
$backend->customScript(
<<<'EOS'
var globalvar = {
pattern: '\\$custom\\1'
};
EOS
);
// Test matching with HTML5 <header> tags as well
$template = '<html><head></head><body><header>My header</header><p>Body<script></script></p></body></html>';
// The expected outputs
$JsInHead = "<html><head><script type=\"application/javascript\" src=\"http://www.mydomain.com/test.js\">"
. "</script>\n</head><body><header>My header</header><p>Body<script></script></p></body></html>";
$JsInBody = "<html><head></head><body><header>My header</header><p>Body<script type=\"application/javascript\""
. " src=\"http://www.mydomain.com/test.js\"></script><script></script></p></body></html>";
$JsAtEnd = "<html><head></head><body><header>My header</header><p>Body<script></script></p><script "
. "type=\"application/javascript\" src=\"http://www.mydomain.com/test.js\"></script></body></html>";
$expectedScripts = "<script type=\"application/javascript\" src=\"http://www.mydomain.com/test.js\">"
. "</script><script type=\"application/javascript\">//<![CDATA[\n"
. "var globalvar = {\n\tpattern: '\\\\\$custom\\\\1'\n};\n"
. "//]]></script>";
$JsInHead = "<html><head>$expectedScripts</head><body><header>My header</header><p>Body<script></script></p></body></html>";
$JsInBody = "<html><head></head><body><header>My header</header><p>Body$expectedScripts<script></script></p></body></html>";
$JsAtEnd = "<html><head></head><body><header>My header</header><p>Body<script></script></p>$expectedScripts</body></html>";
// Test if the script is before the head tag, not before the body.
// Expected: $JsInHead
$backend->setWriteJavascriptToBody(false);
$backend->setForceJSToBottom(false);
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertNotEquals($JsInBody, $html);
$this->assertNotEquals($JsAtEnd, $html);
$this->assertEquals($JsInHead, $html);
@ -491,7 +499,7 @@ class RequirementsTest extends SapphireTest {
// Expected: $JsInBody
$backend->setWriteJavascriptToBody(true);
$backend->setForceJSToBottom(false);
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertNotEquals($JsAtEnd, $html);
$this->assertEquals($JsInBody, $html);
@ -499,7 +507,7 @@ class RequirementsTest extends SapphireTest {
// Expected: $JsAtEnd
$backend->setWriteJavascriptToBody(false);
$backend->setForceJSToBottom(true);
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertNotEquals($JsInHead, $html);
$this->assertNotEquals($JsInBody, $html);
$this->assertEquals($JsAtEnd, $html);
@ -508,7 +516,7 @@ class RequirementsTest extends SapphireTest {
// Expected: $JsAtEnd
$backend->setWriteJavascriptToBody(true);
$backend->setForceJSToBottom(true);
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertNotEquals($JsInHead, $html);
$this->assertNotEquals($JsInBody, $html);
$this->assertEquals($JsAtEnd, $html);
@ -528,14 +536,14 @@ class RequirementsTest extends SapphireTest {
$backend->css($basePath .'/RequirementsTest_b.css?foo=bar&bla=blubb');
$backend->setSuffixRequirements(true);
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertRegexp('/RequirementsTest_a\.js\?m=[\d]*"/', $html);
$this->assertRegexp('/RequirementsTest_b\.js\?m=[\d]*&amp;foo=bar&amp;bla=blubb"/', $html);
$this->assertRegexp('/RequirementsTest_a\.css\?m=[\d]*"/', $html);
$this->assertRegexp('/RequirementsTest_b\.css\?m=[\d]*&amp;foo=bar&amp;bla=blubb"/', $html);
$backend->setSuffixRequirements(false);
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertNotContains('RequirementsTest_a.js=', $html);
$this->assertNotRegexp('/RequirementsTest_a\.js\?m=[\d]*"/', $html);
$this->assertNotRegexp('/RequirementsTest_b\.js\?m=[\d]*&amp;foo=bar&amp;bla=blubb"/', $html);
@ -567,7 +575,7 @@ class RequirementsTest extends SapphireTest {
$this->assertEquals([
$basePath . '/RequirementsTest_c.js' => $basePath . '/RequirementsTest_c.js'
], $backend->getProvidedScripts());
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertRegExp('/src=".*\/RequirementsTest_a\.js/', $html);
$this->assertRegExp('/src=".*\/RequirementsTest_b\.js/', $html);
$this->assertNotRegExp('/src=".*\/RequirementsTest_c\.js/', $html);
@ -586,7 +594,7 @@ class RequirementsTest extends SapphireTest {
$this->assertEquals([
$basePath . '/RequirementsTest_c.js' => $basePath . '/RequirementsTest_c.js'
], $backend->getProvidedScripts());
$html = $backend->includeInHTML(false, $template);
$html = $backend->includeInHTML($template);
$this->assertRegExp('/src=".*\/combined_a/', $html);
$this->assertRegExp('/src=".*\/RequirementsTest_b\.js/', $html);
$this->assertNotRegExp('/src=".*\/combined_c/', $html);

View File

@ -193,7 +193,7 @@ class Requirements implements Flushable {
* (e.g. 'screen,projector')
*/
public static function themedCSS($name, $module = null, $media = null) {
return self::backend()->themedCSS($name, $module, $media);
self::backend()->themedCSS($name, $module, $media);
}
/**
@ -253,13 +253,20 @@ class Requirements implements Flushable {
* requirements. Needs to receive a valid HTML/XHTML template in the $content parameter,
* including a head and body tag.
*
* @param string $templateFile No longer used, only retained for compatibility
* @param string $content HTML content that has already been parsed from the $templateFile
* through {@link SSViewer}
* @return string HTML content augmented with the requirements tags
*/
public static function includeInHTML($templateFile, $content) {
return self::backend()->includeInHTML($templateFile, $content);
public static function includeInHTML($content) {
if(func_num_args() > 1) {
Deprecation::notice(
'5.0',
'$templateFile argument is deprecated. includeInHTML takes a sole $content parameter now.'
);
$content = func_get_arg(1);
}
return self::backend()->includeInHTML($content);
}
/**
@ -269,7 +276,7 @@ class Requirements implements Flushable {
* @param SS_HTTPResponse $response
*/
public static function include_in_response(SS_HTTPResponse $response) {
return self::backend()->includeInResponse($response);
self::backend()->includeInResponse($response);
}
/**
@ -354,7 +361,7 @@ class Requirements implements Flushable {
* but doesn't delete the directory itself
*/
public static function delete_all_combined_files() {
return self::backend()->deleteAllCombinedFiles();
self::backend()->deleteAllCombinedFiles();
}
/**
@ -368,7 +375,7 @@ class Requirements implements Flushable {
* Do the heavy lifting involved in combining the combined files.
*/
public static function process_combined_files() {
return self::backend()->processCombinedFiles();
self::backend()->processCombinedFiles();
}
/**
@ -452,7 +459,7 @@ class Requirements implements Flushable {
* Output debugging information
*/
public static function debug() {
return self::backend()->debug();
self::backend()->debug();
}
}
@ -890,8 +897,6 @@ class Requirements_Backend
} else {
$this->customScript[] = $script;
}
$script .= "\n";
}
/**
@ -1100,12 +1105,19 @@ class Requirements_Backend
* requirements. Needs to receive a valid HTML/XHTML template in the $content parameter,
* including a head and body tag.
*
* @param string $templateFile No longer used, only retained for compatibility
* @param string $content HTML content that has already been parsed from the $templateFile
* through {@link SSViewer}
* @return string HTML content augmented with the requirements tags
*/
public function includeInHTML($templateFile, $content) {
public function includeInHTML($content) {
if(func_num_args() > 1) {
Deprecation::notice(
'5.0',
'$templateFile argument is deprecated. includeInHTML takes a sole $content parameter now.'
);
$content = func_get_arg(1);
}
// Skip if content isn't injectable, or there is nothing to inject
$tagsAvailable = preg_match('#</head\b#', $content);
$hasFiles = $this->css || $this->javascript || $this->customCSS || $this->customScript || $this->customHeadTags;
@ -1119,25 +1131,25 @@ class Requirements_Backend
$this->processCombinedFiles();
foreach($this->getJavascript() as $file) {
$path = Convert::raw2xml($this->pathForFile($file));
$path = Convert::raw2att($this->pathForFile($file));
if($path) {
$jsRequirements .= "<script type=\"application/javascript\" src=\"$path\"></script>\n";
$jsRequirements .= "<script type=\"application/javascript\" src=\"$path\"></script>";
}
}
// Add all inline JavaScript *after* including external files they might rely on
foreach($this->getCustomScripts() as $script) {
$jsRequirements .= "<script type=\"application/javascript\">\n//<![CDATA[\n";
$jsRequirements .= "<script type=\"application/javascript\">//<![CDATA[\n";
$jsRequirements .= "$script\n";
$jsRequirements .= "\n//]]>\n</script>\n";
$jsRequirements .= "//]]></script>";
}
foreach($this->getCSS() as $file => $params) {
$path = Convert::raw2xml($this->pathForFile($file));
$path = Convert::raw2att($this->pathForFile($file));
if($path) {
$media = (isset($params['media']) && !empty($params['media']))
? " media=\"{$params['media']}\"" : "";
$requirements .= "<link rel=\"stylesheet\" type=\"text/css\"{$media} href=\"$path\" />\n";
$requirements .= "<link rel=\"stylesheet\" type=\"text/css\" {$media} href=\"$path\" />\n";
}
}
@ -1149,50 +1161,101 @@ class Requirements_Backend
$requirements .= "$customHeadTag\n";
}
if ($this->getForceJSToBottom()) {
// Remove all newlines from code to preserve layout
$jsRequirements = preg_replace('/>\n*/', '>', $jsRequirements);
// Inject CSS into body
$content = $this->insertTagsIntoHead($requirements, $content);
// Inject scripts
if ($this->getForceJSToBottom()) {
$content = $this->insertScriptsAtBottom($jsRequirements, $content);
} elseif($this->getWriteJavascriptToBody()) {
$content = $this->insertScriptsIntoBody($jsRequirements, $content);
} else {
$content = $this->insertTagsIntoHead($jsRequirements, $content);
}
return $content;
}
/**
* Given a block of HTML, insert the given scripts at the bottom before
* the closing </body> tag
*
* @param string $jsRequirements String containing one or more javascript <script /> tags
* @param string $content HTML body
* @return string Merged HTML
*/
protected function insertScriptsAtBottom($jsRequirements, $content) {
// Forcefully put the scripts at the bottom of the body instead of before the first
// script tag.
$content = preg_replace("/(<\/body[^>]*>)/i", $jsRequirements . "\\1", $content);
// Put CSS at the bottom of the head
$content = preg_replace("/(<\/head>)/i", $requirements . "\\1", $content);
} elseif($this->getWriteJavascriptToBody()) {
// Remove all newlines from code to preserve layout
$jsRequirements = preg_replace('/>\n*/', '>', $jsRequirements);
$content = preg_replace(
'/(<\/body[^>]*>)/i',
$this->escapeReplacement($jsRequirements) . '\\1',
$content
);
return $content;
}
/**
* Given a block of HTML, insert the given scripts inside the <body></body>
*
* @param string $jsRequirements String containing one or more javascript <script /> tags
* @param string $content HTML body
* @return string Merged HTML
*/
protected function insertScriptsIntoBody($jsRequirements, $content) {
// If your template already has script tags in the body, then we try to put our script
// tags just before those. Otherwise, we put it at the bottom.
$p2 = stripos($content, '<body');
$p1 = stripos($content, '<script', $p2);
$bodyTagPosition = stripos($content, '<body');
$scriptTagPosition = stripos($content, '<script', $bodyTagPosition);
$commentTags = array();
$canWriteToBody = ($p1 !== false)
$canWriteToBody = ($scriptTagPosition !== false)
&&
// Check that the script tag is not inside a html comment tag
!(
preg_match('/.*(?|(<!--)|(-->))/U', $content, $commentTags, 0, $p1)
preg_match('/.*(?|(<!--)|(-->))/U', $content, $commentTags, 0, $scriptTagPosition)
&&
$commentTags[1] == '-->'
);
if($canWriteToBody) {
$content = substr($content,0,$p1) . $jsRequirements . substr($content,$p1);
// Insert content before existing script tags
$content = substr($content, 0, $scriptTagPosition)
. $jsRequirements
. substr($content, $scriptTagPosition);
} else {
$content = preg_replace("/(<\/body[^>]*>)/i", $jsRequirements . "\\1", $content);
// Insert content at bottom of page otherwise
$content = $this->insertScriptsAtBottom($jsRequirements, $content);
}
// 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);
}
return $content;
}
/**
* Given a block of HTML, insert the given code inside the <head></head> block
*
* @param string $jsRequirements String containing one or more html tags
* @param string $content HTML body
* @return string Merged HTML
*/
protected function insertTagsIntoHead($jsRequirements, $content) {
$content = preg_replace(
'/(<\/head>)/i',
$this->escapeReplacement($jsRequirements) . '\\1',
$content
);
return $content;
}
/**
* Safely escape a literal string for use in preg_replace replacement
*
* @param string $replacement
* @return string
*/
protected function escapeReplacement($replacement) {
return addcslashes($replacement, '\\$');
}
/**
* Attach requirements inclusion to X-Include-JS and X-Include-CSS headers on the given
* HTTP Response
@ -1239,7 +1302,7 @@ class Requirements_Backend
* requirements
* @param bool $langOnly Only include language files, not the base libraries
*
* @return array
* @return array|null All relative files if $return is true, or null otherwise
*/
public function add_i18n_javascript($langDir, $return = false, $langOnly = false) {
$files = array();
@ -1255,8 +1318,8 @@ class Requirements_Backend
$candidates = array(
'en.js',
'en_US.js',
i18n::get_lang_from_locale(i18n::default_locale()) . '.js',
i18n::default_locale() . '.js',
i18n::get_lang_from_locale(i18n::config()->default_locale) . '.js',
i18n::config()->default_locale . '.js',
i18n::get_lang_from_locale(i18n::get_locale()) . '.js',
i18n::get_locale() . '.js',
);
@ -1278,6 +1341,7 @@ class Requirements_Backend
foreach($files as $file) {
$this->javascript($file);
}
return null;
}
}
@ -1456,7 +1520,7 @@ class Requirements_Backend
/**
* Includes all combined files, including blocked ones
*
* @return type
* @return array
*/
protected function getAllCombinedFiles() {
return $this->combinedFiles;
@ -1611,8 +1675,8 @@ class Requirements_Backend
/**
* Given a filename and list of files, generate a new filename unique to these files
*
* @param string $name
* @param array $files
* @param string $combinedFile
* @param array $fileList
* @return string
*/
protected function hashedCombinedFilename($combinedFile, $fileList) {

View File

@ -1128,7 +1128,7 @@ class SSViewer implements Flushable {
$output = $this->includeGeneratedTemplate($cacheFile, $item, $arguments, $underlay, $inheritedScope);
if($this->includeRequirements) {
$output = Requirements::includeInHTML($template, $output);
$output = Requirements::includeInHTML($output);
}
array_pop(SSViewer::$topLevel);