BUG Fix FormAction title encoding

BUG Fix TreeMultiSelectField using the wrong label
BUG Fix encoding of selected title on TreeDropdownField
BUG Fix DataDifferencer trying to compare non-comparable fields (non-dbfield objects)
BUG: Fix issue with TreeMultiSelectField not saving
BUG: Fix issue with GridFieldPrintButton
ENHANCEMENT Instead of using multiple api calls to encode dbfield values, delegate this operation to the individual fields via forTemplate
Instead of using a new API to communicate html encoding to treeselect, just ensure all content is HTML encoded, and enable html_titles in jstree.

docs/en/04_Changelogs/rc/3.1.10-rc1.md

@@ -0,0 +1,15 @@
+# 3.1.10-rc1
+
+## Upgrading
+
+### Form Actions
+
+Form action titles are now safely XML encoded, although this was an implicit assumption, it is now explicitly enforced.
+XML encoding titles will not cause an error, but is deprecated at framework 4.0. FormAction buttons with custom HTML
+content should be assigned using the `FormAction::setButtonContent` method instead.
+
+## Changelog
+
+ * [framework](https://github.com/silverstripe/silverstripe-framework/releases/tag/3.1.10-rc1)
+ * [cms](https://github.com/silverstripe/silverstripe-cms/releases/tag/3.1.10-rc1)
+ * [installer](https://github.com/silverstripe/silverstripe-installer/releases/tag/3.1.10-rc1)

forms/FormAction.php

@@ -17,31 +17,47 @@
  */
 class FormAction extends FormField {
 
+	/**
+	 * Action name, normally prefixed with 'action_'
+	 *
+	 * @var string
+	 */
 	protected $action;
 	
 	/**
 	 * Enables the use of <button> instead of <input>
 	 * in {@link Field()} - for more customizeable styling.
 	 * 
-	 * @var boolean $useButtonTag
+	 * @var boolean
 	 */
 	public $useButtonTag = false;
-	
+
+	/**
+	 * Literal button content, used when useButtonTag is true.
+	 *
+	 * @var string
+	 */
 	protected $buttonContent = null;
 	
 	/**
 	 * Create a new action button.
 	 *
-	 * @param action The method to call when the button is clicked
-	 * @param title The label on the button
-	 * @param form The parent form, auto-set when the field is placed inside a form 
+	 * @param string $action The method to call when the button is clicked
+	 * @param string $title The label on the button. This should be plain text, not escaped as HTML.
+	 * @param Form form The parent form, auto-set when the field is placed inside a form
 	 */
 	public function __construct($action, $title = "", $form = null) {
 		$this->action = "action_$action";
+		$this->setForm($form);
 		
-		parent::__construct($this->action, $title, null, $form);
+		parent::__construct($this->action, $title);
 	}
 
+	/**
+	 * Get the action name
+	 *
+	 * @return string
+	 */
 	public function actionName() {
 		return substr($this->name, 7);
 	}
@@ -49,6 +65,9 @@ class FormAction extends FormField {
 	/**
 	 * Set the full action name, including action_
 	 * This provides an opportunity to replace it with something else
+	 *
+	 * @param string $fullAction
+	 * @return $this
 	 */
 	public function setFullAction($fullAction) {
 		$this->action = $fullAction;
@@ -76,6 +95,22 @@ class FormAction extends FormField {
 		return 'action';
 	}
 
+	public function Title() {
+		$title = parent::Title();
+
+		// Remove this method override in 4.0
+		$decoded = Convert::xml2raw($title);
+		if($decoded !== $title) {
+			Deprecation::notice(
+				'4.0',
+				'The FormAction title field should not be html encoded. Use buttonContent to set custom html instead'
+			);
+			return $decoded;
+		}
+
+		return $title;
+	}
+
 	public function getAttributes() {
 		$type = (isset($this->attributes['src'])) ? 'image' : 'submit';
 		
@@ -92,6 +127,9 @@ class FormAction extends FormField {
 
 	/**
 	 * Add content inside a button field.
+	 *
+	 * @param string $content
+	 * @return $this
 	 */
 	public function setButtonContent($content) {
 		$this->buttonContent = (string) $content;
@@ -99,14 +137,19 @@ class FormAction extends FormField {
 	}
 
 	/**
-	 * @return String
+	 * Gets the content inside the button field
+	 *
+	 * @return string
 	 */
 	public function getButtonContent() {
 		return $this->buttonContent;
 	}
 
 	/**
-	 * @param Boolean
+	 * Enable or disable the rendering of this action as a <button />
+	 *
+	 * @param boolean
+	 * @return $this
 	 */
 	public function setUseButtonTag($bool) {
 		$this->useButtonTag = $bool;
@@ -114,7 +157,9 @@ class FormAction extends FormField {
 	}
 
 	/**
-	 * @return Boolean
+	 * Determine if this action is rendered as a <button />
+	 *
+	 * @return boolean
 	 */
 	public function getUseButtonTag() {
 		return $this->useButtonTag;

forms/TreeDropdownField.php

@@ -71,13 +71,15 @@ class TreeDropdownField extends FormField {
 	 *
 	 * @param string $name the field name
 	 * @param string $title the field label
-	 * @param sourceObject The object-type to list in the tree.  Must be a 
-	 *		{@link Hierarchy} subclass.  Alternatively, you can set this to an 
-	 *		array of key/value pairs, like a {@link DropdownField} source.  In 
-	 *		this case, the field will act like show a flat list of tree items, 
-	 *		without any hierarchy. This is most useful in conjunction with 
-	 *		{@link TreeMultiselectField}, for presenting a set of checkboxes in 
-	 *		a compact view.
+	 * @param string|array $sourceObject The object-type to list in the tree. This could
+	 * be one of the following:
+	 * - A DataObject class name with the {@link Hierarchy} extension.
+	 * - An array of key/value pairs, like a {@link DropdownField} source. In
+	 *   this case, the field will act like show a flat list of tree items,
+	 *	 without any hierarchy. This is most useful in conjunction with
+	 *   {@link TreeMultiselectField}, for presenting a set of checkboxes in
+	 *   a compact view. Note, that all value strings must be XML encoded
+	 *   safely prior to being passed in.
 	 *
 	 * @param string $keyField to field on the source class to save as the 
 	 *		field value (default ID).
@@ -100,24 +102,6 @@ class TreeDropdownField extends FormField {
 		parent::__construct($name, $title);
 	}
 
-	/**
-	 * Helper for the front end to know if we should escape the label value
-	 *
-	 * @return bool Whether the label field should be escaped
-	 */
-	public function getEscapeLabelField() {
-		// be defensive
-		$escape = true;
-		$sourceClass = $this->getSourceObject();
-		//if it's an array, then it's an explicit set of values and we have to assume they've escaped their values already
-		//if the field is cast as XML, then we don't need to escape
-		if (is_array($sourceClass) || (is_a($sourceClass, 'ViewableData', true) && singleton($sourceClass)->escapeTypeForField($this->getLabelField()) == 'xml')) {
-			$escape = false;
-		}
-
-		return $escape;
-	}
-
 	/**
 	 * Set the ID of the root node of the tree. This defaults to 0 - i.e.
 	 * displays the whole tree.
@@ -213,14 +197,14 @@ class TreeDropdownField extends FormField {
 		Requirements::css(FRAMEWORK_DIR . '/css/TreeDropdownField.css');
 	
 		$record = $this->Value() ? $this->objectForKey($this->Value()) : null;
-		if($record) {
-			$title = $record->{$this->labelField};
+		if($record instanceof ViewableData) {
+			$title = $record->obj($this->labelField)->forTemplate();
+		} elseif($record) {
+			$title = Convert::raw2xml($record->{$this->labelField});
+		} else if($this->showSearch) {
+			$title = _t('DropdownField.CHOOSESEARCH', '(Choose or Search)', 'start value of a dropdown');
 		} else {
-			if($this->showSearch) {
-				$title = _t('DropdownField.CHOOSESEARCH', '(Choose or Search)', 'start value of a dropdown');
-			} else {
-				$title = _t('DropdownField.CHOOSE', '(Choose)', 'start value of a dropdown');
-			}
+			$title = _t('DropdownField.CHOOSE', '(Choose)', 'start value of a dropdown');
 		}
 
 		// TODO Implement for TreeMultiSelectField
@@ -305,8 +289,7 @@ class TreeDropdownField extends FormField {
 		}
 
 		$self = $this;
-		$escapeLabelField = ($obj->escapeTypeForField($this->labelField) != 'xml');
-		$titleFn = function(&$child) use(&$self, $escapeLabelField) {
+		$titleFn = function(&$child) use(&$self) {
 			$keyField = $self->keyField;
 			$labelField = $self->labelField;
 			return sprintf(
@@ -318,7 +301,7 @@ class TreeDropdownField extends FormField {
 				Convert::raw2xml($child->markingClasses()),
 				($self->nodeIsDisabled($child)) ? 'disabled' : '',
 				(int)$child->ID,
-				$escapeLabelField ? Convert::raw2xml($child->$labelField) : $child->$labelField
+				$child->obj($labelField)->forTemplate()
 			);
 		};
 

forms/TreeMultiselectField.php

@@ -100,23 +100,20 @@ class TreeMultiselectField extends TreeDropdownField {
 		Requirements::css(FRAMEWORK_DIR . '/css/TreeDropdownField.css');
 	
 		$value = '';
-		$itemList = '';
+		$titleArray = array();
+		$idArray = array();
 		$items = $this->getItems();
 
 		if($items && count($items)) {
-			foreach($items as $id => $item) {
-				$title = $item->Title;
-				if ($item instanceof ViewableData && $item->escapeTypeForField('Title') != 'xml') {
-					$title = Convert::raw2xml($title);
-				}
-				$titleArray[] = $title;
+			foreach($items as $item) {
 				$idArray[] = $item->ID;
+				$titleArray[] = ($item instanceof ViewableData)
+					? $item->obj($this->labelField)->forTemplate()
+					: Convert::raw2xml($item->{$this->labelField});
 			}
 				
-			if(isset($titleArray)) {
-				$title = implode(", ", $titleArray);
-				$value = implode(",", $idArray);
-			}
+			$title = implode(", ", $titleArray);
+			$value = implode(",", $idArray);
 		} else {
 			$title = _t('DropdownField.CHOOSE', '(Choose)', 'start value of a dropdown');
 		} 
@@ -133,6 +130,7 @@ class TreeMultiselectField extends TreeDropdownField {
 			array(
 				'Title' => $title,
 				'Link' => $dataUrlTree,
+				'Value' => $value
 			)
 		);
 		return $this->customise($properties)->renderWith('TreeDropdownField');

forms/gridfield/GridFieldPrintButton.php

@@ -192,7 +192,7 @@ class GridFieldPrintButton implements GridField_HTMLProvider, GridField_ActionPr
 			foreach($printColumns as $field => $label) {
 				$value = $gridField->getDataFieldValue($item, $field);
 
-				if ($item->escapeTypeForField('Title') != 'xml') {
+				if($item->escapeTypeForField($field) != 'xml') {
 					$value = Convert::raw2xml($value);
 				}
 

javascript/TreeDropdownField.js

@@ -139,10 +139,8 @@
 			},
 			setTitle: function(title) {
 				title = title || this.data('title') || strings.fieldTitle;
-
-				var func = this.data('escape-label-field') ? 'text' : 'html';
-
-				this.find('.treedropdownfield-title')[func](title);
+				
+				this.find('.treedropdownfield-title').html(title);
 				this.data('title', title); // separate view from storage (important for search cancellation)
 			},
 			getTitle: function() {
@@ -163,7 +161,7 @@
 						
 						if(title) {
 							self.setTitle(title);
-							self.data('title', title)
+							self.data('title', title);
 						}
 						if(node) tree.jstree('select_node', node);
 					}
@@ -237,6 +235,7 @@
 				var self = this;
 				return {
 					'core': {
+						'html_titles': true,
 						// 'initially_open': ['record-0'],
 						'animation': 0
 					},

model/DataDifferencer.php

@@ -85,15 +85,25 @@ class DataDifferencer extends ViewableData {
 			if(in_array($field, $this->ignoredFields)) continue;
 			if(in_array($field, array_keys($hasOnes))) continue;
 
-			$escape = false;
-			if ($this->toRecord->escapeTypeForField($field) != 'xml') {
-				$escape = true;
-			}
+			// Check if a field from-value is comparable
+			$toField = $this->toRecord->obj($field);
+			if(!($toField instanceof DBField)) continue;
+			$toValue = $toField->forTemplate();
+
+			// Show only to value
 			if(!$this->fromRecord) {
-				$val = $escape ? Convert::raw2xml($this->toRecord->$field) : $this->toRecord->$field;
-				$diffed->setField($field, "<ins>" . $val . "</ins>");
-			} else if($this->fromRecord->$field != $this->toRecord->$field) {
-				$diffed->setField($field, Diff::compareHTML($this->fromRecord->$field, $this->toRecord->$field, $escape));
+				$diffed->setField($field, "<ins>{$toValue}</ins>");
+				continue;
+			}
+
+			// Check if a field to-value is comparable
+			$fromField = $this->fromRecord->obj($field);
+			if(!($fromField instanceof DBField)) continue;
+			$fromValue = $fromField->forTemplate();
+
+			// Show changes between the two, if any exist
+			if($fromValue != $toValue) {
+				$diffed->setField($field, Diff::compareHTML($fromValue, $toValue));
 			}
 		}
 

templates/forms/FormAction.ss

@@ -1,6 +1,6 @@
 <% if $UseButtonTag %>
 	<button $AttributesHTML>
-		<% if $ButtonContent %>$ButtonContent<% else %>$Title<% end_if %>
+		<% if $ButtonContent %>$ButtonContent<% else %>$Title.XML<% end_if %>
 	</button>
 <% else %>
 	<input $AttributesHTML />

templates/forms/TreeDropdownField.ss

@@ -1,8 +1,7 @@
 <div id="TreeDropdownField_$ID"
-     class="TreeDropdownField <% if extraClass %> $extraClass<% end_if %><% if ShowSearch %> searchable<% end_if %>"
-     data-url-tree="$Link(tree)"
+     class="TreeDropdownField <% if $extraClass %> $extraClass<% end_if %><% if $ShowSearch %> searchable<% end_if %>"
+     data-url-tree="$Link('tree')"
      data-title="$Title.ATT"
-     data-escape-label-field="$EscapeLabelField"
      <% if $Description %>title="$Description.ATT"<% end_if %>
      <% if $Metadata %>data-metadata="$Metadata.ATT"<% end_if %>>
 	<input id="$ID" type="hidden" name="$Name.ATT" value="$Value.ATT" />

tests/model/DataDifferencerTest.php

@@ -17,7 +17,7 @@ class DataDifferencerTest extends SapphireTest {
 	public function testArrayValues() {
 		$obj1 = $this->objFromFixture('DataDifferencerTest_Object', 'obj1');
 		// create a new version
-		$obj1->Choices = array('a');
+		$obj1->Choices = 'a';
 		$obj1->write();
 		$obj1v1 = Versioned::get_version('DataDifferencerTest_Object', $obj1->ID, $obj1->Version-1);
 		$obj1v2 = Versioned::get_version('DataDifferencerTest_Object', $obj1->ID, $obj1->Version);
@@ -88,14 +88,6 @@ class DataDifferencerTest_Object extends DataObject implements TestOnly {
 		return $fields;
 	}
 	
-	public function getChoices() {
-		return explode(',', $this->getField('Choices'));
-	}
-	
-	public function setChoices($val) {
-		$this->setField('Choices', (is_array($val)) ? implode(',', $val) : $val);
-	}
-	
 }
 
 class DataDifferencerTest_HasOneRelationObject extends DataObject implements TestOnly {

view/ViewableData.php

@@ -273,9 +273,7 @@ class ViewableData extends Object implements IteratorAggregate {
 	 * @return string 'xml'|'raw'
 	 */
 	public function escapeTypeForField($field) {
-		if(!$class = $this->castingClass($field)) {
-			$class = $this->config()->get('default_cast');
-		}
+		$class = $this->castingClass($field) ?: $this->config()->default_cast;
 
 		return Config::inst()->get($class, 'escape_type', Config::FIRST_SET);
 	}
@@ -367,7 +365,7 @@ class ViewableData extends Object implements IteratorAggregate {
 			
 			if(!is_object($value) && ($this->castingClass($fieldName) || $forceReturnedObject)) {
 				if(!$castConstructor = $this->castingHelper($fieldName)) {
-					$castConstructor = $this->stat('default_cast');
+					$castConstructor = $this->config()->default_cast;
 				}
 				
 				$valueObject = Object::create_from_string($castConstructor, $fieldName);
@@ -382,7 +380,7 @@ class ViewableData extends Object implements IteratorAggregate {
 		}
 		
 		if(!is_object($value) && $forceReturnedObject) {
-			$default = Config::inst()->get('ViewableData', 'default_cast', Config::FIRST_SET);
+			$default = $this->config()->default_cast;
 			$castedValue = new $default($fieldName);
 			$castedValue->setValue($value);
 			$value = $castedValue;