ENHANCEMENT Backport of querystring work to 3.x (#8026)

* WIP Backport of querystring work to 3.x

* Remove dataextension requirement

* Fix up bootstrapping

* more backporting

* Bug fix some tests

* Fix up some tests

* Fix support for custom stages
Don't set empty stage

* Better cache typehint

* Make sure useDraftSite(false) re-enables secure site

* Remove unnecessary guard around controller property

_config/versionedstate.yml

@@ -0,0 +1,9 @@
+---
+Name: versionedstate
+---
+RequestHandler:
+  extensions:
+    - VersionedStateExtension
+DataObject:
+  extensions:
+    - VersionedStateExtension

admin/code/LeftAndMain.php

@@ -460,8 +460,11 @@ class LeftAndMain extends Controller implements PermissionProvider {
 		// replaced TableListField.ss or Form.ss.
 		Config::inst()->update('SSViewer', 'theme_enabled', false);
 
-		//set the reading mode for the admin to stage
+		// Set the current reading mode
-		Versioned::reading_stage('Stage');
+		Versioned::reading_stage(Versioned::DRAFT);
+
+        // Set default reading mode to suppress ?stage=Stage querystring params in CMS
+        Versioned::set_default_reading_mode(Versioned::get_reading_mode());
 	}
 
 	public function handleRequest(SS_HTTPRequest $request, DataModel $model = null) {
@@ -560,7 +563,7 @@ class LeftAndMain extends Controller implements PermissionProvider {
 			'/', // trailing slash needed if $action is null!
 			"$action"
 		);
-		$this->extend('updateLink', $link);
+		$this->extend('updateLink', $link, $action);
 		return $link;
 	}
 

cache/Cache.php

@@ -146,7 +146,7 @@ class SS_Cache {
 	 * @param string $frontend (optional) The type of Zend_Cache frontend
 	 * @param array $frontendOptions (optional) Any frontend options to use.
 	 *
-	 * @return Zend_Cache_Frontend The cache object
+	 * @return Zend_Cache_Core The cache object
 	 */
 	public static function factory($for, $frontend='Output', $frontendOptions=null) {
 		self::init();

control/Controller.php

@@ -89,13 +89,6 @@ class Controller extends RequestHandler implements TemplateGlobalProvider {
 		$this->baseInitCalled = true;
 	}
 
-	/**
-	 * Returns a link to this controller.  Overload with your own Link rules if they exist.
-	 */
-	public function Link() {
-		return get_class($this) .'/';
-	}
-
 	/**
 	 * Executes this controller, and return an {@link SS_HTTPResponse} object with the result.
 	 *

control/Director.php

@@ -222,7 +222,7 @@ class Director implements TemplateGlobalProvider {
 
 		// These are needed so that calling Director::test() doesnt muck with whoever is calling it.
 		// Really, it's some inappropriate coupling and should be resolved by making less use of statics
-		$oldStage = Versioned::current_stage();
+		$oldMode = Versioned::get_reading_mode();
 		$getVars = array();
 
 		if(!$httpMethod) $httpMethod = ($postVars || is_array($postVars)) ? "POST" : "GET";
@@ -248,7 +248,7 @@ class Director implements TemplateGlobalProvider {
 		// Set callback to invoke prior to return
 		$onCleanup = function() use(
 			$existingRequestVars, $existingGetVars, $existingPostVars, $existingSessionVars,
-			$existingCookies, $existingServer, $existingRequirementsBackend, $oldStage
+			$existingCookies, $existingServer, $existingRequirementsBackend, $oldMode
 		) {
 			// Restore the superglobals
 			$_REQUEST = $existingRequestVars;
@@ -262,7 +262,7 @@ class Director implements TemplateGlobalProvider {
 
 			// These are needed so that calling Director::test() doesnt muck with whoever is calling it.
 			// Really, it's some inappropriate coupling and should be resolved by making less use of statics
-			Versioned::reading_stage($oldStage);
+			Versioned::set_reading_mode($oldMode);
 
 			Injector::unnest(); // Restore old CookieJar, etc
 			Config::unnest();

control/RequestHandler.php

@@ -36,6 +36,14 @@
 class RequestHandler extends ViewableData {
 
 	/**
+     * Optional url_segment for this request handler
+     *
+     * @config
+     * @var string|null
+     */
+    private static $url_segment = null;
+
+	/**
 	 * @var SS_HTTPRequest $request The request object that the controller was called with.
 	 * Set in {@link handleRequest()}. Useful to generate the {}
 	 */
@@ -496,4 +504,20 @@ class RequestHandler extends ViewableData {
 	public function setRequest($request) {
 		$this->request = $request;
 	}
+
+	/**
+	 * Returns a link to this controller.  Overload with your own Link rules if they exist.
+	 *
+	 * @param string $action Optional action (soft-supported via func_get_args)
+	 * @return string
+	 */
+	public function Link() {
+		$action = func_num_args() ? func_get_arg(0) : null;
+		$urlSegment = $this->config()->get('url_segment') ?: get_class($this);
+		$link = Controller::join_links($urlSegment, $action, '/');
+
+		// Give extensions the chance to modify by reference
+		$this->extend('updateLink', $link);
+		return $link;
+	}
 }

control/VersionedRequestFilter.php

@@ -38,7 +38,7 @@ class VersionedRequestFilter implements RequestFilter {
 			die;
 		}
 
-		Versioned::choose_site_stage();
+		Versioned::choose_site_stage($request);
 		$dummyController->popCurrent();
 		return true;
 	}

core/Object.php

@@ -572,13 +572,6 @@ abstract class SS_Object {
 		Config::inst()->extraConfigSourcesChanged($class);
 
 		Injector::inst()->unregisterNamedObject($class);
-
-		// load statics now for DataObject classes
-		if(is_subclass_of($class, 'DataObject')) {
-			if(!is_subclass_of($extensionClass, 'DataExtension')) {
-				user_error("$extensionClass cannot be applied to $class without being a DataExtension", E_USER_ERROR);
-			}
-		}
 	}
 
 
@@ -653,7 +646,11 @@ abstract class SS_Object {
 
 	// --------------------------------------------------------------------------------------------------------------
 
-	private static $unextendable_classes = array('SS_Object', 'Object', 'ViewableData', 'RequestHandler');
+	private static $unextendable_classes = array(
+		'SS_Object',
+		'Object',
+		'ViewableData',
+	);
 
 	static public function get_extra_config_sources($class = null) {
 		if($class === null) $class = get_called_class();

dev/FunctionalTest.php

@@ -381,10 +381,12 @@ class FunctionalTest extends SapphireTest {
 		if($enabled) {
 			$this->session()->inst_set('readingMode', 'Stage.Stage');
 			$this->session()->inst_set('unsecuredDraftSite', true);
+			Versioned::set_draft_site_secured(false);
 		}
 		else {
 			$this->session()->inst_set('readingMode', 'Stage.Live');
 			$this->session()->inst_set('unsecuredDraftSite', false);
+			Versioned::set_draft_site_secured(true);
 		}
 	}
 

forms/Form.php

@@ -1070,11 +1070,18 @@ class Form extends RequestHandler {
 	public function FormAction() {
 		if ($this->formActionPath) {
 			return $this->formActionPath;
-		} elseif($this->controller->hasMethod("FormObjectLink")) {
-			return $this->controller->FormObjectLink($this->name);
-		} else {
-			return Controller::join_links($this->controller->Link(), $this->name);
 		}
+
+		// Respect FormObjectLink() method
+		if($this->controller->hasMethod("FormObjectLink")) {
+			$link = $this->controller->FormObjectLink($this->getName());
+		} else {
+			$link = Controller::join_links($this->controller->Link(), $this->getName());
+		}
+
+		// Join with action and decorate
+        $this->extend('updateLink', $link);
+        return $link;
 	}
 
 	/**

forms/FormField.php

@@ -274,7 +274,9 @@ class FormField extends RequestHandler {
 	 * @return string
 	 */
 	public function Link($action = null) {
-		return Controller::join_links($this->form->FormAction(), 'field/' . $this->name, $action);
+		$link = Controller::join_links($this->form->FormAction(), 'field/' . $this->name, $action);
+		$this->extend('updateLink', $link, $action);
+        return $link;
 	}
 
 	/**

model/Versioned.php

@@ -4,17 +4,15 @@
  * The Versioned extension allows your DataObjects to have several versions, allowing you to rollback changes and view
  * history. An example of this is the pages used in the CMS.
  *
- * @property int $Version
- *
  * @package framework
  * @subpackage model
  *
- * @property DataObject owner
+ * @property DataObject $owner
- * @property int  RecordID
+ * @property int  $RecordID
- * @property int  Version
+ * @property int  $Version
- * @property bool WasPublished
+ * @property bool $WasPublished
- * @property int  AuthorID
+ * @property int  $AuthorID
- * @property int  PublisherID
+ * @property int  $PublisherID
  */
 class Versioned extends DataExtension implements TemplateGlobalProvider {
 
@@ -41,6 +39,16 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 */
 	const DEFAULT_MODE = 'Stage.Live';
 
+	/**
+     * The Public stage.
+     */
+    const LIVE = 'Live';
+
+    /**
+     * The draft (default) stage
+     */
+    const DRAFT = 'Stage';
+
 	/**
 	 * A version that a DataObject should be when it is 'migrating', that is, when it is in the process of moving from
 	 * one stage to another.
@@ -54,9 +62,37 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 */
 	protected static $cache_versionnumber;
 
-	/** @var string */
+    /**
+     * Set if draft site is secured or not. Fails over to
+     * $draft_site_secured if unset
+     *
+     * @var bool|null
+     */
+    protected static $is_draft_site_secured = null;
+
+    /**
+     * Default config for $is_draft_site_secured
+     *
+     * @config
+     * @var bool
+     */
+    private static $draft_site_secured = true;
+
+	/**
+	 * Current reading mode
+	 *
+	 * @var string
+	 */
 	protected static $reading_mode = null;
 
+    /**
+     * Default reading mode, if none set.
+     * Any modes which differ to this value should be assigned via querystring / session (if enabled)
+     *
+     * @var null
+     */
+    protected static $default_reading_mode = self::DEFAULT_MODE;
+
 	/**
 	 * Flag which is temporarily changed during the write() process to influence augmentWrite() behaviour. If set to
 	 * true, no new version will be created for the following write. Needs to be public as other classes introspect this
@@ -150,11 +186,24 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 */
 	private static $non_live_permissions = array('CMS_ACCESS_LeftAndMain', 'CMS_ACCESS_CMSMain', 'VIEW_DRAFT_CONTENT');
 
+	/**
+     * Use PHP's session storage for the "reading mode" and "unsecuredDraftSite",
+     * instead of explicitly relying on the "stage" query parameter.
+     * This is considered bad practice, since it can cause draft content
+     * to leak under live URLs to unauthorised users, depending on HTTP cache settings.
+     *
+     * @config
+     * @var bool
+     */
+    private static $use_session = true;
+
 	/**
 	 * Reset static configuration variables to their default values.
 	 */
 	public static function reset() {
-		self::$reading_mode = '';
+		self::set_reading_mode(null);
+		self::set_default_reading_mode(null);
+		self::set_draft_site_secured(null);
 
 		Session::clear('readingMode');
 	}
@@ -184,17 +233,12 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 * @param DataQuery $dataQuery
 	 */
 	public function augmentDataQueryCreation(SQLQuery &$query, DataQuery &$dataQuery) {
-		$parts = explode('.', Versioned::get_reading_mode());
+		// Convert reading mode to dataquery params and assign
-
+        $args = VersionedReadingMode::toDataQueryParams(Versioned::get_reading_mode());
-		if($parts[0] == 'Archive') {
+        if ($args) {
-			$dataQuery->setQueryParam('Versioned.mode', 'archive');
+            foreach ($args as $key => $value) {
-			$dataQuery->setQueryParam('Versioned.date', $parts[1]);
+                $dataQuery->setQueryParam($key, $value);
-
+            }
-		} else if($parts[0] == 'Stage' && $parts[1] != $this->defaultStage
-				&& array_search($parts[1],$this->stages) !== false) {
-
-			$dataQuery->setQueryParam('Versioned.mode', 'stage');
-			$dataQuery->setQueryParam('Versioned.stage', $parts[1]);
         }
 	}
 
@@ -253,24 +297,30 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 
 		// Reading a specific stage (Stage or Live)
 		case 'stage':
+			// Check stage is available on object
 			$stage = $dataQuery->getQueryParam('Versioned.stage');
-			if($stage && ($stage != $this->defaultStage)) {
+			if (!$stage || !in_array($stage, $this->stages) || $stage === $this->defaultStage) {
-				foreach($query->getFrom() as $table => $dummy) {
+				break;
+			}
+			foreach ($query->getFrom() as $table => $dummy) {
 				// Only rewrite table names that are actually part of the subclass tree
 				// This helps prevent rewriting of other tables that get joined in, in
 				// particular, many_many tables
-					if(class_exists($table) && ($table == $this->owner->class
+				if (class_exists($table) && ($table == $this->owner->class
 						|| is_subclass_of($table, $this->owner->class)
 						|| is_subclass_of($this->owner->class, $table))) {
 					$query->renameTable($table, $table . '_' . $stage);
 				}
 			}
-			}
 			break;
 
 		// Reading a specific stage, but only return items that aren't in any other stage
 		case 'stage_unique':
+			// Check stage is available on object
 			$stage = $dataQuery->getQueryParam('Versioned.stage');
+			if (!$stage || !in_array($stage, $this->stages) || $stage === $this->defaultStage) {
+				break;
+			}
 
 			// Recurse to do the default stage behavior (must be first, we rely on stage renaming happening before
 			// below)
@@ -791,15 +841,15 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 * @return bool False is returned if the current viewing mode denies visibility
 	 */
 	public function canViewVersioned($member = null) {
-		// Bypass when live stage
+		// Bypass if site is unsecured
-		$mode = $this->owner->getSourceQueryParam("Versioned.mode");
+		if (!self::get_draft_site_secured()) {
-		$stage = $this->owner->getSourceQueryParam("Versioned.stage");
-		if ($mode === 'stage' && $stage === static::get_live_stage()) {
 			return true;
 		}
 
-		// Bypass if site is unsecured
+		// Bypass when live stage
-		if (Session::get('unsecuredDraftSite')) {
+		$mode = $this->owner->getSourceQueryParam("Versioned.mode");
+		$stage = $this->owner->getSourceQueryParam("Versioned.stage");
+		if ($mode === 'stage' && $stage === self::LIVE) {
 			return true;
 		}
 
@@ -1055,6 +1105,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 
 		$query = $list->dataQuery()->query();
 
+		$baseTable = null;
 		foreach($query->getFrom() as $table => $tableJoin) {
 			if(is_string($tableJoin) && $tableJoin[0] == '"') {
 				$baseTable = str_replace('"','',$tableJoin);
@@ -1139,6 +1190,16 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 			return true;
 		}
 
+        // Request is allowed if unsecuredDraftSite is enabled
+        if (!static::get_draft_site_secured()) {
+            return true;
+        }
+
+        // Predict if choose_site_stage() will allow unsecured draft assignment by session
+        if (Config::inst()->get('Versioned', 'use_session') && Session::get('unsecuredDraftSite')) {
+            return true;
+        }
+
 		// Check permissions with member ID in session.
 		$member = Member::currentUser();
 		$permissions = Config::inst()->get(get_called_class(), 'non_live_permissions');
@@ -1150,36 +1211,45 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 * - If $_GET['stage'] is set, then it will use that stage, and store it in the session.
 	 * - If $_GET['archiveDate'] is set, it will use that date, and store it in the session.
 	 * - If neither of these are set, it checks the session, otherwise the stage is set to 'Live'.
+	 *
+	 * @param SS_HTTPRequest|null $request
 	 */
-	public static function choose_site_stage() {
+	public static function choose_site_stage(SS_HTTPRequest $request = null) {
-		// Check any pre-existing session mode
+		if (!$request) {
-		$preexistingMode = Session::get('readingMode');
+			throw new InvalidArgumentException("Request not found");
+		}
+		$mode = static::get_default_reading_mode();
 
-		// Determine the reading mode
+        // Check any pre-existing session mode
-		if(isset($_GET['stage'])) {
+        $useSession = Config::inst()->get('Versioned', 'use_session');
-			$stage = ucfirst(strtolower($_GET['stage']));
+        $updateSession = false;
-			if(!in_array($stage, array('Stage', 'Live'))) $stage = 'Live';
+        if ($useSession) {
-			$mode = 'Stage.' . $stage;
+            // Boot reading mode from session
-		} elseif (isset($_GET['archiveDate']) && strtotime($_GET['archiveDate'])) {
+            $mode = Session::get('readingMode') ?: $mode;
-			$mode = 'Archive.' . $_GET['archiveDate'];
+
-		} elseif($preexistingMode) {
+            // Set draft site security if disabled for this session
-			$mode = $preexistingMode;
+            if (Session::get('unsecuredDraftSite')) {
-		} else {
+                static::set_draft_site_secured(false);
-			$mode = self::DEFAULT_MODE;
+            }
+        }
+
+		// Verify if querystring contains valid reading mode
+        $queryMode = VersionedReadingMode::fromQueryString($request->getVars());
+        if ($queryMode) {
+            $mode = $queryMode;
+            $updateSession = true;
         }
 
         // Save reading mode
         Versioned::set_reading_mode($mode);
 
-		// Try not to store the mode in the session if not needed
+		// Set mode if session enabled
-		if(($preexistingMode && $preexistingMode !== $mode)
+        if ($useSession && $updateSession) {
-			|| (!$preexistingMode && $mode !== self::DEFAULT_MODE)
-		) {
             Session::set('readingMode', $mode);
         }
 
 		if(!headers_sent() && !Director::is_cli()) {
-			if(Versioned::current_stage() == 'Live') {
+			if(Versioned::current_stage() === self::LIVE) {
 				// clear the cookie if it's set
 				if(Cookie::get('bypassStaticCache')) {
 					Cookie::force_expiry('bypassStaticCache', null, null, false, true /* httponly */);
@@ -1217,7 +1287,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 * @return string
 	 */
 	public static function get_live_stage() {
-		return "Live";
+		return self::LIVE;
 	}
 
 	/**
@@ -1249,9 +1319,52 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 * @param string $stage
 	 */
 	public static function reading_stage($stage) {
+		VersionedReadingMode::validateStage($stage);
 		Versioned::set_reading_mode('Stage.' . $stage);
 	}
 
+    /**
+     * Replace default mode.
+     * An non-default mode should be specified via querystring arguments.
+     *
+     * @param string $mode
+     */
+    public static function set_default_reading_mode($mode) {
+        self::$default_reading_mode = $mode;
+    }
+
+    /**
+     * Get default reading mode
+     *
+     * @return string
+     */
+    public static function get_default_reading_mode() {
+        return self::$default_reading_mode ?: self::DEFAULT_MODE;
+    }
+
+    /**
+     * Check if draft site should be secured.
+     * Can be turned off if draft site unauthenticated
+     *
+     * @return bool
+     */
+    public static function get_draft_site_secured() {
+        if (isset(static::$is_draft_site_secured)) {
+            return (bool)static::$is_draft_site_secured;
+        }
+        // Config default
+        return (bool)Config::inst()->get('Versioned', 'draft_site_secured');
+    }
+
+    /**
+     * Set if the draft site should be secured or not
+     *
+     * @param bool $secured
+     */
+    public static function set_draft_site_secured($secured) {
+        static::$is_draft_site_secured = $secured;
+    }
+
 	/**
 	 * Set the reading archive date.
 	 *

model/VersionedReadingMode.php

@@ -0,0 +1,144 @@
+<?php
+
+/**
+ * Converter helpers for versioned args
+ */
+class VersionedReadingMode
+{
+    /**
+     * Convert reading mode string to dataquery params.
+     * Only supports stage / archive
+     *
+     * @param string $mode Reading mode string
+     * @return array|null
+     */
+    public static function toDataQueryParams($mode)
+    {
+        if (empty($mode)) {
+            return null;
+        }
+        if (!is_string($mode)) {
+            throw new InvalidArgumentException("mode must be a string");
+        }
+        $parts = explode('.', $mode);
+        switch ($parts[0]) {
+            case 'Archive':
+                return array(
+                    'Versioned.mode' => 'archive',
+                    'Versioned.date' => $parts[1],
+				);
+            case 'Stage':
+                self::validateStage($parts[1]);
+                return array(
+                    'Versioned.mode' => 'stage',
+                    'Versioned.stage' => $parts[1],
+				);
+            default:
+                // Unsupported mode
+                return null;
+        }
+    }
+
+    /**
+     * Converts dataquery params to original reading mode.
+     * Only supports stage / archive
+     *
+     * @param array $params
+     * @return string|null
+     */
+    public static function fromDataQueryParams($params)
+    {
+        // Switch on reading mode
+        if (empty($params["Versioned.mode"])) {
+            return null;
+        }
+
+        switch ($params["Versioned.mode"]) {
+            case 'archive':
+                return 'Archive.' . $params['Versioned.date'];
+            case 'stage':
+                return 'Stage.' . $params['Versioned.stage'];
+            default:
+                return null;
+        }
+    }
+
+    /**
+     * Convert querystring arguments to reading mode.
+     * Only supports stage / archive mode
+     *
+     * @param array|string $query Querystring arguments (array or string)
+     * @return string|null Reading mode, or null if not found / supported
+     */
+    public static function fromQueryString($query)
+    {
+        if (is_string($query)) {
+            parse_str($query, $query);
+        }
+        if (empty($query)) {
+            return null;
+        }
+
+        // Archive date is specified
+		if (isset($query['archiveDate']) && strtotime($query['archiveDate'])) {
+            return 'Archive.' . $query['archiveDate'];
+        }
+
+        // Stage is specified by itself
+        if (isset($query['stage']) && strcasecmp($query['stage'], Versioned::DRAFT) === 0) {
+            return 'Stage.' . Versioned::DRAFT;
+        }
+        if (isset($query['stage']) && strcasecmp($query['stage'], Versioned::LIVE) === 0) {
+            return 'Stage.' . Versioned::LIVE;
+        }
+
+        // Unsupported query mode
+        return null;
+    }
+
+    /**
+     * Build querystring arguments for current reading mode.
+     * Supports stage / archive only.
+     *
+     * @param string $mode
+     * @return array List of querystring arguments as an array
+     */
+    public static function toQueryString($mode)
+    {
+        if (empty($mode)) {
+            return null;
+        }
+        if (!is_string($mode)) {
+            throw new InvalidArgumentException("mode must be a string");
+        }
+        $parts = explode('.', $mode);
+        switch ($parts[0]) {
+            case 'Archive':
+                return array(
+                    'archiveDate' => $parts[1],
+				);
+            case 'Stage':
+                self::validateStage($parts[1]);
+                return array(
+                    'stage' => $parts[1],
+				);
+            default:
+                // Unsupported mode
+                return null;
+        }
+    }
+
+    /**
+     * Validate the stage is valid, throwing an exception if it's not
+     *
+     * @param string $stage
+     */
+    public static function validateStage($stage)
+    {
+    	// Any stage is allowed in 3.x. Note that 4.x only allows Stage / Live
+		// Any string that contains no dots is ok.
+		if (empty($stage) || !preg_match('/^([^.]+)$/', $stage)) {
+            throw new InvalidArgumentException("Invalid stage name \"{$stage}\"");
+        }
+    }
+}

model/VersionedStateExtension.php

@@ -0,0 +1,89 @@
+<?php
+/**
+ * Persists versioned state between requests via querystring arguments
+ *
+ * @property Controller|DataObject $owner
+ */
+class VersionedStateExtension extends Extension
+{
+    /**
+     * Auto-append current stage if we're in draft,
+     * to avoid relying on session state for this,
+     * and the related potential of showing draft content
+     * without varying the URL itself.
+     *
+     * Assumes that if the user has access to view the current
+     * record in draft stage, they can also view other draft records.
+     * Does not concern itself with verifying permissions for performance reasons.
+     *
+     * This should also pull through to form actions.
+     *
+     * @param string $link
+     */
+    public function updateLink(&$link)
+    {
+        // Skip if link already contains reading mode
+        if ($this->hasVersionedQuery($link)) {
+            return;
+        }
+
+        // Skip if current mode matches default mode
+        // See LeftAndMain::init() for example of this being overridden.
+        $readingMode = $this->getReadingmode();
+        if ($readingMode === Versioned::get_default_reading_mode()) {
+            return;
+        }
+
+        // Determine if query args are supported for the current mode
+        $queryargs = VersionedReadingMode::toQueryString($readingMode);
+        if (!$queryargs) {
+            return;
+        }
+
+        // Decorate
+        $link = Controller::join_links(
+            $link,
+            '?' . http_build_query($queryargs)
+        );
+    }
+
+    /**
+     * Check if link contains versioned queryargs
+     *
+     * @param string $link
+     * @return bool
+     */
+    protected function hasVersionedQuery($link)
+    {
+        // Find querystrings
+        $parts = explode('?', $link, 2);
+        if (count($parts) < 2) {
+            return false;
+        }
+
+        // Parse args
+        $readingMode = VersionedReadingMode::fromQueryString($parts[1]);
+        return !empty($readingMode);
+    }
+
+    /**
+     * Get reading mode for the record / controller being decorated
+     *
+     * @return string
+     */
+    protected function getReadingmode()
+    {
+        $default = Versioned::get_reading_mode();
+
+        // Non dataobjects use global mode
+        if (! $this->owner instanceof DataObject) {
+            return $default;
+        }
+
+        // Respect source query params (so records selected from live will have live urls)
+        $queryParams = $this->owner->getSourceQueryParams();
+        return VersionedReadingMode::fromDataQueryParams($queryParams)
+            // Fall back to default otherwise
+            ?: $default;
+    }
+}

security/CMSSecurity.php

@@ -43,7 +43,10 @@ class CMSSecurity extends Security {
 	}
 
 	public function Link($action = null) {
-		return Controller::join_links(Director::baseURL(), "CMSSecurity", $action);
+		$link = Controller::join_links(Director::baseURL(), "CMSSecurity", $action);
+		// Give extensions the chance to modify by reference
+		$this->extend('updateLink', $link, $action);
+		return $link;
 	}
 
 	/**

security/Security.php

@@ -399,7 +399,11 @@ class Security extends Controller implements TemplateGlobalProvider {
 	 * @return string Returns the link to the given action
 	 */
 	public function Link($action = null) {
-		return Controller::join_links(Director::baseURL(), "Security", $action);
+		$link = Controller::join_links(Director::baseURL(), "Security", $action);
+
+		// Give extensions the chance to modify by reference
+		$this->extend('updateLink', $link, $action);
+		return $link;
 	}
 
 	/**

tests/model/VersionedReadingModeTest.php

@@ -0,0 +1,175 @@
+<?php
+
+class VersionedReadingModeTest extends SapphireTest
+{
+    /**
+     * @dataProvider provideReadingModes()
+     *
+     * @param string $readingMode
+     * @param array $dataQuery
+     * @param array $queryStringArray
+     * @param string $queryString
+     */
+    public function testToDataQueryParams($readingMode, $dataQuery, $queryStringArray, $queryString)
+    {
+        $this->assertEquals(
+            $dataQuery,
+            VersionedReadingMode::toDataQueryParams($readingMode),
+            "Convert {$readingMode} to dataquery parameters"
+        );
+    }
+    /**
+     * @dataProvider provideReadingModes()
+     *
+     * @param string $readingMode
+     * @param array $dataQuery
+     * @param array $queryStringArray
+     * @param string $queryString
+     */
+    public function testFromDataQueryParameters($readingMode, $dataQuery, $queryStringArray, $queryString)
+    {
+        $this->assertEquals(
+            $readingMode,
+            VersionedReadingMode::fromDataQueryParams($dataQuery),
+            "Convert {$readingMode} from dataquery parameters"
+        );
+    }
+
+    /**
+     * @dataProvider provideReadingModes()
+     *
+     * @param string $readingMode
+     * @param array $dataQuery
+     * @param array $queryStringArray
+     * @param string $queryString
+     */
+    public function testToQueryString($readingMode, $dataQuery, $queryStringArray, $queryString)
+    {
+        $this->assertEquals(
+            $queryStringArray,
+            VersionedReadingMode::toQueryString($readingMode),
+            "Convert {$readingMode} to querystring array"
+        );
+    }
+
+    /**
+     * @dataProvider provideReadingModes()
+     *
+     * @param string $readingMode
+     * @param array $dataQuery
+     * @param array $queryStringArray
+     * @param string $queryString
+     */
+    public function testFromQueryString($readingMode, $dataQuery, $queryStringArray, $queryString)
+    {
+        $this->assertEquals(
+            $readingMode,
+            VersionedReadingMode::fromQueryString($queryStringArray),
+            "Convert {$readingMode} from querystring array"
+        );
+        $this->assertEquals(
+            $readingMode,
+            VersionedReadingMode::fromQueryString($queryString),
+            "Convert {$readingMode} from querystring encoded string"
+        );
+    }
+
+    /**
+     * Return list of reading modes in order:
+     *  - reading mode string
+     *  - dataquery params array
+     *  - query string array
+     *  - query string (string)
+     * @return array
+     */
+    public function provideReadingModes()
+    {
+        return array(
+            // Draft
+            array(
+                'Stage.Stage',
+                array(
+                    'Versioned.mode' => 'stage',
+                    'Versioned.stage' => 'Stage',
+				),
+                array(
+                    'stage' => 'Stage',
+				),
+                'stage=Stage'
+			),
+            // Live
+            array(
+                'Stage.Live',
+                array(
+                    'Versioned.mode' => 'stage',
+                    'Versioned.stage' => 'Live',
+				),
+                array(
+                    'stage' => 'Live',
+				),
+                'stage=Live'
+			),
+            // Draft archive
+            array(
+                'Archive.2017-11-15 11:31:42',
+                array(
+                    'Versioned.mode' => 'archive',
+                    'Versioned.date' => '2017-11-15 11:31:42',
+				),
+                array(
+                    'archiveDate' => '2017-11-15 11:31:42',
+				),
+                'archiveDate=2017-11-15+11%3A31%3A42',
+			),
+            // Live archive
+            array(
+                'Archive.2017-11-15 11:31:42',
+                array(
+                    'Versioned.mode' => 'archive',
+                    'Versioned.date' => '2017-11-15 11:31:42',
+				),
+                array(
+                    'archiveDate' => '2017-11-15 11:31:42',
+				),
+                'archiveDate=2017-11-15+11%3A31%3A42',
+			),
+		);
+    }
+
+    /**
+     * @dataProvider provideTestInvalidStage
+     * @param string $stage
+     */
+    public function testInvalidStage($stage)
+    {
+    	$this->setExpectedException('InvalidArgumentException');
+        VersionedReadingMode::validateStage($stage);
+    }
+
+    public function provideTestInvalidStage()
+    {
+        return array(
+            array(''),
+			array('Stage.stage'),
+		);
+    }
+
+    /**
+     * @dataProvider provideTestValidStage
+     * @param string $stage
+     */
+    public function testValidStage($stage)
+    {
+        VersionedReadingMode::validateStage($stage);
+        $this->assertTrue(true, 'Stage is valid');
+    }
+
+    public function provideTestValidStage()
+    {
+        return array(
+            array('anything'),
+            array(Versioned::DRAFT),
+            array(Versioned::LIVE),
+		);
+    }
+}

tests/model/VersionedTest.php

@@ -296,7 +296,7 @@ class VersionedTest extends SapphireTest {
 	}
 
 	public function testWritingNewToStage() {
-		$origStage = Versioned::current_stage();
+		$origMode = Versioned::get_reading_mode();
 
 		Versioned::reading_stage("Stage");
 		$page = new VersionedTest_DataObject();
@@ -315,7 +315,7 @@ class VersionedTest extends SapphireTest {
 		$this->assertEquals(1, $stage->count());
 		$this->assertEquals($stage->First()->Title, 'testWritingNewToStage');
 
-		Versioned::reading_stage($origStage);
+		Versioned::set_reading_mode($origMode);
 	}
 
 	/**
@@ -325,7 +325,7 @@ class VersionedTest extends SapphireTest {
 	 * the VersionedTest_DataObject record though.
 	 */
 	public function testWritingNewToLive() {
-		$origStage = Versioned::current_stage();
+		$origMode = Versioned::get_reading_mode();
 
 		Versioned::reading_stage("Live");
 		$page = new VersionedTest_DataObject();
@@ -344,7 +344,7 @@ class VersionedTest extends SapphireTest {
 		));
 		$this->assertEquals(0, $stage->count());
 
-		Versioned::reading_stage($origStage);
+		Versioned::set_reading_mode($origMode);
 	}
 
 	/**
@@ -635,14 +635,37 @@ class VersionedTest extends SapphireTest {
 		Versioned::set_reading_mode($originalMode);
 	}
 
-	/**
+	public function testReadingNotPersistentWhenUseSessionFalse()
-	 * Tests that reading mode persists between requests
+    {
-	 */
+        Config::inst()->update('Versioned', 'use_session', false);
-	public function testReadingPersistent() {
+
-		$session = Injector::inst()->create('Session', array());
+        $session = new Session(array());
         $adminID = $this->logInWithPermission('ADMIN');
         $session->inst_set('loggedInAs', $adminID);
 
+        Director::test('/?stage=Stage', null, $session);
+        $this->assertNull(
+            $session->inst_get('readingMode'),
+            'Check querystring does not change reading mode'
+        );
+
+        Director::test('/', null, $session);
+        $this->assertNull(
+            $session->inst_get('readingMode'),
+            'Check that subsequent requests in the same session do not have a changed reading mode'
+        );
+    }
+
+    /**
+     * Tests that reading mode persists between requests
+     */
+    public function testReadingPersistentWhenUseSessionTrue()
+    {
+        Config::inst()->update('Versioned', 'use_session', true);
+
+        $session = new Session(array());
+        $adminID = $this->logInWithPermission('ADMIN');
+        $session->inst_set('loggedInAs', $adminID);
         // Set to stage
         Director::test('/?stage=Stage', null, $session);
         $this->assertEquals(
@@ -656,8 +679,7 @@ class VersionedTest extends SapphireTest {
             $session->inst_get('readingMode'),
             'Check that subsequent requests in the same session remain in Stage mode'
         );
-
+        // Default stage stored anyway (in case default changes)
-		// Test live persists
         Director::test('/?stage=Live', null, $session);
         $this->assertEquals(
             'Stage.Live',
@@ -670,27 +692,33 @@ class VersionedTest extends SapphireTest {
             $session->inst_get('readingMode'),
             'Check that subsequent requests in the same session remain in Live mode'
         );
-
+        // Test that session doesn't redundantly modify session stage without querystring args
-		// 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());
         $session2->inst_set('loggedInAs', $adminID);
         Director::test('/', null, $session2);
         $this->assertArrayNotHasKey('readingMode', $session2->inst_changedData());
         Director::test('/?stage=Live', null, $session2);
-		$this->assertArrayNotHasKey('readingMode', $session2->inst_changedData());
+        $this->assertArrayHasKey('readingMode', $session2->inst_changedData());
-
         // Test choose_site_stage
         unset($_GET['stage']);
         unset($_GET['archiveDate']);
+        $request = new SS_HTTPRequest('GET', '/');
+        Session::clear_all();
         Session::set('readingMode', 'Stage.Stage');
-		Versioned::choose_site_stage();
+        Versioned::choose_site_stage($request);
         $this->assertEquals('Stage.Stage', Versioned::get_reading_mode());
         Session::set('readingMode', 'Archive.2014-01-01');
-		Versioned::choose_site_stage();
+        Versioned::choose_site_stage($request);
         $this->assertEquals('Archive.2014-01-01', Versioned::get_reading_mode());
         Session::clear('readingMode');
-		Versioned::choose_site_stage();
+        Versioned::choose_site_stage($request);
         $this->assertEquals('Stage.Live', Versioned::get_reading_mode());
+        // Ensure stage is reset to Live when logging out
+        Session::set('readingMode', 'Stage.Stage');
+        Versioned::choose_site_stage($request);
+        Session::clear_all();
+        Versioned::choose_site_stage($request);
+        $this->assertSame('Stage.Live', Versioned::get_reading_mode());
     }
 
 	/**

tests/view/SSViewerCacheBlockTest.php

@@ -145,7 +145,7 @@ class SSViewerCacheBlockTest extends SapphireTest {
 
 	public function testVersionedCache() {
 
-		$origStage = Versioned::current_stage();
+		$origMode = Versioned::get_reading_mode();
 
 		// Run without caching in stage to prove data is uncached
 		$this->_reset(false);
@@ -211,7 +211,7 @@ class SSViewerCacheBlockTest extends SapphireTest {
 			$this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data)
 		);
 
-		Versioned::reading_stage($origStage);
+		Versioned::set_reading_mode($origMode);
 	}
 
 	/**