From 0ee3a683a5d0d4ccad8595fe77bce601c1c42e12 Mon Sep 17 00:00:00 2001 From: Sean Harvey Date: Thu, 19 Jun 2014 10:38:26 +1200 Subject: [PATCH] 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 | 2 +- control/Director.php | 11 +- control/Session.php | 144 ++++++++++-------- dev/SapphireTest.php | 2 +- dev/TestSession.php | 2 +- .../testing/creating-a-functional-test.md | 4 +- tests/FakeController.php | 4 +- tests/api/RestfulServiceTest.php | 2 +- tests/control/SessionTest.php | 10 +- tests/model/VersionedTest.php | 6 +- 10 files changed, 101 insertions(+), 86 deletions(-) diff --git a/control/Controller.php b/control/Controller.php index 78f1c6e46..e0416661c 100644 --- a/control/Controller.php +++ b/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()); } } } diff --git a/control/Director.php b/control/Director.php index a8a7aeca3..6d1c636fd 100644 --- a/control/Director.php +++ b/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(); diff --git a/control/Session.php b/control/Session.php index 5940e92aa..71886726e 100644 --- a/control/Session.php +++ b/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); } /** diff --git a/dev/SapphireTest.php b/dev/SapphireTest.php index 4e4c7d9eb..0fd6cb4f1 100644 --- a/dev/SapphireTest.php +++ b/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(); diff --git a/dev/TestSession.php b/dev/TestSession.php index afb71eb7f..398bbfe71 100644 --- a/dev/TestSession.php +++ b/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(); diff --git a/docs/en/topics/testing/creating-a-functional-test.md b/docs/en/topics/testing/creating-a-functional-test.md index 763c4ca4d..23e963264 100644 --- a/docs/en/topics/testing/creating-a-functional-test.md +++ b/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. \ No newline at end of file +directly with your website or CMS interface by remote controlling an actual browser, driven by natural language assertions. diff --git a/tests/FakeController.php b/tests/FakeController.php index 726c2a668..481dab7d0 100644 --- a/tests/FakeController.php +++ b/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(); } -} \ No newline at end of file +} diff --git a/tests/api/RestfulServiceTest.php b/tests/api/RestfulServiceTest.php index fc101b7c6..5196965cf 100644 --- a/tests/api/RestfulServiceTest.php +++ b/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 diff --git a/tests/control/SessionTest.php b/tests/control/SessionTest.php index 49f7a427b..42dc39cb0 100644 --- a/tests/control/SessionTest.php +++ b/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); } } diff --git a/tests/model/VersionedTest.php b/tests/model/VersionedTest.php index 0b7246cac..f2a1f9183 100644 --- a/tests/model/VersionedTest.php +++ b/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")' ); -} \ No newline at end of file +}