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 based GridField state management #11255

Open forsdahl opened 1 month ago

forsdahl commented 1 month ago

Description

The current GridField state management implementation is a bit lacking when dealing with multiple GridFields, especially with the new nested GridField component. It tries to handle GridField states by appending all GridField states as different request variables, and the code that does this is scattered in a few different places.

Our implementation of GridField state management is a GridFieldStateManager class that handles GridField states for all GridFields, not just the current one. The GridFieldStateManager implementation feels like a natural place to handle this, and it should preferably be the only place that handles it. The way our GridFieldStateManager does this is to store modified GridField states in session, and to append a single request parameter, via the addStateToURL-method, which is already used by all GridFieldStateAware components. This single request parameter simply contains a key, by which the stored GridField states are found from session. In this way it can handle GridField states via session differently per browser tab/window. With this implementation one could also in the future have different implementations of GridFieldStateManager which store the states somewhere else than session.

The implementation does require some changes to core except the GridFieldStateManager implementation, for example the addAllStateToUrl-method in GridField becomes much simpler.

Additional context or points of discussion

One point to consider here is if the GridFieldStateManager interface needs some new methods. It doesn't currently have any notion of when the GridFieldStateManager implementation should actually store a given GridField state. This means our implementation currently tries to figure this out when some GridFieldStateAware component calls the addStateToURL method, which isn't ideal. It then has to try to figure out if the state has actually changed from the default, and for performance reasons it only checks this once, even if the state might actually be changed later in the request. Ideally we could add a storeState method to the interface, which could actually be called from the GridState class whenever some of the state data changes (after all GridField_StateProvider components initDefaultState methods have been called).

Notes

Acceptance criteria

Excludes

PRs

maxime-rainville commented 1 month ago

Just a couple questions to make sure I understand what is happening.

None of those are deal breakers in my mind. We can maybe talk about those at our next catch up.

maxime-rainville commented 1 month ago

Other random questions. Have we considered storing the GridState elsewhere than the user session? e.g. Some sort of shared cache server side or in the DB.

I'm not suggesting we do this. Just pondering the "ifs".

Security wise, since the GridState is stored in a user's session, I take it there's no risk of one user gaining access to another user's GridState data? Even if there was, would we even care?

Is there a way we could do this without breaking change so people could start using this in CMS 5?

forsdahl commented 1 month ago

@maxime-rainville

  • Since this is tied to the user session, I take it that those URL are ephemeral? In other words, if you logout those states are gone. This would preclude the ability to bookmark a GridField URL with a specific search term or to send that link to another user.

You are correct, in our proposed solution those URLs are dependent on the user session. With that implementation you cannot send a link to a specific filtered GridField state to another user no. If your session expires, then yes the state will also be gone. I see no real solution to this when using session-based storing of the states, but in practice we haven't found this a problem. If a user needs to log in again, they do not in our experience expect all the state to be preserved.

  • In your proposed solution, when a request alters the GridState, GridFieldStateManager will create a unique key to track that new state. However, multiple GridField component could try to update the GridState within one request. That's why you want a GridFieldStateManager::storeState() method so you can update the state multiple times within one request without creating new keys.

You got this correct, that is precicely why I propose the storeState method to be added to the interface.

  • If someone manually recreates a state identical to a previous state in the user's session, I take the solution will still create a brand state key to add the URL?

Yes, if the request doesn't yet contain any state key, then a new one will be created and it is always random and not tied to any given state. If the request already contains a state key, then that key will be used.

Have we considered storing the GridState elsewhere than the user session? e.g. Some sort of shared cache server side or in the DB.

We have considered this internally, and we have actually thought about using some in-memory cache for this, for example Redis. With the proposed solution, one could easily just implement another GridFieldStateManager class for this.

Security wise, since the GridState is stored in a user's session, I take it there's no risk of one user gaining access to another user's GridState data? Even if there was, would we even care?

Storing the state in session should security-wise be safe. Since the default authentication handling is session-based anyway, it is no less secure than that.

Is there a way we could do this without breaking change so people could start using this in CMS 5?

If we just add the storeState method to the GridFieldStateManagerInterface, then that is a breaking change if someone has done their own implementation of GridFieldStateManager. I don't know how common it actually is to do this, I haven't seen any public modules that do it. However, if we really want the change to be non-breaking, we could split that storeState functionality out into another interface, so that implementing it is optional for the GridFieldStateManager implementation. In that way I see no real breaking change other than states now not being preserved when the session expires.

michalkleiner commented 1 month ago

Would it be possible to combine state from the URL and the local? So that the URL can contain an initial state that can be shared with the URL and then local stored state would override that, similar to how e.g. option objects can be merged.

forsdahl commented 1 month ago

Would it be possible to combine state from the URL and the local? So that the URL can contain an initial state that can be shared with the URL and then local stored state would override that, similar to how e.g. option objects can be merged.

One problem here is what states are included in the URL? Including all states for all GridFields in the URL is part of the original problem here. The states quickly get quite big as json-encoded text, and including for example all nested GridField states in the URL quickly becomes problematic. I have toyed with the idea to use just one request parameter for all GridField states, and encoding all the json-encoded states in some way to keep the total parameter size smaller. However, for example just base64 encoding them doesn't really help. you still end up with a very long parameter size.

michalkleiner commented 1 month ago

How many gridfields do you have on a page or in a form? In the URL only the state for the viewed ones would be needed, so we wouldn't need to include a state for all of them. Alternatively there could be a separate request to the backend to fetch the state by the gridfield identifier if the backend wanted to provide one. There we could mix predefined state with local state as well. That wouldn't work if the local state id was randomised, but is it really needed to be randomised? Sorry if I'm asking stupid questions about things that were already solved.

forsdahl commented 1 month ago

Its not really about how many GridFields are on a specific page or form, but how many are present in the whole path that took the user to the current edit form. The main problem to solve here is for the user to always end up with the same GridField state when going back one or more steps from the current view. Going back either via browser history, the back-button in the CMS or via breadcrumbs should all behave the same way, and the amount of GridFields that are affected might actually be quite many, especially with nested GridFields with GridFieldDetailForm components.

maxime-rainville commented 4 weeks ago

We've decided to retarget this to CMS 5 following a meteing with @forsdahl.

We'll have a follow up card to clean up a few rough edges in CMS 6: https://github.com/silverstripe/silverstripe-framework/issues/11267

We'll also create a follow up card implement behat test.