Better support for overloading start and destroy methods in Session

Move functionality from static start and destroy functions into instance
methods, allowing these to be overloaded. This works the same way as
calling Session::set() which then in turn calls inst_set()

Additionally use Injector to create the default Session instance to
allow the class to be swapped out.

control/Controller.php

@@ -439,7 +439,7 @@ class Controller extends RequestHandler implements TemplateGlobalProvider {
 			if(isset(self::$controller_stack[1])) {
 				$this->session = self::$controller_stack[1]->getSession();
 			} else {
-				$this->session = new Session(null);
+				$this->session = Injector::inst()->create('Session', array());
 			}
 		}
 	}

control/Director.php

@@ -135,12 +135,13 @@ class Director implements TemplateGlobalProvider {
 			$req->addHeader($header, $value);
 		}
 
+		// Initiate an empty session - doesn't initialize an actual PHP session until saved (see below)
+		$session = Injector::inst()->create('Session', isset($_SESSION) ? $_SESSION : array());
+
 		// Only resume a session if its not started already, and a session identifier exists
 		if(!isset($_SESSION) && Session::request_contains_session_id()) {
-			Session::start();
+			$session->inst_start();
 		}
-		// Initiate an empty session - doesn't initialize an actual PHP session until saved (see belwo)
-		$session = new Session(isset($_SESSION) ? $_SESSION : null);
 
 		$output = Injector::inst()->get('RequestProcessor')->preRequest($req, $session, $model);
 		
@@ -151,7 +152,7 @@ class Director implements TemplateGlobalProvider {
 
 		$result = Director::handleRequest($req, $session, $model);
 
-		// Save session data (and start/resume it if required)
+		// Save session data. Note that inst_save() will start/resume the session if required.
 		$session->inst_save();
 
 		// Return code for a redirection request
@@ -229,7 +230,7 @@ class Director implements TemplateGlobalProvider {
 		
 		if(!$httpMethod) $httpMethod = ($postVars || is_array($postVars)) ? "POST" : "GET";
 		
-		if(!$session) $session = new Session(null);
+		if(!$session) $session = Injector::inst()->create('Session', array());
 
 		// Back up the current values of the superglobals
 		$existingRequestVars = isset($_REQUEST) ? $_REQUEST : array();

control/Session.php

@@ -139,20 +139,19 @@ class Session {
 	/**
 	 * 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.
+	 * @param $data array|Session Can be an array of data (such as $_SESSION) or another Session object to clone.
 	 */
 	public function __construct($data) {
 		if($data instanceof Session) $data = $data->inst_getAll();
 
 		$this->data = $data;
-		
+
 		if (isset($this->data['HTTP_USER_AGENT'])) {
 			if ($this->data['HTTP_USER_AGENT'] != $this->userAgent()) {
 				// Funny business detected!
 				$this->inst_clearAll();
-				
-				Session::destroy();
-				Session::start();
+				$this->inst_destroy();
+				$this->inst_start();
 			}
 		}
 	}
@@ -347,11 +346,78 @@ class Session {
 		if(Controller::has_curr()) {
 			return Controller::curr()->getSession();
 		} else {
-			if(!self::$default_session) self::$default_session = new Session(isset($_SESSION) ? $_SESSION : array());
+			if(!self::$default_session) {
+				self::$default_session = Injector::inst()->create('Session', isset($_SESSION) ? $_SESSION : array());
+			}
+
 			return self::$default_session;
 		}
 	}
 
+	public function inst_start($sid = null) {
+		$path = Config::inst()->get('Session', 'cookie_path');
+		if(!$path) $path = Director::baseURL();
+		$domain = Config::inst()->get('Session', 'cookie_domain');
+		$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');
+
+		if(!session_id() && !headers_sent()) {
+			if($domain) {
+				session_set_cookie_params($timeout, $path, $domain, $secure, true);
+			} else {
+				session_set_cookie_params($timeout, $path, null, $secure, true);
+			}
+
+			// 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();
+
+			$this->data = isset($_SESSION) ? $_SESSION : array();
+		}
+
+		// Modify the timeout behaviour so it's the *inactive* time before the session expires.
+		// By default it's the total session lifetime
+		if($timeout && !headers_sent()) {
+			Cookie::set(session_name(), session_id(), $timeout/86400, $path, $domain ? $domain
+				: null, $secure, true);
+		}
+	}
+
+	public function inst_destroy($removeCookie = true) {
+		if(session_id()) {
+			if($removeCookie) {
+				$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');
+
+				if($domain) {
+					Cookie::set(session_name(), '', null, $path, $domain, $secure, true);
+				} else {
+					Cookie::set(session_name(), '', null, $path, null, $secure, true);
+				}
+
+				unset($_COOKIE[session_name()]);
+			}
+
+			session_destroy();
+
+			// Clean up the superglobal - session_destroy does not do it.
+			// http://nz1.php.net/manual/en/function.session-destroy.php
+			unset($_SESSION);
+			$this->data = array();
+		}
+	}
+
 	public function inst_set($name, $val) {
 		// Quicker execution path for "."-free names
 		if(strpos($name,'.') === false) {
@@ -472,7 +538,11 @@ class Session {
 	public function inst_save() {
 		if($this->changedData) {
 			$this->inst_finalize();
-			if(!isset($_SESSION)) Session::start();
+
+			if(!isset($_SESSION)) {
+				$this->inst_start();
+			}
+
 			$this->recursivelyApply($this->changedData, $_SESSION);
 		}
 	}
@@ -529,41 +599,7 @@ class Session {
 	 * @param string $sid Start the session with a specific ID
 	 */
 	public static function start($sid = null) {
-		$path = Config::inst()->get('Session', 'cookie_path');
-		if(!$path) $path = Director::baseURL();
-		$domain = Config::inst()->get('Session', 'cookie_domain');
-		$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');
-
-		if(!session_id() && !headers_sent()) {
-			if($domain) {
-				session_set_cookie_params($timeout, $path, $domain,
-					$secure /* secure */, true /* httponly */);
-			} else {
-				session_set_cookie_params($timeout, $path, null,
-					$secure /* secure */, true /* httponly */);
-			}
-
-			// 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.
-		// By default it's the total session lifetime
-		if($timeout && !headers_sent()) {
-			Cookie::set(session_name(), session_id(), $timeout/86400, $path, $domain ? $domain
-				: null, $secure, true);
-		}
+		self::current_session()->inst_start($sid);
 	}
 
 	/**
@@ -572,29 +608,7 @@ class Session {
 	 * @param bool $removeCookie If set to TRUE, removes the user's cookie, FALSE does not remove
 	 */
 	public static function destroy($removeCookie = true) {
-		if(session_id()) {
-			if($removeCookie) {
-				$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');
-				
-				if($domain) {
-					Cookie::set(session_name(), '', null, $path, $domain, $secure, true);
-				}
-				else {
-					Cookie::set(session_name(), '', null, $path, null, $secure, true);
-				}
-				
-				unset($_COOKIE[session_name()]);
-			}
-
-			session_destroy();
-
-			// Clean up the superglobal - session_destroy does not do it.
-			// http://nz1.php.net/manual/en/function.session-destroy.php
-			unset($_SESSION);
-		}
+		self::current_session()->inst_destroy($removeCookie);
 	}
 	
 	/**

dev/SapphireTest.php

@@ -201,7 +201,7 @@ class SapphireTest extends PHPUnit_Framework_TestCase {
 		DataObject::reset();
 		if(class_exists('SiteTree')) SiteTree::reset();
 		Hierarchy::reset();
-		if(Controller::has_curr()) Controller::curr()->setSession(new Session(array()));
+		if(Controller::has_curr()) Controller::curr()->setSession(Injector::inst()->create('Session', array()));
 		Security::$database_is_ready = null;
 		
 		$fixtureFile = static::get_fixture_file();

dev/TestSession.php

@@ -23,7 +23,7 @@ class TestSession {
 	private $lastUrl;
 
 	public function __construct() {
-		$this->session = new Session(array());
+		$this->session = Injector::inst()->create('Session', array());
 		$this->controller = new Controller();
 		$this->controller->setSession($this->session);
 		$this->controller->pushCurrent();

docs/en/topics/testing/creating-a-functional-test.md

@@ -18,7 +18,7 @@ URLs.  Here is an example from the subsites module:
 		 * Return a session that has a user logged in as an administrator
 		 */
 		public function adminLoggedInSession() {
-			return new Session(array(
+			return Injector::inst()->create('Session', array(
 				'loggedInAs' => $this->idFromFixture('Member', 'admin')
 			));
 		}
@@ -66,4 +66,4 @@ the *_t()* method to avoid tests failing when i18n is enabled.
 
 Note that for a more highlevel testing approach, SilverStripe also supports
 [behaviour-driven testing through Behat](https://github.com/silverstripe-labs/silverstripe-behat-extension). It interacts
-directly with your website or CMS interface by remote controlling an actual browser, driven by natural language assertions.
+directly with your website or CMS interface by remote controlling an actual browser, driven by natural language assertions.

tests/FakeController.php

@@ -5,7 +5,7 @@ class FakeController extends Controller {
 	public function __construct() {
 		parent::__construct();
 
-		$session = new Session(isset($_SESSION) ? $_SESSION : null);
+		$session = Injector::inst()->create('Session', isset($_SESSION) ? $_SESSION : array());
 		$this->setSession($session);
 		
 		$this->pushCurrent();
@@ -21,4 +21,4 @@ class FakeController extends Controller {
 		
 		$this->init();
 	}
-}
+}

tests/api/RestfulServiceTest.php

@@ -362,7 +362,7 @@ class RestfulServiceTest_MockRestfulService extends RestfulService {
 	public function request($subURL = '', $method = "GET", $data = null, $headers = null, $curlOptions = array()) {
 		
 		if(!$this->session) {
-			$this->session = new Session(array());
+			$this->session = Injector::inst()->create('Session', array());
 		}
 		
 		$url = $this->baseURL . $subURL; // Url for the request

tests/control/SessionTest.php

@@ -47,7 +47,7 @@ class SessionTest extends SapphireTest {
 	}
 
 	public function testSettingExistingDoesntClear() {
-		$s = new Session(array('something' => array('does' => 'exist')));
+		$s = Injector::inst()->create('Session', array('something' => array('does' => 'exist')));
 
 		$s->inst_set('something.does', 'exist');
 		$result = $s->inst_changedData();
@@ -59,7 +59,7 @@ class SessionTest extends SapphireTest {
 	 * Check that changedData isn't populated with junk when clearing non-existent entries.
 	 */
 	public function testClearElementThatDoesntExist() {
-		$s = new Session(array('something' => array('does' => 'exist')));
+		$s = Injector::inst()->create('Session', array('something' => array('does' => 'exist')));
 
 		$s->inst_clear('something.doesnt.exist');
 		$result = $s->inst_changedData();
@@ -77,7 +77,7 @@ class SessionTest extends SapphireTest {
 	 * Check that changedData is populated with clearing data.
 	 */
 	public function testClearElementThatDoesExist() {
-		$s = new Session(array('something' => array('does' => 'exist')));
+		$s = Injector::inst()->create('Session', array('something' => array('does' => 'exist')));
 
 		$s->inst_clear('something.does');
 		$result = $s->inst_changedData();
@@ -97,7 +97,7 @@ class SessionTest extends SapphireTest {
 		$_SERVER['HTTP_USER_AGENT'] = 'Test Agent';
 
 		// Generate our session
-		$s = new Session(array());
+		$s = Injector::inst()->create('Session', array());
 		$s->inst_set('val', 123);
 		$s->inst_finalize();
 
@@ -105,7 +105,7 @@ class SessionTest extends SapphireTest {
 		$_SERVER['HTTP_USER_AGENT'] = 'Fake Agent';
 		
 		// Verify the new session reset our values
-		$s2 = new Session($s);
+		$s2 = Injector::inst()->create('Session', $s);
 		$this->assertNotEquals($s2->inst_get('val'), 123);
 	}
 }

tests/model/VersionedTest.php

@@ -537,7 +537,7 @@ class VersionedTest extends SapphireTest {
 	 * Tests that reading mode persists between requests
 	 */
 	public function testReadingPersistent() {
-		$session = new Session(array());
+		$session = Injector::inst()->create('Session', array());
 		
 		// Set to stage
 		Director::test('/?stage=Stage', null, $session);
@@ -568,7 +568,7 @@ class VersionedTest extends SapphireTest {
 		);
 		
 		// Test that session doesn't redundantly store the default stage if it doesn't need to
-		$session2 = new Session(array());
+		$session2 = Injector::inst()->create('Session', array());
 		Director::test('/', null, $session2);
 		$this->assertEmpty($session2->inst_changedData());
 		Director::test('/?stage=Live', null, $session2);
@@ -660,4 +660,4 @@ class VersionedTest_SingleStage extends DataObject implements TestOnly {
 	private static $extensions = array(
 		'Versioned("Stage")'
 	);
-}
+}