Skip to content

Loading…

BUG Fix Versioned stage not persisting in Session #2918

Merged
merged 1 commit into from

4 participants

@tractorcow

This fixes the issue noted at silverstripe/silverstripe-cms#962

The problem is that the VersionedRequestFilter was discarding the presented Session object when determining the persistent reading mode. Since there is no current Controller at this stage, the Session object would otherwise be inacessible, and attempts to use Session::set/get would create an (ultimately discarded) object saved into Session::$default_session.

This was followed up with a general cleanup of the RequestFilter classes and PHPDoc. VersionedRequestFilter and RequestProcessor now actually implement the RequestFilter interface.

@tractorcow tractorcow referenced this pull request in silverstripe/silverstripe-cms
Closed

readingMode session value not persisting #962

@simonwelsh

Are you able to add a functional test that ensures this works?

@simonwelsh simonwelsh added the 3.1 label
@tractorcow

I should add tests to every pull request, no exceptions. I'll write some later. :)

@simonwelsh

I should add tests to every pull request, no exceptions.

There's hope for you yet! :p

@tractorcow

It's been a while, but test cases are finally here.

I can also confirm that the test cases fail with the original code.

@tractorcow

Well, that error didn't come up when I ran my test locally. Will investigate and sort this out.

@tractorcow

Test case runs fine locally... I've noticed travis and local test cases disagreeing whenever versioned comes into the picture. This is not the first test case I've found that produces this result.

Maybe this weekend (or if i happen to uncover this in the week) I'll figure out what the problem is.

@tractorcow

After lots of testing, I've found that the cause of this test failure (as well as several other test failures) is actually due to a disruptive test case (DirectorTest::testRequestFilterInDirectorTest) which was breaking the request filter. The problem is that there is no reliable solution to temporarily alter injected dependencies.

For the time being, this test case has been marked incomplete. It's necessary to develop a method for temporarily altering the injector instance, similar to the way that Config::nest and ::unnest (at least are meant to) behave.

@tractorcow

Merging of #3058 would allow the DirectorTest above to be re-enabled. If it's merged in I'll update this pull request to demonstrate this.

@tractorcow tractorcow added this to the 3.1.5 milestone
@hafriedlander
SilverStripe Ltd. member

Can't merge anymore because I merged PR #3058. Rebase plz?

@tractorcow tractorcow 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
ae573f8
@tractorcow

I've now rebased this, thank you @hafriedlander.

@hafriedlander hafriedlander merged commit 54cbb53 into silverstripe:3.1

2 checks passed

Details continuous-integration/travis-ci The Travis CI build passed
Details default Scrutinizer: No new issues — Travis: Passed
@tractorcow tractorcow deleted the tractorcow:pulls/3.1-requestfilter-versioned-fixes branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 6, 2014
  1. @tractorcow

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

    tractorcow committed with tractorcow
    BUG Disabled disruptive test case in DirectorTest
    API RequestProcessor and VersionedRequestFilter now both correctly implement RequestFilter
    Better PHPDoc on RequestFilter and implementations
View
10 control/Director.php
@@ -165,13 +165,13 @@ public static function direct($url, DataModel $model) {
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) {
- $response->output();
- }
+ if ($res !== false) {
+ $response->output();
+ }
}
// Handle a controller
} else if($result) {
View
14 control/RequestFilter.php
@@ -11,16 +11,24 @@
* @subpackage control
*/
interface RequestFilter {
+
/**
* Filter executed before a request processes
*
- * @return boolean (optional)
- * Whether to continue processing other filters
+ * @param SS_HTTPRequest $request Request container object
+ * @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);
-}
+}
View
17 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 @@ public function preRequest(SS_HTTPRequest $request, Session $session, DataModel
}
}
- /**
- * 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);
View
10 control/VersionedRequestFilter.php
@@ -5,13 +5,15 @@
* @package framework
* @subpackage control
*/
-class VersionedRequestFilter {
+class VersionedRequestFilter implements RequestFilter {
- public function preRequest() {
- Versioned::choose_site_stage();
+ public function preRequest(SS_HTTPRequest $request, Session $session, DataModel $model) {
+ Versioned::choose_site_stage($session);
+ return true;
}
- public function postRequest() {
+ public function postRequest(SS_HTTPRequest $request, SS_HTTPResponse $response, DataModel $model) {
+ return true;
}
}
View
12 model/Versioned.php
@@ -935,20 +935,24 @@ public function baseTable($stage = null) {
*
* 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");
View
36 tests/model/VersionedTest.php
@@ -533,6 +533,42 @@ public function testLazyLoadFields() {
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'
+ );
+
+ }
}
Something went wrong with that request. Please try again.