Permalink
Browse files

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.
  • Loading branch information...
1 parent bd6ca59 commit f63d137d499b3556e51111a94ea5bd2de5ff4258 @halkyon halkyon committed Apr 26, 2012
Showing with 89 additions and 31 deletions.
  1. +2 −0 control/Director.php
  2. +43 −22 control/Session.php
  3. +1 −3 main.php
  4. +10 −2 model/Versioned.php
  5. +9 −4 security/SecurityToken.php
  6. +24 −0 tests/control/SessionTest.php
View
@@ -94,6 +94,8 @@ static function direct($url, DataModel $model) {
}
// 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);
View
@@ -103,6 +103,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'.
*
* To make cookies visible on all subdomains then the domain
@@ -177,17 +188,6 @@ public static function get_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
* until the session expires in seconds. For example:
@@ -300,12 +300,14 @@ public function inst_set($name, $val) {
$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,22 +357,30 @@ public function inst_clear($name) {
$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) {
$this->inst_clear($key);
}
}
}
-
+
public function inst_getAll() {
return $this->data;
}
@@ -380,7 +390,10 @@ public function inst_getAll() {
* 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 @@ protected function recursivelyApply($data, &$dest) {
}
}
}
-
+
+ /**
+ * 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 @@ public static function start($sid = null) {
// 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);
View
@@ -59,9 +59,7 @@
/**
* 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'])) {
View
@@ -791,9 +791,17 @@ static function choose_site_stage() {
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;
+ }
}
}
}
View
@@ -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 @@ function getName() {
* @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;
}
/**
@@ -45,6 +45,30 @@ function testGetAllElements() {
$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();

0 comments on commit f63d137

Please sign in to comment.