From 4b4fbabed5d70bf577e4b0d6fdbc9dab9da80451 Mon Sep 17 00:00:00 2001 From: Serge Latyntcev Date: Thu, 8 Nov 2018 14:36:16 +1300 Subject: [PATCH 01/12] FIX TreeMultiselectField passes value 'unchanged' as null to ORM for 'ID' column key --- src/Forms/TreeMultiselectField.php | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/Forms/TreeMultiselectField.php b/src/Forms/TreeMultiselectField.php index 72de4dc11..1c0166f64 100644 --- a/src/Forms/TreeMultiselectField.php +++ b/src/Forms/TreeMultiselectField.php @@ -261,4 +261,30 @@ class TreeMultiselectField extends TreeDropdownField $copy->setTitleField($this->getTitleField()); return $copy; } + + /** + * {@inheritdoc} + * + * @deprecated 4.0..5.0 + */ + protected function objectForKey($key) + { + /** + * Fixes https://github.com/silverstripe/silverstripe-framework/issues/8332 + * + * Due to historic reasons, the default (empty) value for this field is 'unchanged', even though + * the field is usually integer on the database side. + * MySQL handles that gracefully and returns an empty result in that case, + * whereas some other databases (e.g. PostgreSQL) do not support comparison + * of numeric types with string values, issuing a database error. + * + * This fix is not ideal, but supposed to keep backward compatibility for SS4. + * Since SS5 this method should be removed and NULL should be used instead of 'unchanged'. + */ + if ($this->getKeyField() ==='ID' && $key === 'unchanged') { + $key = null; + } + + return parent::objectForKey($key); + } } From 15aaf9db9fe1679cf8b01b74fce3eee841278495 Mon Sep 17 00:00:00 2001 From: Serge Latyntcev Date: Tue, 13 Nov 2018 10:20:49 +1300 Subject: [PATCH 02/12] Fix a code style typo --- src/Forms/TreeMultiselectField.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Forms/TreeMultiselectField.php b/src/Forms/TreeMultiselectField.php index 1c0166f64..422b5739a 100644 --- a/src/Forms/TreeMultiselectField.php +++ b/src/Forms/TreeMultiselectField.php @@ -265,7 +265,7 @@ class TreeMultiselectField extends TreeDropdownField /** * {@inheritdoc} * - * @deprecated 4.0..5.0 + * @internal To be removed in 5.0 */ protected function objectForKey($key) { @@ -281,7 +281,7 @@ class TreeMultiselectField extends TreeDropdownField * This fix is not ideal, but supposed to keep backward compatibility for SS4. * Since SS5 this method should be removed and NULL should be used instead of 'unchanged'. */ - if ($this->getKeyField() ==='ID' && $key === 'unchanged') { + if ($this->getKeyField() === 'ID' && $key === 'unchanged') { $key = null; } From 80885fc23111e1ebee9bc1540cabc30c7398555e Mon Sep 17 00:00:00 2001 From: Serge Latyntcev Date: Tue, 20 Nov 2018 14:04:12 +1300 Subject: [PATCH 03/12] ADD php test TreeMultiselectField::testEmptyChoiceReadonly --- tests/php/Forms/TreeMultiselectFieldTest.php | 33 ++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/tests/php/Forms/TreeMultiselectFieldTest.php b/tests/php/Forms/TreeMultiselectFieldTest.php index cbb540c6d..ffff5491f 100644 --- a/tests/php/Forms/TreeMultiselectFieldTest.php +++ b/tests/php/Forms/TreeMultiselectFieldTest.php @@ -10,14 +10,43 @@ class TreeMultiselectFieldTest extends SapphireTest { protected static $fixture_file = 'TreeDropdownFieldTest.yml'; + public function testEmptyChoiceReadonly() + { + $field = new TreeMultiselectField('TestTree', 'Test tree', File::class); + + $asdf = $this->objFromFixture(File::class, 'asdf'); + $subfolderfile1 = $this->objFromFixture(File::class, 'subfolderfile1'); + + $schemaStateDefaults = $field->getSchemaStateDefaults(); + $this->assertArraySubset(['id' => 'TestTree', 'name' => 'TestTree', 'value' => 'unchanged'], $schemaStateDefaults, $strict = true); + + $html = $field->performReadonlyTransformation()->Field(); + + $this->assertEquals( + <<<"HTML" + + ('none') + +HTML + , + $html + ); + } + public function testReadonly() { $field = new TreeMultiselectField('TestTree', 'Test tree', File::class); + $asdf = $this->objFromFixture(File::class, 'asdf'); $subfolderfile1 = $this->objFromFixture(File::class, 'subfolderfile1'); + $field->setValue(implode(',', [$asdf->ID, $subfolderfile1->ID])); - $readonlyField = $field->performReadonlyTransformation(); + $schemaStateDefaults = $field->getSchemaStateDefaults(); + $this->assertArraySubset(['id' => 'TestTree', 'name' => 'TestTree', 'value' => [$asdf->ID, $subfolderfile1->ID]], $schemaStateDefaults, $strict = true); + + $html = (string)$field->performReadonlyTransformation()->Field(); + $this->assertEquals( <<<"HTML" @@ -25,7 +54,7 @@ class TreeMultiselectFieldTest extends SapphireTest HTML , - (string)$readonlyField->Field() + $html ); } } From 9ce6d91b76e525a6fc81e02023e9e53cdf82e047 Mon Sep 17 00:00:00 2001 From: Serge Latyntcev Date: Thu, 22 Nov 2018 12:11:18 +1300 Subject: [PATCH 04/12] FIX / TreeMultiselectField::objectForKey handles list of IDs correctly --- src/Forms/TreeMultiselectField.php | 6 +++++- tests/php/Forms/TreeMultiselectFieldTest.php | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Forms/TreeMultiselectField.php b/src/Forms/TreeMultiselectField.php index 422b5739a..372890f6b 100644 --- a/src/Forms/TreeMultiselectField.php +++ b/src/Forms/TreeMultiselectField.php @@ -279,10 +279,14 @@ class TreeMultiselectField extends TreeDropdownField * of numeric types with string values, issuing a database error. * * This fix is not ideal, but supposed to keep backward compatibility for SS4. - * Since SS5 this method should be removed and NULL should be used instead of 'unchanged'. + * + * In 5.0 this method to be removed and NULL should be used instead of 'unchanged' (or an empty array. to be decided). + * In 5.0 this class to be refactored so that $this->value is always an array of values (or null) */ if ($this->getKeyField() === 'ID' && $key === 'unchanged') { $key = null; + } elseif (is_string($key)) { + $key = preg_split('/\s*,\s*/', trim($key)); } return parent::objectForKey($key); diff --git a/tests/php/Forms/TreeMultiselectFieldTest.php b/tests/php/Forms/TreeMultiselectFieldTest.php index ffff5491f..2686b71a2 100644 --- a/tests/php/Forms/TreeMultiselectFieldTest.php +++ b/tests/php/Forms/TreeMultiselectFieldTest.php @@ -20,6 +20,9 @@ class TreeMultiselectFieldTest extends SapphireTest $schemaStateDefaults = $field->getSchemaStateDefaults(); $this->assertArraySubset(['id' => 'TestTree', 'name' => 'TestTree', 'value' => 'unchanged'], $schemaStateDefaults, $strict = true); + $items = $field->getItems(); + $this->assertCount(0, $items, $message = 'there must be no items selected'); + $html = $field->performReadonlyTransformation()->Field(); $this->assertEquals( @@ -45,6 +48,9 @@ HTML $schemaStateDefaults = $field->getSchemaStateDefaults(); $this->assertArraySubset(['id' => 'TestTree', 'name' => 'TestTree', 'value' => [$asdf->ID, $subfolderfile1->ID]], $schemaStateDefaults, $strict = true); + $items = $field->getItems(); + $this->assertCount(2, $items, $message = 'there must be exactly 2 items selected'); + $html = (string)$field->performReadonlyTransformation()->Field(); $this->assertEquals( From f526c794fcb5f1ba71ab5c047030735ffc3bc49d Mon Sep 17 00:00:00 2001 From: Serge Latyntcev Date: Fri, 23 Nov 2018 15:17:17 +1300 Subject: [PATCH 05/12] Minor / Refactor php tests for TreeMultiselectField --- tests/php/Forms/TreeMultiselectFieldTest.php | 265 ++++++++++++++++--- 1 file changed, 231 insertions(+), 34 deletions(-) diff --git a/tests/php/Forms/TreeMultiselectFieldTest.php b/tests/php/Forms/TreeMultiselectFieldTest.php index 2686b71a2..1eabcb220 100644 --- a/tests/php/Forms/TreeMultiselectFieldTest.php +++ b/tests/php/Forms/TreeMultiselectFieldTest.php @@ -4,63 +4,260 @@ namespace SilverStripe\Forms\Tests; use SilverStripe\Assets\File; use SilverStripe\Dev\SapphireTest; +use SilverStripe\Forms\Form; +use SilverStripe\Forms\FormTemplateHelper; use SilverStripe\Forms\TreeMultiselectField; class TreeMultiselectFieldTest extends SapphireTest { protected static $fixture_file = 'TreeDropdownFieldTest.yml'; - public function testEmptyChoiceReadonly() + protected $formId = 'TheFormID'; + protected $fieldName = 'TestTree'; + + /** + * {@inheritdoc} + */ + public function setUp() { - $field = new TreeMultiselectField('TestTree', 'Test tree', File::class); + parent::setUp(); - $asdf = $this->objFromFixture(File::class, 'asdf'); - $subfolderfile1 = $this->objFromFixture(File::class, 'subfolderfile1'); + $this->form = $this->buildFormMock(); + $this->field = $this->buildField($this->form); + $this->folders = $this->loadFolders(); - $schemaStateDefaults = $field->getSchemaStateDefaults(); - $this->assertArraySubset(['id' => 'TestTree', 'name' => 'TestTree', 'value' => 'unchanged'], $schemaStateDefaults, $strict = true); - - $items = $field->getItems(); - $this->assertCount(0, $items, $message = 'there must be no items selected'); - - $html = $field->performReadonlyTransformation()->Field(); - - $this->assertEquals( - <<<"HTML" - - ('none') - -HTML - , - $html + $this->folderIds = array_map( + static function ($f) { + return $f->ID; + }, + $this->folders ); + $this->fieldValue = implode(',', $this->folderIds); } - public function testReadonly() + /** + * Build a new mock object of a Form + * + * @return Form + */ + protected function buildFormMock() { - $field = new TreeMultiselectField('TestTree', 'Test tree', File::class); + $form = $this->createMock(Form::class); + $form->method('getTemplateHelper') + ->willReturn(FormTemplateHelper::singleton()); + + $form->method('getHTMLID') + ->willReturn($this->formId); + + return $form; + } + + /** + * Build a new instance of TreeMultiselectField + * + * @param Form $form The field form + * + * @return TreeMultiselectField + */ + protected function buildField(Form $form) + { + $field = new TreeMultiselectField($this->fieldName, 'Test tree', File::class); + $field->setForm($form); + + return $field; + } + + /** + * Load several files from the fixtures and return them in an array + * + * @return File[] + */ + protected function loadFolders() + { $asdf = $this->objFromFixture(File::class, 'asdf'); $subfolderfile1 = $this->objFromFixture(File::class, 'subfolderfile1'); - $field->setValue(implode(',', [$asdf->ID, $subfolderfile1->ID])); + return [$asdf, $subfolderfile1]; + } + + /** + * Test the TreeMultiselectField behaviour with no selected values + */ + public function testEmpty() + { + $field = $this->field; + + $fieldId = $field->ID(); + $this->assertEquals($fieldId, sprintf('%s_%s', $this->formId, $this->fieldName)); $schemaStateDefaults = $field->getSchemaStateDefaults(); - $this->assertArraySubset(['id' => 'TestTree', 'name' => 'TestTree', 'value' => [$asdf->ID, $subfolderfile1->ID]], $schemaStateDefaults, $strict = true); + $this->assertArraySubset( + [ + 'id' => $fieldId, + 'name' => $this->fieldName, + 'value' => 'unchanged' + ], + $schemaStateDefaults, + true + ); + + $schemaDataDefaults = $field->getSchemaDataDefaults(); + $this->assertArraySubset( + [ + 'id' => $fieldId, + 'name' => $this->fieldName, + 'type' => 'text', + 'schemaType' => 'SingleSelect', + 'component' => 'TreeDropdownField', + 'holderId' => sprintf('%s_Holder', $fieldId), + 'title' => 'Test tree', + 'extraClass' => 'treemultiselect multiple searchable', + 'data' => [ + 'urlTree' => 'field/TestTree/tree', + 'showSearch' => true, + 'emptyString' => '(Choose File)', + 'hasEmptyDefault' => false, + 'multiple' => true + ] + ], + $schemaDataDefaults, + true + ); $items = $field->getItems(); - $this->assertCount(2, $items, $message = 'there must be exactly 2 items selected'); + $this->assertCount(0, $items, 'there must be no items selected'); - $html = (string)$field->performReadonlyTransformation()->Field(); + $html = $field->Field(); + $this->assertContains($field->ID(), $html); + $this->assertContains('unchanged', $html); + } - $this->assertEquals( - <<<"HTML" - - <Special & characters>, TestFile1InSubfolder - -HTML - , - $html + + /** + * Test the field with some values set + */ + public function testChanged() + { + $field = $this->field; + + $field->setValue($this->fieldValue); + + $schemaStateDefaults = $field->getSchemaStateDefaults(); + $this->assertArraySubset( + [ + 'id' => $field->ID(), + 'name' => 'TestTree', + 'value' => $this->folderIds + ], + $schemaStateDefaults, + true ); + + $items = $field->getItems(); + $this->assertCount(2, $items, 'there must be exactly 2 items selected'); + + $html = $field->Field(); + $this->assertContains($field->ID(), $html); + $this->assertContains($this->fieldValue, $html); + } + + /** + * Test empty field in readonly mode + */ + public function testEmptyReadonly() + { + $field = $this->field->performReadonlyTransformation(); + + $schemaStateDefaults = $field->getSchemaStateDefaults(); + $this->assertArraySubset( + [ + 'id' => $field->ID(), + 'name' => 'TestTree', + 'value' => 'unchanged' + ], + $schemaStateDefaults, + true + ); + + $schemaDataDefaults = $field->getSchemaDataDefaults(); + $this->assertArraySubset( + [ + 'id' => $field->ID(), + 'name' => $this->fieldName, + 'type' => 'text', + 'schemaType' => 'SingleSelect', + 'component' => 'TreeDropdownField', + 'holderId' => sprintf('%s_Holder', $field->ID()), + 'title' => 'Test tree', + 'extraClass' => 'treemultiselectfield_readonly multiple searchable', + 'data' => [ + 'urlTree' => 'field/TestTree/tree', + 'showSearch' => true, + 'emptyString' => '(Choose File)', + 'hasEmptyDefault' => false, + 'multiple' => true + ] + ], + $schemaDataDefaults, + true + ); + + $items = $field->getItems(); + $this->assertCount(0, $items, 'there must be 0 selected items'); + + $html = $field->Field(); + $this->assertContains($field->ID(), $html); + } + + /** + * Test changed field in readonly mode + */ + public function testChangedReadonly() + { + $field = $this->field; + $field->setValue($this->fieldValue); + $field = $field->performReadonlyTransformation(); + + $schemaStateDefaults = $field->getSchemaStateDefaults(); + $this->assertArraySubset( + [ + 'id' => $field->ID(), + 'name' => 'TestTree', + 'value' => $this->folderIds + ], + $schemaStateDefaults, + true + ); + + $schemaDataDefaults = $field->getSchemaDataDefaults(); + $this->assertArraySubset( + [ + 'id' => $field->ID(), + 'name' => $this->fieldName, + 'type' => 'text', + 'schemaType' => 'SingleSelect', + 'component' => 'TreeDropdownField', + 'holderId' => sprintf('%s_Holder', $field->ID()), + 'title' => 'Test tree', + 'extraClass' => 'treemultiselectfield_readonly multiple searchable', + 'data' => [ + 'urlTree' => 'field/TestTree/tree', + 'showSearch' => true, + 'emptyString' => '(Choose File)', + 'hasEmptyDefault' => false, + 'multiple' => true + ] + ], + $schemaDataDefaults, + true + ); + + $items = $field->getItems(); + $this->assertCount(2, $items, 'there must be exactly 2 selected items'); + + $html = $field->Field(); + $this->assertContains($field->ID(), $html); + $this->assertContains($this->fieldValue, $html); } } From 38f8217f019576c341426e155588a39d30854548 Mon Sep 17 00:00:00 2001 From: Serge Latyntcev Date: Thu, 29 Nov 2018 09:55:28 +1300 Subject: [PATCH 06/12] TreeMultiselectFieldTest / setUp is protected in PHPUnit5 --- tests/php/Forms/TreeMultiselectFieldTest.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/php/Forms/TreeMultiselectFieldTest.php b/tests/php/Forms/TreeMultiselectFieldTest.php index 1eabcb220..a6a431f2b 100644 --- a/tests/php/Forms/TreeMultiselectFieldTest.php +++ b/tests/php/Forms/TreeMultiselectFieldTest.php @@ -15,10 +15,7 @@ class TreeMultiselectFieldTest extends SapphireTest protected $formId = 'TheFormID'; protected $fieldName = 'TestTree'; - /** - * {@inheritdoc} - */ - public function setUp() + protected function setUp() { parent::setUp(); From 4ee63eb4e7e379b35c7ed6570eb3e438d08ae7cb Mon Sep 17 00:00:00 2001 From: Serge Latyntcev Date: Thu, 29 Nov 2018 12:13:56 +1300 Subject: [PATCH 07/12] TreeMultiselectFieldTest / make scrutinizer happy --- tests/php/Forms/TreeMultiselectFieldTest.php | 35 ++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/php/Forms/TreeMultiselectFieldTest.php b/tests/php/Forms/TreeMultiselectFieldTest.php index a6a431f2b..5b2f14a65 100644 --- a/tests/php/Forms/TreeMultiselectFieldTest.php +++ b/tests/php/Forms/TreeMultiselectFieldTest.php @@ -15,6 +15,41 @@ class TreeMultiselectFieldTest extends SapphireTest protected $formId = 'TheFormID'; protected $fieldName = 'TestTree'; + /** + * Mock object of a generic form + * + * @var Form + */ + protected $form; + + /** + * Instance of the TreeMultiselectField + * + * @var TreeMultiselectField + */ + protected $field; + + /** + * The File objects of folders loaded from the fixture + * + * @var File[] + */ + protected $folders; + + /** + * The array of folder ids + * + * @var int[] + */ + protected $folderIds; + + /** + * Concatenated folder ids for use as a value for the field + * + * @var string + */ + protected $fieldValue; + protected function setUp() { parent::setUp(); From b4c8f699eb4dd75089100c1b180cd0df25146206 Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Wed, 21 Nov 2018 16:55:57 +1300 Subject: [PATCH 08/12] FIX Provide alternatives to session for storing GridField_FormAction state --- _config/gridfield.yml | 6 +++ .../GridField/FormAction/AttributeStore.php | 51 +++++++++++++++++++ .../GridField/FormAction/SessionStore.php | 50 ++++++++++++++++++ src/Forms/GridField/FormAction/StateStore.php | 30 +++++++++++ src/Forms/GridField/GridField.php | 10 +++- src/Forms/GridField/GridField_FormAction.php | 43 +++++++++++----- 6 files changed, 176 insertions(+), 14 deletions(-) create mode 100644 _config/gridfield.yml create mode 100644 src/Forms/GridField/FormAction/AttributeStore.php create mode 100644 src/Forms/GridField/FormAction/SessionStore.php create mode 100644 src/Forms/GridField/FormAction/StateStore.php diff --git a/_config/gridfield.yml b/_config/gridfield.yml new file mode 100644 index 000000000..a47da229f --- /dev/null +++ b/_config/gridfield.yml @@ -0,0 +1,6 @@ +--- +Name: gridfieldconfig +--- +SilverStripe\Core\Injector\Injector: + SilverStripe\Forms\GridField\FormAction\StateStore: + class: SilverStripe\Forms\GridField\FormAction\SessionStore diff --git a/src/Forms/GridField/FormAction/AttributeStore.php b/src/Forms/GridField/FormAction/AttributeStore.php new file mode 100644 index 000000000..60f736d74 --- /dev/null +++ b/src/Forms/GridField/FormAction/AttributeStore.php @@ -0,0 +1,51 @@ +request = $request; + } + + /** + * Save the given state against the given ID returning an associative array to be added as attributes on the form + * action + * + * @param string $id + * @param array $state + * @return array + */ + public function save($id, array $state) + { + // Just save the state in the attributes of the action + return [ + 'data-action-state' => json_encode($state), + ]; + } + + /** + * Load state for a given ID + * + * @param string $id + * @return mixed + */ + public function load($id) + { + // Check the request + return json_decode($this->request->requestVar('ActionState'), true); + } +} diff --git a/src/Forms/GridField/FormAction/SessionStore.php b/src/Forms/GridField/FormAction/SessionStore.php new file mode 100644 index 000000000..35f2337b8 --- /dev/null +++ b/src/Forms/GridField/FormAction/SessionStore.php @@ -0,0 +1,50 @@ +request = $request; + } + + /** + * Save the given state against the given ID returning an associative array to be added as attributes on the form + * action + * + * @param string $id + * @param array $state + * @return array + */ + public function save($id, array $state) + { + $this->request->getSession()->set($id, $state); + + // This adapter does not require any additional attributes... + return []; + } + + /** + * Load state for a given ID + * + * @param string $id + * @return mixed + */ + public function load($id) + { + return $this->request->getSession()->get($id); + } +} diff --git a/src/Forms/GridField/FormAction/StateStore.php b/src/Forms/GridField/FormAction/StateStore.php new file mode 100644 index 000000000..e415a518b --- /dev/null +++ b/src/Forms/GridField/FormAction/StateStore.php @@ -0,0 +1,30 @@ +setValue($fieldData['GridState']); } + // Fetch the store for the "state" of actions (not the GridField) + /** @var StateStore $store */ + $store = Injector::inst()->create(StateStore::class . '.' . $this->getName(), $request); + foreach ($data as $dataKey => $dataValue) { if (preg_match('/^action_gridFieldAlterAction\?StateID=(.*)/', $dataKey, $matches)) { - $stateChange = $request->getSession()->get($matches[1]); + $stateChange = $store->load($matches[1]); + $actionName = $stateChange['actionName']; $arguments = array(); diff --git a/src/Forms/GridField/GridField_FormAction.php b/src/Forms/GridField/GridField_FormAction.php index 10dc772bc..a253d8b3b 100644 --- a/src/Forms/GridField/GridField_FormAction.php +++ b/src/Forms/GridField/GridField_FormAction.php @@ -3,8 +3,10 @@ namespace SilverStripe\Forms\GridField; use SilverStripe\Control\Controller; +use SilverStripe\Core\Injector\Injector; use SilverStripe\Forms\Form; use SilverStripe\Forms\FormAction; +use SilverStripe\Forms\GridField\FormAction\StateStore; /** * This class is the base class when you want to have an action that alters the state of the @@ -12,6 +14,11 @@ use SilverStripe\Forms\FormAction; */ class GridField_FormAction extends FormAction { + /** + * A common string prefix for keys generated to store form action "state" against + */ + const STATE_KEY_PREFIX = 'gf_'; + /** * @var GridField */ @@ -80,29 +87,39 @@ class GridField_FormAction extends FormAction */ public function getAttributes() { - // Store state in session, and pass ID to client side. + // Determine the state that goes with this action $state = array( 'grid' => $this->getNameFromParent(), 'actionName' => $this->actionName, 'args' => $this->args, ); - // Ensure $id doesn't contain only numeric characters - $id = 'gf_' . substr(md5(serialize($state)), 0, 8); + // Generate a key and attach it to the action name + $key = static::STATE_KEY_PREFIX . substr(md5(serialize($state)), 0, 8); + // Note: This field needs to be less than 65 chars, otherwise Suhosin security patch will strip it + $name = 'action_gridFieldAlterAction?StateID=' . $key; - $session = Controller::curr()->getRequest()->getSession(); - $session->set($id, $state); - $actionData['StateID'] = $id; + // Define attributes + $attributes = array( + 'name' => $name, + 'data-url' => $this->gridField->Link(), + 'type' => "button", + ); + // Create a "store" for the "state" of this action + /** @var StateStore $store */ + $store = Injector::inst()->create( + StateStore::class . '.' . $this->gridField->getName(), + // For some reason `getRequest` on GridField_FormAction does not return the correct request + Controller::curr()->getRequest() + ); + // Store the state and update attributes as required + $attributes += $store->save($key, $state); + + // Return attributes return array_merge( parent::getAttributes(), - array( - // Note: This field needs to be less than 65 chars, otherwise Suhosin security patch - // will strip it from the requests - 'name' => 'action_gridFieldAlterAction' . '?' . http_build_query($actionData), - 'data-url' => $this->gridField->Link(), - 'type' => "button", - ) + $attributes ); } From ddaa22986f86c634d5e0c849f727c6eb4c4f2509 Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Thu, 22 Nov 2018 13:05:43 +1300 Subject: [PATCH 09/12] Updating StateStore interface not to define a constructor & fixing GridFieldFilterHeader to add required attributes --- .../FormAction/AbstractRequestAwareStore.php | 34 +++++++++++++++++++ .../GridField/FormAction/AttributeStore.php | 19 ++--------- .../GridField/FormAction/SessionStore.php | 21 +++--------- src/Forms/GridField/FormAction/StateStore.php | 9 +---- src/Forms/GridField/GridField.php | 2 +- src/Forms/GridField/GridFieldFilterHeader.php | 8 +++-- src/Forms/GridField/GridField_FormAction.php | 6 +--- 7 files changed, 50 insertions(+), 49 deletions(-) create mode 100644 src/Forms/GridField/FormAction/AbstractRequestAwareStore.php diff --git a/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php b/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php new file mode 100644 index 000000000..57c11e519 --- /dev/null +++ b/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php @@ -0,0 +1,34 @@ + '%$' . HTTPRequest::class, + ]; + + /** + * @var HTTPRequest + */ + protected $request; + + /** + * @return HTTPRequest + */ + public function getRequest() + { + return $this->request; + } + + /** + * @param HTTPRequest $request + * @return $this + */ + public function setRequest($request) + { + $this->request = $request; + return $this; + } +} diff --git a/src/Forms/GridField/FormAction/AttributeStore.php b/src/Forms/GridField/FormAction/AttributeStore.php index 60f736d74..b04d7c21e 100644 --- a/src/Forms/GridField/FormAction/AttributeStore.php +++ b/src/Forms/GridField/FormAction/AttributeStore.php @@ -6,21 +6,8 @@ use SilverStripe\Control\HTTPRequest; /** * Stores GridField action state on an attribute on the action and then analyses request parameters to load it back */ -class AttributeStore implements StateStore +class AttributeStore extends AbstractRequestAwareStore implements StateStore { - /** - * @var HTTPRequest - */ - protected $request; - - /** - * @param HTTPRequest $request - */ - public function __construct(HTTPRequest $request) - { - $this->request = $request; - } - /** * Save the given state against the given ID returning an associative array to be added as attributes on the form * action @@ -41,11 +28,11 @@ class AttributeStore implements StateStore * Load state for a given ID * * @param string $id - * @return mixed + * @return array */ public function load($id) { // Check the request - return json_decode($this->request->requestVar('ActionState'), true); + return (array) json_decode((string) $this->getRequest()->requestVar('ActionState'), true); } } diff --git a/src/Forms/GridField/FormAction/SessionStore.php b/src/Forms/GridField/FormAction/SessionStore.php index 35f2337b8..0798d0c6a 100644 --- a/src/Forms/GridField/FormAction/SessionStore.php +++ b/src/Forms/GridField/FormAction/SessionStore.php @@ -6,21 +6,8 @@ use SilverStripe\Control\HTTPRequest; /** * Stores GridField action state in the session in exactly the same way it has in the past */ -class SessionStore implements StateStore +class SessionStore extends AbstractRequestAwareStore implements StateStore { - /** - * @var HTTPRequest - */ - protected $request; - - /** - * @param HTTPRequest $request - */ - public function __construct(HTTPRequest $request) - { - $this->request = $request; - } - /** * Save the given state against the given ID returning an associative array to be added as attributes on the form * action @@ -31,7 +18,7 @@ class SessionStore implements StateStore */ public function save($id, array $state) { - $this->request->getSession()->set($id, $state); + $this->getRequest()->getSession()->set($id, $state); // This adapter does not require any additional attributes... return []; @@ -41,10 +28,10 @@ class SessionStore implements StateStore * Load state for a given ID * * @param string $id - * @return mixed + * @return array */ public function load($id) { - return $this->request->getSession()->get($id); + return (array) $this->getRequest()->getSession()->get($id); } } diff --git a/src/Forms/GridField/FormAction/StateStore.php b/src/Forms/GridField/FormAction/StateStore.php index e415a518b..3a0b89c70 100644 --- a/src/Forms/GridField/FormAction/StateStore.php +++ b/src/Forms/GridField/FormAction/StateStore.php @@ -1,15 +1,8 @@ create(StateStore::class . '.' . $this->getName(), $request); + $store = Injector::inst()->create(StateStore::class . '.' . $this->getName()); foreach ($data as $dataKey => $dataValue) { if (preg_match('/^action_gridFieldAlterAction\?StateID=(.*)/', $dataKey, $matches)) { diff --git a/src/Forms/GridField/GridFieldFilterHeader.php b/src/Forms/GridField/GridFieldFilterHeader.php index 2d2440877..d51ea5023 100755 --- a/src/Forms/GridField/GridFieldFilterHeader.php +++ b/src/Forms/GridField/GridFieldFilterHeader.php @@ -289,14 +289,18 @@ class GridFieldFilterHeader implements GridField_URLHandler, GridField_HTMLProvi }, array_keys($filters)), $filters); } + $searchAction = GridField_FormAction::create($gridField, 'filter', false, 'filter', null); + $clearAction = GridField_FormAction::create($gridField, 'reset', false, 'reset', null); $schema = [ 'formSchemaUrl' => $schemaUrl, 'name' => $searchField, 'placeholder' => _t(__CLASS__ . '.Search', 'Search "{name}"', ['name' => $name]), 'filters' => $filters ?: new \stdClass, // stdClass maps to empty json object '{}' 'gridfield' => $gridField->getName(), - 'searchAction' => GridField_FormAction::create($gridField, 'filter', false, 'filter', null)->getAttribute('name'), - 'clearAction' => GridField_FormAction::create($gridField, 'reset', false, 'reset', null)->getAttribute('name') + 'searchAction' => $searchAction->getAttribute('name'), + 'searchActionState' => $searchAction->getAttribute('data-action-state'), + 'clearAction' => $clearAction->getAttribute('name'), + 'clearActionState' => $clearAction->getAttribute('data-action-state'), ]; return Convert::raw2json($schema); diff --git a/src/Forms/GridField/GridField_FormAction.php b/src/Forms/GridField/GridField_FormAction.php index a253d8b3b..7e791410d 100644 --- a/src/Forms/GridField/GridField_FormAction.php +++ b/src/Forms/GridField/GridField_FormAction.php @@ -108,11 +108,7 @@ class GridField_FormAction extends FormAction // Create a "store" for the "state" of this action /** @var StateStore $store */ - $store = Injector::inst()->create( - StateStore::class . '.' . $this->gridField->getName(), - // For some reason `getRequest` on GridField_FormAction does not return the correct request - Controller::curr()->getRequest() - ); + $store = Injector::inst()->create(StateStore::class . '.' . $this->gridField->getName()); // Store the state and update attributes as required $attributes += $store->save($key, $state); From aace1da1f04af84b339eae4aeb89abe3c96d3257 Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Thu, 22 Nov 2018 13:17:09 +1300 Subject: [PATCH 10/12] DOCS Adding notes on configuring the storage method for GridField_FormActions --- .../03_Forms/Field_types/04_GridField.md | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/docs/en/02_Developer_Guides/03_Forms/Field_types/04_GridField.md b/docs/en/02_Developer_Guides/03_Forms/Field_types/04_GridField.md index 3750c0761..43ce4ac95 100644 --- a/docs/en/02_Developer_Guides/03_Forms/Field_types/04_GridField.md +++ b/docs/en/02_Developer_Guides/03_Forms/Field_types/04_GridField.md @@ -472,11 +472,29 @@ functionality. See [How to Create a GridFieldComponent](../how_tos/create_a_grid ## Saving the GridField State -`GridState` is a class that is used to contain the current state and actions on the `GridField`. It's transfered +`GridState` is a class that is used to contain the current state and actions on the `GridField`. It's transferred between page requests by being inserted as a hidden field in the form. The `GridState_Component` sets and gets data from the `GridState`. +## Saving GridField_FormAction state + +By default state used for performing form actions is saved in the session and tagged with a key like `gf_abcd1234`. In +some cases session may not be an appropriate storage method. The storage method can be configured: + +```yaml +Name: mysitegridfieldconfig +After: gridfieldconfig +--- +SilverStripe\Core\Injector\Injector: + SilverStripe\Forms\GridField\FormAction\StateStore: + class: SilverStripe\Forms\GridField\FormAction\AttributeStore +``` + +The `AttributeStore` class configures action state to be stored in the DOM and sent back on the request that performs +the action. Custom storage methods can be created and used by implementing the `StateStore` interface and configuring +`Injector` in a similar fasion. + ## API Documentation * [GridField](api:SilverStripe\Forms\GridField\GridField) From 32d096d9e5daa221ae6a2d03a46c19d1c280989f Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Wed, 28 Nov 2018 14:58:51 +1300 Subject: [PATCH 11/12] DOCS Moving interface subscription to the abstract and adding a changelog entry --- .../02_Developer_Guides/03_Forms/Field_types/04_GridField.md | 2 +- docs/en/04_Changelogs/4.3.0.md | 1 + src/Forms/GridField/FormAction/AbstractRequestAwareStore.php | 2 +- src/Forms/GridField/FormAction/AttributeStore.php | 4 +--- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/docs/en/02_Developer_Guides/03_Forms/Field_types/04_GridField.md b/docs/en/02_Developer_Guides/03_Forms/Field_types/04_GridField.md index 43ce4ac95..9461ae801 100644 --- a/docs/en/02_Developer_Guides/03_Forms/Field_types/04_GridField.md +++ b/docs/en/02_Developer_Guides/03_Forms/Field_types/04_GridField.md @@ -493,7 +493,7 @@ SilverStripe\Core\Injector\Injector: The `AttributeStore` class configures action state to be stored in the DOM and sent back on the request that performs the action. Custom storage methods can be created and used by implementing the `StateStore` interface and configuring -`Injector` in a similar fasion. +`Injector` in a similar fashion. ## API Documentation diff --git a/docs/en/04_Changelogs/4.3.0.md b/docs/en/04_Changelogs/4.3.0.md index ab31d003e..ffa49c558 100644 --- a/docs/en/04_Changelogs/4.3.0.md +++ b/docs/en/04_Changelogs/4.3.0.md @@ -8,6 +8,7 @@ - New React-based search UI for the CMS, Asset-Admin, GridFields and ModelAdmins. - A new `GridFieldLazyLoader` component can be added to `GridField`. This will delay the fetching of data until the user access the container Tab of the GridField. - `SilverStripe\VersionedAdmin\Controllers\CMSPageHistoryViewerController` is now the default CMS history controller and `SilverStripe\CMS\Controllers\CMSPageHistoryController` has been deprecated. + - It's now possible to avoid storing GridField data in session. See [the documentation on GridField](../developer_guides/forms/field_types/gridfield/#saving-the-gridfield-state). ## Upgrading {#upgrading} diff --git a/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php b/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php index 57c11e519..c1069aa3f 100644 --- a/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php +++ b/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php @@ -3,7 +3,7 @@ namespace SilverStripe\Forms\GridField\FormAction; use SilverStripe\Control\HTTPRequest; -abstract class AbstractRequestAwareStore +abstract class AbstractRequestAwareStore implements StateStore { private static $dependencies = [ 'request' => '%$' . HTTPRequest::class, diff --git a/src/Forms/GridField/FormAction/AttributeStore.php b/src/Forms/GridField/FormAction/AttributeStore.php index b04d7c21e..1f84fb768 100644 --- a/src/Forms/GridField/FormAction/AttributeStore.php +++ b/src/Forms/GridField/FormAction/AttributeStore.php @@ -1,12 +1,10 @@ Date: Wed, 28 Nov 2018 16:00:51 +1300 Subject: [PATCH 12/12] FIX Switching to use Controller::curr as it was using previously --- .../FormAction/AbstractRequestAwareStore.php | 23 +++---------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php b/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php index c1069aa3f..84f683292 100644 --- a/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php +++ b/src/Forms/GridField/FormAction/AbstractRequestAwareStore.php @@ -1,34 +1,17 @@ '%$' . HTTPRequest::class, - ]; - - /** - * @var HTTPRequest - */ - protected $request; - /** * @return HTTPRequest */ public function getRequest() { - return $this->request; - } - - /** - * @param HTTPRequest $request - * @return $this - */ - public function setRequest($request) - { - $this->request = $request; - return $this; + // Replicating existing functionality from GridField_FormAction + return Controller::curr()->getRequest(); } }