Merge pull request #3894 from tractorcow/pulls/3.1/encoding-fixes

BUG Lots of encoding fixes

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 string $action The method to call when the button is clicked
-	 * @param title The label on the button
+	 * @param string $title The label on the button. This should be plain text, not escaped as HTML.
-	 * @param form The parent form, auto-set when the field is placed inside a form 
+	 * @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 
+	 * @param string|array $sourceObject The object-type to list in the tree. This could
-	 *		{@link Hierarchy} subclass.  Alternatively, you can set this to an 
+	 * be one of the following:
-	 *		array of key/value pairs, like a {@link DropdownField} source.  In 
+	 * - A DataObject class name with the {@link Hierarchy} extension.
-	 *		this case, the field will act like show a flat list of tree items, 
+	 * - An array of key/value pairs, like a {@link DropdownField} source. In
-	 *		without any hierarchy. This is most useful in conjunction with 
+	 *   this case, the field will act like show a flat list of tree items,
-	 *		{@link TreeMultiselectField}, for presenting a set of checkboxes in 
+	 *	 without any hierarchy. This is most useful in conjunction with
-	 *		a compact view.
+	 *   {@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).
@@ -94,12 +96,14 @@ class TreeDropdownField extends FormField {
 		$this->keyField     = $keyField;
 		$this->labelField   = $labelField;
 		$this->showSearch	= $showSearch;
-		
+
+		$this->addExtraClass('single');
+
 		parent::__construct($name, $title);
 	}
-	
+
 	/**
-	 * Set the ID of the root node of the tree. This defaults to 0 - i.e. 
+	 * Set the ID of the root node of the tree. This defaults to 0 - i.e.
 	 * displays the whole tree.
 	 *
 	 * @param int $ID
@@ -193,14 +197,14 @@ class TreeDropdownField extends FormField {
 		Requirements::css(FRAMEWORK_DIR . '/css/TreeDropdownField.css');
 	
 		$record = $this->Value() ? $this->objectForKey($this->Value()) : null;
-		if($record) {
+		if($record instanceof ViewableData) {
-			$title = $record->{$this->labelField};
+			$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.CHOOSE', '(Choose)', 'start value of a dropdown');
-				$title = _t('DropdownField.CHOOSESEARCH', '(Choose or Search)', 'start value of a dropdown');
-			} else {
-				$title = _t('DropdownField.CHOOSE', '(Choose)', 'start value of a dropdown');
-			}
 		}
 
 		// TODO Implement for TreeMultiSelectField
@@ -285,8 +289,7 @@ class TreeDropdownField extends FormField {
 		}
 
 		$self = $this;
-		$escapeLabelField = ($obj->escapeTypeForField($this->labelField) != 'xml');
+		$titleFn = function(&$child) use(&$self) {
-		$titleFn = function(&$child) use(&$self, $escapeLabelField) {
 			$keyField = $self->keyField;
 			$labelField = $self->labelField;
 			return sprintf(
@@ -298,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

@@ -46,6 +46,8 @@
 class TreeMultiselectField extends TreeDropdownField {
 	public function __construct($name, $title=null, $sourceObject="Group", $keyField="ID", $labelField="Title") {
 		parent::__construct($name, $title, $sourceObject, $keyField, $labelField);
+		$this->removeExtraClass('single');
+		$this->addExtraClass('multiple');
 		$this->value = 'unchanged';
 	}
 
@@ -98,19 +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) {
+			foreach($items as $item) {
-				$titleArray[] = $item->Title;
 				$idArray[] = $item->ID;
+				$titleArray[] = ($item instanceof ViewableData)
+					? $item->obj($this->labelField)->forTemplate()
+					: Convert::raw2xml($item->{$this->labelField});
 			}
 				
-			if(isset($titleArray)) {
+			$title = implode(", ", $titleArray);
-				$title = implode(", ", $titleArray);
+			$value = implode(",", $idArray);
-				$value = implode(",", $idArray);
-			}
 		} else {
 			$title = _t('DropdownField.CHOOSE', '(Choose)', 'start value of a dropdown');
 		} 
@@ -118,30 +121,19 @@ class TreeMultiselectField extends TreeDropdownField {
 		$dataUrlTree = '';
 		if ($this->form){
 			$dataUrlTree = $this->Link('tree');
-			if (isset($idArray) && count($idArray)){
+			if (!empty($idArray)){
 				$dataUrlTree = Controller::join_links($dataUrlTree, '?forceValue='.implode(',',$idArray));
 			}
 		}
-		return FormField::create_tag(
+		$properties = array_merge(
-			'div',
+			$properties,
-			array (
+			array(
-				'id'    => "TreeDropdownField_{$this->id()}",
+				'Title' => $title,
-				'class' => 'TreeDropdownField multiple' . ($this->extraClass() ? " {$this->extraClass()}" : '')
+				'Link' => $dataUrlTree,
-					. ($this->showSearch ? " searchable" : ''),
+				'Value' => $value
-				'data-url-tree' => $dataUrlTree,
-				'data-title' => $title,
-				'title' => $this->getDescription()
-			),
-			FormField::create_tag(
-				'input',
-				array (
-					'id'    => $this->id(),
-					'type'  => 'hidden',
-					'name'  => $this->name,
-					'value' => $value
-				)
 			)
 		);
+		return $this->customise($properties)->renderWith('TreeDropdownField');
 	}
 
 	/**

forms/gridfield/GridFieldPrintButton.php

@@ -192,6 +192,10 @@ class GridFieldPrintButton implements GridField_HTMLProvider, GridField_ActionPr
 			foreach($printColumns as $field => $label) {
 				$value = $gridField->getDataFieldValue($item, $field);
 
+				if($item->escapeTypeForField($field) != 'xml') {
+					$value = Convert::raw2xml($value);
+				}
+
 				$itemRow->push(new ArrayData(array(
 					"CellString" => $value,
 				)));

javascript/TreeDropdownField.js

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

model/DataDifferencer.php

@@ -85,10 +85,25 @@ class DataDifferencer extends ViewableData {
 			if(in_array($field, $this->ignoredFields)) continue;
 			if(in_array($field, array_keys($hasOnes))) continue;
 
+			// 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) {
-				$diffed->setField($field, "<ins>" . $this->toRecord->$field . "</ins>");
+				$diffed->setField($field, "<ins>{$toValue}</ins>");
-			} else if($this->fromRecord->$field != $this->toRecord->$field) {
+				continue;
-				$diffed->setField($field, Diff::compareHTML($this->fromRecord->$field, $this->toRecord->$field));
+			}
+
+			// 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));
 			}
 		}
 

model/DataObject.php

@@ -3435,6 +3435,8 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 	 * @var array
 	 */
 	private static $casting = array(
+		"ID" => 'Int',
+		"ClassName" => 'Varchar',
 		"LastEdited" => "SS_Datetime",
 		"Created" => "SS_Datetime",
 		"Title" => 'Text',

security/PermissionCheckboxSetField.php

@@ -99,7 +99,7 @@ class PermissionCheckboxSetField extends FormField {
 				if(!isset($uninheritedCodes[$permission->Code])) $uninheritedCodes[$permission->Code] = array();
 				$uninheritedCodes[$permission->Code][] = _t(
 					'PermissionCheckboxSetField.AssignedTo', 'assigned to "{title}"',
-					array('title' => $record->Title)
+					array('title' => $record->dbObject('Title')->forTemplate())
 				);
 			}
 
@@ -115,7 +115,7 @@ class PermissionCheckboxSetField extends FormField {
 								'PermissionCheckboxSetField.FromRole',
 								'inherited from role "{title}"',
 								'A permission inherited from a certain permission role',
-								array('title' => $role->Title)
+								array('title' => $role->dbObject('Title')->forTemplate())
 							);
 						}
 					}
@@ -134,7 +134,7 @@ class PermissionCheckboxSetField extends FormField {
 										'PermissionCheckboxSetField.FromRoleOnGroup',
 										'inherited from role "%s" on group "%s"',
 										'A permission inherited from a role on a certain group',
-										array('roletitle' => $role->Title, 'grouptitle' => $parent->Title)
+										array('roletitle' => $role->dbObject('Title')->forTemplate(), 'grouptitle' => $parent->dbObject('Title')->forTemplate())
 									);
 								}
 							}
@@ -149,7 +149,7 @@ class PermissionCheckboxSetField extends FormField {
 									'PermissionCheckboxSetField.FromGroup',
 									'inherited from group "{title}"',
 									'A permission inherited from a certain group',
-									array('title' => $parent->Title)
+									array('title' => $parent->dbObject('Title')->forTemplate())
 								);
 							}
 						}

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,6 +1,6 @@
 <div id="TreeDropdownField_$ID"
-     class="TreeDropdownField single<% if extraClass %> $extraClass<% end_if %><% if ShowSearch %> searchable<% end_if %>"
+     class="TreeDropdownField <% if $extraClass %> $extraClass<% end_if %><% if $ShowSearch %> searchable<% end_if %>"
-     data-url-tree="$Link(tree)"
+     data-url-tree="$Link('tree')"
      data-title="$Title.ATT"
      <% if $Description %>title="$Description.ATT"<% end_if %>
      <% if $Metadata %>data-metadata="$Metadata.ATT"<% end_if %>>

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,13 +273,11 @@ class ViewableData extends Object implements IteratorAggregate {
 	 * @return string 'xml'|'raw'
 	 */
 	public function escapeTypeForField($field) {
-		if(!$class = $this->castingClass($field)) {
+		$class = $this->castingClass($field) ?: $this->config()->default_cast;
-			$class = self::$default_cast;
+
-		}
-		
 		return Config::inst()->get($class, 'escape_type', Config::FIRST_SET);
 	}
-	
+
 	/**
 	 * Save the casting cache for this object (including data from any failovers) into a variable
 	 *
@@ -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;