From 3797049a31bfddb53f9bfd6454b2726fc4a6aef4 Mon Sep 17 00:00:00 2001 From: Simon Gow Date: Thu, 6 Sep 2018 18:16:06 +1200 Subject: [PATCH 1/7] Resolve Performance issues with TagField TagField hydrates the entire result set because a parent class calls `toArray()` on the $source. This fix changes the source to an empty array, so no such manipulation can be made. --- src/TagField.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/TagField.php b/src/TagField.php index 409a9d9..88b0b16 100644 --- a/src/TagField.php +++ b/src/TagField.php @@ -71,7 +71,7 @@ class TagField extends DropdownField { $this->setSourceList($source); $this->setTitleField($titleField); - parent::__construct($name, $title, $source, $value); + parent::__construct($name, $title, [], $value); } /** From fa754f3fd3cde7129473f380f5f471d97c933a52 Mon Sep 17 00:00:00 2001 From: Simon Gow Date: Fri, 7 Sep 2018 17:28:05 +1200 Subject: [PATCH 2/7] TagField lazy load shouldnt render Options TagField should only render options if the lazy load value isn't set. Options from here come from subsequent POSTs, and rendering the options for every request significantly slows down both the form generation and render. --- src/TagField.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/TagField.php b/src/TagField.php index 88b0b16..3968c3f 100644 --- a/src/TagField.php +++ b/src/TagField.php @@ -262,6 +262,20 @@ class TagField extends DropdownField $ids = $values->column($this->getTitleField()); $titleField = $this->getTitleField(); + + if ($this->shouldLazyLoad) { + // only render options that are selected as everything else should be lazy loaded, and or loaded by the form + foreach ($values as $value) { + $options->push( + ArrayData::create(array( + 'Title' => $value->$titleField, + 'Value' => $value->Title, + 'Selected' => true, // only values are iterated. + )) + ); + } + return $options; + } foreach ($source as $object) { $options->push( From 024e648e5a807cc09f702d818ef1a1fcfffe035d Mon Sep 17 00:00:00 2001 From: Simon Gow Date: Thu, 13 Sep 2018 11:18:55 +1200 Subject: [PATCH 3/7] Added getSource() function to populate source inline with the api. Source isn't populated to the parent because it's transformed directly to an array. Instead we're lazy loading the source when it's requested. --- src/TagField.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/TagField.php b/src/TagField.php index 3968c3f..986ed4e 100644 --- a/src/TagField.php +++ b/src/TagField.php @@ -312,6 +312,21 @@ class TagField extends DropdownField return parent::setValue(array_filter($value)); } + /** + * Gets the source array if required + * + * Note: this is expensive for a SS_List + * + * @return array + */ + public function getSource() + { + if (is_null($this->source)) { + $this->setSource($this->getSourceList()); + } + return $this->source; + } + /** * {@inheritdoc} */ From 690f0cc7934bd51355c30acb8b61a2e806838568 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 17 Sep 2018 18:18:40 +1200 Subject: [PATCH 4/7] BUG Fix for $source left null BUG Fix missing imports Semver compatible setSource() --- src/TagField.php | 57 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/src/TagField.php b/src/TagField.php index 986ed4e..7d83062 100644 --- a/src/TagField.php +++ b/src/TagField.php @@ -2,15 +2,18 @@ namespace SilverStripe\TagField; +use Exception; use SilverStripe\Control\Controller; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; use SilverStripe\Core\Injector\Injector; use SilverStripe\Forms\DropdownField; +use SilverStripe\Forms\Validator; use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\DataList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataObjectInterface; +use SilverStripe\ORM\Relation; use SilverStripe\ORM\SS_List; use SilverStripe\View\ArrayData; use SilverStripe\View\Requirements; @@ -67,11 +70,10 @@ class TagField extends DropdownField * @param null|DataList $value * @param string $titleField */ - public function __construct($name, $title = '', $source = [], $value = null, $titleField = 'Title') + public function __construct($name, $title = '', $source = null, $value = null, $titleField = 'Title') { - $this->setSourceList($source); $this->setTitleField($titleField); - parent::__construct($name, $title, [], $value); + parent::__construct($name, $title, $source, $value); } /** @@ -175,7 +177,9 @@ class TagField extends DropdownField } /** - * Get the DataList source. The 4.x upgrade for SelectField::setSource starts to convert this to an array + * Get the DataList source. The 4.x upgrade for SelectField::setSource starts to convert this to an array. + * If empty use getSource() for array version + * * @return DataList */ public function getSourceList() @@ -185,7 +189,8 @@ class TagField extends DropdownField /** * Set the model class name for tags - * @param DataList $className + * + * @param DataList $sourceList * @return self */ public function setSourceList($sourceList) @@ -322,11 +327,29 @@ class TagField extends DropdownField public function getSource() { if (is_null($this->source)) { - $this->setSource($this->getSourceList()); + $this->source = $this->getListMap($this->getSourceList()); } return $this->source; } + /** + * Intercept DataList source + * + * @param mixed $source + * @return $this + */ + public function setSource($source) + { + // When setting a datalist force internal list to null + if ($source instanceof DataList) { + $this->source = null; + $this->setSourceList($source); + } else { + parent::setSource($source); + } + return $this; + } + /** * {@inheritdoc} */ @@ -342,7 +365,8 @@ class TagField extends DropdownField } /** - * {@inheritdoc} + * @param DataObject|DataObjectInterface $record DataObject to save data into + * @throws Exception */ public function saveInto(DataObjectInterface $record) { @@ -350,8 +374,9 @@ class TagField extends DropdownField $name = $this->getName(); $titleField = $this->getTitleField(); - $source = $this->getSource(); $values = $this->Value(); + + /** @var Relation $relation */ $relation = $record->$name(); $ids = array(); @@ -384,13 +409,16 @@ class TagField extends DropdownField * Get or create tag with the given value * * @param string $term - * @return DataObject + * @return DataObject|false */ protected function getOrCreateTag($term) { // Check if existing record can be found - /** @var DataList $source */ $source = $this->getSourceList(); + if (!$source) { + return false; + } + $titleField = $this->getTitleField(); $record = $source ->filter($titleField, $term) @@ -436,10 +464,10 @@ class TagField extends DropdownField */ protected function getTags($term) { - /** - * @var array $source - */ $source = $this->getSourceList(); + if (!$source) { + return []; + } $titleField = $this->getTitleField(); @@ -476,10 +504,11 @@ class TagField extends DropdownField /** * Converts the field to a readonly variant. * - * @return TagField_Readonly + * @return ReadonlyTagField */ public function performReadonlyTransformation() { + /** @var ReadonlyTagField $copy */ $copy = $this->castedCopy(ReadonlyTagField::class); $copy->setSourceList($this->getSourceList()); return $copy; From ce8ba85182a012c28f91f54b3b24b7e3d5da2486 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 17 Sep 2018 18:24:03 +1200 Subject: [PATCH 5/7] Revert default argument change --- src/TagField.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/TagField.php b/src/TagField.php index 7d83062..e194188 100644 --- a/src/TagField.php +++ b/src/TagField.php @@ -66,11 +66,11 @@ class TagField extends DropdownField /** * @param string $name * @param string $title - * @param null|DataList $source + * @param null|DataList|array $source * @param null|DataList $value * @param string $titleField */ - public function __construct($name, $title = '', $source = null, $value = null, $titleField = 'Title') + public function __construct($name, $title = '', $source = [], $value = null, $titleField = 'Title') { $this->setTitleField($titleField); parent::__construct($name, $title, $source, $value); From 77bad9b9437c68eb833c293c2674fd811fbac725 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 18 Sep 2018 09:12:44 +1200 Subject: [PATCH 6/7] Use getShouldLazyLoad() accesser --- src/TagField.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/TagField.php b/src/TagField.php index e194188..fc37ab2 100644 --- a/src/TagField.php +++ b/src/TagField.php @@ -216,7 +216,7 @@ class TagField extends DropdownField $this->setAttribute('multiple', 'multiple'); } - if ($this->shouldLazyLoad) { + if ($this->getShouldLazyLoad()) { $this->setAttribute('data-ss-tag-field-suggest-url', $this->getSuggestURL()); } else { $properties = array_merge($properties, array( @@ -268,7 +268,7 @@ class TagField extends DropdownField $titleField = $this->getTitleField(); - if ($this->shouldLazyLoad) { + if ($this->getShouldLazyLoad()) { // only render options that are selected as everything else should be lazy loaded, and or loaded by the form foreach ($values as $value) { $options->push( From 3ff72be24c7e3bfab595efa2c745984ae0e7fbbf Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Tue, 19 Feb 2019 11:01:58 +0700 Subject: [PATCH 7/7] FIX StringTagField now works with SS-2018-021/CVE-2019-5715 by serialising arrays before write --- src/StringTagField.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/StringTagField.php b/src/StringTagField.php index edcbe65..eee2057 100644 --- a/src/StringTagField.php +++ b/src/StringTagField.php @@ -258,10 +258,20 @@ class StringTagField extends DropdownField $name = $this->getName(); - $record->$name = join(',', $this->Value()); + $record->$name = $this->dataValue(); $record->write(); } + /** + * Ensure that arrays are imploded before being saved + * + * @return mixed|string + */ + public function dataValue() + { + return implode(',', $this->value); + } + /** * Returns a JSON string of tags, for lazy loading. *