From f63d137d499b3556e51111a94ea5bd2de5ff4258 Mon Sep 17 00:00:00 2001 From: Sean Harvey Date: Thu, 26 Apr 2012 16:43:58 +1200 Subject: [PATCH] ENHANCEMENT Session::start() now only called when there is changed session data to be saved, and started on Director::direct() when there is a cookie (or request var) containing the current PHP session name. --- control/Director.php | 2 ++ control/Session.php | 65 +++++++++++++++++++++++------------ main.php | 4 +-- model/Versioned.php | 12 +++++-- security/SecurityToken.php | 13 ++++--- tests/control/SessionTest.php | 24 +++++++++++++ 6 files changed, 89 insertions(+), 31 deletions(-) diff --git a/control/Director.php b/control/Director.php index 14f3aa104..2b8350989 100644 --- a/control/Director.php +++ b/control/Director.php @@ -94,6 +94,8 @@ class Director implements TemplateGlobalProvider { } // Load the session into the controller + if(!isset($_SESSION) && (isset($_COOKIE[session_name()]) || isset($_REQUEST[session_name()]))) Session::start(); + $session = new Session(isset($_SESSION) ? $_SESSION : null); $result = Director::handleRequest($req, $session, $model); diff --git a/control/Session.php b/control/Session.php index 1df478e3f..a4f13b55c 100644 --- a/control/Session.php +++ b/control/Session.php @@ -102,6 +102,17 @@ class Session { protected $changedData = array(); + /** + * Start PHP session, then create a new Session object with the given start data. + * + * @param $data Can be an array of data (such as $_SESSION) or another Session object to clone. + */ + function __construct($data) { + if($data instanceof Session) $data = $data->inst_getAll(); + + $this->data = $data; + } + /** * Cookie domain, for example 'www.php.net'. * @@ -176,17 +187,6 @@ class Session { return self::$session_store_path; } - /** - * Create a new session object, with the given starting data - * - * @param $data Can be an array of data (such as $_SESSION) or another Session object to clone. - */ - function __construct($data) { - if($data instanceof Session) $data = $data->inst_getAll(); - - $this->data = $data; - } - /** * Provide an array of rules specifing timeouts for IPv4 address ranges or * individual IPv4 addresses. The key is an IP address or range and the value is the time @@ -300,12 +300,14 @@ class Session { $var = &$var[$n]; $diffVar = &$diffVar[$n]; } - - $var = $val; - $diffVar = $val; + + if($var !== $val) { + $var = $val; + $diffVar = $val; + } } } - + public function inst_addToArray($name, $val) { $names = explode('.', $name); @@ -355,14 +357,22 @@ class Session { $diffVar = &$this->changedData; foreach($names as $n) { + // don't clear a record that doesn't exist + if(!isset($var[$n])) return; $var = &$var[$n]; + } + + // only loop to find data within diffVar if var is proven to exist in the above loop + foreach($names as $n) { $diffVar = &$diffVar[$n]; } - $var = null; - $diffVar = null; + if($var !== null) { + $var = null; + $diffVar = null; + } } - + public function inst_clearAll() { if($this->data && is_array($this->data)) { foreach(array_keys($this->data) as $key) { @@ -370,7 +380,7 @@ class Session { } } } - + public function inst_getAll() { return $this->data; } @@ -380,7 +390,10 @@ class Session { * Only save the changes, so that anyone manipulating $_SESSION directly doesn't get burned. */ public function inst_save() { - $this->recursivelyApply($this->changedData, $_SESSION); + if($this->changedData) { + if(!isset($_SESSION)) Session::start(); + $this->recursivelyApply($this->changedData, $_SESSION); + } } /** @@ -397,7 +410,15 @@ class Session { } } } - + + /** + * Return the changed data, for debugging purposes. + * @return array + */ + public function inst_changedData() { + return $this->changedData; + } + /** * Sets the appropriate form message in session, with type. This will be shown once, * for the form specified. @@ -431,7 +452,7 @@ class Session { // Allow storing the session in a non standard location if($session_path) session_save_path($session_path); - + // @ 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); diff --git a/main.php b/main.php index 70651e19c..1391b81cd 100644 --- a/main.php +++ b/main.php @@ -59,9 +59,7 @@ if (version_compare(phpversion(), '5.3.2', '<')) { /** * Include SilverStripe's core code */ -require_once("core/Core.php"); - -Session::start(); +require_once('core/Core.php'); // IIS will sometimes generate this. if(!empty($_SERVER['HTTP_X_ORIGINAL_URL'])) { diff --git a/model/Versioned.php b/model/Versioned.php index 53fb299de..03736a069 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -791,9 +791,17 @@ class Versioned extends DataExtension { if(!headers_sent() && !Director::is_cli()) { if(Versioned::current_stage() == 'Live') { - Cookie::set('bypassStaticCache', null, 0, null, null, false, true /* httponly */); + // clear the cookie if it's set + if(!empty($_COOKIE['bypassStaticCache'])) { + Cookie::set('bypassStaticCache', null, 0, null, null, false, true /* httponly */); + unset($_COOKIE['bypassStaticCache']); + } } else { - Cookie::set('bypassStaticCache', '1', 0, null, null, false, true /* httponly */); + // set the cookie if it's cleared + if(empty($_COOKIE['bypassStaticCache'])) { + Cookie::set('bypassStaticCache', '1', 0, null, null, false, true /* httponly */); + $_COOKIE['bypassStaticCache'] = 1; + } } } } diff --git a/security/SecurityToken.php b/security/SecurityToken.php index 259d8507c..d3afbbf55 100644 --- a/security/SecurityToken.php +++ b/security/SecurityToken.php @@ -55,9 +55,6 @@ class SecurityToken extends Object implements TemplateGlobalProvider { */ function __construct($name = null) { $this->name = ($name) ? $name : self::get_default_name(); - // only regenerate if the token isn't already set in the session - if(!$this->getValue()) $this->setValue($this->generate()); - parent::__construct(); } @@ -132,7 +129,15 @@ class SecurityToken extends Object implements TemplateGlobalProvider { * @return String */ function getValue() { - return Session::get($this->getName()); + $value = Session::get($this->getName()); + + // only regenerate if the token isn't already set in the session + if(!$value) { + $value = $this->generate(); + $this->setValue($value); + } + + return $value; } /** diff --git a/tests/control/SessionTest.php b/tests/control/SessionTest.php index 976efe5a8..78debff69 100644 --- a/tests/control/SessionTest.php +++ b/tests/control/SessionTest.php @@ -45,6 +45,30 @@ class SessionTest extends SapphireTest { $this->assertEquals($session, array('Test' => 'Test', 'Test-2' => 'Test-2')); } + /** + * Check that changedData isn't populated with junk when clearing non-existent entries. + */ + function testClearElementThatDoesntExist() { + $s = new Session(array('something' => array('does' => 'exist'))); + + $s->inst_clear('something.doesnt.exist'); + $this->assertEquals(array(), $s->inst_changedData()); + + $s->inst_set('something-else', 'val'); + $s->inst_clear('something-new'); + $this->assertEquals(array('something-else' => 'val'), $s->inst_changedData()); + } + + /** + * Check that changedData is populated with clearing data. + */ + function testClearElementThatDoesExist() { + $s = new Session(array('something' => array('does' => 'exist'))); + + $s->inst_clear('something.does'); + $this->assertEquals(array('something' => array('does' => null)), $s->inst_changedData()); + } + function testNonStandardPath(){ Session::set_session_store_path(realpath(dirname($_SERVER['DOCUMENT_ROOT']) . '/../session')); Session::start();