-
Notifications
You must be signed in to change notification settings - Fork 821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Session GridField state manager #11288
Open
forsdahl
wants to merge
5
commits into
silverstripe:5
Choose a base branch
from
creamarketing:session-gridfield-state-manager
base: 5
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4714a83
NEW Initial SessionGridFieldStateManager implementation, for managing
forsdahl d448708
Added Unit Tests for SessionGridFieldStateManager
forsdahl e9cd597
Fixed SessionGridFieldStateManager unit test to work regardless of
forsdahl 898ef9b
Removed custom interface for SessionGridFieldStateManager, the extra …
forsdahl 6b69db1
Fix for UnitTest failure with new SessionGridFieldStateManager functi…
forsdahl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -454,6 +454,13 @@ protected function getFormActions() | |||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
$gridState = $this->gridField->getState(false); | ||||||||||||||||||||||||||||||||||||
$actions->push(HiddenField::create($manager->getStateKey($this->gridField), null, $gridState)); | ||||||||||||||||||||||||||||||||||||
if (ClassInfo::hasMethod($manager, 'getStateRequestVar')) { | ||||||||||||||||||||||||||||||||||||
$stateRequestVar = $manager->getStateRequestVar(); | ||||||||||||||||||||||||||||||||||||
$stateValue = $this->getRequest()->requestVar($stateRequestVar); | ||||||||||||||||||||||||||||||||||||
if ($stateValue) { | ||||||||||||||||||||||||||||||||||||
$actions->push(HiddenField::create($stateRequestVar, '', $stateValue)); | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
Comment on lines
456
to
+463
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per #11288 (comment)
Suggested change
|
||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
$actions->push($this->getRightGroupField()); | ||||||||||||||||||||||||||||||||||||
} else { // adding new record | ||||||||||||||||||||||||||||||||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
<?php | ||
|
||
namespace SilverStripe\Forms\GridField; | ||
|
||
use SilverStripe\Control\Controller; | ||
use SilverStripe\Control\HTTPRequest; | ||
|
||
/** | ||
* Creates a unique key for managing GridField states in user Session, for both storage and retrieval. | ||
* Only stores states and generates a session key if a state is requested to be stored | ||
* (i.e. the state is changed from the default). | ||
* If a session state key is present in the request, it will always be used instead of generating a new one. | ||
*/ | ||
class SessionGridFieldStateManager implements GridFieldStateManagerInterface | ||
{ | ||
protected static $state_ids = []; | ||
|
||
protected function getStateID(GridField $gridField, $create = false): ?string | ||
{ | ||
$requestVar = $this->getStateRequestVar(); | ||
$sessionStateID = $gridField->getForm()?->getRequestHandler()->getRequest()->requestVar($requestVar); | ||
if (!$sessionStateID) { | ||
$sessionStateID = Controller::curr()->getRequest()->requestVar($requestVar); | ||
} | ||
if ($sessionStateID) { | ||
return $sessionStateID; | ||
} | ||
$stateKey = $this->getStateKey($gridField); | ||
if (isset(self::$state_ids[$stateKey])) { | ||
$sessionStateID = self::$state_ids[$stateKey]; | ||
} elseif ($create) { | ||
$sessionStateID = substr(md5(time()), 0, 8); | ||
// we don't want session state id to be strictly numeric, since this is used as a session key, | ||
// and session keys in php has to be usable as variable names | ||
if (is_numeric($sessionStateID)) { | ||
$sessionStateID .= 'a'; | ||
} | ||
self::$state_ids[$stateKey] = $sessionStateID; | ||
} | ||
return $sessionStateID; | ||
} | ||
|
||
public function storeState(GridField $gridField, $value = null) | ||
{ | ||
$sessionStateID = $this->getStateID($gridField, true); | ||
$sessionState = Controller::curr()->getRequest()->getSession()->get($sessionStateID); | ||
if (!$sessionState) { | ||
$sessionState = []; | ||
} | ||
$stateKey = $this->getStateKey($gridField); | ||
$sessionState[$stateKey] = $value ?? $gridField->getState(false)->Value(); | ||
Controller::curr()->getRequest()->getSession()->set($sessionStateID, $sessionState); | ||
} | ||
|
||
public function getStateRequestVar(): string | ||
{ | ||
return 'gridSessionState'; | ||
} | ||
|
||
/** | ||
* @param GridField $gridField | ||
* @return string | ||
*/ | ||
public function getStateKey(GridField $gridField): string | ||
{ | ||
$record = $gridField->getForm()?->getRecord(); | ||
return $gridField->getName() . '-' . ($record ? $record->ID : 0); | ||
} | ||
|
||
/** | ||
* @param GridField $gridField | ||
* @param string $url | ||
* @return string | ||
*/ | ||
public function addStateToURL(GridField $gridField, string $url): string | ||
{ | ||
$sessionStateID = $this->getStateID($gridField); | ||
if ($sessionStateID) { | ||
return Controller::join_links($url, '?' . $this->getStateRequestVar() . '=' . $sessionStateID); | ||
} | ||
return $url; | ||
} | ||
|
||
/** | ||
* @param GridField $gridField | ||
* @param HTTPRequest $request | ||
* @return string|null | ||
*/ | ||
public function getStateFromRequest(GridField $gridField, HTTPRequest $request): ?string | ||
{ | ||
$gridSessionStateID = $request->requestVar($this->getStateRequestVar()); | ||
if ($gridSessionStateID) { | ||
$sessionState = $request->getSession()->get($gridSessionStateID); | ||
$stateKey = $this->getStateKey($gridField); | ||
if ($sessionState && isset($sessionState[$stateKey])) { | ||
return $sessionState[$stateKey]; | ||
} | ||
} | ||
return null; | ||
} | ||
} |
140 changes: 140 additions & 0 deletions
140
tests/php/Forms/GridField/SessionGridFieldStateManagerTest.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
<?php | ||
|
||
namespace SilverStripe\Forms\Tests\GridField; | ||
|
||
use SilverStripe\Control\Controller; | ||
use SilverStripe\Control\HTTPRequest; | ||
use SilverStripe\Control\Session; | ||
use SilverStripe\Core\Injector\Injector; | ||
use SilverStripe\Dev\SapphireTest; | ||
use SilverStripe\Forms\FieldList; | ||
use SilverStripe\Forms\Form; | ||
use SilverStripe\Forms\GridField\GridField; | ||
use SilverStripe\Forms\GridField\GridFieldStateManagerInterface; | ||
use SilverStripe\Forms\GridField\SessionGridFieldStateManager; | ||
use SilverStripe\Forms\Tests\GridField\GridFieldPrintButtonTest\TestObject; | ||
|
||
class SessionGridFieldStateManagerTest extends SapphireTest | ||
{ | ||
protected function setUp(): void | ||
{ | ||
parent::setUp(); | ||
// configure the injector to use the session grid field state manager | ||
Injector::inst()->registerService(new SessionGridFieldStateManager(), GridFieldStateManagerInterface::class); | ||
} | ||
|
||
public function testStateKey() | ||
{ | ||
$manager = new SessionGridFieldStateManager(); | ||
$controller = new Controller(); | ||
$form1 = new Form($controller, 'form1', new FieldList(), new FieldList()); | ||
$testObject = new TestObject(); | ||
$testObject->ID = 1; | ||
$form2 = new Form($controller, 'form2', new FieldList(), new FieldList()); | ||
$form2->loadDataFrom($testObject); | ||
|
||
$grid1 = new GridField('A'); | ||
$grid2 = new GridField('B'); | ||
$grid1->setForm($form1); | ||
$grid2->setForm($form2); | ||
$this->assertEquals('A-0', $manager->getStateKey($grid1)); | ||
$this->assertEquals('B-1', $manager->getStateKey($grid2)); | ||
} | ||
|
||
public function testAddStateToURL() | ||
{ | ||
$manager = new SessionGridFieldStateManager(); | ||
$grid = new GridField('TestGrid'); | ||
$grid->getState()->testValue = 'foo'; | ||
$stateRequestVar = $manager->getStateRequestVar(); | ||
$link = '/link-to/something'; | ||
$this->assertTrue( | ||
preg_match( | ||
"|^$link\?{$stateRequestVar}=[a-zA-Z0-9]+$|", | ||
$manager->addStateToURL($grid, $link) | ||
) == 1 | ||
); | ||
|
||
$link = '/link-to/something-else?someParam=somevalue'; | ||
$this->assertTrue( | ||
preg_match( | ||
"|^/link-to/something-else\?someParam=somevalue&{$stateRequestVar}=[a-zA-Z0-9]+$|", | ||
$manager->addStateToURL($grid, $link) | ||
) == 1 | ||
); | ||
} | ||
|
||
public function testGetStateFromRequest() | ||
{ | ||
$manager = new SessionGridFieldStateManager(); | ||
|
||
$session = new Session([]); | ||
$request = new HTTPRequest( | ||
'GET', | ||
'/link-to/something', | ||
[ | ||
$manager->getStateRequestVar() => 'testGetStateFromRequest' | ||
] | ||
); | ||
$request->setSession($session); | ||
|
||
$controller = new Controller(); | ||
$controller->setRequest($request); | ||
$controller->pushCurrent(); | ||
$form = new Form($controller, 'form1', new FieldList(), new FieldList()); | ||
$grid = new GridField('TestGrid'); | ||
$grid->setForm($form); | ||
|
||
$grid->getState()->testValue = 'foo'; | ||
$state = $grid->getState(false)->Value() ?? '{}'; | ||
$result = $manager->getStateFromRequest($grid, $request); | ||
|
||
$this->assertEquals($state, $result); | ||
$controller->popCurrent(); | ||
} | ||
|
||
public function testDefaultStateLeavesURLUnchanged() | ||
{ | ||
$manager = new SessionGridFieldStateManager(); | ||
$grid = new GridField('DefaultStateGrid'); | ||
$grid->getState(false)->getData()->testValue->initDefaults(['foo' => 'bar']); | ||
$link = '/link-to/something'; | ||
|
||
$this->assertEquals('{}', $grid->getState(false)->Value()); | ||
|
||
$this->assertEquals( | ||
'/link-to/something', | ||
$manager->addStateToURL($grid, $link) | ||
); | ||
} | ||
|
||
public function testStoreState() | ||
{ | ||
$manager = new SessionGridFieldStateManager(); | ||
|
||
$session = new Session([]); | ||
$request = new HTTPRequest( | ||
'GET', | ||
'/link-to/something', | ||
[ | ||
$manager->getStateRequestVar() => 'testStoreState' | ||
] | ||
); | ||
$request->setSession($session); | ||
|
||
$controller = new Controller(); | ||
$controller->setRequest($request); | ||
$controller->pushCurrent(); | ||
$form = new Form($controller, 'form1', new FieldList(), new FieldList()); | ||
$grid = new GridField('TestGrid'); | ||
$grid->setForm($form); | ||
|
||
$grid->getState()->testValue = 'foo'; | ||
$state = $grid->getState(false)->Value() ?? '{}'; | ||
|
||
$manager->storeState($grid); | ||
$this->assertEquals($state, $session->get('testStoreState')['TestGrid-0']); | ||
|
||
$controller->popCurrent(); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both of these hidden fields? Can you please give me a short rundown of what they each do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not really need both no.
The old one is there for backwards compatibility with the default GridStateManager. That one contains the whole grid field state as json data.
The new one (from the
getStateRequestVar
method) needs a different value, i.e. the given state key value from the request. It is needed with SessionGridFieldStateManager to retain the session state key when for example saving a record.