From 62a242154ec3508fe9b174a40713c8520ac1684c Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 3 Aug 2016 11:23:17 +1200 Subject: [PATCH] [ss-2016-015] Fix value / title escaping in CheckboxSetField and OptionsetField --- forms/CheckboxSetField.php | 69 +++++++++++++++------------- forms/OptionsetField.php | 43 +++++++++-------- templates/forms/CheckboxSetField.ss | 4 +- templates/forms/OptionsetField.ss | 2 +- tests/forms/CheckboxSetFieldTest.php | 69 ++++++++++++++++++---------- tests/forms/OptionsetFieldTest.php | 20 ++++++-- 6 files changed, 125 insertions(+), 82 deletions(-) diff --git a/forms/CheckboxSetField.php b/forms/CheckboxSetField.php index d138da16d..868f4f662 100644 --- a/forms/CheckboxSetField.php +++ b/forms/CheckboxSetField.php @@ -4,7 +4,7 @@ * * ASSUMPTION -> IF you pass your source as an array, you pass values as an array too. Likewise objects are handled * the same. - * + * * Example: * * new CheckboxSetField( @@ -19,7 +19,7 @@ * $value = "1" * ); * - * + * * Saving * The checkbox set field will save its data in one of ways: * - If the field name matches a many-many join on the object being edited, that many-many join will be updated to @@ -27,17 +27,17 @@ * the database records. * - If the field name matches a database field, a comma-separated list of values will be saved to that field. The * keys can be text or numbers. - * + * * @todo Document the different source data that can be used * with this form field - e.g ComponentSet, ArrayList, * array. Is it also appropriate to accept so many different * types of data when just using an array would be appropriate? - * + * * @package forms * @subpackage fields-basic */ class CheckboxSetField extends OptionsetField { - + /** * @var array */ @@ -82,7 +82,7 @@ class CheckboxSetField extends OptionsetField { } } } - + // Source is not an array if(!is_array($source) && !is_a($source, 'SQLMap')) { if(is_array($values)) { @@ -126,19 +126,22 @@ class CheckboxSetField extends OptionsetField { if(is_array($source)) { unset($source['']); } - + $options = array(); - + if ($source == null) { $source = array(); } foreach($source as $value => $item) { - if($item instanceof DataObject) { + // Ensure $title is cast for template + if ($item instanceof DataObject) { $value = $item->ID; - $title = $item->Title; - } else { + $title = $item->obj('Title'); + } elseif ($item instanceof DBField) { $title = $item; + } else { + $title = DBField::create_field('Text', $item); } $itemID = $this->ID() . '_' . preg_replace('/[^a-zA-Z0-9]/', '', $value); @@ -168,21 +171,21 @@ class CheckboxSetField extends OptionsetField { * Default selections, regardless of the {@link setValue()} settings. * Note: Items marked as disabled through {@link setDisabledItems()} can still be * selected by default through this method. - * + * * @param Array $items Collection of array keys, as defined in the $source array */ public function setDefaultItems($items) { $this->defaultItems = $items; return $this; } - + /** * @return Array */ public function getDefaultItems() { return $this->defaultItems; } - + /** * Load a value into this CheckboxSetField */ @@ -198,7 +201,7 @@ class CheckboxSetField extends OptionsetField { return $this; } - + /** * Save the current value of this CheckboxSetField into a DataObject. * If the field it is saving to is a has_many or many_many relationship, @@ -228,11 +231,11 @@ class CheckboxSetField extends OptionsetField { } } } - + /** - * Return the CheckboxSetField value as a string + * Return the CheckboxSetField value as a string * selected item keys. - * + * * @return string */ public function dataValue() { @@ -243,30 +246,30 @@ class CheckboxSetField extends OptionsetField { $filtered[] = str_replace(",", "{comma}", $item); } } - + return implode(',', $filtered); } - + return ''; } - + public function performDisabledTransformation() { $clone = clone $this; $clone->setDisabled(true); - + return $clone; } - + /** * Transforms the source data for this CheckboxSetField * into a comma separated list of values. - * + * * @return ReadonlyField */ public function performReadonlyTransformation() { $values = ''; $data = array(); - + $items = $this->value; if($this->source) { foreach($this->source as $source) { @@ -275,7 +278,7 @@ class CheckboxSetField extends OptionsetField { } } } - + if($items) { // Items is a DO Set if($items instanceof SS_List) { @@ -283,13 +286,13 @@ class CheckboxSetField extends OptionsetField { $data[] = $item->Title; } if($data) $values = implode(', ', $data); - + // Items is an array or single piece of string (including comma seperated string) } else { if(!is_array($items)) { $items = preg_split('/ *, */', trim($items)); } - + foreach($items as $item) { if(is_array($item)) { $data[] = $item['Title']; @@ -301,23 +304,23 @@ class CheckboxSetField extends OptionsetField { $data[] = $item; } } - + $values = implode(', ', $data); } } - + $field = $this->castedCopy('ReadonlyField'); $field->setValue($values); - + return $field; } public function Type() { return 'optionset checkboxset'; } - + public function ExtraOptions() { return FormField::ExtraOptions(); } - + } diff --git a/forms/OptionsetField.php b/forms/OptionsetField.php index 010c9ff22..cc16bc5f2 100644 --- a/forms/OptionsetField.php +++ b/forms/OptionsetField.php @@ -1,13 +1,13 @@ Usage - * + * * * new OptionsetField( * $name = "Foobar", @@ -22,15 +22,15 @@ * $value = "1" * ); * - * - * You can use the helper functions on data object set to create the source array. eg: - * + * + * You can use the helper functions on data object set to create the source array. eg: + * * * //Database request for the object * $map = FooBar::get()->map(); * // returns an SS_Map object containing an array of ID => Title * - * // Instantiate the OptionsetField + * // Instantiate the OptionsetField * $FieldList = new FieldList( * new OptionsetField( * $name = "Foobar", @@ -42,16 +42,16 @@ * * // Pass the fields to the form constructor. etc * - * + * * @see CheckboxSetField for multiple selections through checkboxes instead. * @see DropdownField for a simple checked="checked"<% end_if %><% if $isDisabled %> disabled="disabled"<% end_if %> /> + checked="checked"<% end_if %><% if $isDisabled %> disabled="disabled"<% end_if %> /> - + <% end_loop %> <% else %>
  • No options available
    • diff --git a/templates/forms/OptionsetField.ss b/templates/forms/OptionsetField.ss index bb8818a6f..d9e38156d 100644 --- a/templates/forms/OptionsetField.ss +++ b/templates/forms/OptionsetField.ss @@ -1,7 +1,7 @@