From 68c3279fd9b342f9664146c0131d185ca17c340a Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 26 Oct 2017 13:04:30 +1300 Subject: [PATCH] BUG Ensure readonly tree dropdown is safely encoded Removed legacy entwine dead code Added soft-deprecation to label field --- src/Forms/LookupField.php | 15 ++--- src/Forms/TreeDropdownField.php | 67 ++++++-------------- src/Forms/TreeDropdownField_Readonly.php | 11 ++-- src/Forms/TreeMultiselectField_Readonly.php | 31 ++++----- tests/php/Forms/TreeDropdownFieldTest.php | 47 +++++++++----- tests/php/Forms/TreeDropdownFieldTest.yml | 1 + tests/php/Forms/TreeMultiselectFieldTest.php | 31 +++++++++ 7 files changed, 110 insertions(+), 93 deletions(-) create mode 100644 tests/php/Forms/TreeMultiselectFieldTest.php diff --git a/src/Forms/LookupField.php b/src/Forms/LookupField.php index df0d08291..e618c9aa1 100644 --- a/src/Forms/LookupField.php +++ b/src/Forms/LookupField.php @@ -2,10 +2,10 @@ namespace SilverStripe\Forms; -use SilverStripe\ORM\ArrayLib; -use SilverStripe\ORM\FieldType\DBField; -use SilverStripe\ORM\DataObjectInterface; use SilverStripe\Core\Convert; +use SilverStripe\ORM\ArrayLib; +use SilverStripe\ORM\DataObjectInterface; +use SilverStripe\ORM\FieldType\DBField; /** * Read-only complement of {@link DropdownField}. @@ -36,23 +36,22 @@ class LookupField extends MultiSelectField $values = $this->getValueArray(); // Get selected values - $mapped = array(); + $mapped = []; foreach ($values as $value) { if (isset($source[$value])) { - $mapped[] = $source[$value]; + $mapped[] = Convert::raw2xml($source[$value]); } } // Don't check if string arguments are matching against the source, // as they might be generated HTML diff views instead of the actual values if ($this->value && is_string($this->value) && empty($mapped)) { - $mapped = array(trim($this->value)); - $values = array(); + $mapped[] = Convert::raw2xml(trim($this->value)); + $values = []; } if ($mapped) { $attrValue = implode(', ', array_values($mapped)); - $inputValue = implode(', ', array_values($values)); } else { $attrValue = '('._t('SilverStripe\\Forms\\FormField.NONE', 'none').')'; diff --git a/src/Forms/TreeDropdownField.php b/src/Forms/TreeDropdownField.php index 8223bd47e..01ff04b4f 100644 --- a/src/Forms/TreeDropdownField.php +++ b/src/Forms/TreeDropdownField.php @@ -2,18 +2,16 @@ namespace SilverStripe\Forms; +use Exception; +use InvalidArgumentException; use SilverStripe\Assets\Folder; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; -use SilverStripe\Core\Convert; use SilverStripe\ORM\DataList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\ORM\Hierarchy\Hierarchy; use SilverStripe\ORM\Hierarchy\MarkedSet; -use SilverStripe\View\ViewableData; -use Exception; -use InvalidArgumentException; /** * Dropdown-like field that allows you to select an item from a hierarchical @@ -425,39 +423,6 @@ class TreeDropdownField extends FormField return $this; } - /** - * @param array $properties - * @return string - */ - public function Field($properties = array()) - { - $record = $this->Value() ? $this->objectForKey($this->Value()) : null; - if ($record instanceof ViewableData) { - $title = $record->obj($this->getLabelField())->forTemplate(); - } elseif ($record) { - $title = Convert::raw2xml($record->{$this->getLabelField()}); - } else { - $title = $this->getEmptyString(); - } - - // TODO Implement for TreeMultiSelectField - $metadata = array( - 'id' => $record ? $record->ID : null, - 'ClassName' => $record ? $record->ClassName : $this->getSourceObject() - ); - - $properties = array_merge( - $properties, - array( - 'Title' => $title, - 'EmptyTitle' => $this->getEmptyString(), - 'Metadata' => ($metadata) ? Convert::raw2json($metadata) : null, - ) - ); - - return parent::Field($properties); - } - public function extraClass() { return implode(' ', array(parent::extraClass(), ($this->getShowSearch() ? "searchable" : null))); @@ -633,6 +598,9 @@ class TreeDropdownField extends FormField } /** + * HTML-encoded label for this node, including css classes and other markup. + * + * @deprecated 4.0...5.0 Use setTitleField() * @param string $field * @return $this */ @@ -643,6 +611,9 @@ class TreeDropdownField extends FormField } /** + * HTML-encoded label for this node, including css classes and other markup. + * + * @deprecated 4.0...5.0 Use getTitleField() * @return string */ public function getLabelField() @@ -651,7 +622,7 @@ class TreeDropdownField extends FormField } /** - * Field to use for item titles + * Field to use for plain text item titles. * * @return string */ @@ -795,14 +766,16 @@ class TreeDropdownField extends FormField $sourceObject = $this->getSourceObject(); $filters = array(); - if (singleton($sourceObject)->hasDatabaseField($this->getLabelField())) { - $filters["{$this->getLabelField()}:PartialMatch"] = $this->search; - } else { - if (singleton($sourceObject)->hasDatabaseField('Title')) { - $filters["Title:PartialMatch"] = $this->search; - } - if (singleton($sourceObject)->hasDatabaseField('Name')) { - $filters["Name:PartialMatch"] = $this->search; + $sourceObjectInstance = DataObject::singleton($sourceObject); + $candidates = array_unique([ + $this->getLabelField(), + $this->getTitleField(), + 'Title', + 'Name' + ]); + foreach ($candidates as $candidate) { + if ($sourceObjectInstance->hasDatabaseField($candidate)) { + $filters["{$candidate}:PartialMatch"] = $this->search; } } @@ -810,7 +783,7 @@ class TreeDropdownField extends FormField throw new InvalidArgumentException(sprintf( 'Cannot query by %s.%s, not a valid database column', $sourceObject, - $this->getLabelField() + $this->getTitleField() )); } return DataObject::get($this->getSourceObject())->filterAny($filters); diff --git a/src/Forms/TreeDropdownField_Readonly.php b/src/Forms/TreeDropdownField_Readonly.php index c86e270f6..fed1a1765 100644 --- a/src/Forms/TreeDropdownField_Readonly.php +++ b/src/Forms/TreeDropdownField_Readonly.php @@ -8,18 +8,15 @@ class TreeDropdownField_Readonly extends TreeDropdownField public function Field($properties = array()) { - $fieldName = $this->getLabelField(); + $fieldName = $this->getTitleField(); if ($this->value) { $keyObj = $this->objectForKey($this->value); - $obj = $keyObj ? $keyObj->$fieldName : ''; + $title = $keyObj ? $keyObj->$fieldName : ''; } else { - $obj = null; + $title = null; } - $source = array( - $this->value => $obj - ); - + $source = [ $this->value => $title ]; $field = new LookupField($this->name, $this->title, $source); $field->setValue($this->value); $field->setForm($this->form); diff --git a/src/Forms/TreeMultiselectField_Readonly.php b/src/Forms/TreeMultiselectField_Readonly.php index 884f1b20e..74cc989cf 100644 --- a/src/Forms/TreeMultiselectField_Readonly.php +++ b/src/Forms/TreeMultiselectField_Readonly.php @@ -9,27 +9,28 @@ class TreeMultiselectField_Readonly extends TreeMultiselectField public function Field($properties = array()) { - $titleArray = $itemIDs = array(); - $titleList = $itemIDsList = ""; - if ($items = $this->getItems()) { - foreach ($items as $item) { - $titleArray[] = $item->Title; - } - foreach ($items as $item) { - $itemIDs[] = $item->ID; - } - if ($titleArray) { - $titleList = implode(", ", $titleArray); - } - if ($itemIDs) { - $itemIDsList = implode(",", $itemIDs); - } + // Build list of titles + $titleField = $this->getTitleField(); + $items = $this->getItems(); + $titleArray = []; + foreach ($items as $item) { + $titleArray[] = $item->$titleField; } + $titleList = implode(", ", $titleArray); + // Build list of values + $itemIDs = []; + foreach ($items as $item) { + $itemIDs[] = $item->ID; + } + $itemIDsList = implode(",", $itemIDs); + + // Readonly field for display $field = new ReadonlyField($this->name . '_ReadonlyValue', $this->title); $field->setValue($titleList); $field->setForm($this->form); + // Store values to hidden field $valueField = new HiddenField($this->name); $valueField->setValue($itemIDsList); $valueField->setForm($this->form); diff --git a/tests/php/Forms/TreeDropdownFieldTest.php b/tests/php/Forms/TreeDropdownFieldTest.php index bc49151cb..0ef977009 100644 --- a/tests/php/Forms/TreeDropdownFieldTest.php +++ b/tests/php/Forms/TreeDropdownFieldTest.php @@ -19,18 +19,18 @@ class TreeDropdownFieldTest extends SapphireTest { $field = new TreeDropdownField('TestTree', 'Test tree', Folder::class); $folder = $this->objFromFixture(Folder::class, 'folder1-subfolder1'); - + $schema = $field->getSchemaStateDefaults(); $this->assertFalse(isset($schema['data']['valueObject'])); - + $field->setValue($folder->ID); - + $schema = $field->getSchemaStateDefaults(); $this->assertEquals($folder->ID, $schema['data']['valueObject']['id']); $this->assertTrue(isset($schema['data']['valueObject'])); $this->assertFalse($schema['data']['showSelectedPath']); $this->assertEquals('', $schema['data']['valueObject']['titlePath']); - + $field->setShowSelectedPath(true); $schema = $field->getSchemaStateDefaults(); $this->assertTrue($schema['data']['showSelectedPath']); @@ -39,64 +39,64 @@ class TreeDropdownFieldTest extends SapphireTest $schema['data']['valueObject']['titlePath'] ); } - + public function testTreeSearchJson() { $field = new TreeDropdownField('TestTree', 'Test tree', Folder::class); - + // case insensitive search against keyword 'sub' for folders $request = new HTTPRequest('GET', 'url', array('search'=>'sub', 'format' => 'json')); $request->setSession(new Session([])); $response = $field->tree($request); $tree = json_decode($response->getBody(), true); - + $folder1 = $this->objFromFixture(Folder::class, 'folder1'); $folder1Subfolder1 = $this->objFromFixture(Folder::class, 'folder1-subfolder1'); - + $this->assertContains( $folder1->Name, array_column($tree['children'], 'title'), $folder1->Name.' is found in the json' ); - + $filtered = array_filter($tree['children'], function ($entry) use ($folder1) { return $folder1->Name === $entry['title']; }); $folder1Tree = array_pop($filtered); - + $this->assertContains( $folder1Subfolder1->Name, array_column($folder1Tree['children'], 'title'), $folder1Subfolder1->Name.' is found in the folder1 entry in the json' ); } - + public function testTreeSearchJsonFlatlist() { $field = new TreeDropdownField('TestTree', 'Test tree', Folder::class); - + // case insensitive search against keyword 'sub' for folders $request = new HTTPRequest('GET', 'url', array('search'=>'sub', 'format' => 'json', 'flatList' => '1')); $request->setSession(new Session([])); $response = $field->tree($request); $tree = json_decode($response->getBody(), true); - + $folder1 = $this->objFromFixture(Folder::class, 'folder1'); $folder1Subfolder1 = $this->objFromFixture(Folder::class, 'folder1-subfolder1'); - + $this->assertNotContains( $folder1->Name, array_column($tree['children'], 'title'), $folder1->Name.' is not found in the json' ); - + $this->assertContains( $folder1Subfolder1->Name, array_column($tree['children'], 'title'), $folder1Subfolder1->Name.' is found in the json' ); } - + public function testTreeSearch() { $field = new TreeDropdownField('TestTree', 'Test tree', Folder::class); @@ -194,4 +194,19 @@ class TreeDropdownFieldTest extends SapphireTest $file3->Name.' is not found' ); } + + public function testReadonly() + { + $field = new TreeDropdownField('TestTree', 'Test tree', File::class); + $asdf = $this->objFromFixture(File::class, 'asdf'); + $field->setValue($asdf->ID); + $readonlyField = $field->performReadonlyTransformation(); + $this->assertEquals( + <<<"HTML" +<Special & characters> +HTML + , + (string)$readonlyField->Field() + ); + } } diff --git a/tests/php/Forms/TreeDropdownFieldTest.yml b/tests/php/Forms/TreeDropdownFieldTest.yml index 31c141d6e..7546885b3 100644 --- a/tests/php/Forms/TreeDropdownFieldTest.yml +++ b/tests/php/Forms/TreeDropdownFieldTest.yml @@ -11,6 +11,7 @@ SilverStripe\Assets\Folder: SilverStripe\Assets\File: asdf: Filename: assets/FileTest.txt + Title: '' subfolderfile1: Filename: assets/FileTest-subfolder/TestFile1InSubfolder.txt Name: TestFile1InSubfolder diff --git a/tests/php/Forms/TreeMultiselectFieldTest.php b/tests/php/Forms/TreeMultiselectFieldTest.php new file mode 100644 index 000000000..cbb540c6d --- /dev/null +++ b/tests/php/Forms/TreeMultiselectFieldTest.php @@ -0,0 +1,31 @@ +objFromFixture(File::class, 'asdf'); + $subfolderfile1 = $this->objFromFixture(File::class, 'subfolderfile1'); + $field->setValue(implode(',', [$asdf->ID, $subfolderfile1->ID])); + + $readonlyField = $field->performReadonlyTransformation(); + $this->assertEquals( + <<<"HTML" + + <Special & characters>, TestFile1InSubfolder + +HTML + , + (string)$readonlyField->Field() + ); + } +}