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

@@ -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);

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 <code>array</code> 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;
+			if($var !== $val) {
-			$diffVar = $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;
+		if($var !== null) {
-		$diffVar = 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);

main.php

@@ -59,9 +59,7 @@ if (version_compare(phpversion(), '5.3.2', '<')) {
 /**
  * Include SilverStripe's core code
  */
-require_once("core/Core.php");
+require_once('core/Core.php');
-
-Session::start();
 
 // IIS will sometimes generate this.
 if(!empty($_SERVER['HTTP_X_ORIGINAL_URL'])) {

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;
+				}
 			}
 		}
 	}

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;
 	}
 	
 	/**

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();