Session GridField state manager

[forsdahl]

Description

Provides a new session-based GridField state manager, for storing all affected GridField states into session instead of in request variables.
Might be causing linting issues from the new required methods not being present in the current GridFieldStateManagerInterface interface, so manual checks have to be done for checking the state manager implementation for them.

Manual testing steps

Can be tested in a standard Silverstripe 5.x installation by adding the following yaml config:

---
Name: customgridfieldconfig
After: gridfieldconfig
---
SilverStripe\Core\Injector\Injector:
  SilverStripe\Forms\GridField\GridFieldStateManagerInterface:
    class: SilverStripe\Forms\GridField\SessionGridFieldStateManager

Issues

• Session based GridField state management #11255

Pull request checklist

• The target branch is correct
  • See picking the right version
• All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
• The commit messages follow our commit message guidelines
• The PR follows our contribution guidelines
• Code changes follow our coding conventions
• This change is covered with tests (or tests aren't necessary for this change)
• Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
• CI is green

[GuySartorelli]

I haven't done a full review yet but nothing in the code stands out as obviously bad. I'll aim to do a proper review in the next couple of days. In the meantime, there's some PHP unit test failures which look relevant.

Might be causing linting issues from the new required methods not being present in the current GridFieldStateManagerInterface interface

What linting issues do you mean? The CI PHP linting job is happy with it.

[GuySartorelli]

Had a quick look and it broadly looks good. I haven't checked every scenario yet. I did find a couple of things:

Existing class with similar funcitonality?

There is already a SilverStripe\Forms\GridField\FormAction\SessionStore class which seems to be storing and fetching some state related to GridField in the session - have you looked at that class at all to see how it relates to this new feature?

Child gridfield state is too sticky

In a scenario where you have a parent DataObject model (I'll call this Parent) in a ModelAdmin, and that has a many_many relation to another model (I'll call it Child):

1. Filter or sort the Parent gridfield inside the ModelAdmin
2. Click into one of the Parent records
3. Filter or sort the Child gridfield
4. Go back to the Parent gridfield inside ModelAdmin (either click back in the browser or use the breadcrumbs)
5. Click on the same Parent record again and view the Child gridfield

With the session-based state, the child gridfield is still filtered/sorted when I view it again.

With the existing URL-based state, the child gridfield is reset to its default state. It's debatable, but to me this is the desirable behaviour. It would confuse me if the child gridfield is always filtered whenever I am clicking through the parent record to get to it.

[GuySartorelli]

Do we need both of these hidden fields? Can you please give me a short rundown of what they each do?

[forsdahl]

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.

[forsdahl]

Child gridfield state is too sticky

Retaining that Child state is needed in some cases, for example when going one level further. If you filter the Child gridfield, and then click one record for editing and go back, it needs to retain its state in that scenario.
I agree that it would be preferred if it didn't retain the state when going back one step and opening the same record again, but it is unclear how to separate that scenario from the one where retaining the state is needed. So far retaining the state has had higher priority for us than the scenario where it feels too sticky.

[forsdahl]

Existing class with similar funcitonality?

I have looked at SilverStripe\Forms\GridField\FormAction\SessionStore yes, it is used for GridField_FormAction states. It could be used for actually saving and reading values from the current session (which is all it does), but would perhaps result in more code, and coupling between two otherwise unrelated classes.

[forsdahl]

What linting issues do you mean? The CI PHP linting job is happy with it.

Seems the CI linter doesn't mind that syntax like the linter in my IDE does, so no problem there after all.

[GuySartorelli]

I agree that it would be preferred if it didn't retain the state when going back one step and opening the same record again, but it is unclear how to separate that scenario from the one where retaining the state is needed. So far retaining the state has had higher priority for us than the scenario where it feels too sticky.

Is there a clean way to reset the state for all gridfields other than the current one and its "parents" when clicking on a child row?

If not, it's probably okay for CMS 5 since this is opt-in, but I wouldn't want to go ahead with #11267 with this sticky behaviour - and the whole point of this is to do #11267.

[GuySartorelli]

As per #11288 (comment)

Suggested change

	$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));
	}
	}
	if (ClassInfo::hasMethod($manager, 'getStateRequestVar')) {
	$stateRequestVar = $manager->getStateRequestVar();
	$stateValue = $this->getRequest()->requestVar($stateRequestVar);
	if ($stateValue) {
	$actions->push(HiddenField::create($stateRequestVar, '', $stateValue));
	}
	} else {
	$actions->push(HiddenField::create($manager->getStateKey($this->gridField), null, $gridState));
	}

[forsdahl]

This is now changed