FIX Provide alternatives to session for storing GridField_FormAction state #8627

ScopeyNZ · 2018-11-21T04:02:44Z

Currently the session is used to store GridField action state. This PR doesn't change that by default. I've abstracted the storage method into an interface and provided an alternate that will just pass down the state to the DOM and then route it back when a request is built to perform an action.

I tried to go down the path of invalidating this action state but it's hard to tell exactly when the user navigates away from the GridField - especially as multiple GridFields can exist on one page, and some are loaded through AJAX requests.

Relies on silverstripe/silverstripe-admin#757

robbieaverill · 2018-11-21T08:33:10Z

src/Forms/GridField/FormAction/AttributeStore.php

+    public function load($id)
+    {
+        // Check the request
+        return json_decode($this->request->requestVar('ActionState'), true);


It might be worth casting the requestVar as a string, since json_decode(true, true) would return int(1). Maybe also update the PHPdoc to be explicit about what it can return instead of "mixed"

robbieaverill · 2018-11-21T08:34:48Z

src/Forms/GridField/FormAction/StateStore.php

+    /**
+     * @param HTTPRequest $request
+     */
+    public function __construct(HTTPRequest $request);


I'm in two minds about whether it's a good idea to have the constructor in this interface

I've just read this and now believe we should remove the constructor: https://www.alainschlesser.com/including-constructor-interface/

Yeah at the time I wasn't really happy with this code either. I need the request in both implementations of the interface and I don't think it's a good pattern to fetch the request from global state (Controller::curr or Injector). I'll just suck it up though; remove the constructor and use Controller::curr.

You could always give it a setter and use injector calls to provide it

Or put the constructor in an abstract parent class - separating the implementation from the interface entirely

Yeah having the constructor in an abstract is a possibility but then I'll have to do some introspection on the type of object configured for the interface which sounds pretty hard. I think I'll make an abstract but just use private static $dependencies. This would be much better with constructor DI 😉

robbieaverill · 2018-11-21T08:35:01Z

src/Forms/GridField/FormAction/StateStore.php

+     * Load state for a given ID
+     *
+     * @param string $id
+     * @return mixed


We should be more specific than this - are we expecting an array?

robbieaverill · 2018-11-21T16:41:00Z

There are also some minimal docs that could use a nudge too: https://docs.silverstripe.org/en/4/developer_guides/forms/field_types/gridfield/#saving-the-gridfield-state

ScopeyNZ · 2018-11-22T00:10:28Z

Thanks for the heads up. The GridField state is distinct form GridField_FormAction state. Something that took me a few moments to realise when I started this work 😅 .

I'll add a section about configuring it to use attributes/request params instead of session.

Otherwise I'm done with implementation now.

ScopeyNZ · 2018-11-22T00:17:30Z

Docs pushed 🙂

maxime-rainville

I had a look to make sure Session wasn't being referenced by GrdiFields outside of SessionStore.

Needs a changelog entry.
If practical, could we have some unit test for the following:
- SessionStore
- AttributeStore
- GridField_FormAction::getAttribute()

Didn't quite get to run locally to confirm the change work as expected. I'll do that tomorrow.

maxime-rainville · 2018-11-26T01:40:47Z

src/Forms/GridField/FormAction/AttributeStore.php

+/**
+ * Stores GridField action state on an attribute on the action and then analyses request parameters to load it back
+ */
+class AttributeStore extends AbstractRequestAwareStore implements StateStore


Shouldn't AbstractRequestAwareStore be the class implementing the StateStore interface, instead of its child class? Is there a case where you would want a RequestAwareStore that wasn't a StateStore?

Fair call. I'll change this and add some tests hopefully tomorrow.

src/Forms/GridField/FormAction/AttributeStore.php

src/Forms/GridField/GridField.php

maxime-rainville

Me happy. SessionStore is still implementing StateStore. Feel free to address this or not.

maxime-rainville · 2018-11-29T06:17:25Z

src/Forms/GridField/FormAction/SessionStore.php

+/**
+ * Stores GridField action state in the session in exactly the same way it has in the past
+ */
+class SessionStore extends AbstractRequestAwareStore implements StateStore


AbstractRequestAwareStore is implementing StateStore now. This implements statement is kind of redundant.

…state

…idFieldFilterHeader to add required attributes

…mActions

…elog entry

maxime-rainville

Still happy.

maxime-rainville · 2018-12-02T22:43:21Z

@ScopeyNZ The Travis fail isn't related to behat. It's something about the History Viewer controller. Is this related to this PR?

ScopeyNZ · 2018-12-02T22:45:02Z

I think this was just resolved in CMS. Rerunning the job.

maxime-rainville · 2018-12-02T22:52:09Z

Cool ... I'll merge on green.

ScopeyNZ mentioned this pull request Nov 21, 2018

NEW Allow state to be passed back on grid field actions silverstripe/silverstripe-admin#757

Merged

ScopeyNZ force-pushed the pulls/4.3/abstr-action-state branch from ee733a1 to 7a49b69 Compare November 21, 2018 04:10

robbieaverill reviewed Nov 21, 2018

View reviewed changes

ScopeyNZ self-assigned this Nov 21, 2018

ScopeyNZ force-pushed the pulls/4.3/abstr-action-state branch from 205c4f7 to 8283481 Compare November 22, 2018 00:06

ScopeyNZ force-pushed the pulls/4.3/abstr-action-state branch from 8283481 to f092a96 Compare November 22, 2018 00:17

maxime-rainville assigned maxime-rainville and unassigned ScopeyNZ Nov 22, 2018

Cheddam mentioned this pull request Nov 26, 2018

GridField actions cause heavy sessions, data is never cleaned up #8589

Closed

2 tasks

maxime-rainville requested changes Nov 26, 2018

View reviewed changes

src/Forms/GridField/GridField.php Show resolved Hide resolved

maxime-rainville approved these changes Nov 29, 2018

View reviewed changes

ScopeyNZ added 5 commits November 30, 2018 15:40

FIX Provide alternatives to session for storing GridField_FormAction …

b4c8f69

…state

Updating StateStore interface not to define a constructor & fixing Gr…

ddaa229

…idFieldFilterHeader to add required attributes

DOCS Adding notes on configuring the storage method for GridField_For…

aace1da

…mActions

DOCS Moving interface subscription to the abstract and adding a chang…

32d096d

…elog entry

FIX Switching to use Controller::curr as it was using previously

c7b5b1e

ScopeyNZ force-pushed the pulls/4.3/abstr-action-state branch from 85af377 to c7b5b1e Compare November 30, 2018 02:41

maxime-rainville approved these changes Dec 2, 2018

View reviewed changes

maxime-rainville merged commit 731ef00 into silverstripe:4.3 Dec 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Provide alternatives to session for storing GridField_FormAction state #8627

FIX Provide alternatives to session for storing GridField_FormAction state #8627

ScopeyNZ commented Nov 21, 2018 •

edited

Loading

robbieaverill Nov 21, 2018

robbieaverill Nov 21, 2018

robbieaverill Nov 21, 2018

ScopeyNZ Nov 21, 2018

robbieaverill Nov 21, 2018

robbieaverill Nov 21, 2018

ScopeyNZ Nov 21, 2018

robbieaverill Nov 21, 2018

robbieaverill commented Nov 21, 2018

ScopeyNZ commented Nov 22, 2018

ScopeyNZ commented Nov 22, 2018

maxime-rainville left a comment

maxime-rainville Nov 26, 2018

ScopeyNZ Nov 26, 2018

maxime-rainville left a comment

maxime-rainville Nov 29, 2018

maxime-rainville left a comment

maxime-rainville commented Dec 2, 2018 •

edited

Loading

ScopeyNZ commented Dec 2, 2018

maxime-rainville commented Dec 2, 2018

FIX Provide alternatives to session for storing GridField_FormAction state #8627

FIX Provide alternatives to session for storing GridField_FormAction state #8627

Conversation

ScopeyNZ commented Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbieaverill commented Nov 21, 2018

ScopeyNZ commented Nov 22, 2018

ScopeyNZ commented Nov 22, 2018

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville left a comment

Choose a reason for hiding this comment

maxime-rainville commented Dec 2, 2018 • edited Loading

ScopeyNZ commented Dec 2, 2018

maxime-rainville commented Dec 2, 2018

ScopeyNZ commented Nov 21, 2018 •

edited

Loading

maxime-rainville commented Dec 2, 2018 •

edited

Loading