silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 820 forks source link

Session GridField state manager #11288

Open forsdahl opened 1 week ago

forsdahl commented 1 week ago

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

Pull request checklist

GuySartorelli commented 5 days ago

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.

forsdahl commented 2 days ago

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 commented 2 days ago

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 commented 2 days ago

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.