-
Notifications
You must be signed in to change notification settings - Fork 822
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 based GridField state management #11255
Comments
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. |
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? |
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.
You got this correct, that is precicely why I propose the
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.
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
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.
If we just add the |
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. |
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. |
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 |
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. TheGridFieldStateManager
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 theaddStateToURL
-method, which is already used by allGridFieldStateAware
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 theaddAllStateToUrl
-method inGridField
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 theGridFieldStateManager
implementation should actually store a given GridField state. This means our implementation currently tries to figure this out when someGridFieldStateAware
component calls theaddStateToURL
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 theGridState
class whenever some of the state data changes (after allGridField_StateProvider
componentsinitDefaultState
methods have been called).Notes
Acceptance criteria
SessionGridFieldStateManager
is created and stored the GridState in the user's session and targeted at CMS 5.GridFieldStateManagerInterface
.GridFieldStateManager
is still the default State manager in CMS 5.Excludes
PRs
The text was updated successfully, but these errors were encountered: