Merge pull request #9072 from open-sausages/pulls/4.4/good-griddance

BUGFIX: Nested gridfields accessing incorrect URL state

_config/gridfield.yml

@@ -4,3 +4,5 @@ Name: gridfieldconfig
 SilverStripe\Core\Injector\Injector:
   SilverStripe\Forms\GridField\FormAction\StateStore:
     class: SilverStripe\Forms\GridField\FormAction\SessionStore
+  SilverStripe\Forms\GridField\GridFieldStateManagerInterface:
+    class: SilverStripe\Forms\GridField\GridFieldStateManager

src/Forms/GridField/GridFieldDetailForm.php

@@ -8,6 +8,7 @@ use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Control\RequestHandler;
 use SilverStripe\Core\ClassInfo;
 use SilverStripe\Core\Extensible;
+use SilverStripe\Core\Injector\Injectable;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\Validator;
@@ -30,7 +31,7 @@ use SilverStripe\ORM\Filterable;
 class GridFieldDetailForm implements GridField_URLHandler
 {
 
-    use Extensible;
+    use Extensible, Injectable, GridFieldStateAware;
 
     /**
      * @var string
@@ -106,15 +107,36 @@ class GridFieldDetailForm implements GridField_URLHandler
      */
     public function handleItem($gridField, $request)
     {
-        if ($gridStateStr = $request->getVar('gridState')) {
-            $gridField->getState(false)->setValue($gridStateStr);
-        }
-
         // Our getController could either give us a true Controller, if this is the top-level GridField.
         // It could also give us a RequestHandler in the form of GridFieldDetailForm_ItemRequest if this is a
         // nested GridField.
         $requestHandler = $gridField->getForm()->getController();
+        $record = $this->getRecordFromRequest($gridField, $request);
+        if (!$record) {
+            return $requestHandler->httpError(404, 'That record was not found');
+        }
+        $handler = $this->getItemRequestHandler($gridField, $record, $requestHandler);
+        $manager = $this->getStateManager();
+        if ($gridStateStr = $manager->getStateFromRequest($gridField, $request)) {
+            $gridField->getState(false)->setValue($gridStateStr);
+        }
 
+        // if no validator has been set on the GridField and the record has a
+        // CMS validator, use that.
+        if (!$this->getValidator() && ClassInfo::hasMethod($record, 'getCMSValidator')) {
+            $this->setValidator($record->getCMSValidator());
+        }
+
+        return $handler->handleRequest($request);
+    }
+
+    /**
+     * @param GridField $gridField
+     * @param HTTPRequest $request
+     * @return DataObject|null
+     */
+    protected function getRecordFromRequest(GridField $gridField, HTTPRequest $request): ?DataObject
+    {
         /** @var DataObject $record */
         if (is_numeric($request->param('ID'))) {
             /** @var Filterable $dataList */
@@ -124,15 +146,7 @@ class GridFieldDetailForm implements GridField_URLHandler
             $record = Injector::inst()->create($gridField->getModelClass());
         }
 
-        $handler = $this->getItemRequestHandler($gridField, $record, $requestHandler);
-
-        // if no validator has been set on the GridField and the record has a
-        // CMS validator, use that.
-        if (!$this->getValidator() && ClassInfo::hasMethod($record, 'getCMSValidator')) {
-            $this->setValidator($record->getCMSValidator());
-        }
-
-        return $handler->handleRequest($request);
+        return $record;
     }
 
     /**

src/Forms/GridField/GridFieldDetailForm_ItemRequest.php

@@ -27,12 +27,13 @@ use SilverStripe\View\SSViewer;
 
 class GridFieldDetailForm_ItemRequest extends RequestHandler
 {
+    use GridFieldStateAware;
 
-    private static $allowed_actions = array(
+    private static $allowed_actions = [
         'edit',
         'view',
         'ItemEditForm'
-    );
+    ];
 
     /**
      *
@@ -70,10 +71,10 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
      */
     protected $template = null;
 
-    private static $url_handlers = array(
+    private static $url_handlers = [
         '$Action!' => '$Action',
         '' => 'edit',
-    );
+    ];
 
     /**
      *
@@ -287,7 +288,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
         /** @var GridFieldDetailForm $component */
         $component = $this->gridField->getConfig()->getComponentByType(GridFieldDetailForm::class);
         $paginator = $this->getGridField()->getConfig()->getComponentByType(GridFieldPaginator::class);
-        $gridState = $this->getRequest()->requestVar('gridState');
+        $gridState = $this->getStateManager()->getStateFromRequest($this->gridField, $this->getRequest());
         if ($component && $paginator && $component->getShowPagination()) {
             $previousIsDisabled = !$this->getPreviousRecordID();
             $nextIsDisabled = !$this->getNextRecordID();
@@ -349,7 +350,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
     protected function getFormActions()
     {
         $actions = FieldList::create();
-
+        $manager = $this->getStateManager();
         if ($this->record->ID !== 0) { // existing record
             if ($this->record->canEdit()) {
                 $actions->push(FormAction::create('doSave', _t('SilverStripe\\Forms\\GridField\\GridFieldDetailForm.Save', 'Save'))
@@ -363,9 +364,9 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
                     ->addExtraClass('btn-outline-danger btn-hide-outline font-icon-trash-bin action--delete'));
             }
 
-            $gridState = $this->getRequest()->requestVar('gridState');
+            $gridState = $manager->getStateFromRequest($this->gridField, $this->getRequest());
             $this->gridField->getState(false)->setValue($gridState);
-            $actions->push(HiddenField::create('gridState', null, $gridState));
+            $actions->push(HiddenField::create($manager->getStateKey($this->gridField), null, $gridState));
 
             $actions->push($this->getRightGroupField());
         } else { // adding new record
@@ -498,12 +499,13 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
      */
     public function getEditLink($id)
     {
-        return Controller::join_links(
+        $link = Controller::join_links(
             $this->gridField->Link(),
             'item',
-            $id,
-            '?gridState=' . urlencode($this->gridField->getState(false)->Value())
+            $id
         );
+
+        return $this->getStateManager()->addStateToURL($this->gridField, $link);
     }
 
     /**
@@ -515,7 +517,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
         $gridField = $this->getGridField();
         $list = $gridField->getManipulatedList();
         $state = $gridField->getState(false);
-        $gridStateStr = $this->getRequest()->requestVar('gridState');
+        $gridStateStr = $this->getStateManager()->getStateFromRequest($this->gridField, $this->getRequest());
         if (!empty($gridStateStr)) {
             $state->setValue($gridStateStr);
         }

src/Forms/GridField/GridFieldEditButton.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\Forms\GridField;
 
 use SilverStripe\Control\Controller;
+use SilverStripe\Core\Injector\Injectable;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\View\ArrayData;
 use SilverStripe\View\SSViewer;
@@ -19,6 +20,8 @@ use SilverStripe\View\SSViewer;
  */
 class GridFieldEditButton implements GridField_ColumnProvider, GridField_ActionProvider, GridField_ActionMenuLink
 {
+    use Injectable, GridFieldStateAware;
+
     /**
      * HTML classes to be added to GridField edit buttons
      *
@@ -62,12 +65,13 @@ class GridFieldEditButton implements GridField_ColumnProvider, GridField_ActionP
      */
     public function getUrl($gridField, $record, $columnName)
     {
-        return Controller::join_links(
+        $link = Controller::join_links(
             $gridField->Link('item'),
             $record->ID,
-            'edit',
-            '?gridState=' . urlencode($gridField->getState(false)->Value())
+            'edit'
         );
+
+        return $this->getStateManager()->addStateToURL($gridField, $link);
     }
 
     /**

src/Forms/GridField/GridFieldStateAware.php

@@ -0,0 +1,40 @@
+<?php
+
+
+namespace SilverStripe\Forms\GridField;
+
+use SilverStripe\Core\Injector\Injector;
+
+/**
+ * A trait that makes a class able to consume and use a {@link GridFieldStateManagerInterface}
+ * implementation
+ */
+trait GridFieldStateAware
+{
+    /**
+     * @var GridFieldStateManagerInterface
+     */
+    protected $stateManager;
+
+    /**
+     * @param GridFieldStateManagerInterface $manager
+     * @return self
+     */
+    public function setStateManager(GridFieldStateManagerInterface $manager): self
+    {
+        $this->stateManager = $manager;
+
+        return $this;
+    }
+
+    /**
+     * Fallback on the direct Injector access, but allow a custom implementation
+     * to be applied
+     *
+     * @return GridFieldStateManagerInterface
+     */
+    public function getStateManager(): GridFieldStateManagerInterface
+    {
+        return $this->stateManager ?: Injector::inst()->get(GridFieldStateManagerInterface::class);
+    }
+}

src/Forms/GridField/GridFieldStateManager.php

@@ -0,0 +1,56 @@
+<?php
+
+
+namespace SilverStripe\Forms\GridField;
+
+use SilverStripe\Control\HTTP;
+use SilverStripe\Control\HTTPRequest;
+
+/**
+ * Creates a unique key for the gridfield, and uses that to write to and retrieve
+ * its state from the request
+ */
+class GridFieldStateManager implements GridFieldStateManagerInterface
+{
+    /**
+     * @param GridField $gridField
+     * @return string
+     */
+    public function getStateKey(GridField $gridField): string
+    {
+        $i = 0;
+        $form = $gridField->getForm();
+        if ($form) {
+            $controller = $form->getController();
+            while ($controller instanceof GridFieldDetailForm_ItemRequest) {
+                $controller = $controller->getController();
+                $i++;
+            }
+        }
+
+        return sprintf('%s-%s-%s', 'gridState', $gridField->getName(), $i);
+    }
+
+    /**
+     * @param GridField $gridField
+     * @param string $url
+     * @return string
+     */
+    public function addStateToURL(GridField $gridField, string $url): string
+    {
+        $key = $this->getStateKey($gridField);
+        $value = $gridField->getState(false)->Value();
+
+        return HTTP::setGetVar($key, $value, $url);
+    }
+
+    /**
+     * @param GridField $gridField
+     * @param HTTPRequest $request
+     * @return string|null
+     */
+    public function getStateFromRequest(GridField $gridField, HTTPRequest $request): ?string
+    {
+        return $request->requestVar($this->getStateKey($gridField));
+    }
+}

src/Forms/GridField/GridFieldStateManagerInterface.php

@@ -0,0 +1,33 @@
+<?php
+
+
+namespace SilverStripe\Forms\GridField;
+
+use SilverStripe\Control\HTTPRequest;
+
+/**
+ * Defines a class that can create a key for a gridfield and apply its
+ * state to a request, and consume state from the request
+ */
+interface GridFieldStateManagerInterface
+{
+    /**
+     * @param GridField $gridField
+     * @return string
+     */
+    public function getStateKey(GridField $gridField): string;
+
+    /**
+     * @param GridField $gridField
+     * @param string $url
+     * @return string
+     */
+    public function addStateToURL(GridField $gridField, string $url): string;
+
+    /**
+     * @param GridField $gridField
+     * @param HTTPRequest $request
+     * @return string|null
+     */
+    public function getStateFromRequest(GridField $gridField, HTTPRequest $request): ?string;
+}

tests/php/Forms/GridField/GridFieldStateManagerTest.php

@@ -0,0 +1,82 @@
+<?php
+
+namespace SilverStripe\Forms\Tests\GridField;
+
+use SilverStripe\Control\Controller;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Forms\FieldList;
+use SilverStripe\Forms\GridField\GridField;
+use SilverStripe\Forms\GridField\GridFieldDetailForm;
+use SilverStripe\Forms\GridField\GridFieldDetailForm_ItemRequest;
+use SilverStripe\Forms\GridField\GridFieldStateManager;
+use SilverStripe\Forms\Form;
+use SilverStripe\Forms\Tests\GridField\GridFieldPrintButtonTest\TestObject;
+use SilverStripe\View\ArrayData;
+
+class GridFieldStateManagerTest extends SapphireTest
+{
+    public function testStateKey()
+    {
+        $manager = new GridFieldStateManager();
+        $controller = new Controller();
+        $form1 = new Form($controller, 'form1', new FieldList(), new FieldList());
+        $itemRequest = new GridFieldDetailForm_ItemRequest(
+            new GridField('test'),
+            new GridFieldDetailForm(),
+            new TestObject(),
+            $controller,
+            'itemRequest'
+        );
+        $form2 = new Form($itemRequest, 'form1', new FieldList(), new FieldList());
+
+        $grid1 = new GridField('A');
+        $grid2 = new GridField('B');
+        $grid1->setForm($form1);
+        $grid2->setForm($form2);
+        $this->assertEquals('gridState-A-0', $manager->getStateKey($grid1));
+        $this->assertEquals('gridState-B-1', $manager->getStateKey($grid2));
+    }
+
+    public function testAddStateToURL()
+    {
+        $manager = new GridFieldStateManager();
+        $grid = new GridField('TestGrid');
+        $grid->getState()->testValue = 'foo';
+        $link = '/link-to/something';
+        $state = $grid->getState(false)->Value();
+        $this->assertEquals(
+            '/link-to/something?gridState-TestGrid-0=' . urlencode($state),
+            $manager->addStateToURL($grid, $link)
+        );
+
+        $link = '/link-to/something-else?someParam=somevalue';
+        $state = $grid->getState(false)->Value();
+        $this->assertEquals(
+            '/link-to/something-else?someParam=somevalue&gridState-TestGrid-0=' . urlencode($state),
+            $manager->addStateToURL($grid, $link)
+        );
+    }
+
+    public function testGetStateFromRequest()
+    {
+        $manager = new GridFieldStateManager();
+        $controller = new Controller();
+        $form = new Form($controller, 'form1', new FieldList(), new FieldList());
+        $grid = new GridField('TestGrid');
+        $grid->setForm($form);
+
+        $grid->getState()->testValue = 'foo';
+        $state = urlencode($grid->getState(false)->Value());
+        $request = new HTTPRequest(
+            'GET',
+            '/link-to/something',
+            [
+                $manager->getStateKey($grid) => $state
+            ]
+        );
+        $result = $manager->getStateFromRequest($grid, $request);
+
+        $this->assertEquals($state, $result);
+    }
+}