Don't include default value in url grid state #9190

maxime-rainville · 2019-08-23T03:29:28Z

Parent issue

Resolves GridField: Exclude default values from persisted state (esp in URL) #9186

To do (from reviews)

Remove isDirty
Restore original toArray behaviour and create getChangesArray
~~[ ] Set this->defaults for getState's 2nd argument~~
~~[ ] Restore default application in GridFieldPaginator line 145~~
Implement GridField_StateProvider (sminnee)
Fix manual-page-selection feature and ensure that tests for the underlying issues are included.
Consider whether to hoist GridFieldStateAware functionally up from individual components to GridField itslef

robbieaverill · 2019-08-23T04:07:28Z

src/Forms/GridField/GridState_Data.php

+        foreach ($this->data as $name => $datum) {
+            if ($datum instanceof self && $datum->isDirty()) {
+                return true;
+            } else {


You don't need the else here, since you've returned already

If datum instance of self but it's not dirty, should you drop down into the second test or simply return clean?

maxime-rainville · 2019-08-26T06:28:35Z

Added some unit test, but I don't quite have them working.

sminnee · 2020-07-09T21:57:21Z

Oh I had no idea that you PRed my bug from last year lol. This would be great for my project; I might see if I have commit rights to open-sausages to rebase this and see if I can fix.

sminnee

A few changes recommended

src/Forms/GridField/GridState_Data.php

sminnee · 2020-08-09T22:10:32Z

src/Forms/GridField/GridState_Data.php

+     */
+    public function initDefaults(array $defaults): void
+    {
+        foreach ($defaults as $key => $name) {


$name should be $value, I think?

sminnee · 2020-08-09T22:12:12Z

src/Forms/GridField/GridState_Data.php

@@ -46,7 +65,7 @@ public function getData($name, $default = null)
            $this->data[$name] = $default;


As suggested here, we should probably set $this->defaults too

src/Forms/GridField/GridState_Data.php

sminnee · 2020-08-09T22:14:27Z

src/Forms/GridField/GridState_Data.php

+        foreach ($this->data as $name => $datum) {
+            if ($datum instanceof self && $datum->isDirty()) {
+                return true;
+            } else {


If datum instance of self but it's not dirty, should you drop down into the second test or simply return clean?

src/Forms/GridField/GridFieldPaginator.php

src/Forms/GridField/GridState_Data.php

sminnee · 2020-08-09T22:23:50Z

src/Forms/GridField/GridState_Data.php

+     * be considered dirty.
+     * @return bool
+     */
+    public function isDirty(): bool


What's the benefit of using isDirty() instead of relying on the content of toArray to do this work?

sminnee · 2020-08-10T00:05:26Z

One final thing for this: it still suffixes ?gridState-risk-0=%5B%5D on URLs.

sminnee · 2020-08-10T00:30:23Z

OK there is one major issue still remaining on this. In short, 'better buttons' are broken now.

That is because in GridFieldDetailForm_ItemRequest, this is called:

        $data = $state->getData();
        $paginator = $data->getData('GridFieldPaginator');
        if (!$paginator) {
            return false;
        }

This call doesn't trigger the generation of default values for GridFieldPaginator.

sminnee · 2020-08-10T10:07:59Z

Probably the best bet here is yet another GridField component interface, GridField_StateProvider, with an initDefaultState method. It can be called before GridState::getData() returns a value.

Keen to get feedback on that, as adding another interface is non-trivial, but it's an opt-in feature for component providers.

sminnee · 2020-08-12T00:29:34Z

@silverstripe/core-team any feedback on adding GridField_StateProvider as another GridField component interface, to let components define default state?

robbieaverill · 2020-08-12T14:58:39Z

Sounds like a decent idea to me. I'd be keen to hear @unclecheese's thoughts, as someone who probably has a good idea of where GridField is likely to go in React land

sminnee · 2020-08-12T19:57:24Z

Can I challenge the idea that GridField will go anywhere in React land? I imagine that starting from scratch with a grid-like control would be 100 times easier and you'd break everyone's plug-one regardless.

Perhaps Aaron has thoughts on that, though.

ScopeyNZ · 2020-08-12T21:18:32Z

Sounds good as long as it's GridField\StateProvider 😉

sminnee · 2020-08-12T21:42:33Z

Sounds good as long as it's GridField\StateProvider 😉

Is it worth being inconsistent with the rest of them to do that, though? Happy to go with consensus but consistency is what prompted my use of underscore.

unclecheese · 2020-08-12T21:47:54Z

Been a while since I've thought about this issue, but to my recollection, all the research on react gridfield was based on the assumption that it would be a parallel api to the traditional gridfield that devs could opt into (or more likely, out of).

It does mean a rewrite of components, but not as much as you'd think, and some may not even be affected. A lot of those plugins never touch the UI and only do backend work which affords more room for backward compatibility.

robbieaverill · 2020-08-12T22:01:12Z

I agree with @sminnee, consistency is more important.

We could create new namespaced interfaces and extend them from the old ones to prevent semver violations, but that could be done separately to this PR.

sminnee · 2020-08-12T23:06:18Z

OK I'll take "implement GridField_StateProvider" as an action and we can review in more detail when I have some code.

sminnee · 2020-08-13T21:53:15Z

@maxime-rainville with the new initDefaultState() method, the changes you made introducing getXXXState() methods isn't really necessary any more. I'm inclined to revert these changes and re-test, for two reasons:

Reduce the scope of the change to only the necessary stuff
Confirm that these changes aren't inadvertently papering over some required changes due to an API accidentally being changed.

Happy to get your feedback on that idea, though...

src/Forms/GridField/GridState_Data.php

src/Forms/GridField/GridField.php

…default

maxime-rainville

Humm ... look likes pagination is not working any more. When you try to punch in a page number to jump to, it won't let you do it if you're on page no 1. You can still use the arrows to navigate.

If you switch to a different page using the arrow, than it will allow you to jump to a specific page.

https://youtu.be/jqVGO22j4IA

I'll have a look. See if I can fix it.

maxime-rainville · 2020-08-17T11:26:56Z

Last commit should fix it. json_encode([]) outputs [] by default. We want it to output {} otherwise the front end gets confused and treats the empty state has an array instead of as an object.

I'll do some more manual testing tomorrow, to make sure everything works.

sminnee · 2020-08-17T20:19:44Z

Cheers; I might patch this PR into an internal production project as well to get some further testing. Given the number of "oops, found one more bug" iterations we've had it could use some more in-app testing before merge I think ;-)

sminnee · 2020-08-18T04:09:57Z

Nice find, Max! My initial testing confirms that this is working.

I've noticed a related bug that is pre-existing but might be worth looking into as part of this PR.

Open a modeladmin
Go to page 2
Open a detail record
Go back to the list by clicking the top menu

Result: The get param says page 2, but page 1 is shown

sminnee · 2020-08-18T04:38:29Z

OK I've had a look at this bug. It's a bit odd — there are 4 components that each take responsibility for fetching grid state from the URL, rather than GridField doing this for us...

sminnee · 2020-08-18T04:45:17Z

It looks like this was introduced by a couple of key commits:

cc71289 Betterbuttons was imported into the framework, which first required that gridstate was passed in a get var
b3093b7 GridFieldStateManager was created to allow nested gridfields and provide some abstraction for this

@unclecheese was the leader of both of these so would be good to get his view on whether the GridFieldStateAware should be hoisted up to the new GridField::initState() method that this PR creates.

A few things that need to be considered:

Use the history API to populate state in the URL when it changes
Check that going from page 4 to page 1 clears the grid state

sminnee · 2020-08-18T05:24:19Z

I've flagged 3 tickets above that might also be fixed by this.

maxime-rainville · 2020-08-18T10:47:52Z

Yeah ... there's a few things that are weird in the better-button/gridifield interaction.

Did some testing on this PR this afternoon. It doesn't introduce any new weirdness and makes the URLs definitively better. So my instinct would be to merge it as is and look at the other issues independently.

maxime-rainville · 2020-08-28T11:01:21Z

I've developed a fix for a related issue while working on this one silverstripe/silverstripe-admin#1103

sminnee mentioned this pull request Aug 23, 2019

GridField: Exclude default values from persisted state (esp in URL) #9186

Closed

1 task

maxime-rainville changed the title ~~Don't include default value in url grid state~~ Don't include default value in url grid state (WIP) Aug 23, 2019

robbieaverill reviewed Aug 23, 2019

View reviewed changes

robbieaverill added affects/v4 change/minor labels Aug 23, 2019

maxime-rainville force-pushed the pulls/4/test-state branch from af36407 to b6faea6 Compare August 23, 2019 05:15

sminnee self-assigned this Jul 9, 2020

sminnee self-requested a review July 9, 2020 21:57

sminnee force-pushed the pulls/4/test-state branch from 4e03f83 to 93495fe Compare August 9, 2020 22:01

sminnee requested changes Aug 9, 2020

View reviewed changes

sminnee changed the title ~~Don't include default value in url grid state (WIP)~~ [WIP] Don't include default value in url grid state Aug 12, 2020

sminnee force-pushed the pulls/4/test-state branch from 7bb657e to 09302e3 Compare August 13, 2020 02:07

sminnee changed the title ~~[WIP] Don't include default value in url grid state~~ Don't include default value in url grid state Aug 16, 2020

sminnee approved these changes Aug 16, 2020

View reviewed changes

maxime-rainville commented Aug 16, 2020

View reviewed changes

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

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

API Only include gridfield state value that differ from the expected …

70ffb32

…default

maxime-rainville force-pushed the pulls/4/test-state branch from 83b3a24 to 70ffb32 Compare August 17, 2020 00:12

maxime-rainville commented Aug 17, 2020

View reviewed changes

sminnee changed the title ~~Don't include default value in url grid state~~ [WIP] Don't include default value in url grid state Aug 17, 2020

Make sure GridState always outputs a JSON Object string

a43414d

Fix unit test

5226d96

This was referenced Aug 18, 2020

GridField should respect pagination and filtering on save #9556

Open

GridField Filter Tags Remove Button does not clear GridState GridFieldFilterHeader Column #9607

Open

GridFieldFilterHeader doesn't add search parameters to URL #9080

Closed

maxime-rainville changed the title ~~[WIP] Don't include default value in url grid state~~ Don't include default value in url grid state Aug 18, 2020

emteknetnz added impact/low type/enhancement complexity/medium labels Sep 8, 2020

bergice merged commit 4607151 into silverstripe:4 Nov 17, 2020

maxime-rainville deleted the pulls/4/test-state branch November 18, 2020 03:20

emteknetnz mentioned this pull request Jan 19, 2021

GridState breaks when clearing an advanced filter #9828

Closed

1 task

GuySartorelli mentioned this pull request May 22, 2022

Navigating back from a record in a GridField doesn't respect selected pagination or filters #9380

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't include default value in url grid state #9190

Don't include default value in url grid state #9190

maxime-rainville commented Aug 23, 2019 •

edited by bergice

Loading

robbieaverill Aug 23, 2019

sminnee Aug 9, 2020

maxime-rainville commented Aug 26, 2019

sminnee commented Jul 9, 2020

sminnee left a comment

sminnee Aug 9, 2020

sminnee Aug 9, 2020

sminnee Aug 9, 2020

sminnee Aug 9, 2020

sminnee commented Aug 10, 2020

sminnee commented Aug 10, 2020

sminnee commented Aug 10, 2020

sminnee commented Aug 12, 2020

robbieaverill commented Aug 12, 2020

sminnee commented Aug 12, 2020

ScopeyNZ commented Aug 12, 2020

sminnee commented Aug 12, 2020

unclecheese commented Aug 12, 2020

robbieaverill commented Aug 12, 2020

sminnee commented Aug 12, 2020

sminnee commented Aug 13, 2020

maxime-rainville left a comment •

edited

Loading

maxime-rainville commented Aug 17, 2020

sminnee commented Aug 17, 2020 •

edited

Loading

sminnee commented Aug 18, 2020

sminnee commented Aug 18, 2020

sminnee commented Aug 18, 2020

sminnee commented Aug 18, 2020

maxime-rainville commented Aug 18, 2020

maxime-rainville commented Aug 28, 2020

		@@ -46,7 +65,7 @@ public function getData($name, $default = null)
		$this->data[$name] = $default;

Don't include default value in url grid state #9190

Don't include default value in url grid state #9190

Conversation

maxime-rainville commented Aug 23, 2019 • edited by bergice Loading

Parent issue

To do (from reviews)

robbieaverill Aug 23, 2019

Choose a reason for hiding this comment

sminnee Aug 9, 2020

Choose a reason for hiding this comment

maxime-rainville commented Aug 26, 2019

sminnee commented Jul 9, 2020

sminnee left a comment

Choose a reason for hiding this comment

sminnee Aug 9, 2020

Choose a reason for hiding this comment

sminnee Aug 9, 2020

Choose a reason for hiding this comment

sminnee Aug 9, 2020

Choose a reason for hiding this comment

sminnee Aug 9, 2020

Choose a reason for hiding this comment

sminnee commented Aug 10, 2020

sminnee commented Aug 10, 2020

sminnee commented Aug 10, 2020

sminnee commented Aug 12, 2020

robbieaverill commented Aug 12, 2020

sminnee commented Aug 12, 2020

ScopeyNZ commented Aug 12, 2020

sminnee commented Aug 12, 2020

unclecheese commented Aug 12, 2020

robbieaverill commented Aug 12, 2020

sminnee commented Aug 12, 2020

sminnee commented Aug 13, 2020

maxime-rainville left a comment • edited Loading

Choose a reason for hiding this comment

maxime-rainville commented Aug 17, 2020

sminnee commented Aug 17, 2020 • edited Loading

sminnee commented Aug 18, 2020

sminnee commented Aug 18, 2020

sminnee commented Aug 18, 2020

sminnee commented Aug 18, 2020

maxime-rainville commented Aug 18, 2020

maxime-rainville commented Aug 28, 2020

maxime-rainville commented Aug 23, 2019 •

edited by bergice

Loading

maxime-rainville left a comment •

edited

Loading

sminnee commented Aug 17, 2020 •

edited

Loading