BUG Fix Versioned stage not persisting in Session. Fixes #962

BUG Disabled disruptive test case in DirectorTest
API RequestProcessor and VersionedRequestFilter now both correctly implement RequestFilter
Better PHPDoc on RequestFilter and implementations

control/Director.php

@@ -165,13 +165,13 @@ class Director implements TemplateGlobalProvider {
 					DataModel::inst()
 				);
 			} else {
-			$response = new SS_HTTPResponse();
+				$response = new SS_HTTPResponse();
 				$response->redirect($url);
-			$res = Injector::inst()->get('RequestProcessor')->postRequest($req, $response, $model);
+				$res = Injector::inst()->get('RequestProcessor')->postRequest($req, $response, $model);
 
-			if ($res !== false) {
+				if ($res !== false) {
-				$response->output();
+					$response->output();
-			}
+				}
 			}
 		// Handle a controller
 		} else if($result) {

control/RequestFilter.php

@@ -11,16 +11,24 @@
  * @subpackage control
  */
 interface RequestFilter {
+	
 	/**
 	 * Filter executed before a request processes
 	 * 
-	 * @return boolean (optional)
+	 * @param SS_HTTPRequest $request Request container object
-	 *				Whether to continue processing other filters
+	 * @param Session $session Request session
+	 * @param DataModel $model Current DataModel
+	 * @return boolean Whether to continue processing other filters. Null or true will continue processing (optional)
 	 */
 	public function preRequest(SS_HTTPRequest $request, Session $session, DataModel $model);
 
 	/**
 	 * Filter executed AFTER a request
+	 * 
+	 * @param SS_HTTPRequest $request Request container object
+	 * @param SS_HTTPResponse $response Response output object
+	 * @param DataModel $model Current DataModel
+	 * @return boolean Whether to continue processing other filters. Null or true will continue processing (optional)
 	 */
 	public function postRequest(SS_HTTPRequest $request, SS_HTTPResponse $response, DataModel $model);
-}
+}

control/RequestProcessor.php

@@ -1,17 +1,29 @@
 <?php
 
 /**
+ * Represents a request processer that delegates pre and post request handling to nested request filters
+ * 
  * @package framework
  * @subpackage control
  */
-class RequestProcessor {
+class RequestProcessor implements RequestFilter {
 
+	/**
+	 * List of currently assigned request filters
+	 *
+	 * @var array
+	 */
 	private $filters = array();
 
 	public function __construct($filters = array()) {
 		$this->filters = $filters;
 	}
 
+	/**
+	 * Assign a list of request filters
+	 * 
+	 * @param array $filters
+	 */
 	public function setFilters($filters) {
 		$this->filters = $filters;
 	}
@@ -25,9 +37,6 @@ class RequestProcessor {
 		}
 	}
 
-	/**
-	 * Filter executed AFTER a request
-	 */
 	public function postRequest(SS_HTTPRequest $request, SS_HTTPResponse $response, DataModel $model) {
 		foreach ($this->filters as $filter) {
 			$res = $filter->postRequest($request, $response, $model);

control/VersionedRequestFilter.php

@@ -5,13 +5,15 @@
  * @package framework
  * @subpackage control
  */
-class VersionedRequestFilter {
+class VersionedRequestFilter implements RequestFilter {
 
-	public function preRequest() {
+	public function preRequest(SS_HTTPRequest $request, Session $session, DataModel $model) {
-		Versioned::choose_site_stage();
+		Versioned::choose_site_stage($session);
+		return true;
 	}
 
-	public function postRequest() {
+	public function postRequest(SS_HTTPRequest $request, SS_HTTPResponse $response, DataModel $model) {
+		return true;
 	}
 
 }

model/Versioned.php

@@ -935,20 +935,24 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 *
 	 * If neither of these are set, it checks the session, otherwise the stage 
 	 * is set to 'Live'.
+	 * 
+	 * @param Session $session Optional session within which to store the resulting stage
 	 */
-	public static function choose_site_stage() {
+	public static function choose_site_stage($session = null) {
+		if(!$session) $session = Session::current_session();
+		
 		if(isset($_GET['stage'])) {
 			$stage = ucfirst(strtolower($_GET['stage']));
 			
 			if(!in_array($stage, array('Stage', 'Live'))) $stage = 'Live';
 
-			Session::set('readingMode', 'Stage.' . $stage);
+			$session->inst_set('readingMode', 'Stage.' . $stage);
 		}
 		if(isset($_GET['archiveDate']) && strtotime($_GET['archiveDate'])) {
-			Session::set('readingMode', 'Archive.' . $_GET['archiveDate']);
+			$session->inst_set('readingMode', 'Archive.' . $_GET['archiveDate']);
 		}
 		
-		if($mode = Session::get('readingMode')) {
+		if($mode = $session->inst_get('readingMode')) {
 			Versioned::set_reading_mode($mode);
 		} else {
 			Versioned::reading_stage("Live");

tests/model/VersionedTest.php

@@ -533,6 +533,42 @@ class VersionedTest extends SapphireTest {
 		
 		Versioned::set_reading_mode($originalMode);
 	}
+	
+	/**
+	 * Tests that reading mode persists between requests
+	 */
+	public function testReadingPersistent() {
+		$session = new Session(array());
+		
+		// Set to stage
+		Director::test('/?stage=Stage', null, $session);
+		$this->assertEquals(
+			'Stage.Stage',
+			$session->inst_get('readingMode'),
+			'Check querystring changes reading mode to Stage'
+		);
+		Director::test('/', null, $session);
+		$this->assertEquals(
+			'Stage.Stage',
+			$session->inst_get('readingMode'),
+			'Check that subsequent requests in the same session remain in Stage mode'
+		);
+		
+		// Test live persists
+		Director::test('/?stage=Live', null, $session);
+		$this->assertEquals(
+			'Stage.Live',
+			$session->inst_get('readingMode'),
+			'Check querystring changes reading mode to Live'
+		);
+		Director::test('/', null, $session);
+		$this->assertEquals(
+			'Stage.Live',
+			$session->inst_get('readingMode'),
+			'Check that subsequent requests in the same session remain in Live mode'
+		);
+		
+	}
 
 }