From 4b4fbabed5d70bf577e4b0d6fdbc9dab9da80451 Mon Sep 17 00:00:00 2001 From: Serge Latyntcev Date: Thu, 8 Nov 2018 14:36:16 +1300 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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();