From 7aeaf74c223611514f01a8db34c91f64b1f9c950 Mon Sep 17 00:00:00 2001 From: Jeremy Thomerson Date: Fri, 21 Jun 2013 02:16:07 +0000 Subject: [PATCH 1/9] MINOR: fix Email class modifying SSViewer.source_file_comments config val The Email class was updating the SSViewer.source_file_comments value and not resetting it. If someone had this value set to true in their system and then used the Email class, it seems there would be an unintended side-effect of having the source file comments turned off. --- email/Email.php | 4 +++- tests/view/SSViewerTest.php | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/email/Email.php b/email/Email.php index 714650bb9..6aa8c4811 100644 --- a/email/Email.php +++ b/email/Email.php @@ -349,8 +349,9 @@ class Email extends ViewableData { * and it won't be plain email :) */ protected function parseVariables($isPlain = false) { + $origState = Config::inst()->get('SSViewer', 'source_file_comments'); Config::inst()->update('SSViewer', 'source_file_comments', false); - + if(!$this->parseVariables_done) { $this->parseVariables_done = true; @@ -373,6 +374,7 @@ class Email extends ViewableData { // Rewrite relative URLs $this->body = HTTP::absoluteURLs($fullBody); } + Config::inst()->update('SSViewer', 'source_file_comments', $origState); } /** diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php index 92b181b28..4a10ff41e 100644 --- a/tests/view/SSViewerTest.php +++ b/tests/view/SSViewerTest.php @@ -1062,7 +1062,7 @@ after') $origEnv = Config::inst()->get('Director', 'environment_type'); Config::inst()->update('Director', 'environment_type', 'dev'); Config::inst()->update('SSViewer', 'source_file_comments', true); - $f = FRAMEWORK_PATH . '/tests/templates/SSViewerTestComments'; + $f = FRAMEWORK_PATH . '/tests/templates/SSViewerTestComments'; $templates = array( array( 'name' => 'SSViewerTestCommentsFullSource', From feb03f5443329252eb5d975ed925b7ab4f1b5970 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 28 Jun 2013 16:45:33 +1200 Subject: [PATCH 2/9] BUG Fixed issue where time value was being parsed incorrectly in some locales --- forms/TimeField.php | 35 +++++++++++++++++++++++++++++++---- tests/forms/TimeFieldTest.php | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/forms/TimeField.php b/forms/TimeField.php index 14fc9709e..173f34d79 100644 --- a/forms/TimeField.php +++ b/forms/TimeField.php @@ -76,6 +76,33 @@ class TimeField extends TextField { public function Type() { return 'time text'; } + + /** + * Parses a time into a Zend_Date object + * + * @param string $value Raw value + * @param string $format Format string to check against + * @param string $locale Optional locale to parse against + * @param boolean $exactMatch Flag indicating that the date must be in this + * exact format, and is unchanged after being parsed and written out + * + * @return Zend_Date Returns the Zend_Date, or null if not in the specified format + */ + protected function parseTime($value, $format, $locale = null, $exactMatch = false) { + // Check if the date is in the correct format + if(!Zend_Date::isDate($value, $format)) return null; + + // Parse the value + $valueObject = new Zend_Date($value, $format, $locale); + + // For exact matches, ensure the value preserves formatting after conversion + if($exactMatch && ($value !== $valueObject->get($format))) { + return null; + } else { + return $valueObject; + } + } + /** * Sets the internal value to ISO date format. @@ -83,6 +110,7 @@ class TimeField extends TextField { * @param String|Array $val */ public function setValue($val) { + // Fuzzy matching through strtotime() to support a wider range of times, // e.g. 11am. This means that validate() might not fire. // Note: Time formats are assumed to be less ambiguous than dates across locales. @@ -99,13 +127,12 @@ class TimeField extends TextField { $this->valueObj = null; } // load ISO time from database (usually through Form->loadDataForm()) - else if(Zend_Date::isDate($val, $this->getConfig('datavalueformat'))) { - $this->valueObj = new Zend_Date($val, $this->getConfig('datavalueformat')); + // Requires exact format to prevent false positives from locale specific times + else if($this->valueObj = $this->parseTime($val, $this->getConfig('datavalueformat'), null, true)) { $this->value = $this->valueObj->get($this->getConfig('timeformat')); } // Set in current locale (as string) - else if(Zend_Date::isDate($val, $this->getConfig('timeformat'), $this->locale)) { - $this->valueObj = new Zend_Date($val, $this->getConfig('timeformat'), $this->locale); + else if($this->valueObj = $this->parseTime($val, $this->getConfig('timeformat'), $this->locale)) { $this->value = $this->valueObj->get($this->getConfig('timeformat')); } // Fallback: Set incorrect value so validate() can pick it up diff --git a/tests/forms/TimeFieldTest.php b/tests/forms/TimeFieldTest.php index b8050a6d7..6bd2b1861 100644 --- a/tests/forms/TimeFieldTest.php +++ b/tests/forms/TimeFieldTest.php @@ -100,4 +100,36 @@ class TimeFieldTest extends SapphireTest { $field->setValue(''); $this->assertEquals($field->dataValue(), ''); } + + /** + * Test that AM/PM is preserved correctly in various situations + */ + public function testPreserveAMPM() { + + // Test with timeformat that includes hour + + // Check pm + $f = new TimeField('Time', 'Time'); + $f->setConfig('timeformat', 'h:mm:ss a'); + $f->setValue('3:59 pm'); + $this->assertEquals($f->dataValue(), '15:59:00'); + + // Check am + $f = new TimeField('Time', 'Time'); + $f->setConfig('timeformat', 'h:mm:ss a'); + $f->setValue('3:59 am'); + $this->assertEquals($f->dataValue(), '03:59:00'); + + // Check with ISO date/time + $f = new TimeField('Time', 'Time'); + $f->setConfig('timeformat', 'h:mm:ss a'); + $f->setValue('15:59:00'); + $this->assertEquals($f->dataValue(), '15:59:00'); + + // ISO am + $f = new TimeField('Time', 'Time'); + $f->setConfig('timeformat', 'h:mm:ss a'); + $f->setValue('03:59:00'); + $this->assertEquals($f->dataValue(), '03:59:00'); + } } From 041f5f51a54b38d04403ac49fb7b9eb5b87f1d04 Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Thu, 4 Jul 2013 15:57:35 +0100 Subject: [PATCH 3/9] FIX: UploadField action buttons aren't disabled when editing an item Toggle disabled classes/attributes based on form visibility instead of .toggle() Use jQuery.attr() simply because it looks nicer --- javascript/UploadField.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/javascript/UploadField.js b/javascript/UploadField.js index 1d6715732..a824deb8c 100644 --- a/javascript/UploadField.js +++ b/javascript/UploadField.js @@ -383,6 +383,7 @@ $( 'div.ss-upload:not(.disabled):not(.readonly) .ss-uploadfield-item-edit').entwine({ onclick: function(e) { var editform = this.closest('.ss-uploadfield-item').find('.ss-uploadfield-item-editform'); + var itemInfo = editform.prev('.ss-uploadfield-item-info'); var disabled; var iframe = editform.find('iframe'); @@ -406,8 +407,15 @@ disabled=this.find('ss-uploadfield-item-edit').siblings(); } editform.parent('.ss-uploadfield-item').removeClass('ui-state-warning'); - disabled.toggleClass('ui-state-disabled'); editform.toggleEditForm(); + + if (itemInfo.find('.toggle-details-icon').hasClass('opened')) { + disabled.addClass('ui-state-disabled'); + disabled.attr('disabled', 'disabled'); + } else { + disabled.removeClass('ui-state-disabled'); + disabled.removeAttr('disabled'); + } } e.preventDefault(); // Avoid a form submit return false; From 0aeb2293bbc78422d6d546cffc6635bd8800185a Mon Sep 17 00:00:00 2001 From: Cam Spiers Date: Mon, 1 Jul 2013 14:51:32 +1200 Subject: [PATCH 4/9] Allow module directories to be named with more valid characters ensuring that module names in fragment meta-data are correct. Unit tests for ConfigManifest reference path parsing --- core/manifest/ConfigManifest.php | 2 +- tests/core/manifest/ConfigManifestTest.php | 158 ++++++++++++++++++ .../mysite/_config/addyamlconfigfile.yml | 59 +++++++ 3 files changed, 218 insertions(+), 1 deletion(-) create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/addyamlconfigfile.yml diff --git a/core/manifest/ConfigManifest.php b/core/manifest/ConfigManifest.php index 1398518aa..7b58455c9 100644 --- a/core/manifest/ConfigManifest.php +++ b/core/manifest/ConfigManifest.php @@ -291,7 +291,7 @@ class SS_ConfigManifest { // For each, parse out into module/file#name, and set any missing to "*" $header[$order] = array(); foreach($orderparts as $part) { - preg_match('! (?P\*|\w+)? (\/ (?P\*|\w+))? (\# (?P\*|\w+))? !x', + preg_match('! (?P\*|[^\/#]+)? (\/ (?P\*|\w+))? (\# (?P\*|\w+))? !x', $part, $match); $header[$order][] = array( diff --git a/tests/core/manifest/ConfigManifestTest.php b/tests/core/manifest/ConfigManifestTest.php index 27a2da60f..5f0895ef0 100644 --- a/tests/core/manifest/ConfigManifestTest.php +++ b/tests/core/manifest/ConfigManifestTest.php @@ -13,6 +13,164 @@ class ConfigManifestTest extends SapphireTest { return $manifest->get('ConfigManifestTest', $name); } + /** + * This is a helper method for displaying a relevant message about a parsing failure + */ + protected function getParsedAsMessage($path) { + return sprintf('Reference path "%s" failed to parse correctly', $path); + } + + /** + * This test checks the processing of before and after reference paths (module-name/filename#fragment) + * This method uses fixture/configmanifest/mysite/_config/addyamlconfigfile.yml as a fixture + */ + public function testAddYAMLConfigFileReferencePathParsing() { + // Use a mock to avoid testing unrelated functionality + $manifest = $this->getMockBuilder('SS_ConfigManifest') + ->disableOriginalConstructor() + ->setMethods(array('addModule')) + ->getMock(); + + // This tests that the addModule method is called with the correct value + $manifest->expects($this->once()) + ->method('addModule') + ->with($this->equalTo(dirname(__FILE__).'/fixtures/configmanifest/mysite')); + + // Call the method to be tested + $manifest->addYAMLConfigFile( + 'addyamlconfigfile.yml', + dirname(__FILE__).'/fixtures/configmanifest/mysite/_config/addyamlconfigfile.yml', + false + ); + + // There is no getter for yamlConfigFragments + $property = new ReflectionProperty('SS_ConfigManifest', 'yamlConfigFragments'); + $property->setAccessible(true); + + // Get the result back from the parsing + $result = $property->getValue($manifest); + + $this->assertEquals( + array( + array( + 'module' => 'mysite', + 'file' => 'testfile', + 'name' => 'fragment', + ), + ), + @$result[0]['after'], + $this->getParsedAsMessage('mysite/testfile#fragment') + ); + + $this->assertEquals( + array( + array( + 'module' => 'test-module', + 'file' => 'testfile', + 'name' => 'fragment', + ), + ), + @$result[1]['after'], + $this->getParsedAsMessage('test-module/testfile#fragment') + ); + + $this->assertEquals( + array( + array( + 'module' => '*', + 'file' => '*', + 'name' => '*', + ), + ), + @$result[2]['after'], + $this->getParsedAsMessage('*') + ); + + $this->assertEquals( + array( + array( + 'module' => '*', + 'file' => 'testfile', + 'name' => '*' + ), + ), + @$result[3]['after'], + $this->getParsedAsMessage('*/testfile') + ); + + $this->assertEquals( + array( + array( + 'module' => '*', + 'file' => '*', + 'name' => 'fragment' + ), + ), + @$result[4]['after'], + $this->getParsedAsMessage('*/*#fragment') + ); + + $this->assertEquals( + array( + array( + 'module' => '*', + 'file' => '*', + 'name' => 'fragment' + ), + ), + @$result[5]['after'], + $this->getParsedAsMessage('#fragment') + ); + + $this->assertEquals( + array( + array( + 'module' => 'test-module', + 'file' => '*', + 'name' => 'fragment' + ), + ), + @$result[6]['after'], + $this->getParsedAsMessage('test-module#fragment') + ); + + $this->assertEquals( + array( + array( + 'module' => 'test-module', + 'file' => '*', + 'name' => '*' + ), + ), + @$result[7]['after'], + $this->getParsedAsMessage('test-module') + ); + + $this->assertEquals( + array( + array( + 'module' => 'test-module', + 'file' => '*', + 'name' => '*' + ), + ), + @$result[8]['after'], + $this->getParsedAsMessage('test-module/*') + ); + + $this->assertEquals( + array( + array( + 'module' => 'test-module', + 'file' => '*', + 'name' => '*' + ), + ), + @$result[9]['after'], + $this->getParsedAsMessage('test-module/*/#*') + ); + } + public function testClassRules() { $config = $this->getConfigFixtureValue('Class'); diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/addyamlconfigfile.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/addyamlconfigfile.yml new file mode 100644 index 000000000..936a28d6d --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/addyamlconfigfile.yml @@ -0,0 +1,59 @@ +--- +After: 'mysite/testfile#fragment' +--- +ClassManifestTest: + testParam: false + +--- +After: 'test-module/testfile#fragment' +--- +ClassManifestTest: + testParam: false + +--- +After: '*' +--- +ClassManifestTest: + testParam: false + +--- +After: '*/testfile' +--- +ClassManifestTest: + testParam: false + +--- +After: '*/*#fragment' +--- +ClassManifestTest: + testParam: false + +--- +After: '#fragment' +--- +ClassManifestTest: + testParam: false + +--- +After: 'test-module#fragment' +--- +ClassManifestTest: + testParam: false + +--- +After: 'test-module' +--- +ClassManifestTest: + testParam: false + +--- +After: 'test-module/*' +--- +ClassManifestTest: + testParam: false + +--- +After: 'test-module/*#*' +--- +ClassManifestTest: + testParam: false \ No newline at end of file From c6b4d993cc8b771318c8dc1d522919dd1eb6447f Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Fri, 5 Jul 2013 16:03:51 +1200 Subject: [PATCH 5/9] FIX Director::forceSSL and forceWWW not setting Vary header If you have a Varnish box in front of a SilverStripe install, and you call forceSSL, the Vary header wouldnt get sent. As a result Varnish would respond with the same redirect reponse after the redirect, leading to an infinite loop --- control/Director.php | 29 ++++++++++++++++++++++------- control/HTTP.php | 6 +++--- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/control/Director.php b/control/Director.php index a3686d279..c3029a209 100644 --- a/control/Director.php +++ b/control/Director.php @@ -714,6 +714,26 @@ class Director implements TemplateGlobalProvider { return Director::protocol() . $login . $_SERVER['HTTP_HOST'] . Director::baseURL(); } + /** + * Skip any further processing and immediately respond with a redirect to the passed URL. + * + * @param string $destURL - The URL to redirect to + */ + protected static function force_redirect($destURL) { + $response = new SS_HTTPResponse( + "

Your browser is not accepting header redirects

". + "

Please click here", + 301 + ); + + HTTP::add_cache_headers($response); + $response->addHeader('Location', $destURL); + + // TODO: Use an exception - ATM we can be called from _config.php, before Director#handleRequest's try block + $response->output(); + die; + } + /** * Force the site to run on SSL. * @@ -782,10 +802,7 @@ class Director implements TemplateGlobalProvider { if(class_exists('SapphireTest', false) && SapphireTest::is_running_test()) { return $destURL; } else { - if(!headers_sent()) header("Location: $destURL"); - - die("

Your browser is not accepting header redirects

" - . "

Please click here"); + self::force_redirect($destURL); } } else { return false; @@ -800,9 +817,7 @@ class Director implements TemplateGlobalProvider { $destURL = str_replace(Director::protocol(), Director::protocol() . 'www.', Director::absoluteURL($_SERVER['REQUEST_URI'])); - header("Location: $destURL", true, 301); - die("

Your browser is not accepting header redirects

" - . "

Please click here"); + self::force_redirect($destURL); } } diff --git a/control/HTTP.php b/control/HTTP.php index 03f7cdbcb..b1f154b9b 100644 --- a/control/HTTP.php +++ b/control/HTTP.php @@ -338,11 +338,11 @@ class HTTP { $responseHeaders["Cache-Control"] = "max-age=" . self::$cache_age . ", must-revalidate, no-transform"; $responseHeaders["Pragma"] = ""; - // To do: User-Agent should only be added in situations where you *are* actually + // To do: User-Agent should only be added in situations where you *are* actually // varying according to user-agent. $responseHeaders['Vary'] = 'Cookie, X-Forwarded-Protocol, User-Agent, Accept'; - - } else { + } + else { $responseHeaders["Cache-Control"] = "no-cache, max-age=0, must-revalidate, no-transform"; } From 2886f6ee14f48bc814a8c020835ad14d56a5f107 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Sat, 6 Jul 2013 15:15:49 +1200 Subject: [PATCH 6/9] FIX Session was started every time, even if no data set Session tracks the user agent in the session, to add some detection of stolen session IDs. However this was causing a session to always be created, even if this request didnt store any data in the session. --- control/Session.php | 25 +++++++++++++++---------- tests/control/SessionTest.php | 1 + 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/control/Session.php b/control/Session.php index 8c24d3ba5..7373a5c9f 100644 --- a/control/Session.php +++ b/control/Session.php @@ -128,6 +128,14 @@ class Session { protected $changedData = array(); + protected function userAgent() { + if (isset($_SERVER['HTTP_USER_AGENT'])) { + return $_SERVER['HTTP_USER_AGENT']; + } else { + return ''; + } + } + /** * Start PHP session, then create a new Session object with the given start data. * @@ -138,14 +146,8 @@ class Session { $this->data = $data; - if (isset($_SERVER['HTTP_USER_AGENT'])) { - $ua = $_SERVER['HTTP_USER_AGENT']; - } else { - $ua = ''; - } - if (isset($this->data['HTTP_USER_AGENT'])) { - if ($this->data['HTTP_USER_AGENT'] != $ua) { + if ($this->data['HTTP_USER_AGENT'] != $this->userAgent()) { // Funny business detected! $this->inst_clearAll(); @@ -153,8 +155,6 @@ class Session { Session::start(); } } - - $this->inst_set('HTTP_USER_AGENT', $ua); } /** @@ -460,13 +460,18 @@ class Session { public function inst_getAll() { return $this->data; } - + + public function inst_finalize() { + $this->inst_set('HTTP_USER_AGENT', $this->userAgent()); + } + /** * Save data to session * Only save the changes, so that anyone manipulating $_SESSION directly doesn't get burned. */ public function inst_save() { if($this->changedData) { + $this->inst_finalize(); if(!isset($_SESSION)) Session::start(); $this->recursivelyApply($this->changedData, $_SESSION); } diff --git a/tests/control/SessionTest.php b/tests/control/SessionTest.php index aa7e317c7..49f7a427b 100644 --- a/tests/control/SessionTest.php +++ b/tests/control/SessionTest.php @@ -99,6 +99,7 @@ class SessionTest extends SapphireTest { // Generate our session $s = new Session(array()); $s->inst_set('val', 123); + $s->inst_finalize(); // Change our UA $_SERVER['HTTP_USER_AGENT'] = 'Fake Agent'; From d629d9422fecb858fca53cbbb51ed0ef3d8cfa36 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Sat, 6 Jul 2013 15:10:43 +1200 Subject: [PATCH 7/9] FIX Session::$cookie_secure so Sessions still work via HTTP Session::$cookie_secure adds the secure property to the session Set-Cookie command, so that the browser wouldnt send it to the server over an unencrypted link. However the server would still send the cookie to the browser unencrypted. Also Sessions would stop working properly in HTTP, but SilverStripe needs them for several things, such as form validation This patch effectively causes HTTP and HTTPS requests to each have their own session when cookie_secure is true. The two sessions are independant from each other, so information set in the session via HTTPS is safe from attacks on the session via HTTP, but parts of the site that use HTTP and the session will still work --- control/Director.php | 2 +- control/Session.php | 17 +++++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/control/Director.php b/control/Director.php index a3686d279..8597e09db 100644 --- a/control/Director.php +++ b/control/Director.php @@ -127,7 +127,7 @@ class Director implements TemplateGlobalProvider { } // Only resume a session if its not started already, and a session identifier exists - if(!isset($_SESSION) && (isset($_COOKIE[session_name()]) || isset($_REQUEST[session_name()]))) { + if(!isset($_SESSION) && Session::request_contains_session_id()) { Session::start(); } // Initiate an empty session - doesn't initialize an actual PHP session until saved (see belwo) diff --git a/control/Session.php b/control/Session.php index 7373a5c9f..a4aaf0b89 100644 --- a/control/Session.php +++ b/control/Session.php @@ -513,6 +513,16 @@ class Session { Session::set("FormInfo.$formname.formError.type", $type); } + /** + * Is there a session ID in the request? + * @return bool + */ + public static function request_contains_session_id() { + $secure = Director::is_https() && Config::inst()->get('Session', 'cookie_secure'); + $name = $secure ? 'SECSESSID' : session_name(); + return isset($_COOKIE[$name]) || isset($_REQUEST[$name]); + } + /** * Initialize session. * @@ -522,7 +532,7 @@ class Session { $path = Config::inst()->get('Session', 'cookie_path'); if(!$path) $path = Director::baseURL(); $domain = Config::inst()->get('Session', 'cookie_domain'); - $secure = Config::inst()->get('Session', 'cookie_secure'); + $secure = Director::is_https() && Config::inst()->get('Session', 'cookie_secure'); $session_path = Config::inst()->get('Session', 'session_store_path'); $timeout = Config::inst()->get('Session', 'timeout'); @@ -538,11 +548,14 @@ class Session { // Allow storing the session in a non standard location if($session_path) session_save_path($session_path); + // If we want a secure cookie for HTTPS, use a seperate session name. This lets us have a + // seperate (less secure) session for non-HTTPS requests + if($secure) session_name('SECSESSID'); + // @ is to supress win32 warnings/notices when session wasn't cleaned up properly // There's nothing we can do about this, because it's an operating system function! if($sid) session_id($sid); @session_start(); - } // Modify the timeout behaviour so it's the *inactive* time before the session expires. From f6ff39369fb1646d95fd66bbd45dca10c9f18e65 Mon Sep 17 00:00:00 2001 From: Jeremy Thomerson Date: Wed, 26 Jun 2013 12:57:53 +0000 Subject: [PATCH 8/9] FEATURE: <% include %> inherits iterator scope of parent template --- .../SSViewerTestIncludeScopeInheritance.ss | 3 ++ ...iewerTestIncludeScopeInheritanceInclude.ss | 1 + ...ewerTestIncludeScopeInheritanceWithArgs.ss | 3 ++ tests/view/SSViewerTest.php | 52 ++++++++++++++++++- view/SSTemplateParser.php | 2 +- view/SSTemplateParser.php.inc | 2 +- view/SSViewer.php | 36 ++++++++----- 7 files changed, 82 insertions(+), 17 deletions(-) create mode 100644 tests/templates/SSViewerTestIncludeScopeInheritance.ss create mode 100644 tests/templates/SSViewerTestIncludeScopeInheritanceInclude.ss create mode 100644 tests/templates/SSViewerTestIncludeScopeInheritanceWithArgs.ss diff --git a/tests/templates/SSViewerTestIncludeScopeInheritance.ss b/tests/templates/SSViewerTestIncludeScopeInheritance.ss new file mode 100644 index 000000000..6c106c703 --- /dev/null +++ b/tests/templates/SSViewerTestIncludeScopeInheritance.ss @@ -0,0 +1,3 @@ +<% loop Items %> + <% include SSViewerTestIncludeScopeInheritanceInclude %> +<% end_loop %> diff --git a/tests/templates/SSViewerTestIncludeScopeInheritanceInclude.ss b/tests/templates/SSViewerTestIncludeScopeInheritanceInclude.ss new file mode 100644 index 000000000..51b2dc190 --- /dev/null +++ b/tests/templates/SSViewerTestIncludeScopeInheritanceInclude.ss @@ -0,0 +1 @@ +$Title <% if ArgA %>- $ArgA <% end_if %>- <%if First %>First-<% end_if %><% if Last %>Last-<% end_if %><%if MultipleOf(2) %>EVEN<% else %>ODD<% end_if %> top:$Top.Title diff --git a/tests/templates/SSViewerTestIncludeScopeInheritanceWithArgs.ss b/tests/templates/SSViewerTestIncludeScopeInheritanceWithArgs.ss new file mode 100644 index 000000000..d897bf044 --- /dev/null +++ b/tests/templates/SSViewerTestIncludeScopeInheritanceWithArgs.ss @@ -0,0 +1,3 @@ +<% loop Items %> + <% include SSViewerTestIncludeScopeInheritanceInclude ArgA=$Title %> +<% end_loop %> diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php index 5e7589165..187bf0504 100644 --- a/tests/view/SSViewerTest.php +++ b/tests/view/SSViewerTest.php @@ -29,7 +29,57 @@ class SSViewerTest extends SapphireTest { $result = $data->renderWith("SSViewerTestPartialTemplate"); $this->assertEquals('Test partial template: var value', trim(preg_replace("//U",'',$result))); } - + + public function testIncludeScopeInheritance() { + $data = $this->getScopeInheritanceTestData(); + $expected = array( + 'Item 1 - First-ODD top:Item 1', + 'Item 2 - EVEN top:Item 2', + 'Item 3 - ODD top:Item 3', + 'Item 4 - EVEN top:Item 4', + 'Item 5 - ODD top:Item 5', + 'Item 6 - Last-EVEN top:Item 6', + ); + + $result = $data->renderWith('SSViewerTestIncludeScopeInheritance'); + $this->assertExpectedStrings($result, $expected); + + // reset results for the tests that include arguments (the title is passed as an arg) + $expected = array( + 'Item 1 - Item 1 - First-ODD top:Item 1', + 'Item 2 - Item 2 - EVEN top:Item 2', + 'Item 3 - Item 3 - ODD top:Item 3', + 'Item 4 - Item 4 - EVEN top:Item 4', + 'Item 5 - Item 5 - ODD top:Item 5', + 'Item 6 - Item 6 - Last-EVEN top:Item 6', + ); + + $result = $data->renderWith('SSViewerTestIncludeScopeInheritanceWithArgs'); + $this->assertExpectedStrings($result, $expected); + } + + private function getScopeInheritanceTestData() { + return new ArrayData(array( + 'Title' => 'TopTitleValue', + 'Items' => new ArrayList(array( + new ArrayData(array('Title' => 'Item 1')), + new ArrayData(array('Title' => 'Item 2')), + new ArrayData(array('Title' => 'Item 3')), + new ArrayData(array('Title' => 'Item 4')), + new ArrayData(array('Title' => 'Item 5')), + new ArrayData(array('Title' => 'Item 6')) + )) + )); + } + + private function assertExpectedStrings($result, $expected) { + foreach ($expected as $expectedStr) { + $this->assertTrue( + (boolean) preg_match("/{$expectedStr}/", $result), + "Didn't find '{$expectedStr}' in:\n{$result}" + ); + } + } /** * Small helper to render templates from strings diff --git a/view/SSTemplateParser.php b/view/SSTemplateParser.php index efe7e27ea..f516aa6b0 100644 --- a/view/SSTemplateParser.php +++ b/view/SSTemplateParser.php @@ -3242,7 +3242,7 @@ class SSTemplateParser extends Parser { $arguments = $res['arguments']; $res['php'] = '$val .= SSViewer::execute_template('.$template.', $scope->getItem(), array(' . - implode(',', $arguments)."));\n"; + implode(',', $arguments)."), \$scope);\n"; if($this->includeDebuggingComments) { // Add include filename comments on dev sites $res['php'] = diff --git a/view/SSTemplateParser.php.inc b/view/SSTemplateParser.php.inc index 3c1349efe..7b789b2af 100644 --- a/view/SSTemplateParser.php.inc +++ b/view/SSTemplateParser.php.inc @@ -701,7 +701,7 @@ class SSTemplateParser extends Parser { $arguments = $res['arguments']; $res['php'] = '$val .= SSViewer::execute_template('.$template.', $scope->getItem(), array(' . - implode(',', $arguments)."));\n"; + implode(',', $arguments)."), \$scope);\n"; if($this->includeDebuggingComments) { // Add include filename comments on dev sites $res['php'] = diff --git a/view/SSViewer.php b/view/SSViewer.php index 40df8f510..f11a335ff 100644 --- a/view/SSViewer.php +++ b/view/SSViewer.php @@ -47,11 +47,17 @@ class SSViewer_Scope { private $localIndex; - public function __construct($item){ + public function __construct($item, $inheritedScope = null) { $this->item = $item; $this->localIndex = 0; $this->localStack = array(); - $this->itemStack[] = array($this->item, null, 0, null, null, 0); + if ($inheritedScope instanceof SSViewer_Scope) { + $this->itemIterator = $inheritedScope->itemIterator; + $this->itemIteratorTotal = $inheritedScope->itemIteratorTotal; + $this->itemStack[] = array($this->item, $this->itemIterator, $this->itemIteratorTotal, null, null, 0); + } else { + $this->itemStack[] = array($this->item, null, 0, null, null, 0); + } } public function getItem(){ @@ -357,8 +363,8 @@ class SSViewer_DataPresenter extends SSViewer_Scope { */ protected $underlay; - public function __construct($item, $overlay = null, $underlay = null){ - parent::__construct($item); + public function __construct($item, $overlay = null, $underlay = null, $inheritedScope = null) { + parent::__construct($item, $inheritedScope); // Build up global property providers array only once per request if (self::$globalProperties === null) { @@ -553,7 +559,7 @@ class SSViewer { * @var boolean $source_file_comments */ private static $source_file_comments = false; - + /** * Set whether HTML comments indicating the source .SS file used to render this page should be * included in the output. This is enabled by default @@ -895,10 +901,11 @@ class SSViewer { * @param Object $item - The item to use as the root scope for the template * @param array|null $overlay - Any variables to layer on top of the scope * @param array|null $underlay - Any variables to layer underneath the scope + * @param Object $inheritedScope - the current scope of a parent template including a sub-template * * @return string - The result of executing the template */ - protected function includeGeneratedTemplate($cacheFile, $item, $overlay, $underlay) { + protected function includeGeneratedTemplate($cacheFile, $item, $overlay, $underlay, $inheritedScope = null) { if(isset($_GET['showtemplate']) && $_GET['showtemplate'] && Permission::check('ADMIN')) { $lines = file($cacheFile); echo "

Template: $cacheFile

"; @@ -910,7 +917,7 @@ class SSViewer { } $cache = $this->getPartialCacheStore(); - $scope = new SSViewer_DataPresenter($item, $overlay, $underlay); + $scope = new SSViewer_DataPresenter($item, $overlay, $underlay, $inheritedScope); $val = ''; include($cacheFile); @@ -930,11 +937,12 @@ class SSViewer { * Note: You can call this method indirectly by {@link ViewableData->renderWith()}. * * @param ViewableData $item - * @param SS_Cache $cache Optional cache backend. + * @param array|null $arguments - arguments to an included template + * @param Object $inheritedScope - the current scope of a parent template including a sub-template * * @return HTMLText Parsed template output. */ - public function process($item, $arguments = null) { + public function process($item, $arguments = null, $inheritedScope = null) { SSViewer::$topLevel[] = $item; if ($arguments && $arguments instanceof Zend_Cache_Core) { @@ -979,7 +987,7 @@ class SSViewer { } } - $output = $this->includeGeneratedTemplate($cacheFile, $item, $arguments, $underlay); + $output = $this->includeGeneratedTemplate($cacheFile, $item, $arguments, $underlay, $inheritedScope); if($this->includeRequirements) { $output = Requirements::includeInHTML($template, $output); @@ -1009,11 +1017,11 @@ class SSViewer { * Execute the given template, passing it the given data. * Used by the <% include %> template tag to process templates. */ - public static function execute_template($template, $data, $arguments = null) { + public static function execute_template($template, $data, $arguments = null, $scope = null) { $v = new SSViewer($template); $v->includeRequirements(false); - return $v->process($data, $arguments); + return $v->process($data, $arguments, $scope); } public static function parseTemplateContent($content, $template="") { @@ -1071,7 +1079,7 @@ class SSViewer_FromString extends SSViewer { $this->content = $content; } - public function process($item, $arguments = null) { + public function process($item, $arguments = null, $scope = null) { if ($arguments && $arguments instanceof Zend_Cache_Core) { Deprecation::notice('3.0', 'Use setPartialCacheStore to override the partial cache storage backend, ' . 'the second argument to process is now an array of variables.'); @@ -1086,7 +1094,7 @@ class SSViewer_FromString extends SSViewer { fwrite($fh, $template); fclose($fh); - $val = $this->includeGeneratedTemplate($tmpFile, $item, $arguments, null); + $val = $this->includeGeneratedTemplate($tmpFile, $item, $arguments, null, $scope); unlink($tmpFile); return $val; From 2d30592f72408508e74ae649adce875ad6994036 Mon Sep 17 00:00:00 2001 From: Cam Spiers Date: Mon, 8 Jul 2013 20:41:46 +1200 Subject: [PATCH 9/9] Improve memory performance when generating config static and class caches --- core/manifest/ClassManifest.php | 13 ++++++++++++- core/manifest/ConfigStaticManifest.php | 2 +- tests/core/manifest/ClassManifestTest.php | 13 ++++++++----- .../manifest/NamespacedClassManifestTest.php | 19 +++++++++++-------- 4 files changed, 32 insertions(+), 15 deletions(-) diff --git a/core/manifest/ClassManifest.php b/core/manifest/ClassManifest.php index ded2f4839..042871f21 100644 --- a/core/manifest/ClassManifest.php +++ b/core/manifest/ClassManifest.php @@ -273,6 +273,15 @@ class SS_ClassManifest { return $modules; } + /** + * Used to set up files that we want to exclude from parsing for performance reasons. + */ + protected function setDefaults() + { + $this->classes['sstemplateparser'] = FRAMEWORK_PATH.'/view/SSTemplateParser.php'; + $this->classes['sstemplateparseexception'] = FRAMEWORK_PATH.'/view/SSTemplateParser.php'; + } + /** * Completely regenerates the manifest file. * @@ -289,10 +298,12 @@ class SS_ClassManifest { $this->$reset = array(); } + $this->setDefaults(); + $finder = new ManifestFileFinder(); $finder->setOptions(array( 'name_regex' => '/^(_config.php|[^_].*\.php)$/', - 'ignore_files' => array('index.php', 'main.php', 'cli-script.php'), + 'ignore_files' => array('index.php', 'main.php', 'cli-script.php', 'SSTemplateParser.php'), 'ignore_tests' => !$this->tests, 'file_callback' => array($this, 'handleFile'), 'dir_callback' => array($this, 'handleDir') diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php index 5dddd9266..4f3776816 100644 --- a/core/manifest/ConfigStaticManifest.php +++ b/core/manifest/ConfigStaticManifest.php @@ -100,7 +100,7 @@ class SS_ConfigStaticManifest { $finder = new ManifestFileFinder(); $finder->setOptions(array( 'name_regex' => '/^([^_].*\.php)$/', - 'ignore_files' => array('index.php', 'main.php', 'cli-script.php'), + 'ignore_files' => array('index.php', 'main.php', 'cli-script.php', 'SSTemplateParser.php'), 'ignore_tests' => !$this->tests, 'file_callback' => array($this, 'handleFile') )); diff --git a/tests/core/manifest/ClassManifestTest.php b/tests/core/manifest/ClassManifestTest.php index 64fd0a1a4..fa82e8d12 100644 --- a/tests/core/manifest/ClassManifestTest.php +++ b/tests/core/manifest/ClassManifestTest.php @@ -36,17 +36,20 @@ class ClassManifestTest extends SapphireTest { public function testGetClasses() { $expect = array( - 'classa' => "{$this->base}/module/classes/ClassA.php", - 'classb' => "{$this->base}/module/classes/ClassB.php", - 'classc' => "{$this->base}/module/classes/ClassC.php", - 'classd' => "{$this->base}/module/classes/ClassD.php" + 'classb' => "{$this->base}/module/classes/ClassB.php", + 'classa' => "{$this->base}/module/classes/ClassA.php", + 'classb' => "{$this->base}/module/classes/ClassB.php", + 'classc' => "{$this->base}/module/classes/ClassC.php", + 'classd' => "{$this->base}/module/classes/ClassD.php", + 'sstemplateparser' => FRAMEWORK_PATH."/view/SSTemplateParser.php", + 'sstemplateparseexception' => FRAMEWORK_PATH."/view/SSTemplateParser.php" ); $this->assertEquals($expect, $this->manifest->getClasses()); } public function testGetClassNames() { $this->assertEquals( - array('classa', 'classb', 'classc', 'classd'), + array('sstemplateparser', 'sstemplateparseexception', 'classa', 'classb', 'classc', 'classd'), $this->manifest->getClassNames()); } diff --git a/tests/core/manifest/NamespacedClassManifestTest.php b/tests/core/manifest/NamespacedClassManifestTest.php index 7cc2df39d..c87dea4e4 100644 --- a/tests/core/manifest/NamespacedClassManifestTest.php +++ b/tests/core/manifest/NamespacedClassManifestTest.php @@ -12,7 +12,7 @@ class NamespacedClassManifestTest extends SapphireTest { public function setUp() { parent::setUp(); - + $this->base = dirname(__FILE__) . '/fixtures/namespaced_classmanifest'; $this->manifest = new SS_ClassManifest($this->base, false, true, false); } @@ -41,17 +41,20 @@ class NamespacedClassManifestTest extends SapphireTest { 'silverstripe\test\classe' => "{$this->base}/module/classes/ClassE.php", 'silverstripe\test\classf' => "{$this->base}/module/classes/ClassF.php", 'silverstripe\test\classg' => "{$this->base}/module/classes/ClassG.php", - 'silverstripe\test\classh' => "{$this->base}/module/classes/ClassH.php" + 'silverstripe\test\classh' => "{$this->base}/module/classes/ClassH.php", + 'sstemplateparser' => FRAMEWORK_PATH."/view/SSTemplateParser.php", + 'sstemplateparseexception' => FRAMEWORK_PATH."/view/SSTemplateParser.php" ); - + $this->assertEquals($expect, $this->manifest->getClasses()); } public function testGetClassNames() { $this->assertEquals( - array('silverstripe\test\classa', 'silverstripe\test\classb', 'silverstripe\test\classc', - 'silverstripe\test\classd', 'silverstripe\test\classe', 'silverstripe\test\classf', - 'silverstripe\test\classg', 'silverstripe\test\classh'), + array('sstemplateparser', 'sstemplateparseexception', 'silverstripe\test\classa', + 'silverstripe\test\classb', 'silverstripe\test\classc', 'silverstripe\test\classd', + 'silverstripe\test\classe', 'silverstripe\test\classf', 'silverstripe\test\classg', + 'silverstripe\test\classh'), $this->manifest->getClassNames()); } @@ -59,7 +62,7 @@ class NamespacedClassManifestTest extends SapphireTest { $expect = array( 'silverstripe\test\classa' => array('silverstripe\test\ClassB', 'silverstripe\test\ClassH'), ); - + $this->assertEquals($expect, $this->manifest->getDescendants()); } @@ -109,7 +112,7 @@ class NamespacedClassManifestTest extends SapphireTest { $expect = array("{$this->base}/module/_config.php"); $this->assertEquals($expect, $this->manifest->getConfigs()); } - + public function testGetModules() { $expect = array( "module" => "{$this->base}/module",