Don't use session and FormSchema to manage server-side React validation responses

src/Assets/Storage/DBFile.php

@@ -122,7 +122,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
     public function getTag()
     {
 		$template = $this->getFrontendTemplate();
-        if (empty($template)) {
+		if(empty($template)) {
 			return '';
 		}
 		return (string)$this->renderWith($template);
@@ -137,12 +137,12 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
     {
 		// Check that path is available
 		$url = $this->getURL();
-        if (empty($url)) {
+		if(empty($url)) {
 			return null;
 		}
 
 		// Image template for supported images
-        if ($this->getIsImage()) {
+		if($this->getIsImage()) {
 			return 'DBFile_image';
 		}
 
@@ -157,7 +157,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 	 */
     public function getBasename()
     {
-        if (!$this->exists()) {
+		if(!$this->exists()) {
 			return null;
 		}
 		return basename($this->getSourceURL());
@@ -170,7 +170,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 	 */
     public function getExtension()
     {
-        if (!$this->exists()) {
+		if(!$this->exists()) {
 			return null;
 		}
 		return pathinfo($this->Filename, PATHINFO_EXTENSION);
@@ -184,7 +184,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
     public function getTitle()
     {
 		// If customised, use the customised title
-        if ($this->failover && ($title = $this->failover->Title)) {
+		if($this->failover && ($title = $this->failover->Title)) {
 			return $title;
 		}
 		// fallback to using base name
@@ -198,7 +198,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 			->getStore()
 			->setFromLocalFile($path, $filename, $hash, $variant, $config);
 		// Update from result
-        if ($result) {
+		if($result) {
 			$this->setValue($result);
 		}
 		return $result;
@@ -211,7 +211,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 			->getStore()
 			->setFromStream($stream, $filename, $hash, $variant, $config);
 		// Update from result
-        if ($result) {
+		if($result) {
 			$this->setValue($result);
 		}
 		return $result;
@@ -224,7 +224,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 			->getStore()
 			->setFromString($data, $filename, $hash, $variant, $config);
 		// Update from result
-        if ($result) {
+		if($result) {
 			$this->setValue($result);
 		}
 		return $result;
@@ -232,7 +232,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function getStream()
     {
-        if (!$this->exists()) {
+		if(!$this->exists()) {
 			return null;
 		}
 		return $this
@@ -242,7 +242,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function getString()
     {
-        if (!$this->exists()) {
+		if(!$this->exists()) {
 			return null;
 		}
 		return $this
@@ -252,7 +252,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function getURL($grant = true)
     {
-        if (!$this->exists()) {
+		if(!$this->exists()) {
 			return null;
 		}
 		$url = $this->getSourceURL($grant);
@@ -282,7 +282,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 	 */
     public function getAbsoluteURL()
     {
-        if (!$this->exists()) {
+		if(!$this->exists()) {
 			return null;
 		}
 		return Director::absoluteURL($this->getURL());
@@ -290,7 +290,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function getMetaData()
     {
-        if (!$this->exists()) {
+		if(!$this->exists()) {
 			return null;
 		}
 		return $this
@@ -300,7 +300,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function getMimeType()
     {
-        if (!$this->exists()) {
+		if(!$this->exists()) {
 			return null;
 		}
 		return $this
@@ -310,7 +310,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function getValue()
     {
-        if (!$this->exists()) {
+		if(!$this->exists()) {
 			return null;
 		}
 		return array(
@@ -322,7 +322,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function getVisibility()
     {
-        if (empty($this->Filename)) {
+		if(empty($this->Filename)) {
 			return null;
 		}
 		return $this
@@ -332,7 +332,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function exists()
     {
-        if (empty($this->Filename)) {
+		if(empty($this->Filename)) {
 			return false;
 		}
 		return $this
@@ -363,7 +363,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
     public function getAbsoluteSize()
     {
 		$metadata = $this->getMetaData();
-        if (isset($metadata['size'])) {
+		if(isset($metadata['size'])) {
 			return $metadata['size'];
 		}
 		return 0;
@@ -399,7 +399,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 	 */
     public function setAllowedCategories($categories)
     {
-        if (is_string($categories)) {
+		if(is_string($categories)) {
 			$categories = preg_split('/\s*,\s*/', $categories);
 		}
 		$this->allowedCategories = (array)$categories;
@@ -431,13 +431,13 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
 		// Validate true if within the list of allowed extensions
 		$allowed = $this->getAllowedExtensions();
-        if ($allowed) {
+		if($allowed) {
 			return in_array($extension, $allowed);
 		}
 
 		// If no extensions are configured, fallback to global list
 		$globalList = File::config()->allowed_extensions;
-        if (in_array($extension, $globalList)) {
+		if(in_array($extension, $globalList)) {
 			return true;
 		}
 
@@ -455,7 +455,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
     {
 		$result = new ValidationResult();
 		$this->validate($result, $filename);
-        if (!$result->valid()) {
+		if(!$result->valid()) {
 			throw new ValidationException($result);
 		}
 	}
@@ -470,16 +470,16 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 	 */
     public function validate(ValidationResult $result, $filename = null)
     {
-        if (empty($filename)) {
+		if(empty($filename)) {
 			$filename = $this->getFilename();
 		}
-        if (empty($filename) || $this->isValidFilename($filename)) {
+		if(empty($filename) || $this->isValidFilename($filename)) {
 			return true;
 		}
 
 		// Check allowed extensions
 		$extensions = $this->getAllowedExtensions();
-        if (empty($extensions)) {
+		if(empty($extensions)) {
 			$extensions = File::config()->allowed_extensions;
 		}
 		sort($extensions);
@@ -487,16 +487,16 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 			'File.INVALIDEXTENSION',
 			'Extension is not allowed (valid: {extensions})',
 			'Argument 1: Comma-separated list of valid extensions',
-            array('extensions' => wordwrap(implode(', ', $extensions)))
+			array('extensions' => wordwrap(implode(', ',$extensions)))
 		);
-        $result->error($message);
+		$result->addError($message);
 		return false;
 	}
 
     public function setField($field, $value, $markChanged = true)
     {
 		// Catch filename validation on direct assignment
-        if ($field === 'Filename' && $value) {
+		if($field === 'Filename' && $value) {
 			$this->assertFilenameValid($value);
 		}
 
@@ -512,7 +512,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
     public function getSize()
     {
 		$size = $this->getAbsoluteSize();
-        if ($size) {
+		if($size) {
 			return File::format_size($size);
 		}
 		return false;
@@ -520,7 +520,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function deleteFile()
     {
-        if (!$this->Filename) {
+		if(!$this->Filename) {
 			return false;
 		}
 
@@ -531,7 +531,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function publishFile()
     {
-        if ($this->Filename) {
+		if($this->Filename) {
 			$this
 				->getStore()
 				->publish($this->Filename, $this->Hash);
@@ -540,7 +540,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function protectFile()
     {
-        if ($this->Filename) {
+		if($this->Filename) {
 			$this
 				->getStore()
 				->protect($this->Filename, $this->Hash);
@@ -549,7 +549,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function grantFile()
     {
-        if ($this->Filename) {
+		if($this->Filename) {
 			$this
 				->getStore()
 				->grant($this->Filename, $this->Hash);
@@ -558,7 +558,7 @@ class DBFile extends DBComposite implements AssetContainer, Thumbnail
 
     public function revokeFile()
     {
-        if ($this->Filename) {
+		if($this->Filename) {
 			$this
 				->getStore()
 				->revoke($this->Filename, $this->Hash);

src/Dev/FunctionalTest.php

@@ -83,7 +83,7 @@ class FunctionalTest extends SapphireTest
     public function setUp()
     {
 		// Skip calling FunctionalTest directly.
-        if (get_class($this) == __CLASS__) {
+		if(get_class($this) == __CLASS__) {
 			$this->markTestSkipped(sprintf('Skipping %s ', get_class($this)));
 		}
 
@@ -91,12 +91,12 @@ class FunctionalTest extends SapphireTest
 		$this->mainSession = new TestSession();
 
 		// Disable theme, if necessary
-        if (static::get_disable_themes()) {
+		if(static::get_disable_themes()) {
 			SSViewer::config()->update('theme_enabled', false);
 		}
 
 		// Switch to draft site, if necessary
-        if (static::get_use_draft_site()) {
+		if(static::get_use_draft_site()) {
 			$this->useDraftSite();
 		}
 
@@ -155,7 +155,7 @@ class FunctionalTest extends SapphireTest
     {
 		$this->cssParser = null;
 		$response = $this->mainSession->get($url, $session, $headers, $cookies);
-        if ($this->autoFollowRedirection && is_object($response) && $response->getHeader('Location')) {
+		if($this->autoFollowRedirection && is_object($response) && $response->getHeader('Location')) {
 			$response = $this->mainSession->followRedirection();
 		}
 		return $response;
@@ -177,7 +177,7 @@ class FunctionalTest extends SapphireTest
     {
 		$this->cssParser = null;
 		$response = $this->mainSession->post($url, $data, $headers, $session, $body, $cookies);
-        if ($this->autoFollowRedirection && is_object($response) && $response->getHeader('Location')) {
+		if($this->autoFollowRedirection && is_object($response) && $response->getHeader('Location')) {
 			$response = $this->mainSession->followRedirection();
 		}
 		return $response;
@@ -206,7 +206,7 @@ class FunctionalTest extends SapphireTest
     {
 		$this->cssParser = null;
 		$response = $this->mainSession->submitForm($formID, $button, $data);
-        if ($this->autoFollowRedirection && is_object($response) && $response->getHeader('Location')) {
+		if($this->autoFollowRedirection && is_object($response) && $response->getHeader('Location')) {
 			$response = $this->mainSession->followRedirection();
 		}
 		return $response;
@@ -231,8 +231,8 @@ class FunctionalTest extends SapphireTest
     public function findAttribute($object, $attribute)
     {
 		$found = false;
-        foreach ($object->attributes() as $a => $b) {
+		foreach($object->attributes() as $a => $b) {
-            if ($a == $attribute) {
+			if($a == $attribute) {
 				$found = $b;
 			}
 		}
@@ -273,13 +273,9 @@ class FunctionalTest extends SapphireTest
 		$items = $this->cssParser()->getBySelector($selector);
 
 		$actuals = array();
-        if ($items) {
+		if($items) foreach($items as $item) $actuals[trim(preg_replace("/\s+/", " ", (string)$item))] = true;
-            foreach ($items as $item) {
-                $actuals[trim(preg_replace("/[ \n\r\t]+/", " ", $item. ''))] = true;
-            }
-        }
 
-        foreach ($expectedMatches as $match) {
+		foreach($expectedMatches as $match) {
 			$this->assertTrue(
 				isset($actuals[$match]),
 		"Failed asserting the CSS selector '$selector' has a partial match to the expected elements:\n'"
@@ -350,14 +346,14 @@ class FunctionalTest extends SapphireTest
 		$items = $this->cssParser()->getBySelector($selector);
 
 		$actuals = array();
-        if ($items) {
+		if($items) {
 			/** @var SimpleXMLElement $item */
-            foreach ($items as $item) {
+			foreach($items as $item) {
 				$actuals[$item->asXML()] = true;
 			}
 		}
 
-        foreach ($expectedMatches as $match) {
+		foreach($expectedMatches as $match) {
 			$this->assertTrue(
 				isset($actuals[$match]),
 				"Failed asserting the CSS selector '$selector' has a partial match to the expected elements:\n'"
@@ -385,9 +381,9 @@ class FunctionalTest extends SapphireTest
 		$items = $this->cssParser()->getBySelector($selector);
 
 		$actuals = array();
-        if ($items) {
+		if($items) {
 			/** @var SimpleXMLElement $item */
-            foreach ($items as $item) {
+			foreach($items as $item) {
 				$actuals[] = $item->asXML();
 			}
 		}
@@ -427,7 +423,7 @@ class FunctionalTest extends SapphireTest
 	 */
     public function useDraftSite($enabled = true)
     {
-        if ($enabled) {
+		if($enabled) {
 			$this->session()->inst_set('readingMode', 'Stage.Stage');
 			$this->session()->inst_set('unsecuredDraftSite', true);
         } else {

src/Forms/Form.php

@@ -289,7 +289,7 @@ class Form extends RequestHandler
 
 		// Check if CSRF protection is enabled, either on the parent controller or from the default setting. Note that
 		// method_exists() is used as some controllers (e.g. GroupTest) do not always extend from Object.
-        if (method_exists($controller, 'securityTokenEnabled') || (method_exists($controller, 'hasMethod')
+		if(method_exists($controller, 'securityTokenEnabled') || (method_exists($controller, 'hasMethod')
 				&& $controller->hasMethod('securityTokenEnabled'))) {
 			$securityEnabled = $controller->securityTokenEnabled();
 		} else {
@@ -312,38 +312,34 @@ class Form extends RequestHandler
 	);
 
 	/**
-     * Set up current form errors in session to
+	 * Take errors from a ValidationResult and populate the form with the appropriate message.
-     * the current form if appropriate.
 	 *
-     * @return $this
+	 * @param ValidationResult $result The erroneous ValidationResult.  If none passed, this will be atken
+	 * from the session
 	 */
-    public function setupFormErrors()
+	public function setupFormErrors($result = null, $data = null) {
-    {
+		if(!$result) $result = Session::get("FormInfo.{$this->FormName()}.result");
-        $errorInfo = Session::get("FormInfo.{$this->FormName()}");
+		if(!$result) return;
 
-        if (isset($errorInfo['errors']) && is_array($errorInfo['errors'])) {
+		foreach($result->fieldErrors() as $fieldName => $fieldError) {
-            foreach ($errorInfo['errors'] as $error) {
+			$field = $this->fields->dataFieldByName($fieldName);
-                $field = $this->fields->dataFieldByName($error['fieldName']);
+			$field->setError($fieldError['message'], $fieldError['messageType']);
+		}
 
-                if (!$field) {
+		//don't escape the HTML as it should have been escaped when adding it to the validation result
-                    $errorInfo['message'] = $error['message'];
+		$this->setMessage($result->overallMessage(), $result->valid() ? 'good' : 'bad', false);
-                    $errorInfo['type'] = $error['messageType'];
-                } else {
-                    $field->setError($error['message'], $error['messageType']);
-                }
-            }
 
 			// load data in from previous submission upon error
-            if (isset($errorInfo['data'])) {
+		if(!$data) $data = Session::get("FormInfo.{$this->FormName()}.data");
-                $this->loadDataFrom($errorInfo['data']);
+		if($data) $this->loadDataFrom($data);
-            }
 		}
 
-        if (isset($errorInfo['message']) && isset($errorInfo['type'])) {
+	/**
-            $this->setMessage($errorInfo['message'], $errorInfo['type']);
+	 * Save information to the session to be picked up by {@link setUpFormErrors()}
-        }
+	 */
-
+	public function saveFormErrorsToSession($result = null, $data = null) {
-        return $this;
+		Session::set("FormInfo.{$this->FormName()}.result", $result);
+		Session::set("FormInfo.{$this->FormName()}.data", $data);
 		}
 
 	/**
@@ -372,9 +368,9 @@ class Form extends RequestHandler
     public function httpSubmission($request)
     {
 		// Strict method check
-        if ($this->strictFormMethodCheck) {
+		if($this->strictFormMethodCheck) {
 			// Throws an error if the method is bad...
-            if ($this->formMethod != $request->httpMethod()) {
+			if($this->formMethod != $request->httpMethod()) {
 				$response = Controller::curr()->getResponse();
 				$response->addHeader('Allow', $this->formMethod);
 				$this->httpError(405, _t("Form.BAD_METHOD", "This form requires a ".$this->formMethod." submission"));
@@ -394,7 +390,7 @@ class Form extends RequestHandler
 
 		// Protection against CSRF attacks
 		$token = $this->getSecurityToken();
-        if (! $token->checkRequest($request)) {
+		if( ! $token->checkRequest($request)) {
 			$securityID = $token->getName();
 			if (empty($vars[$securityID])) {
                 $this->httpError(400, _t(
@@ -418,10 +414,10 @@ class Form extends RequestHandler
 
 		// Determine the action button clicked
 		$funcName = null;
-        foreach ($vars as $paramName => $paramVal) {
+		foreach($vars as $paramName => $paramVal) {
-            if (substr($paramName, 0, 7) == 'action_') {
+			if(substr($paramName,0,7) == 'action_') {
 				// Break off querystring arguments included in the action
-                if (strpos($paramName, '?') !== false) {
+				if(strpos($paramName,'?') !== false) {
 					list($paramName, $paramVars) = explode('?', $paramName, 2);
 					$newRequestParams = array();
 					parse_str($paramVars, $newRequestParams);
@@ -429,17 +425,17 @@ class Form extends RequestHandler
 				}
 
 				// Cleanup action_, _x and _y from image fields
-                $funcName = preg_replace(array('/^action_/','/_x$|_y$/'), '', $paramName);
+				$funcName = preg_replace(array('/^action_/','/_x$|_y$/'),'',$paramName);
 				break;
 			}
 		}
 
 		// If the action wasn't set, choose the default on the form.
-        if (!isset($funcName) && $defaultAction = $this->defaultAction()) {
+		if(!isset($funcName) && $defaultAction = $this->defaultAction()){
 			$funcName = $defaultAction->actionName();
 		}
 
-        if (isset($funcName)) {
+		if(isset($funcName)) {
 			$this->setButtonClicked($funcName);
 		}
 
@@ -485,18 +481,37 @@ class Form extends RequestHandler
 			}
 		}*/
 
-        // Validate the form
+		// Action handlers may throw ValidationExceptions.
-        if (!$this->validate()) {
+		try {
-            return $this->getValidationErrorResponse();
+			// Or we can use the Valiator attached to the form
+			$result = $this->validationResult();
+			if(!$result->valid()) {
+				return $this->getValidationErrorResponse($result);
 		}
 
 		// First, try a handler method on the controller (has been checked for allowed_actions above already)
-        if ($this->controller->hasMethod($funcName)) {
+		if($this->controller->hasMethod($funcName)) {
 			return $this->controller->$funcName($vars, $this, $request);
 		// Otherwise, try a handler method on the form object.
-        } elseif ($this->hasMethod($funcName)) {
+		} elseif($this->hasMethod($funcName)) {
 			return $this->$funcName($vars, $this, $request);
-        } elseif ($field = $this->checkFieldsForAction($this->Fields(), $funcName)) {
+		} elseif($field = $this->checkFieldsForAction($this->Fields(), $funcName)) {
+			return $field->$funcName($vars, $this, $request);
+		}
+
+		} catch(ValidationException $e) {
+			// The ValdiationResult contains all the relevant metadata
+			$result = $e->getResult();
+			return $this->getValidationErrorResponse($result);
+		}
+
+		// First, try a handler method on the controller (has been checked for allowed_actions above already)
+		if($this->controller->hasMethod($funcName)) {
+			return $this->controller->$funcName($vars, $this, $request);
+		// Otherwise, try a handler method on the form object.
+		} elseif($this->hasMethod($funcName)) {
+			return $this->$funcName($vars, $this, $request);
+		} elseif($field = $this->checkFieldsForAction($this->Fields(), $funcName)) {
 			return $field->$funcName($vars, $this, $request);
 		}
 
@@ -562,36 +577,42 @@ class Form extends RequestHandler
 	 * Behaviour can be influenced by setting {@link $redirectToFormOnValidationError},
 	 * and can be overruled by setting {@link $validationResponseCallback}.
 	 *
+	 * @param ValidationResult $result
 	 * @return HTTPResponse|string
 	 */
-    protected function getValidationErrorResponse()
+	protected function getValidationErrorResponse(ValidationResult $result) {
-    {
 		$callback = $this->getValidationResponseCallback();
-        if ($callback && $callbackResponse = $callback()) {
+		if($callback && $callbackResponse = $callback($result)) {
 			return $callbackResponse;
 		}
 
 		$request = $this->getRequest();
-        if ($request->isAjax()) {
+		if($request->isAjax()) {
 				// Special case for legacy Validator.js implementation
 				// (assumes eval'ed javascript collected through FormResponse)
 				$acceptType = $request->getHeader('Accept');
             if (strpos($acceptType, 'application/json') !== false) {
 					// Send validation errors back as JSON with a flag at the start
-                $response = new HTTPResponse(Convert::array2json($this->validator->getErrors()));
+					$response = new HTTPResponse(Convert::array2json($result->getErrorMetaData()));
 					$response->addHeader('Content-Type', 'application/json');
+
 				} else {
-                $this->setupFormErrors();
+					$this->setupFormErrors($result, $this->getData());
 					// Send the newly rendered form tag as HTML
 					$response = new HTTPResponse($this->forTemplate());
 					$response->addHeader('Content-Type', 'text/html');
 				}
 
 				return $response;
+
 			} else {
-            if ($this->getRedirectToFormOnValidationError()) {
+			// Save the relevant information in the session
-                if ($pageURL = $request->getHeader('Referer')) {
+			$this->saveFormErrorsToSession($result, $this->getData());
-                    if (Director::is_site_url($pageURL)) {
+
+			// Redirect back to the form
+				if($this->getRedirectToFormOnValidationError()) {
+					if($pageURL = $request->getHeader('Referer')) {
+						if(Director::is_site_url($pageURL)) {
 							// Remove existing pragmas
 							$pageURL = preg_replace('/(#.*)/', '', $pageURL);
 							$pageURL = Director::absoluteURL($pageURL, true);
@@ -612,10 +633,10 @@ class Form extends RequestHandler
 	 */
     protected function checkFieldsForAction($fields, $funcName)
     {
-        foreach ($fields as $field) {
+		foreach($fields as $field){
 			/** @skipUpgrade */
-            if (method_exists($field, 'FieldList')) {
+			if(method_exists($field, 'FieldList')) {
-                if ($field = $this->checkFieldsForAction($field->FieldList(), $funcName)) {
+				if($field = $this->checkFieldsForAction($field->FieldList(), $funcName)) {
 					return $field;
 				}
 			} elseif ($field->hasMethod($funcName) && $field->checkAccessAction($funcName)) {
@@ -640,7 +661,7 @@ class Form extends RequestHandler
     {
 		$field = $this->Fields()->dataFieldByName($request->param('FieldName'));
 
-        if ($field) {
+		if($field) {
 			return $field;
 		} else {
 			// falling back to fieldByName, e.g. for getting tabs
@@ -684,18 +705,13 @@ class Form extends RequestHandler
 	/**
 	 * Add a plain text error message to a field on this form.  It will be saved into the session
 	 * and used the next time this form is displayed.
-     * @param string $fieldName
+	 *
-     * @param string $message
+	 * @deprecated 3.2
-     * @param string $messageType
-     * @param bool $escapeHtml
 	 */
-    public function addErrorMessage($fieldName, $message, $messageType, $escapeHtml = true)
+	public function addErrorMessage($fieldName, $message, $messageType) {
-    {
+		Deprecation::notice('3.2', 'Throw a ValidationException instead.');
-        Session::add_to_array("FormInfo.{$this->FormName()}.errors", array(
+
-            'fieldName' => $fieldName,
+		$this->getSessionValidationResult()->addFieldError($fieldName, $message, $messageType);
-            'message' => $escapeHtml ? Convert::raw2xml($message) : $message,
-            'messageType' => $messageType,
-        ));
 	}
 
 	/**
@@ -704,13 +720,13 @@ class Form extends RequestHandler
     public function transform(FormTransformation $trans)
     {
 		$newFields = new FieldList();
-        foreach ($this->fields as $field) {
+		foreach($this->fields as $field) {
 			$newFields->push($field->transform($trans));
 		}
 		$this->fields = $newFields;
 
 		$newActions = new FieldList();
-        foreach ($this->actions as $action) {
+		foreach($this->actions as $action) {
 			$newActions->push($action->transform($trans));
 		}
 		$this->actions = $newActions;
@@ -738,7 +754,7 @@ class Form extends RequestHandler
 	 */
     public function setValidator(Validator $validator)
     {
-        if ($validator) {
+		if($validator) {
 			$this->validator = $validator;
 			$this->validator->setForm($this);
 		}
@@ -828,7 +844,7 @@ class Form extends RequestHandler
 	 */
     public function Fields()
     {
-        foreach ($this->getExtraFields() as $field) {
+		foreach($this->getExtraFields() as $field) {
             if (!$this->fields->fieldByName($field->getName())) {
                 $this->fields->push($field);
             }
@@ -918,7 +934,7 @@ class Form extends RequestHandler
 	 */
     public function getAttribute($name)
     {
-        if (isset($this->attributes[$name])) {
+		if(isset($this->attributes[$name])) {
 			return $this->attributes[$name];
 		}
 		return null;
@@ -938,7 +954,7 @@ class Form extends RequestHandler
 			'class' => $this->extraClass(),
 		);
 
-        if ($this->validator && $this->validator->getErrors()) {
+		if($this->validator && $this->validator->getErrors()) {
             if (!isset($attrs['class'])) {
                 $attrs['class'] = '';
             }
@@ -998,7 +1014,7 @@ class Form extends RequestHandler
 
 		// Create markup
 		$parts = array();
-        foreach ($attrs as $name => $value) {
+		foreach($attrs as $name => $value) {
 			$parts[] = ($value === true) ? "{$name}=\"{$name}\"" : "{$name}=\"" . Convert::raw2att($value) . "\"";
 		}
 
@@ -1029,8 +1045,8 @@ class Form extends RequestHandler
 	 */
     public function getTemplateHelper()
     {
-        if ($this->templateHelper) {
+		if($this->templateHelper) {
-            if (is_string($this->templateHelper)) {
+			if(is_string($this->templateHelper)) {
 				return Injector::inst()->get($this->templateHelper);
 			}
 
@@ -1100,7 +1116,7 @@ class Form extends RequestHandler
     {
 		$templates = SSViewer::get_templates_by_class(get_class($this), '', __CLASS__);
 		// Prefer any custom template
-        if ($this->getTemplate()) {
+		if($this->getTemplate()) {
 			array_unshift($templates, $this->getTemplate());
 		}
 		return $templates;
@@ -1168,7 +1184,7 @@ class Form extends RequestHandler
 	 */
     public function FormMethod()
     {
-        if (in_array($this->formMethod, array('GET','POST'))) {
+		if(in_array($this->formMethod,array('GET','POST'))) {
 			return $this->formMethod;
 		} else {
 			return 'POST';
@@ -1228,7 +1244,7 @@ class Form extends RequestHandler
     {
 		if ($this->formActionPath) {
 			return $this->formActionPath;
-        } elseif ($this->controller->hasMethod("FormObjectLink")) {
+		} elseif($this->controller->hasMethod("FormObjectLink")) {
 			return $this->controller->FormObjectLink($this->name);
 		} else {
 			return Controller::join_links($this->controller->Link(), $this->name);
@@ -1349,38 +1365,17 @@ class Form extends RequestHandler
 	 *
 	 * @return string
 	 */
-    public function Message()
+	public function Message() {
-    {
-        $this->getMessageFromSession();
-
 		return $this->message;
 	}
 
 	/**
 	 * @return string
 	 */
-    public function MessageType()
+	public function MessageType() {
-    {
-        $this->getMessageFromSession();
-
 		return $this->messageType;
 	}
 
-    /**
-     * @return string
-     */
-    protected function getMessageFromSession()
-    {
-        if ($this->message || $this->messageType) {
-            return $this->message;
-        } else {
-            $this->message = Session::get("FormInfo.{$this->FormName()}.formError.message");
-            $this->messageType = Session::get("FormInfo.{$this->FormName()}.formError.type");
-
-            return $this->message;
-        }
-    }
-
 	/**
 	 * Set a status message for the form.
 	 *
@@ -1409,34 +1404,67 @@ class Form extends RequestHandler
 	 */
     public function sessionMessage($message, $type, $escapeHtml = true)
     {
-        Session::set(
+		// Benign message
-            "FormInfo.{$this->FormName()}.formError.message",
+		if($type == "good") {
-            $escapeHtml ? Convert::raw2xml($message) : $message
+			$this->getSessionValidationResult()->addMessage($message, $type, null, $escapeHtml);
+
+		// Bad message causing a validation error
+		} else {
+			$this->getSessionValidationResult()->addError($message, $type, null, $escapeHtml
 		);
-        Session::set("FormInfo.{$this->FormName()}.formError.type", $type);
+		}
 	}
 
-    public static function messageForForm($formName, $message, $type, $escapeHtml = true)
+	/**
-    {
+	 * @deprecated 3.1
-        Session::set(
+	 */
-            "FormInfo.{$formName}.formError.message",
+	public static function messageForForm($formName, $message, $type) {
-            $escapeHtml ? Convert::raw2xml($message) : $message
+		Deprecation::notice('3.1', 'Create an instance of the form you wish to attach a message to.');
-        );
+	}
-        Session::set("FormInfo.{$formName}.formError.type", $type);
+
+	/**
+	 * Returns the ValidationResult stored in the session.
+	 * You can use this to modify messages without throwing a ValidationException.
+	 * If a ValidationResult doesn't yet exist, a new one will be created
+	 *
+	 * @return ValidationResult The ValidationResult object stored in the session
+	 */
+	public function getSessionValidationResult() {
+		$result = Session::get("FormInfo.{$this->FormName()}.result");
+
+		if(!$result || !($result instanceof ValidationResult)) {
+			$result = new ValidationResult;
+			Session::set("FormInfo.{$this->FormName()}.result", $result);
+		}
+
+		return $result;
+	}
+
+	/**
+	 * Sets the ValidationResult in the session to be used with the next view of this form.
+	 * @param ValidationResult $result              The result to save
+	 * @param boolean          $combineWithExisting If true, then this will be added to the existing result.
+	 */
+	public function setSessionValidationResult(ValidationResult $result, $combineWithExisting = false) {
+		if($combineWithExisting) {
+			$existingResult = $this->getSessionValidationResult();
+			$existingResult->combineAnd($result);
+
+		} else {
+			Session::set("FormInfo.{$this->FormName()}.result", $result);
+		}
 	}
 
     public function clearMessage()
     {
 		$this->message  = null;
-        Session::clear("FormInfo.{$this->FormName()}.errors");
+		Session::clear("FormInfo.{$this->FormName()}.result");
-        Session::clear("FormInfo.{$this->FormName()}.formError");
 		Session::clear("FormInfo.{$this->FormName()}.data");
 	}
 
-    public function resetValidation()
+	public function resetValidation() {
-    {
-        Session::clear("FormInfo.{$this->FormName()}.errors");
 		Session::clear("FormInfo.{$this->FormName()}.data");
+		Session::clear("FormInfo.{$this->FormName()}.result");
 	}
 
 	/**
@@ -1464,52 +1492,60 @@ class Form extends RequestHandler
 	/**
 	 * Processing that occurs before a form is executed.
 	 *
+	 * This includes form validation, if it fails, we throw a ValidationException
+	 *
 	 * This includes form validation, if it fails, we redirect back
 	 * to the form with appropriate error messages.
 	 * Always return true if the current form action is exempt from validation
 	 *
 	 * Triggered through {@link httpSubmission()}.
 	 *
+	 *
 	 * Note that CSRF protection takes place in {@link httpSubmission()},
 	 * if it fails the form data will never reach this method.
 	 *
 	 * @return boolean
 	 */
-    public function validate()
+	public function validate(){
-    {
+		$result = $this->validationResult();
-        $action = $this->buttonClicked();
+
-        if ($action && $this->actionIsValidationExempt($action)) {
+		// Valid
+		if($result->valid()) {
 			return true;
-        }
 
-        if ($this->validator) {
+		// Invalid
-            $errors = $this->validator->validate();
-
-            if ($errors) {
-                // Load errors into session and post back
-                $data = $this->getData();
-
-                // Encode validation messages as XML before saving into session state
-                // As per Form::addErrorMessage()
-                $errors = array_map(function ($error) {
-                    // Encode message as XML by default
-                    if ($error['message'] instanceof DBField) {
-                        $error['message'] = $error['message']->forTemplate();
-                        ;
 		} else {
-                        $error['message'] = Convert::raw2xml($error['message']);
+			$this->saveFormErrorsToSession($result, $this->getData());
-                    }
-                    return $error;
-                }, $errors);
-
-                Session::set("FormInfo.{$this->FormName()}.errors", $errors);
-                Session::set("FormInfo.{$this->FormName()}.data", $data);
-
 			return false;
 		}
 	}
 
-        return true;
+	/**
+	 * Experimental method - return a ValidationResult for the validator
+	 * @return [type] [description]
+	 */
+	private function validationResult() {
+		// Start with a "valid" validation result
+		$result = ValidationResult::create();
+
+		// Opportunity to invalidate via validator
+		$action = $this->buttonClicked();
+		if($action && $this->actionIsValidationExempt($action)) {
+			return $result;
+		}
+
+		if($this->validator){
+			$errors = $this->validator->validate();
+
+			// Convert the old-style Validator result into a ValidationResult
+			if($errors){
+				foreach($errors as $error) {
+					$result->addFieldError($error['fieldName'], $error['message'], $error['messageType']);
+					}
+			}
+		}
+
+		return $result;
 	}
 
 	const MERGE_DEFAULT = 0;
@@ -1563,7 +1599,7 @@ class Form extends RequestHandler
 	 */
     public function loadDataFrom($data, $mergeStrategy = 0, $fieldList = null)
     {
-        if (!is_object($data) && !is_array($data)) {
+		if(!is_object($data) && !is_array($data)) {
 			user_error("Form::loadDataFrom() not passed an array or an object", E_USER_WARNING);
 			return $this;
 		}
@@ -1587,7 +1623,7 @@ class Form extends RequestHandler
 			$name = $field->getName();
 
 			// Skip fields that have been excluded
-                if ($fieldList && !in_array($name, $fieldList)) {
+			if($fieldList && !in_array($name, $fieldList)) {
 				continue;
 			}
 
@@ -1601,7 +1637,7 @@ class Form extends RequestHandler
 			// The value from $data for this field
 			$val = null;
 
-                if (is_object($data)) {
+			if(is_object($data)) {
 				$exists = (
 					isset($data->$name) ||
 					$data->hasMethod($name) ||
@@ -1612,15 +1648,15 @@ class Form extends RequestHandler
 					$val = $data->__get($name);
 				}
                 } elseif (is_array($data)) {
-                    if (array_key_exists($name, $data)) {
+				if(array_key_exists($name, $data)) {
 					$exists = true;
 					$val = $data[$name];
                     } // If field is in array-notation we need to access nested data
-                    elseif (strpos($name, '[')) {
+				else if(strpos($name,'[')) {
 					// First encode data using PHP's method of converting nested arrays to form data
 					$flatData = urldecode(http_build_query($data));
 					// Then pull the value out from that flattened string
-                        preg_match('/' . addcslashes($name, '[]') . '=([^&]*)/', $flatData, $matches);
+					preg_match('/' . addcslashes($name,'[]') . '=([^&]*)/', $flatData, $matches);
 
 					if (isset($matches[1])) {
 						$exists = true;
@@ -1630,8 +1666,8 @@ class Form extends RequestHandler
 			}
 
 			// save to the field if either a value is given, or loading of blank/undefined values is forced
-                if ($exists) {
+			if($exists){
-                    if ($val != false || ($mergeStrategy & self::MERGE_IGNORE_FALSEISH) != self::MERGE_IGNORE_FALSEISH) {
+				if ($val != false || ($mergeStrategy & self::MERGE_IGNORE_FALSEISH) != self::MERGE_IGNORE_FALSEISH){
 					// pass original data as well so composite fields can act on the additional information
 					$field->setValue($val, $data);
 				}
@@ -1666,11 +1702,11 @@ class Form extends RequestHandler
 
 			$saveMethod = "save{$field->getName()}";
 
-                if ($field->getName() == "ClassName") {
+			if($field->getName() == "ClassName"){
 				$lastField = $field;
-                } elseif ($dataObject->hasMethod($saveMethod)) {
+			}else if( $dataObject->hasMethod( $saveMethod ) ){
 				$dataObject->$saveMethod( $field->dataValue());
-                } elseif ($field->getName() != "ID") {
+			} else if($field->getName() != "ID"){
 				$field->saveInto($dataObject);
 			}
 		}
@@ -1695,9 +1731,9 @@ class Form extends RequestHandler
 		$dataFields = $this->fields->dataFields();
 		$data = array();
 
-        if ($dataFields) {
+		if($dataFields){
-            foreach ($dataFields as $field) {
+			foreach($dataFields as $field) {
-                if ($field->getName()) {
+				if($field->getName()) {
 					$data[$field->getName()] = $field->dataValue();
 				}
 			}
@@ -1782,7 +1818,7 @@ class Form extends RequestHandler
 			"Actions" => "",
 		));
 
-        if (is_string($template)) {
+		if(is_string($template)) {
 			$template = new SSViewer($template);
 		}
 
@@ -1829,7 +1865,7 @@ class Form extends RequestHandler
 		$actions = $this->actions->dataFields() ?: array();
 
 		$fieldsAndActions = array_merge($fields, $actions);
-        $actions = array_filter($fieldsAndActions, function ($fieldOrAction) {
+		$actions = array_filter($fieldsAndActions, function($fieldOrAction) {
 			return $fieldOrAction instanceof FormAction;
 		});
 
@@ -1844,7 +1880,7 @@ class Form extends RequestHandler
 	 */
     public function defaultAction()
     {
-        if ($this->hasDefaultAction && $this->actions) {
+		if($this->hasDefaultAction && $this->actions) {
 			return $this->actions->first();
 		}
 		return null;
@@ -1934,7 +1970,7 @@ class Form extends RequestHandler
     {
 		//split at white space
 		$classes = preg_split('/\s+/', $class);
-        foreach ($classes as $class) {
+		foreach($classes as $class) {
 			//add classes one by one
 			$this->extraClasses[$class] = $class;
 		}
@@ -1962,12 +1998,12 @@ class Form extends RequestHandler
     public function debug()
     {
 		$result = "<h3>$this->class</h3><ul>";
-        foreach ($this->fields as $field) {
+		foreach($this->fields as $field) {
 			$result .= "<li>$field" . $field->debug() . "</li>";
 		}
 		$result .= "</ul>";
 
-        if ($this->validator) {
+		if( $this->validator ) {
 			/** @skipUpgrade */
 			$result .= '<h3>' . _t('Form.VALIDATOR', 'Validator') . '</h3>' . $this->validator->debug();
 		}

src/Forms/GridField/GridFieldDeleteAction.php

@@ -52,7 +52,7 @@ class GridFieldDeleteAction implements GridField_ColumnProvider, GridField_Actio
 	 */
     public function augmentColumns($gridField, &$columns)
     {
-        if (!in_array('Actions', $columns)) {
+		if(!in_array('Actions', $columns)) {
 			$columns[] = 'Actions';
 		}
 	}
@@ -79,7 +79,7 @@ class GridFieldDeleteAction implements GridField_ColumnProvider, GridField_Actio
 	 */
     public function getColumnMetadata($gridField, $columnName)
     {
-        if ($columnName == 'Actions') {
+		if($columnName == 'Actions') {
 			return array('title' => '');
 		}
 	}
@@ -115,8 +115,8 @@ class GridFieldDeleteAction implements GridField_ColumnProvider, GridField_Actio
 	 */
     public function getColumnContent($gridField, $record, $columnName)
     {
-        if ($this->removeRelation) {
+		if($this->removeRelation) {
-            if (!$record->canEdit()) {
+			if(!$record->canEdit()) {
 				return null;
 			}
 
@@ -130,7 +130,7 @@ class GridFieldDeleteAction implements GridField_ColumnProvider, GridField_Actio
 				->addExtraClass('btn btn--no-text btn--icon-md font-icon-link-broken grid-field__icon-action gridfield-button-unlink')
 				->setAttribute('title', _t('GridAction.UnlinkRelation', "Unlink"));
 		} else {
-            if (!$record->canDelete()) {
+			if(!$record->canDelete()) {
 				return null;
 			}
 
@@ -143,7 +143,7 @@ class GridFieldDeleteAction implements GridField_ColumnProvider, GridField_Actio
             )
 				->addExtraClass('gridfield-button-delete btn--icon-md font-icon-trash-bin btn--no-text grid-field__icon-action')
 				->setAttribute('title', _t('GridAction.Delete', "Delete"))
-                ->setDescription(_t('GridAction.DELETE_DESCRIPTION', 'Delete'));
+				->setDescription(_t('GridAction.DELETE_DESCRIPTION','Delete'));
 		}
 		return $field->Field();
 	}
@@ -159,28 +159,24 @@ class GridFieldDeleteAction implements GridField_ColumnProvider, GridField_Actio
 	 */
     public function handleAction(GridField $gridField, $actionName, $arguments, $data)
     {
-        if ($actionName == 'deleterecord' || $actionName == 'unlinkrelation') {
+		if($actionName == 'deleterecord' || $actionName == 'unlinkrelation') {
 			/** @var DataObject $item */
 			$item = $gridField->getList()->byID($arguments['RecordID']);
-            if (!$item) {
+			if(!$item) {
 				return;
 			}
 
-            if ($actionName == 'deleterecord') {
+			if($actionName == 'deleterecord') {
-                if (!$item->canDelete()) {
+				if(!$item->canDelete()) {
 					throw new ValidationException(
-                        _t('GridFieldAction_Delete.DeletePermissionsFailure', "No delete permissions"),
+						_t('GridFieldAction_Delete.DeletePermissionsFailure',"No delete permissions"));
-                        0
-                    );
 				}
 
 				$item->delete();
 			} else {
-                if (!$item->canEdit()) {
+				if(!$item->canEdit()) {
 					throw new ValidationException(
-                        _t('GridFieldAction_Delete.EditPermissionsFailure', "No permission to unlink record"),
+					_t('GridFieldAction_Delete.EditPermissionsFailure',"No permission to unlink record"));
-                        0
-                    );
 				}
 
 				$gridField->getList()->remove($item);

src/Forms/GridField/GridFieldDetailForm_ItemRequest.php

@@ -524,9 +524,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
 		try {
 			if (!$this->record->canDelete()) {
 				throw new ValidationException(
-                    _t('GridFieldDetailForm.DeletePermissionsFailure', "No delete permissions"),
+					_t('GridFieldDetailForm.DeletePermissionsFailure',"No delete permissions"));
-                    0
-                );
 			}
 
 			$this->record->delete();
@@ -583,7 +581,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
 	{
 		$templates = SSViewer::get_templates_by_class($this, '', __CLASS__);
 		// Prefer any custom template
-        if ($this->getTemplate()) {
+		if($this->getTemplate()) {
 			array_unshift($templates, $this->getTemplate());
 		}
 		return $templates;

src/ORM/Hierarchy/Hierarchy.php

@@ -109,16 +109,17 @@ class Hierarchy extends DataExtension
 
 		// Walk the hierarchy upwards until we reach the top, or until we reach the originating node again.
 		$node = $this->owner;
-        while ($node) {
+		while($node) {
 			if ($node->ParentID==$this->owner->ID) {
 				// Hierarchy is looping.
-                $validationResult->error(
+				$validationResult->addError(
 					_t(
 						'Hierarchy.InfiniteLoopNotAllowed',
 						'Infinite loop found within the "{type}" hierarchy. Please change the parent to resolve this',
 						'First argument is the class that makes up the hierarchy.',
 						array('type' => $this->owner->class)
 					),
+					'bad',
 					'INFINITE_LOOP'
 				);
 				break;
@@ -157,16 +158,16 @@ class Hierarchy extends DataExtension
 			$nodeCountThreshold = null,
 			$nodeCountCallback = null
 	) {
-        if (!is_numeric($nodeCountThreshold)) {
+		if(!is_numeric($nodeCountThreshold)) {
 			$nodeCountThreshold = Config::inst()->get(__CLASS__, 'node_threshold_total');
 		}
 
-        if ($limitToMarked && $rootCall) {
+		if($limitToMarked && $rootCall) {
 			$this->markingFinished($numChildrenMethod);
 		}
 
 
-        if ($nodeCountCallback) {
+		if($nodeCountCallback) {
 			$nodeCountWarning = $nodeCountCallback($this->owner, $this->owner->$numChildrenMethod());
             if ($nodeCountWarning) {
                 return $nodeCountWarning;
@@ -174,7 +175,7 @@ class Hierarchy extends DataExtension
 		}
 
 
-        if ($this->owner->hasMethod($childrenMethod)) {
+		if($this->owner->hasMethod($childrenMethod)) {
 			$children = $this->owner->$childrenMethod($extraArg);
 		} else {
 			$children = null;
@@ -186,17 +187,17 @@ class Hierarchy extends DataExtension
 		}
 
 		$output = null;
-        if ($children) {
+		if($children) {
-            if ($attributes) {
+			if($attributes) {
 				$attributes = " $attributes";
 			}
 
 			$output = "<ul$attributes>\n";
 
-            foreach ($children as $child) {
+			foreach($children as $child) {
-                if (!$limitToMarked || $child->isMarked()) {
+				if(!$limitToMarked || $child->isMarked()) {
 					$foundAChild = true;
-                    if (is_callable($titleEval)) {
+					if(is_callable($titleEval)) {
 						$output .= $titleEval($child, $numChildrenMethod);
 					} else {
 						$output .= eval("return $titleEval;");
@@ -213,7 +214,7 @@ class Hierarchy extends DataExtension
 					) {
 						// Additionally check if node count requirements are met
 						$nodeCountWarning = $nodeCountCallback ? $nodeCountCallback($child, $numChildren) : null;
-                        if ($nodeCountWarning) {
+						if($nodeCountWarning) {
 							$output .= $nodeCountWarning;
 							$child->markClosed();
 						} else {
@@ -228,7 +229,7 @@ class Hierarchy extends DataExtension
                                 $nodeCountThreshold
                             );
 						}
-                    } elseif ($child->isTreeOpened()) {
+					} elseif($child->isTreeOpened()) {
 						// Since we're not loading children, don't mark it as open either
 						$child->markClosed();
 					}
@@ -239,7 +240,7 @@ class Hierarchy extends DataExtension
 			$output .= "</ul>\n";
 		}
 
-        if (isset($foundAChild) && $foundAChild) {
+		if(isset($foundAChild) && $foundAChild) {
 			return $output;
 		}
 		return null;
@@ -274,9 +275,9 @@ class Hierarchy extends DataExtension
 		$this->owner->markUnexpanded();
 
 		// foreach can't handle an ever-growing $nodes list
-        while (list($id, $node) = each($this->markedNodes)) {
+		while(list($id, $node) = each($this->markedNodes)) {
 			$children = $this->markChildren($node, $context, $childrenMethod, $numChildrenMethod);
-            if ($nodeCountThreshold && sizeof($this->markedNodes) > $nodeCountThreshold) {
+			if($nodeCountThreshold && sizeof($this->markedNodes) > $nodeCountThreshold) {
 				// Undo marking children as opened since they're lazy loaded
                 if ($children) {
                     foreach ($children as $child) {
@@ -324,16 +325,16 @@ class Hierarchy extends DataExtension
 	 */
     public function markingFilterMatches($node)
     {
-        if (!$this->markingFilter) {
+		if(!$this->markingFilter) {
 			return true;
 		}
 
-        if (isset($this->markingFilter['parameter']) && $parameterName = $this->markingFilter['parameter']) {
+		if(isset($this->markingFilter['parameter']) && $parameterName = $this->markingFilter['parameter']) {
-            if (is_array($this->markingFilter['value'])) {
+			if(is_array($this->markingFilter['value'])){
 				$ret = false;
-                foreach ($this->markingFilter['value'] as $value) {
+				foreach($this->markingFilter['value'] as $value) {
 					$ret = $ret||$node->$parameterName==$value;
-                    if ($ret == true) {
+					if($ret == true) {
 						break;
 					}
 				}
@@ -341,7 +342,7 @@ class Hierarchy extends DataExtension
 			} else {
 				return ($node->$parameterName == $this->markingFilter['value']);
 			}
-        } elseif ($func = $this->markingFilter['func']) {
+		} else if ($func = $this->markingFilter['func']) {
 			return call_user_func($func, $node);
 		}
 	}
@@ -361,7 +362,7 @@ class Hierarchy extends DataExtension
 		$childrenMethod = "AllChildrenIncludingDeleted",
 		$numChildrenMethod = "numChildren"
 	) {
-        if ($node->hasMethod($childrenMethod)) {
+		if($node->hasMethod($childrenMethod)) {
 			$children = $node->$childrenMethod($context);
 		} else {
 			$children = null;
@@ -373,12 +374,12 @@ class Hierarchy extends DataExtension
 		}
 
 		$node->markExpanded();
-        if ($children) {
+		if($children) {
-            foreach ($children as $child) {
+			foreach($children as $child) {
 				$markingMatches = $this->markingFilterMatches($child);
-                if ($markingMatches) {
+				if($markingMatches) {
 					// Mark a child node as unexpanded if it has children and has not already been expanded
-                    if ($child->$numChildrenMethod() && !$child->isExpanded()) {
+					if($child->$numChildrenMethod() && !$child->isExpanded()) {
 						$child->markUnexpanded();
 					} else {
 						$child->markExpanded();
@@ -400,9 +401,9 @@ class Hierarchy extends DataExtension
     protected function markingFinished($numChildrenMethod = "numChildren")
     {
 		// Mark childless nodes as expanded.
-        if ($this->markedNodes) {
+		if($this->markedNodes) {
-            foreach ($this->markedNodes as $id => $node) {
+			foreach($this->markedNodes as $id => $node) {
-                if (!$node->isExpanded() && !$node->$numChildrenMethod()) {
+				if(!$node->isExpanded() && !$node->$numChildrenMethod()) {
 					$node->markExpanded();
 				}
 			}
@@ -419,14 +420,14 @@ class Hierarchy extends DataExtension
     public function markingClasses($numChildrenMethod = "numChildren")
     {
 		$classes = '';
-        if (!$this->isExpanded()) {
+		if(!$this->isExpanded()) {
 			$classes .= " unexpanded";
 		}
 
 		// Set jstree open state, or mark it as a leaf (closed) if there are no children
-        if (!$this->owner->$numChildrenMethod()) {
+		if(!$this->owner->$numChildrenMethod()) {
 			$classes .= " jstree-leaf closed";
-        } elseif ($this->isTreeOpened()) {
+		} elseif($this->isTreeOpened()) {
 			$classes .= " jstree-open";
 		} else {
 			$classes .= " jstree-closed closed";
@@ -443,9 +444,9 @@ class Hierarchy extends DataExtension
 	 */
     public function markById($id, $open = false)
     {
-        if (isset($this->markedNodes[$id])) {
+		if(isset($this->markedNodes[$id])) {
 			$this->markChildren($this->markedNodes[$id]);
-            if ($open) {
+			if($open) {
 				$this->markedNodes[$id]->markOpened();
 			}
 			return true;
@@ -461,9 +462,9 @@ class Hierarchy extends DataExtension
 	 */
     public function markToExpose($childObj)
     {
-        if (is_object($childObj)) {
+		if(is_object($childObj)){
 			$stack = array_reverse($childObj->parentStack());
-            foreach ($stack as $stackItem) {
+			foreach($stack as $stackItem) {
 				$this->markById($stackItem->ID, true);
 			}
 		}
@@ -488,7 +489,7 @@ class Hierarchy extends DataExtension
     {
 		$p = $this->owner;
 
-        while ($p) {
+		while($p) {
 			$stack[] = $p;
 			$p = $p->ParentID ? $p->Parent() : null;
 		}
@@ -546,7 +547,7 @@ class Hierarchy extends DataExtension
 	 */
     public function markClosed()
     {
-        if (isset(self::$treeOpened[$this->owner->baseClass()][$this->owner->ID])) {
+		if(isset(self::$treeOpened[$this->owner->baseClass()][$this->owner->ID])) {
 			unset(self::$treeOpened[$this->owner->baseClass()][$this->owner->ID]);
 		}
 	}
@@ -606,9 +607,9 @@ class Hierarchy extends DataExtension
 	 */
     public function loadDescendantIDListInto(&$idList)
     {
-        if ($children = $this->AllChildren()) {
+		if($children = $this->AllChildren()) {
-            foreach ($children as $child) {
+			foreach($children as $child) {
-                if (in_array($child->ID, $idList)) {
+				if(in_array($child->ID, $idList)) {
 					continue;
 				}
 				$idList[] = $child->ID;
@@ -628,7 +629,7 @@ class Hierarchy extends DataExtension
 	 */
     public function Children()
     {
-        if (!(isset($this->_cache_children) && $this->_cache_children)) {
+		if(!(isset($this->_cache_children) && $this->_cache_children)) {
 			$result = $this->owner->stageChildren(false);
 			$children = array();
 			foreach ($result as $record) {
@@ -679,14 +680,14 @@ class Hierarchy extends DataExtension
         }
 
 		$baseClass = $this->owner->baseClass();
-        if ($baseClass) {
+		if($baseClass) {
 			$stageChildren = $this->owner->stageChildren(true);
 
 			// Add live site content that doesn't exist on the stage site, if required.
-            if ($this->owner->hasExtension('SilverStripe\ORM\Versioning\Versioned')) {
+			if($this->owner->hasExtension('SilverStripe\ORM\Versioning\Versioned')) {
 				// Next, go through the live children.  Only some of these will be listed
 				$liveChildren = $this->owner->liveChildren(true, true);
-                if ($liveChildren) {
+				if($liveChildren) {
 					$merged = new ArrayList();
 					$merged->merge($stageChildren);
 					$merged->merge($liveChildren);
@@ -713,7 +714,7 @@ class Hierarchy extends DataExtension
 	 */
     public function AllHistoricalChildren()
     {
-        if (!$this->owner->hasExtension('SilverStripe\ORM\Versioning\Versioned')) {
+		if(!$this->owner->hasExtension('SilverStripe\ORM\Versioning\Versioned')) {
 			throw new Exception('Hierarchy->AllHistoricalChildren() only works with Versioned extension applied');
 		}
 
@@ -734,7 +735,7 @@ class Hierarchy extends DataExtension
 	 */
     public function numHistoricalChildren()
     {
-        if (!$this->owner->hasExtension('SilverStripe\ORM\Versioning\Versioned')) {
+		if(!$this->owner->hasExtension('SilverStripe\ORM\Versioning\Versioned')) {
 			throw new Exception('Hierarchy->AllHistoricalChildren() only works with Versioned extension applied');
 		}
 
@@ -751,7 +752,7 @@ class Hierarchy extends DataExtension
     public function numChildren($cache = true)
     {
 		// Build the cache for this class if it doesn't exist.
-        if (!$cache || !is_numeric($this->_cache_numChildren)) {
+		if(!$cache || !is_numeric($this->_cache_numChildren)) {
 			// Hey, this is efficient now!
 			// We call stageChildren(), because Children() has canView() filtering
 			$this->_cache_numChildren = (int)$this->owner->stageChildren(true)->Count();
@@ -815,7 +816,7 @@ class Hierarchy extends DataExtension
 	 */
     public function liveChildren($showAll = false, $onlyDeletedFromStage = false)
     {
-        if (!$this->owner->hasExtension(Versioned::class)) {
+		if(!$this->owner->hasExtension(Versioned::class)) {
 			throw new Exception('Hierarchy->liveChildren() only works with Versioned extension applied');
 		}
 
@@ -835,7 +836,7 @@ class Hierarchy extends DataExtension
 		if ($hide_from_cms_tree && $this->showingCMSTree()) {
 			$children = $children->exclude('ClassName', $hide_from_cms_tree);
 		}
-        if (!$showAll && DataObject::getSchema()->fieldSpec($this->owner, 'ShowInMenus')) {
+		if(!$showAll && DataObject::getSchema()->fieldSpec($this->owner, 'ShowInMenus')) {
 			$children = $children->filter('ShowInMenus', 1);
 		}
 
@@ -852,7 +853,7 @@ class Hierarchy extends DataExtension
     public function getParent($filter = null)
     {
 		$parentID = $this->owner->ParentID;
-        if (empty($parentID)) {
+		if(empty($parentID)) {
 			return null;
 		}
 		$idSQL = $this->owner->getSchema()->sqlColumnForField($this->owner, 'ID');
@@ -872,7 +873,7 @@ class Hierarchy extends DataExtension
 		$ancestors = new ArrayList();
 		$object    = $this->owner;
 
-        while ($object = $object->getParent()) {
+		while($object = $object->getParent()) {
 			$ancestors->push($object);
 		}
 
@@ -922,8 +923,8 @@ class Hierarchy extends DataExtension
     public function naturalNext($className = null, $root = 0, $afterNode = null)
     {
 		// If this node is not the node we are searching from, then we can possibly return this node as a solution
-        if ($afterNode && $afterNode->ID != $this->owner->ID) {
+		if($afterNode && $afterNode->ID != $this->owner->ID) {
-            if (!$className || ($className && $this->owner->class == $className)) {
+			if(!$className || ($className && $this->owner->class == $className)) {
 				return $this->owner;
 			}
 		}
@@ -943,22 +944,22 @@ class Hierarchy extends DataExtension
 		"\"ParentID\"={$this->owner->ParentID}" . ( $afterNode ) ? "\"Sort\"
 		> {$afterNode->Sort}" : "" , '\"Sort\" ASC' ) ) $searchNodes->merge( $siblings );*/
 
-        if ($children) {
+		if($children) {
-            foreach ($children as $node) {
+			foreach($children as $node) {
-                if ($nextNode = $node->naturalNext($className, $node->ID, $this->owner)) {
+				if($nextNode = $node->naturalNext($className, $node->ID, $this->owner)) {
 					break;
 				}
 			}
 
-            if ($nextNode) {
+			if($nextNode) {
 				return $nextNode;
 			}
 		}
 
 		// if this is not an instance of the root class or has the root id, search the parent
-        if (!(is_numeric($root) && $root == $this->owner->ID || $root == $this->owner->class)
+		if(!(is_numeric($root) && $root == $this->owner->ID || $root == $this->owner->class)
 				&& ($parent = $this->owner->Parent())) {
-            return $parent->naturalNext($className, $root, $this->owner);
+			return $parent->naturalNext( $className, $root, $this->owner );
 		}
 
 		return null;

src/ORM/ValidationException.php

@@ -30,25 +30,45 @@ class ValidationException extends Exception
 	 * the error code number.
 	 * @param integer $code The error code number, if not given in the second parameter
 	 */
-    public function __construct($result = null, $message = null, $code = 0)
+	public function __construct($result = null, $code = 0, $dummy = null) {
-    {
+		$exceptionMessage = null;
 
-        // Check arguments
+		// Backwards compatibiliy failover.  The 2nd argument used to be $message, and $code the 3rd.
-        if (!($result instanceof ValidationResult)) {
+		// For callers using that, we ditch the message
-            // Shift parameters if no ValidationResult is given
+		if(!is_numeric($code)) {
-            $code = $message;
+			$exceptionMessage = $code;
-            $message = $result;
+			if($dummy) $code = $dummy;
+		}
 
-            // Infer ValidationResult from parameters
+		if($result instanceof ValidationResult) {
-            $result = new ValidationResult(false, $message);
+			$this->result = $result;
-        } elseif (empty($message)) {
+
-            // Infer message if not given
+		} else if(is_string($result)) {
-            $message = $result->message();
+			$this->result = ValidationResult::create()->addError($result);
+
+		} else if(!$result) {
+			$this->result = ValidationResult::create()->addError(_t("ValdiationExcetpion.DEFAULT_ERROR", "Validation error"));
+
+		} else {
+			throw new InvalidArgumentException(
+				"ValidationExceptions must be passed a ValdiationResult, a string, or nothing at all");
 		}
 
 		// Construct
-        $this->result = $result;
+		parent::__construct($exceptionMessage ? $exceptionMessage : $this->result->message(), $code);
-        parent::__construct($message, $code);
+	}
+
+	/**
+	 * Create a ValidationException with a message for a single field-specific error message.
+	 *
+	 * @param  string $field   The field name
+	 * @param  string $message The error message
+	 * @return ValidationException
+	 */
+	static function create_for_field($field, $message) {
+		$result = new ValidationResult;
+		$result->addFieldError($field, $message);
+		return new ValidationException($result);
 	}
 
 	/**

src/ORM/ValidationResult.php

@@ -13,7 +13,7 @@ class ValidationResult extends Object
 	/**
 	 * @var bool - is the result valid or not
 	 */
-    protected $isValid;
+	protected $isValid = true;
 
 
 	/**
@@ -24,11 +24,17 @@ class ValidationResult extends Object
 	/**
 	 * Create a new ValidationResult.
 	 * By default, it is a successful result.	Call $this->error() to record errors.
-     * @param bool $valid
+	 *
-     * @param string|null $message
+	 * @param void $valid @deprecated
+	 * @param void $message @deprecated
 	 */
-    public function __construct($valid = true, $message = null)
+	public function __construct($valid = null, $message = null) {
-    {
+		if ($message !== null) {
+			Deprecation::notice('3.2', '$message parameter is deprecated please use addMessage or addError instead', false);
+			$this->addError($message);
+		}
+		if ($valid !== null) {
+			Deprecation::notice('3.2', '$valid parameter is deprecated please addError to mark the result as invalid', false);
 		$this->isValid = $valid;
         if ($message) {
             $this->errorList[] = $message;
@@ -37,25 +43,117 @@ class ValidationResult extends Object
 	}
 
 	/**
-     * Record an error against this validation result,
+	 * Return the full error meta-data, suitable for combining with another ValidationResult.
-     * @param string $message The validation error message
-     * @param string $code An optional error code string, that can be accessed with {@link $this->codeList()}.
-     * @return $this
 	 */
-    public function error($message, $code = null)
+	function getErrorMetaData() {
-    {
+		return $this->errorList;
+	}
+
+	/**
+	 * Record a
+	 * against this validation result.
+	 *
+	 * It's better to use addError, addFeildError, addMessage, or addFieldMessage instead.
+	 *
+	 * @param string $message     The message string.
+	 * @param string $code        A codename for this error. Only one message per codename will be added.
+	 *                            This can be usedful for ensuring no duplicate messages
+	 * @param string $fieldName   The field to link the message to.  If omitted; a form-wide message is assumed.
+	 * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS
+	 *                            class to the form, so other values can be used if desired.
+	 * @param bool $escapeHtml    Automatically sanitize the message. Set to FALSE if the message contains HTML.
+	 *                            In that case, you might want to use {@link Convert::raw2xml()} to escape any
+	 *                            user supplied data in the message.
+	 *
+	 * @deprecated 3.2
+	 */
+	public function error($message, $code = null, $fieldName = null, $messageType = "bad", $escapeHtml = true) {
+		Deprecation::notice('3.2', 'Use addError or addFieldError instead.');
+
+		return $this->addFieldError($fieldName, $message, $messageType, $code, $escapeHtml);
+	}
+
+	/**
+	 * Record an error against this validation result,
+	 *
+	 * @param string $message     The message string.
+	 * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS
+	 *                            class to the form, so other values can be used if desired.
+	 * @param string $code        A codename for this error. Only one message per codename will be added.
+	 *                            This can be usedful for ensuring no duplicate messages
+	 * @param bool $escapeHtml    Automatically sanitize the message. Set to FALSE if the message contains HTML.
+	 *                            In that case, you might want to use {@link Convert::raw2xml()} to escape any
+	 *                            user supplied data in the message.
+	 */
+	public function addError($message, $messageType = "bad", $code = null, $escapeHtml = true) {
+
+		return $this->addFieldError(null, $message, $messageType, $code, $escapeHtml);
+	}
+
+	/**
+	 * Record an error against this validation result,
+	 *
+	 * @param string $fieldName   The field to link the message to.  If omitted; a form-wide message is assumed.
+	 * @param string $message     The message string.
+	 * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS
+	 *                            class to the form, so other values can be used if desired.
+	 * @param string $code        A codename for this error. Only one message per codename will be added.
+	 *                            This can be usedful for ensuring no duplicate messages
+	 * @param bool $escapeHtml    Automatically sanitize the message. Set to FALSE if the message contains HTML.
+	 *                            In that case, you might want to use {@link Convert::raw2xml()} to escape any
+	 *                            user supplied data in the message.
+	 */
+	public function addFieldError($fieldName = null, $message, $messageType = "bad", $code = null, $escapeHtml = true) {
 		$this->isValid = false;
 
-        if ($code) {
+		return $this->addFieldMessage($fieldName, $message, $messageType, $code, $escapeHtml);
-            if (!is_numeric($code)) {
+	}
-                $this->errorList[$code] = $message;
+
+	/**
+	 * Add a message to this ValidationResult without necessarily marking it as an error
+	 *
+	 * @param string $message     The message string.
+	 * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS
+	 *                            class to the form, so other values can be used if desired.
+	 * @param string $code        A codename for this error. Only one message per codename will be added.
+	 *                            This can be usedful for ensuring no duplicate messages
+	 * @param bool $escapeHtml    Automatically sanitize the message. Set to FALSE if the message contains HTML.
+	 *                            In that case, you might want to use {@link Convert::raw2xml()} to escape any
+	 *                            user supplied data in the message.
+	 */
+	public function addMessage($message, $messageType = "bad", $code = null, $escapeHtml = true) {
+		return $this->addFieldMessage(null, $message, $messageType, $code, $escapeHtml);
+	}
+
+	/**
+	 * Add a message to this ValidationResult without necessarily marking it as an error
+	 *
+	 * @param string $fieldName   The field to link the message to.  If omitted; a form-wide message is assumed.
+	 * @param string $message     The message string.
+	 * @param string $messageType The type of message: e.g. "bad", "warning", "good", or "required". Passed as a CSS
+	 *                            class to the form, so other values can be used if desired.
+	 * @param string $code        A codename for this error. Only one message per codename will be added.
+	 *                            This can be usedful for ensuring no duplicate messages
+	 * @param bool $escapeHtml    Automatically sanitize the message. Set to FALSE if the message contains HTML.
+	 *                            In that case, you might want to use {@link Convert::raw2xml()} to escape any
+	 *                            user supplied data in the message.
+	 */
+	public function addFieldMessage($fieldName, $message, $messageType = "bad", $code = null, $escapeHtml = true) {
+		$metadata = array(
+			'message' => $escapeHtml ? Convert::raw2xml($message) : $message,
+			'fieldName' => $fieldName,
+			'messageType' => $messageType,
+		);
+
+		if($code) {
+			if(!is_numeric($code)) {
+				$this->errorList[$code] = $metadata;
 			} else {
-                user_error("ValidationResult::error() - Don't use a numeric code '$code'.  Use a string."
+				throw new InvalidArgumentException(
-                    . "I'm going to ignore it.", E_USER_WARNING);
+					"ValidationResult::error() - Don't use a numeric code '$code'.  Use a string.");
-                $this->errorList[$code] = $message;
 			}
 		} else {
-            $this->errorList[] = $message;
+			$this->errorList[] = $metadata;
 		}
 
 		return $this;
@@ -76,7 +174,29 @@ class ValidationResult extends Object
 	 */
     public function messageList()
     {
-        return $this->errorList;
+		$list = array();
+		foreach($this->errorList as $key => $item) {
+			if(is_numeric($key)) $list[] = $item['message'];
+			else $list[$key] = $item['message'];
+		}
+		return $list;
+	}
+
+	/**
+	 * Get the field-specific messages as a map.
+	 * Keys will be field names, and values will be a 2 element map with keys 'messsage', and 'messageType'
+	 */
+	public function fieldErrors() {
+		$output = array();
+		foreach($this->errorList as $key => $item) {
+			if($item['fieldName']) {
+				$output[$item['fieldName']] = array(
+					'message' => $item['message'],
+					'messageType' => $item['messageType']
+				);
+			}
+		}
+		return $output;
 	}
 
 	/**
@@ -100,7 +220,18 @@ class ValidationResult extends Object
 	 */
     public function message()
     {
-        return implode("; ", $this->errorList);
+		return implode("; ", $this->messageList());
+	}
+
+	/**
+	 * The the error message that's not related to a field as a string
+	 */
+	public function overallMessage() {
+		$messages = array();
+		foreach($this->errorList as $item) {
+			if(!$item['fieldName']) $messages[] = $item['message'];
+		}
+		return implode("; ", $messages);
 	}
 
 	/**
@@ -109,7 +240,7 @@ class ValidationResult extends Object
 	 */
     public function starredList()
     {
-        return " * " . implode("\n * ", $this->errorList);
+		return " * " . implode("\n * ", $this->messageList());
 	}
 
 	/**
@@ -123,8 +254,7 @@ class ValidationResult extends Object
     public function combineAnd(ValidationResult $other)
     {
 		$this->isValid = $this->isValid && $other->valid();
-        $this->errorList = array_merge($this->errorList, $other->messageList());
+		$this->errorList = array_merge($this->errorList, $other->getErrorMetaData());
-
 		return $this;
 	}
 }

src/Security/Group.php

@@ -96,7 +96,7 @@ class Group extends DataObject
 		$doSet = new ArrayList();
 
 		$children = Group::get()->filter("ParentID", $this->ID);
-        foreach ($children as $child) {
+		foreach($children as $child) {
 			$doSet->push($child);
 			$doSet->merge($child->getAllChildren());
 		}
@@ -148,7 +148,7 @@ class Group extends DataObject
 		// TODO SecurityAdmin coupling, not easy to get to the form fields through GridFieldDetailForm
 		$permissionsField->setHiddenPermissions((array)Config::inst()->get('SilverStripe\\Admin\\SecurityAdmin', 'hidden_permissions'));
 
-        if ($this->ID) {
+		if($this->ID) {
 			$group = $this;
 			$config = GridFieldConfig_RelationEditor::create();
 			$config->addComponent(new GridFieldButtonRow('after'));
@@ -164,16 +164,16 @@ class Group extends DataObject
 			$detailForm = $config->getComponentByType('SilverStripe\\Forms\\GridField\\GridFieldDetailForm');
 			$detailForm
 				->setValidator(Member_Validator::create())
-                ->setItemEditFormCallback(function ($form, $component) use ($group) {
+				->setItemEditFormCallback(function($form, $component) use($group) {
 					/** @var Form $form */
 					$record = $form->getRecord();
 					$groupsField = $form->Fields()->dataFieldByName('DirectGroups');
-                    if ($groupsField) {
+					if($groupsField) {
 						// If new records are created in a group context,
 						// set this group by default.
-                        if ($record && !$record->ID) {
+						if($record && !$record->ID) {
 							$groupsField->setValue($group->ID);
-                        } elseif ($record && $record->ID) {
+						} elseif($record && $record->ID) {
 							// TODO Mark disabled once chosen.js supports it
 							// $groupsField->setDisabledItems(array($group->ID));
                             $form->Fields()->replaceField(
@@ -183,7 +183,7 @@ class Group extends DataObject
 						}
 					}
 				});
-            $memberList = GridField::create('Members', false, $this->DirectMembers(), $config)
+			$memberList = GridField::create('Members',false, $this->DirectMembers(), $config)
 				->addExtraClass('members_grid');
 			// @todo Implement permission checking on GridField
 			//$memberList->setPermissions(array('edit', 'delete', 'export', 'add', 'inlineadd'));
@@ -193,7 +193,7 @@ class Group extends DataObject
 		// Only add a dropdown for HTML editor configurations if more than one is available.
 		// Otherwise Member->getHtmlEditorConfigForCMS() will default to the 'cms' configuration.
 		$editorConfigMap = HTMLEditorConfig::get_available_configs_map();
-        if (count($editorConfigMap) > 1) {
+		if(count($editorConfigMap) > 1) {
             $fields->addFieldToTab(
                 'Root.Permissions',
 				new DropdownField(
@@ -205,13 +205,13 @@ class Group extends DataObject
 			);
 		}
 
-        if (!Permission::check('EDIT_PERMISSIONS')) {
+		if(!Permission::check('EDIT_PERMISSIONS')) {
 			$fields->removeFieldFromTab('Root', 'Permissions');
 		}
 
 		// Only show the "Roles" tab if permissions are granted to edit them,
 		// and at least one role exists
-        if (Permission::check('APPLY_ROLES') && DataObject::get('SilverStripe\\Security\\PermissionRole')) {
+		if(Permission::check('APPLY_ROLES') && DataObject::get('SilverStripe\\Security\\PermissionRole')) {
 			$fields->findOrMakeTab('Root.Roles', _t('SecurityAdmin.ROLES', 'Roles'));
             $fields->addFieldToTab(
                 'Root.Roles',
@@ -236,14 +236,14 @@ class Group extends DataObject
 
 			// Add roles (and disable all checkboxes for inherited roles)
 			$allRoles = PermissionRole::get();
-            if (!Permission::check('ADMIN')) {
+			if(!Permission::check('ADMIN')) {
 				$allRoles = $allRoles->filter("OnlyAdminCanApply", 0);
 			}
-            if ($this->ID) {
+			if($this->ID) {
 				$groupRoles = $this->Roles();
 				$inheritedRoles = new ArrayList();
 				$ancestors = $this->getAncestors();
-                foreach ($ancestors as $ancestor) {
+				foreach($ancestors as $ancestor) {
 					$ancestorRoles = $ancestor->Roles();
                     if ($ancestorRoles) {
                         $inheritedRoles->merge($ancestorRoles);
@@ -260,7 +260,7 @@ class Group extends DataObject
 					->setDefaultItems($groupRoleIDs)
 					->setAttribute('data-placeholder', _t('Group.AddRole', 'Add a role for this group'))
 					->setDisabledItems($inheritedRoleIDs);
-            if (!$allRoles->count()) {
+			if(!$allRoles->count()) {
 				$rolesField->setAttribute('data-placeholder', _t('Group.NoRoles', 'No roles found'));
 			}
 			$fields->addFieldToTab('Root.Roles', $rolesField);
@@ -285,7 +285,7 @@ class Group extends DataObject
 		$labels['Code'] = _t('Group.Code', 'Group Code', 'Programmatical code identifying a group');
 		$labels['Locked'] = _t('Group.Locked', 'Locked?', 'Group is locked in the security administration area');
 		$labels['Sort'] = _t('Group.Sort', 'Sort Order');
-        if ($includerelations) {
+		if($includerelations){
 			$labels['Parent'] = _t('Group.Parent', 'Parent Group', 'One group has one parent group');
 			$labels['Permissions'] = _t('Group.has_many_Permissions', 'Permissions', 'One group has many permissions');
 			$labels['Members'] = _t('Group.many_many_Members', 'Members', 'One group has many members');
@@ -315,8 +315,8 @@ class Group extends DataObject
 		// Remove the default foreign key filter in prep for re-applying a filter containing all children groups.
 		// Filters are conjunctive in DataQuery by default, so this filter would otherwise overrule any less specific
 		// ones.
-        if (!($result instanceof UnsavedRelationList)) {
+		if(!($result instanceof UnsavedRelationList)) {
-            $result = $result->alterDataQuery(function ($query) {
+			$result = $result->alterDataQuery(function($query){
 				/** @var DataQuery $query */
 				$query->removeFilterOn('Group_Members');
 			});
@@ -351,8 +351,8 @@ class Group extends DataObject
 		$familyIDs = array();
 		$chunkToAdd = array($this->ID);
 
-        while ($chunkToAdd) {
+		while($chunkToAdd) {
-            $familyIDs = array_merge($familyIDs, $chunkToAdd);
+			$familyIDs = array_merge($familyIDs,$chunkToAdd);
 
 			// Get the children of *all* the groups identified in the previous chunk.
 			// This minimises the number of SQL queries necessary
@@ -371,7 +371,7 @@ class Group extends DataObject
     {
 		$parent = $this;
 		$items = [];
-        while (isset($parent) && $parent instanceof Group) {
+		while(isset($parent) && $parent instanceof Group) {
 			$items[] = $parent->ID;
 			$parent = $parent->Parent;
 		}
@@ -399,7 +399,7 @@ class Group extends DataObject
 
     public function getTreeTitle()
     {
-        if ($this->hasMethod('alternateTreeTitle')) {
+		if($this->hasMethod('alternateTreeTitle')) {
 			return $this->alternateTreeTitle();
 		}
 		return htmlspecialchars($this->Title, ENT_QUOTES);
@@ -422,13 +422,13 @@ class Group extends DataObject
 		// Check if the new group hierarchy would add certain "privileged permissions",
 		// and require an admin to perform this change in case it does.
 		// This prevents "sub-admin" users with group editing permissions to increase their privileges.
-        if ($this->Parent()->exists() && !Permission::check('ADMIN')) {
+		if($this->Parent()->exists() && !Permission::check('ADMIN')) {
 			$inheritedCodes = Permission::get()
 				->filter('GroupID', $this->Parent()->collateAncestorIDs())
 				->column('Code');
 			$privilegedCodes = Config::inst()->get('SilverStripe\\Security\\Permission', 'privileged_permissions');
-            if (array_intersect($inheritedCodes, $privilegedCodes)) {
+			if(array_intersect($inheritedCodes, $privilegedCodes)) {
-                $result->error(sprintf(
+				$result->addError(sprintf(
 					_t(
 						'Group.HierarchyPermsError',
 						'Can\'t assign parent group "%s" with privileged permissions (requires ADMIN access)'
@@ -448,7 +448,7 @@ class Group extends DataObject
 		// Only set code property when the group has a custom title, and no code exists.
 		// The "Code" attribute is usually treated as a more permanent identifier than database IDs
 		// in custom application logic, so can't be changed after its first set.
-        if (!$this->Code && $this->Title != _t('SecurityAdmin.NEWGROUP', "New Group")) {
+		if(!$this->Code && $this->Title != _t('SecurityAdmin.NEWGROUP',"New Group")) {
 			$this->setCode($this->Title);
 		}
 	}
@@ -458,12 +458,12 @@ class Group extends DataObject
 		parent::onBeforeDelete();
 
 		// if deleting this group, delete it's children as well
-        foreach ($this->Groups() as $group) {
+		foreach($this->Groups() as $group) {
 			$group->delete();
 		}
 
 		// Delete associated permissions
-        foreach ($this->Permissions() as $permission) {
+		foreach($this->Permissions() as $permission) {
 			$permission->delete();
 		}
 	}
@@ -587,7 +587,7 @@ class Group extends DataObject
 
 		// Add default author group if no other group exists
 		$allGroups = DataObject::get('SilverStripe\\Security\\Group');
-        if (!$allGroups->count()) {
+		if(!$allGroups->count()) {
 			$authorGroup = new Group();
 			$authorGroup->Code = 'content-authors';
 			$authorGroup->Title = _t('Group.DefaultGroupTitleContentAuthors', 'Content Authors');
@@ -601,7 +601,7 @@ class Group extends DataObject
 
 		// Add default admin group if none with permission code ADMIN exists
 		$adminGroups = Permission::get_groups_by_permission('ADMIN');
-        if (!$adminGroups->count()) {
+		if(!$adminGroups->count()) {
 			$adminGroup = new Group();
 			$adminGroup->Code = 'administrators';
 			$adminGroup->Title = _t('Group.DefaultGroupTitleAdministrators', 'Administrators');

src/Security/Member.php

@@ -285,7 +285,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		$admin = Member::get()
 			->filter('Email', Security::default_admin_username())
 			->first();
-        if (!$admin) {
+		if(!$admin) {
 			// 'Password' is not set to avoid creating
 			// persistent logins in the database. See Security::setDefaultAdmin().
 			// Set 'Email' to identify this as the default admin
@@ -296,7 +296,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		}
 
 		// Ensure this user is in the admin group
-        if (!$admin->inGroup($adminGroup)) {
+		if(!$admin->inGroup($adminGroup)) {
 			// Add member to group instead of adding group to member
 			// This bypasses the privilege escallation code in Member_GroupSet
 			$adminGroup
@@ -323,19 +323,19 @@ class Member extends DataObject implements TemplateGlobalProvider
 		}
 
 		// Allow default admin to login as self
-        if ($this->isDefaultAdmin() && Security::check_default_admin($this->Email, $password)) {
+		if($this->isDefaultAdmin() && Security::check_default_admin($this->Email, $password)) {
 			return $result;
 		}
 
 		// Check a password is set on this member
-        if (empty($this->Password) && $this->exists()) {
+		if(empty($this->Password) && $this->exists()) {
-            $result->error(_t('Member.NoPassword', 'There is no password on this member.'));
+			$result->addError(_t('Member.NoPassword','There is no password on this member.'));
 			return $result;
 		}
 
 		$e = PasswordEncryptor::create_for_algorithm($this->PasswordEncryption);
-        if (!$e->check($this->Password, $password, $this->Salt, $this)) {
+		if(!$e->check($this->Password, $password, $this->Salt, $this)) {
-            $result->error(_t(
+			$result->addError(_t (
 				'Member.ERRORWRONGCRED',
 				'The provided details don\'t seem to be correct. Please try again.'
 			));
@@ -367,8 +367,8 @@ class Member extends DataObject implements TemplateGlobalProvider
     {
 		$result = ValidationResult::create();
 
-        if ($this->isLockedOut()) {
+		if($this->isLockedOut()) {
-            $result->error(
+			$result->addError(
 				_t(
 					'Member.ERRORLOCKEDOUT2',
 					'Your account has been temporarily disabled because of too many failed attempts at ' .
@@ -469,7 +469,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		if ($alcDevice = Cookie::get('alc_device')) {
 			RememberLoginHash::get()->filter('DeviceID', $alcDevice)->removeAll();
 		}
-            if ($remember) {
+		if($remember) {
 			$rememberLoginHash = RememberLoginHash::generate($this);
 				$tokenExpiryDays = Config::inst()->get(
 					'SilverStripe\\Security\\RememberLoginHash',
@@ -534,8 +534,8 @@ class Member extends DataObject implements TemplateGlobalProvider
 	 */
     public static function logged_in_session_exists()
     {
-        if ($id = Member::currentUserID()) {
+		if($id = Member::currentUserID()) {
-            if ($member = DataObject::get_by_id('SilverStripe\\Security\\Member', $id)) {
+			if($member = DataObject::get_by_id('SilverStripe\\Security\\Member', $id)) {
                 if ($member->exists()) {
                     return true;
 			}
@@ -558,7 +558,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 			self::$_already_tried_to_auto_log_in = true;
 		}
 
-        if (!Security::config()->autologin_enabled
+		if(!Security::config()->autologin_enabled
 			|| strpos(Cookie::get('alc_enc'), ':') === false
 			|| Session::get("loggedInAs")
 			|| !Security::database_is_ready()
@@ -566,7 +566,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 			return;
 		}
 
-        if (strpos(Cookie::get('alc_enc'), ':') && Cookie::get('alc_device') && !Session::get("loggedInAs")) {
+		if(strpos(Cookie::get('alc_enc'), ':') && Cookie::get('alc_device') && !Session::get("loggedInAs")) {
 			list($uid, $token) = explode(':', Cookie::get('alc_enc'), 2);
 
 			if (!$uid || !$token) {
@@ -582,7 +582,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 			$rememberLoginHash = null;
 
 			// check if autologin token matches
-            if ($member) {
+			if($member) {
 				$hash = $member->encryptWithUserSettings($token);
 				$rememberLoginHash = RememberLoginHash::get()
 					->filter(array(
@@ -590,7 +590,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 						'DeviceID' => $deviceID,
 						'Hash' => $hash
 					))->first();
-                if (!$rememberLoginHash) {
+				if(!$rememberLoginHash) {
 					$member = null;
 				} else {
 					// Check for expired token
@@ -603,11 +603,11 @@ class Member extends DataObject implements TemplateGlobalProvider
 				}
 			}
 
-            if ($member) {
+			if($member) {
 				self::session_regenerate_id();
 				Session::set("loggedInAs", $member->ID);
 				// This lets apache rules detect whether the user has logged in
-                if (Member::config()->login_marker_cookie) {
+				if(Member::config()->login_marker_cookie) {
 					Cookie::set(Member::config()->login_marker_cookie, 1, 0, null, null, false, true);
 				}
 
@@ -707,7 +707,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 			$generator = new RandomGenerator();
 			$token = $generator->randomToken();
 			$hash = $this->encryptWithUserSettings($token);
-        } while (DataObject::get_one('SilverStripe\\Security\\Member', array(
+		} while(DataObject::get_one('SilverStripe\\Security\\Member', array(
 			'"Member"."AutoLoginHash"' => $hash
 		)));
 
@@ -771,7 +771,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 			->filter('TempIDHash', $tempid);
 
 		// Exclude expired
-        if (static::config()->temp_id_lifetime) {
+		if(static::config()->temp_id_lifetime) {
 			$members = $members->filter('TempIDExpired:GreaterThan', DBDatetime::now()->getValue());
 		}
 
@@ -791,7 +791,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 
 		$fields->replaceField('Password', $this->getMemberPasswordField());
 
-        $fields->replaceField('Locale', new DropdownField(
+		$fields->replaceField('Locale', new DropdownField (
 			'Locale',
 			$this->fieldLabel('Locale'),
 			i18n::get_existing_translations()
@@ -826,7 +826,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		);
 
 		// If editing own password, require confirmation of existing
-        if ($editingPassword && $this->ID == Member::currentUserID()) {
+		if($editingPassword && $this->ID == Member::currentUserID()) {
 			$password->setRequireExistingPassword(true);
 		}
 
@@ -865,7 +865,7 @@ class Member extends DataObject implements TemplateGlobalProvider
     {
 		$id = Member::currentUserID();
 
-        if ($id) {
+		if($id) {
 			return DataObject::get_by_id('SilverStripe\\Security\\Member', $id);
 		}
 	}
@@ -878,7 +878,7 @@ class Member extends DataObject implements TemplateGlobalProvider
     public static function currentUserID()
     {
 		$id = Session::get("loggedInAs");
-        if (!$id && !self::$_already_tried_to_auto_log_in) {
+		if(!$id && !self::$_already_tried_to_auto_log_in) {
 			self::autoLogin();
 			$id = Session::get("loggedInAs");
 		}
@@ -898,14 +898,14 @@ class Member extends DataObject implements TemplateGlobalProvider
     {
 		$words = Config::inst()->get('SilverStripe\\Security\\Security', 'word_list');
 
-        if ($words && file_exists($words)) {
+		if($words && file_exists($words)) {
 			$words = file($words);
 
 			list($usec, $sec) = explode(' ', microtime());
 			srand($sec + ((float) $usec * 100000));
 
-            $word = trim($words[rand(0, sizeof($words)-1)]);
+			$word = trim($words[rand(0,sizeof($words)-1)]);
-            $number = rand(10, 999);
+			$number = rand(10,999);
 
 			return $word . $number;
 		} else {
@@ -929,16 +929,16 @@ class Member extends DataObject implements TemplateGlobalProvider
 		// Note: This does not a full replacement for safeguards in the controller layer (e.g. in a registration form),
 		// but rather a last line of defense against data inconsistencies.
 		$identifierField = Member::config()->unique_identifier_field;
-        if ($this->$identifierField) {
+		if($this->$identifierField) {
 			// Note: Same logic as Member_Validator class
 			$filter = array("\"$identifierField\"" => $this->$identifierField);
-            if ($this->ID) {
+			if($this->ID) {
 				$filter[] = array('"Member"."ID" <> ?' => $this->ID);
 			}
 			$existingRecord = DataObject::get_one('SilverStripe\\Security\\Member', $filter);
 
-            if ($existingRecord) {
+			if($existingRecord) {
-                throw new ValidationException(ValidationResult::create(false, _t(
+				throw new ValidationException(ValidationResult::create()->adderror(_t(
 					'Member.ValidationIdentifierFailed',
 					'Can\'t overwrite existing member #{id} with identical identifier ({name} = {value}))',
 					'Values in brackets show "fieldname = value", usually denoting an existing email address',
@@ -970,7 +970,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		// The test on $this->ID is used for when records are initially created.
 		// Note that this only works with cleartext passwords, as we can't rehash
 		// existing passwords.
-        if ((!$this->ID && $this->Password) || $this->isChanged('Password')) {
+		if((!$this->ID && $this->Password) || $this->isChanged('Password')) {
 			//reset salt so that it gets regenerated - this will invalidate any persistant login cookies
 			// or other information encrypted with this Member's settings (see self::encryptWithUserSettings)
 			$this->Salt = '';
@@ -989,9 +989,9 @@ class Member extends DataObject implements TemplateGlobalProvider
 			$this->PasswordEncryption = $encryption_details['algorithm'];
 
 			// If we haven't manually set a password expiry
-            if (!$this->isChanged('PasswordExpiry')) {
+			if(!$this->isChanged('PasswordExpiry')) {
 				// then set it for us
-                if (self::config()->password_expiry_days) {
+				if(self::config()->password_expiry_days) {
 					$this->PasswordExpiry = date('Y-m-d', time() + 86400 * self::config()->password_expiry_days);
 				} else {
 					$this->PasswordExpiry = null;
@@ -1000,7 +1000,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		}
 
 		// save locale
-        if (!$this->Locale) {
+		if(!$this->Locale) {
 			$this->Locale = i18n::get_locale();
 		}
 
@@ -1013,7 +1013,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 
 		Permission::flush_permission_cache();
 
-        if ($this->isChanged('Password')) {
+		if($this->isChanged('Password')) {
 			MemberPassword::log($this);
 		}
 	}
@@ -1050,7 +1050,7 @@ class Member extends DataObject implements TemplateGlobalProvider
     public function onChangeGroups($ids)
     {
 		// unless the current user is an admin already OR the logged in user is an admin
-        if (Permission::check('ADMIN') || Permission::checkMember($this, 'ADMIN')) {
+		if(Permission::check('ADMIN') || Permission::checkMember($this, 'ADMIN')) {
 			return true;
 		}
 
@@ -1091,13 +1091,13 @@ class Member extends DataObject implements TemplateGlobalProvider
 	 */
     public function inGroup($group, $strict = false)
     {
-        if (is_numeric($group)) {
+		if(is_numeric($group)) {
 			$groupCheckObj = DataObject::get_by_id('SilverStripe\\Security\\Group', $group);
-        } elseif (is_string($group)) {
+		} elseif(is_string($group)) {
 			$groupCheckObj = DataObject::get_one('SilverStripe\\Security\\Group', array(
 				'"Group"."Code"' => $group
 			));
-        } elseif ($group instanceof Group) {
+		} elseif($group instanceof Group) {
 			$groupCheckObj = $group;
 		} else {
 			user_error('Member::inGroup(): Wrong format for $group parameter', E_USER_ERROR);
@@ -1132,7 +1132,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 			'"Group"."Code"' => $groupcode
 		));
 
-        if ($group) {
+		if($group) {
 			$this->Groups()->add($group);
 		} else {
             if (!$title) {
@@ -1157,7 +1157,7 @@ class Member extends DataObject implements TemplateGlobalProvider
     {
 		$group = Group::get()->filter(array('Code' => $groupcode))->first();
 
-        if ($group) {
+		if($group) {
 			$this->Groups()->remove($group);
 		}
 	}
@@ -1191,7 +1191,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		$format = $this->config()->title_format;
 		if ($format) {
 			$values = array();
-            foreach ($format['columns'] as $col) {
+			foreach($format['columns'] as $col) {
 				$values[] = $this->getField($col);
 			}
 			return join($format['sep'], $values);
@@ -1199,13 +1199,13 @@ class Member extends DataObject implements TemplateGlobalProvider
         if ($this->getField('ID') === 0) {
 			return $this->getField('Surname');
         } else {
-            if ($this->getField('Surname') && $this->getField('FirstName')) {
+			if($this->getField('Surname') && $this->getField('FirstName')){
 				return $this->getField('Surname') . ', ' . $this->getField('FirstName');
-            } elseif ($this->getField('Surname')) {
+			}elseif($this->getField('Surname')){
 				return $this->getField('Surname');
-            } elseif ($this->getField('FirstName')) {
+			}elseif($this->getField('FirstName')){
 				return $this->getField('FirstName');
-            } else {
+			}else{
 				return null;
 			}
 		}
@@ -1232,7 +1232,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		}
 
 		$columnsWithTablename = array();
-        foreach ($format['columns'] as $column) {
+		foreach($format['columns'] as $column) {
 			$columnsWithTablename[] = static::getSchema()->sqlColumnForField(__CLASS__, $column);
 		}
 
@@ -1288,7 +1288,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 	 */
     public function getDateFormat()
     {
-        if ($this->getField('DateFormat')) {
+		if($this->getField('DateFormat')) {
 			return $this->getField('DateFormat');
 		} else {
 			return i18n::config()->get('date_format');
@@ -1304,7 +1304,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 	 */
     public function getTimeFormat()
     {
-        if ($this->getField('TimeFormat')) {
+		if($this->getField('TimeFormat')) {
 			return $this->getField('TimeFormat');
 		} else {
 			return i18n::config()->get('time_format');
@@ -1352,24 +1352,24 @@ class Member extends DataObject implements TemplateGlobalProvider
     {
 		$groupIDList = array();
 
-        if ($groups instanceof SS_List) {
+		if($groups instanceof SS_List) {
-            foreach ($groups as $group) {
+			foreach( $groups as $group ) {
 				$groupIDList[] = $group->ID;
 			}
-        } elseif (is_array($groups)) {
+		} elseif(is_array($groups)) {
 			$groupIDList = $groups;
-        } elseif ($groups) {
+		} elseif($groups) {
 			$groupIDList[] = $groups;
 		}
 
 		// No groups, return all Members
-        if (!$groupIDList) {
+		if(!$groupIDList) {
 			return Member::get()->sort(array('Surname'=>'ASC', 'FirstName'=>'ASC'))->map();
 		}
 
 		$membersList = new ArrayList();
 		// This is a bit ineffective, but follow the ORM style
-        foreach (Group::get()->byIDs($groupIDList) as $group) {
+		foreach(Group::get()->byIDs($groupIDList) as $group) {
 			$membersList->merge($group->Members());
 		}
 
@@ -1390,7 +1390,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 	 */
     public static function mapInCMSGroups($groups = null)
     {
-        if (!$groups || $groups->Count() == 0) {
+		if(!$groups || $groups->Count() == 0) {
 			$perms = array('ADMIN', 'CMS_ACCESS_AssetAdmin');
 
 			if (class_exists('SilverStripe\\CMS\\Controllers\\CMSMain')) {
@@ -1399,7 +1399,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 				$cmsPerms = LeftAndMain::singleton()->providePermissions();
 			}
 
-            if (!empty($cmsPerms)) {
+			if(!empty($cmsPerms)) {
 				$perms = array_unique(array_merge($perms, array_keys($cmsPerms)));
 			}
 
@@ -1414,11 +1414,11 @@ class Member extends DataObject implements TemplateGlobalProvider
 
 		$groupIDList = array();
 
-        if ($groups instanceof SS_List) {
+		if($groups instanceof SS_List) {
-            foreach ($groups as $group) {
+			foreach($groups as $group) {
 				$groupIDList[] = $group->ID;
 			}
-        } elseif (is_array($groups)) {
+		} elseif(is_array($groups)) {
 			$groupIDList = $groups;
 		}
 
@@ -1426,7 +1426,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		$members = Member::get()
 			->innerJoin("Group_Members", '"Group_Members"."MemberID" = "Member"."ID"')
 			->innerJoin("Group", '"Group"."ID" = "Group_Members"."GroupID"');
-        if ($groupIDList) {
+		if($groupIDList) {
 			$groupClause = DB::placeholders($groupIDList);
 			$members = $members->where(array(
 				"\"Group\".\"ID\" IN ($groupClause)" => $groupIDList
@@ -1454,8 +1454,8 @@ class Member extends DataObject implements TemplateGlobalProvider
             $memberGroups = $this->Groups();
         }
 
-        foreach ($memberGroups as $group) {
+		foreach($memberGroups as $group) {
-            if (in_array($group->Code, $groupList)) {
+			if(in_array($group->Code, $groupList)) {
 				$index = array_search($group->Code, $groupList);
 				unset($groupList[$index]);
 			}
@@ -1477,7 +1477,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		require_once 'Zend/Date.php';
 
 		$self = $this;
-        $this->beforeUpdateCMSFields(function (FieldList $fields) use ($self) {
+		$this->beforeUpdateCMSFields(function(FieldList $fields) use ($self) {
 			/** @var FieldList $mainFields */
 			$mainFields = $fields->fieldByName("Root")->fieldByName("Main")->getChildren();
 
@@ -1491,7 +1491,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 			));
 			$mainFields->removeByName($self->config()->hidden_fields);
 
-            if (! $self->config()->lock_out_after_incorrect_logins) {
+			if( ! $self->config()->lock_out_after_incorrect_logins) {
 				$mainFields->removeByName('FailedLoginCount');
 			}
 
@@ -1505,9 +1505,9 @@ class Member extends DataObject implements TemplateGlobalProvider
 
 			$fields->removeByName('RememberLoginHashes');
 
-            if (Permission::check('EDIT_PERMISSIONS')) {
+			if(Permission::check('EDIT_PERMISSIONS')) {
 				$groupsMap = array();
-                foreach (Group::get() as $group) {
+				foreach(Group::get() as $group) {
 					// Listboxfield values are escaped, use ASCII char instead of &raquo;
 					$groupsMap[$group->ID] = $group->getBreadcrumbs(' > ');
 				}
@@ -1526,7 +1526,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 				// Add permission field (readonly to avoid complicated group assignment logic).
 				// This should only be available for existing records, as new records start
 				// with no permissions until they have a group assignment anyway.
-                if ($self->ID) {
+				if($self->ID) {
 					$permissionsField = new PermissionCheckboxSetField_Readonly(
 						'Permissions',
 						false,
@@ -1581,7 +1581,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 				)
 			);
 			$timeFormatField->setValue($self->TimeFormat);
-            $timeTemplate = SSViewer::get_templates_by_class($formatClass, '_description_time', $formatClass);
+			$timeTemplate = SSViewer::get_templates_by_class($formatClass,'_description_time', $formatClass);
 			$timeFormatField->setDescriptionTemplate($timeTemplate);
 		});
 
@@ -1606,7 +1606,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		$labels['Locale'] = _t('Member.db_Locale', 'Interface Locale');
 		$labels['DateFormat'] = _t('Member.DATEFORMAT', 'Date format');
 		$labels['TimeFormat'] = _t('Member.TIMEFORMAT', 'Time format');
-        if ($includerelations) {
+		if($includerelations){
             $labels['Groups'] = _t(
                 'Member.belongs_many_many_Groups',
                 'Groups',
@@ -1627,21 +1627,21 @@ class Member extends DataObject implements TemplateGlobalProvider
     public function canView($member = null)
     {
         //get member
-        if (!($member instanceof Member)) {
+        if(!($member instanceof Member)) {
             $member = Member::currentUser();
         }
         //check for extensions, we do this first as they can overrule everything
         $extended = $this->extendedCan(__FUNCTION__, $member);
-        if ($extended !== null) {
+        if($extended !== null) {
             return $extended;
         }
 
         //need to be logged in and/or most checks below rely on $member being a Member
-        if (!$member) {
+        if(!$member) {
             return false;
         }
         // members can usually view their own record
-        if ($this->ID == $member->ID) {
+        if($this->ID == $member->ID) {
             return true;
         }
         //standard check
@@ -1658,26 +1658,26 @@ class Member extends DataObject implements TemplateGlobalProvider
     public function canEdit($member = null)
     {
         //get member
-        if (!($member instanceof Member)) {
+        if(!($member instanceof Member)) {
             $member = Member::currentUser();
         }
         //check for extensions, we do this first as they can overrule everything
         $extended = $this->extendedCan(__FUNCTION__, $member);
-        if ($extended !== null) {
+        if($extended !== null) {
             return $extended;
         }
 
         //need to be logged in and/or most checks below rely on $member being a Member
-        if (!$member) {
+        if(!$member) {
             return false;
         }
 
         // HACK: we should not allow for an non-Admin to edit an Admin
-        if (!Permission::checkMember($member, 'ADMIN') && Permission::checkMember($this, 'ADMIN')) {
+        if(!Permission::checkMember($member, 'ADMIN') && Permission::checkMember($this, 'ADMIN')) {
             return false;
         }
         // members can usually edit their own record
-        if ($this->ID == $member->ID) {
+        if($this->ID == $member->ID) {
             return true;
         }
         //standard check
@@ -1692,30 +1692,30 @@ class Member extends DataObject implements TemplateGlobalProvider
      */
     public function canDelete($member = null)
     {
-        if (!($member instanceof Member)) {
+        if(!($member instanceof Member)) {
             $member = Member::currentUser();
         }
         //check for extensions, we do this first as they can overrule everything
         $extended = $this->extendedCan(__FUNCTION__, $member);
-        if ($extended !== null) {
+        if($extended !== null) {
             return $extended;
         }
 
         //need to be logged in and/or most checks below rely on $member being a Member
-        if (!$member) {
+        if(!$member) {
             return false;
         }
         // Members are not allowed to remove themselves,
         // since it would create inconsistencies in the admin UIs.
-        if ($this->ID && $member->ID == $this->ID) {
+        if($this->ID && $member->ID == $this->ID) {
             return false;
         }
 
         // HACK: if you want to delete a member, you have to be a member yourself.
         // this is a hack because what this should do is to stop a user
         // deleting a member who has more privileges (e.g. a non-Admin deleting an Admin)
-        if (Permission::checkMember($this, 'ADMIN')) {
+        if(Permission::checkMember($this, 'ADMIN')) {
-            if (! Permission::checkMember($member, 'ADMIN')) {
+            if( ! Permission::checkMember($member, 'ADMIN')) {
                 return false;
             }
         }
@@ -1730,14 +1730,14 @@ class Member extends DataObject implements TemplateGlobalProvider
     {
 		$valid = parent::validate();
 
-        if (!$this->ID || $this->isChanged('Password')) {
+		if(!$this->ID || $this->isChanged('Password')) {
-            if ($this->Password && self::$password_validator) {
+			if($this->Password && self::$password_validator) {
 				$valid->combineAnd(self::$password_validator->validate($this->Password, $this));
 			}
 		}
 
-        if ((!$this->ID && $this->SetPassword) || $this->isChanged('SetPassword')) {
+		if((!$this->ID && $this->SetPassword) || $this->isChanged('SetPassword')) {
-            if ($this->SetPassword && self::$password_validator) {
+			if($this->SetPassword && self::$password_validator) {
 				$valid->combineAnd(self::$password_validator->validate($this->SetPassword, $this));
 			}
 		}
@@ -1757,7 +1757,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 		$this->Password = $password;
 		$valid = $this->validate();
 
-        if ($valid->valid()) {
+		if($valid->valid()) {
 			$this->AutoLoginHash = null;
 			$this->write();
 		}
@@ -1771,11 +1771,11 @@ class Member extends DataObject implements TemplateGlobalProvider
 	 */
     public function registerFailedLogin()
     {
-        if (self::config()->lock_out_after_incorrect_logins) {
+		if(self::config()->lock_out_after_incorrect_logins) {
 			// Keep a tally of the number of failed log-ins so that we can lock people out
 			$this->FailedLoginCount = $this->FailedLoginCount + 1;
 
-            if ($this->FailedLoginCount >= self::config()->lock_out_after_incorrect_logins) {
+			if($this->FailedLoginCount >= self::config()->lock_out_after_incorrect_logins) {
 				$lockoutMins = self::config()->lock_out_delay_mins;
 				$this->LockedOutUntil = date('Y-m-d H:i:s', DBDatetime::now()->Format('U') + $lockoutMins*60);
 				$this->FailedLoginCount = 0;
@@ -1790,7 +1790,7 @@ class Member extends DataObject implements TemplateGlobalProvider
 	 */
     public function registerSuccessfulLogin()
     {
-        if (self::config()->lock_out_after_incorrect_logins) {
+		if(self::config()->lock_out_after_incorrect_logins) {
 			// Forgive all past login failures
 			$this->FailedLoginCount = 0;
 			$this->write();
@@ -1808,11 +1808,11 @@ class Member extends DataObject implements TemplateGlobalProvider
 		$currentName = '';
 		$currentPriority = 0;
 
-        foreach ($this->Groups() as $group) {
+		foreach($this->Groups() as $group) {
 			$configName = $group->HtmlEditorConfig;
-            if ($configName) {
+			if($configName) {
 				$config = HTMLEditorConfig::get($group->HtmlEditorConfig);
-                if ($config && $config->getOption('priority') > $currentPriority) {
+				if($config && $config->getOption('priority') > $currentPriority) {
 					$currentName = $configName;
 					$currentPriority = $config->getOption('priority');
 				}

src/Security/MemberAuthenticator.php

@@ -44,7 +44,7 @@ class MemberAuthenticator extends Authenticator
 		// Attempt to identify by temporary ID
 		$member = null;
 		$email = null;
-        if (!empty($data['tempid'])) {
+		if(!empty($data['tempid'])) {
 			// Find user by tempid, in case they are re-validating an existing session
 			$member = Member::member_from_tempid($data['tempid']);
             if ($member) {
@@ -54,24 +54,24 @@ class MemberAuthenticator extends Authenticator
 
 		// Otherwise, get email from posted value instead
 		/** @skipUpgrade */
-        if (!$member && !empty($data['Email'])) {
+		if(!$member && !empty($data['Email'])) {
 			$email = $data['Email'];
 		}
 
 		// Check default login (see Security::setDefaultAdmin())
 		$asDefaultAdmin = $email === Security::default_admin_username();
-        if ($asDefaultAdmin) {
+		if($asDefaultAdmin) {
 			// If logging is as default admin, ensure record is setup correctly
 			$member = Member::default_admin();
 			$success = !$member->isLockedOut() && Security::check_default_admin($email, $data['Password']);
 			//protect against failed login
-            if ($success) {
+			if($success) {
 				return $member;
 			}
 		}
 
 		// Attempt to identify user by email
-        if (!$member && $email) {
+		if(!$member && $email) {
 			// Find user by email
 			$member = Member::get()
 				->filter(Member::config()->unique_identifier_field, $email)
@@ -79,21 +79,17 @@ class MemberAuthenticator extends Authenticator
 		}
 
 		// Validate against member if possible
-        if ($member && !$asDefaultAdmin) {
+		if($member && !$asDefaultAdmin) {
 			$result = $member->checkPassword($data['Password']);
 			$success = $result->valid();
 		} else {
-            $result = new ValidationResult(false, _t('Member.ERRORWRONGCRED'));
+			$result = ValidationResult::create()->addError(_t('Member.ERRORWRONGCRED'));
 		}
 
 		// Emit failure to member and form (if available)
-        if (!$success) {
+		if(!$success) {
-            if ($member) {
+			if($member) $member->registerFailedLogin();
-                $member->registerFailedLogin();
+			if($form) $form->setSessionValidationResult($result, true);
-            }
-            if ($form) {
-                $form->sessionMessage($result->message(), 'bad');
-            }
 		} else {
             if ($member) {
                 $member->registerSuccessfulLogin();
@@ -120,12 +116,12 @@ class MemberAuthenticator extends Authenticator
 		// Check email is valid
 		/** @skipUpgrade */
 		$email = isset($data['Email']) ? $data['Email'] : null;
-        if (is_array($email)) {
+		if(is_array($email)) {
 			throw new InvalidArgumentException("Bad email passed to MemberAuthenticator::authenticate(): $email");
 		}
 
 		$attempt = new LoginAttempt();
-        if ($success) {
+		if($success) {
 			// successful login (member is existing with matching password)
 			$attempt->MemberID = $member->ID;
 			$attempt->Status = 'Success';
@@ -135,7 +131,7 @@ class MemberAuthenticator extends Authenticator
 		} else {
 			// Failed login - we're trying to see if a user exists with this email (disregarding wrong passwords)
 			$attempt->Status = 'Failure';
-            if ($member) {
+			if($member) {
 				// Audit logging hook
 				$attempt->MemberID = $member->ID;
 				$member->extend('authenticationFailed');
@@ -174,7 +170,7 @@ class MemberAuthenticator extends Authenticator
 		// when we can rehash passwords to a different hashing algorithm,
 		// bulk-migration doesn't work due to the nature of hashing.
 		// See PasswordEncryptor_LegacyPHPHash class.
-        if ($success && $member && isset(self::$migrate_legacy_hashes[$member->PasswordEncryption])) {
+		if($success && $member && isset(self::$migrate_legacy_hashes[$member->PasswordEncryption])) {
 			$member->Password = $data['Password'];
 			$member->PasswordEncryption = self::$migrate_legacy_hashes[$member->PasswordEncryption];
 			$member->write();

src/Security/PasswordValidator.php

@@ -78,9 +78,9 @@ class PasswordValidator extends Object
     {
 		$valid = ValidationResult::create();
 
-        if ($this->minLength) {
+		if($this->minLength) {
-            if (strlen($password) < $this->minLength) {
+			if(strlen($password) < $this->minLength) {
-                $valid->error(
+				$valid->addError(
 					sprintf(
 						_t(
 							'PasswordValidator.TOOSHORT',
@@ -88,16 +88,17 @@ class PasswordValidator extends Object
 						),
 						$this->minLength
 					),
+					'bad',
 					'TOO_SHORT'
 				);
 			}
 		}
 
-        if ($this->minScore) {
+		if($this->minScore) {
 			$score = 0;
 			$missedTests = array();
-            foreach ($this->testNames as $name) {
+			foreach($this->testNames as $name) {
-                if (preg_match(self::config()->character_strength_tests[$name], $password)) {
+				if(preg_match(self::config()->character_strength_tests[$name], $password)) {
 					$score++;
 				} else {
 					$missedTests[] = _t(
@@ -108,8 +109,8 @@ class PasswordValidator extends Object
 				}
 			}
 
-            if ($score < $this->minScore) {
+			if($score < $this->minScore) {
-                $valid->error(
+				$valid->addError(
 					sprintf(
 						_t(
 							'PasswordValidator.LOWCHARSTRENGTH',
@@ -117,24 +118,26 @@ class PasswordValidator extends Object
 						),
 						implode(', ', $missedTests)
 					),
+					'bad',
 					'LOW_CHARACTER_STRENGTH'
 				);
 			}
 		}
 
-        if ($this->historicalPasswordCount) {
+		if($this->historicalPasswordCount) {
 			$previousPasswords = MemberPassword::get()
 				->where(array('"MemberPassword"."MemberID"' => $member->ID))
 				->sort('"Created" DESC, "ID" DESC')
 				->limit($this->historicalPasswordCount);
 			/** @var MemberPassword $previousPassword */
-            foreach ($previousPasswords as $previousPassword) {
+			foreach($previousPasswords as $previousPassword) {
-                if ($previousPassword->checkPassword($password)) {
+				if($previousPassword->checkPassword($password)) {
-                    $valid->error(
+					$valid->addError(
 						_t(
 							'PasswordValidator.PREVPASSWORD',
 							'You\'ve already used that password in the past, please choose a new password'
 						),
+						'bad',
 						'PREVIOUS_PASSWORD'
 					);
 					break;

src/Security/PermissionRoleCode.php

@@ -33,7 +33,7 @@ class PermissionRoleCode extends DataObject
 			&& in_array($this->Code, $privilegedCodes)
 			&& !Permission::check('ADMIN')
 		) {
-            $result->error(sprintf(
+			$result->addError(sprintf(
 				_t(
 					'PermissionRoleCode.PermsError',
 					'Can\'t assign code "%s" with privileged permissions (requires ADMIN access)'

tests/php/Forms/FormTest.php

@@ -436,6 +436,34 @@ class FormTest extends FunctionalTest {
 		);
 	}
 
+	public function testValidationException() {
+		$this->get('FormTest_Controller');
+
+		$response = $this->post(
+			'FormTest_Controller/Form',
+			array(
+				'Email' => 'test@test.com',
+				'SomeRequiredField' => 'test',
+				'action_triggerException' => 1,
+			)
+		);
+		$this->assertPartialMatchBySelector(
+			'#Form_Form_Email_Holder span.message',
+			array(
+				'Error on Email field'
+			),
+			'Formfield validation shows note on field if invalid'
+		);
+		$this->assertPartialMatchBySelector(
+			'#Form_Form_error',
+			array(
+				'Error at top of form'
+			),
+			'Required fields show a notification on field when left blank'
+		);
+
+	}
+
 	public function testGloballyDisabledSecurityTokenInheritsToNewForm() {
 		SecurityToken::enable();
 
@@ -758,6 +786,7 @@ class FormTest extends FunctionalTest {
 		$form = $this->getStubForm();
 		$form->getController()->handleRequest(new HTTPRequest('GET', '/'), DataModel::inst()); // stub out request
 		$form->sessionMessage('<em>Escaped HTML</em>', 'good', true);
+		$form->setupFormErrors();
 		$parser = new CSSContentParser($form->forTemplate());
 		$messageEls = $parser->getBySelector('.message');
 		$this->assertContains(
@@ -768,6 +797,7 @@ class FormTest extends FunctionalTest {
 		$form = $this->getStubForm();
 		$form->getController()->handleRequest(new HTTPRequest('GET', '/'), DataModel::inst()); // stub out request
 		$form->sessionMessage('<em>Unescaped HTML</em>', 'good', false);
+		$form->setupFormErrors();
 		$parser = new CSSContentParser($form->forTemplate());
 		$messageEls = $parser->getBySelector('.message');
 		$this->assertContains(
@@ -779,7 +809,7 @@ class FormTest extends FunctionalTest {
 	function testFieldMessageEscapeHtml() {
 		$form = $this->getStubForm();
 		$form->getController()->handleRequest(new HTTPRequest('GET', '/'), DataModel::inst()); // stub out request
-		$form->addErrorMessage('key1', '<em>Escaped HTML</em>', 'good', true);
+		$form->getSessionValidationResult()->addFieldMessage('key1', '<em>Escaped HTML</em>', 'good');
 		$form->setupFormErrors();
 		$parser = new CSSContentParser($result = $form->forTemplate());
 		$messageEls = $parser->getBySelector('#Form_Form_key1_Holder .message');
@@ -790,7 +820,7 @@ class FormTest extends FunctionalTest {
 
 		$form = $this->getStubForm();
 		$form->getController()->handleRequest(new HTTPRequest('GET', '/'), DataModel::inst()); // stub out request
-		$form->addErrorMessage('key1', '<em>Unescaped HTML</em>', 'good', false);
+		$form->getSessionValidationResult()->addFieldMessage('key1', '<em>Unescaped HTML</em>', 'good', null, false);
 		$form->setupFormErrors();
 		$parser = new CSSContentParser($form->forTemplate());
 		$messageEls = $parser->getBySelector('#Form_Form_key1_Holder .message');

tests/php/ORM/DataObjectTest.php

@@ -1762,6 +1762,6 @@ class DataObjectTest extends SapphireTest {
 		$staff->Salary = PHP_INT_MAX;
 		$staff->write();
 		$this->assertEquals(PHP_INT_MAX, DataObjectTest\Staff::get()->byID($staff->ID)->Salary);
-	}
+}
 
 }

tests/php/ORM/HierarchyTest.php

@@ -33,6 +33,16 @@ class HierarchyTest extends SapphireTest {
 		$obj2->ParentID = $obj2aa->ID;
 			$obj2->write();
 		}
+		catch (ValidationException $e) {
+			$this->assertContains(
+				Convert::raw2xml('Infinite loop found within the "HierarchyTest_Object" hierarchy'),
+				$e->getMessage()
+			);
+			return;
+		}
+
+		$this->fail('Failed to prevent infinite loop in hierarchy.');
+	}
 
 	/**
 	 * Test Hierarchy::AllHistoricalChildren().

tests/php/ORM/ValidationExceptionTest.php

@@ -13,12 +13,14 @@ class ValidationExceptionTest extends SapphireTest
 	 */
 	public function testCreateFromValidationResult() {
 
-		$result = new ValidationResult(false, 'Not a valid result');
+		$result = new ValidationResult();
+		$result->addError('Not a valid result');
+
 		$exception = new ValidationException($result);
 
 		$this->assertEquals(0, $exception->getCode());
 		$this->assertEquals('Not a valid result', $exception->getMessage());
-		$this->assertEquals(false, $exception->getResult()->valid());
+		$this->assertFalse($exception->getResult()->valid());
 		$this->assertEquals('Not a valid result', $exception->getResult()->message());
 
 	}
@@ -29,8 +31,8 @@ class ValidationExceptionTest extends SapphireTest
 	 */
 	public function testCreateFromComplexValidationResult() {
 		$result = new ValidationResult();
-		$result->error('Invalid type')
+		$result->addError('Invalid type')
-				->error('Out of kiwis');
+				->addError('Out of kiwis');
 		$exception = new ValidationException($result);
 
 		$this->assertEquals(0, $exception->getCode());
@@ -48,7 +50,7 @@ class ValidationExceptionTest extends SapphireTest
 
 		$this->assertEquals(E_USER_ERROR, $exception->getCode());
 		$this->assertEquals('Error inferred from message', $exception->getMessage());
-		$this->assertEquals(false, $exception->getResult()->valid());
+		$this->assertFalse($exception->getResult()->valid());
 		$this->assertEquals('Error inferred from message', $exception->getResult()->message());
 	}
 
@@ -57,12 +59,13 @@ class ValidationExceptionTest extends SapphireTest
 	 * and a custom message
 	 */
 	public function testCreateWithValidationResultAndMessage() {
-		$result = new ValidationResult(false, 'Incorrect placement of cutlery');
+		$result = new ValidationResult();
+		$result->addError('Incorrect placement of cutlery');
 		$exception = new ValidationException($result, 'An error has occurred', E_USER_WARNING);
 
 		$this->assertEquals(E_USER_WARNING, $exception->getCode());
 		$this->assertEquals('An error has occurred', $exception->getMessage());
-		$this->assertEquals(false, $exception->getResult()->valid());
+		$this->assertFalse($exception->getResult()->valid());
 		$this->assertEquals('Incorrect placement of cutlery', $exception->getResult()->message());
 	}
 
@@ -73,8 +76,8 @@ class ValidationExceptionTest extends SapphireTest
 	 */
 	public function testCreateWithComplexValidationResultAndMessage() {
 		$result = new ValidationResult();
-		$result->error('A spork is not a knife')
+		$result->addError('A spork is not a knife')
-				->error('A knife is not a back scratcher');
+				->addError('A knife is not a back scratcher');
 		$exception = new ValidationException($result, 'An error has occurred', E_USER_WARNING);
 
 		$this->assertEquals(E_USER_WARNING, $exception->getCode());
@@ -91,8 +94,8 @@ class ValidationExceptionTest extends SapphireTest
 		$result = new ValidationResult();
 		$anotherresult = new ValidationResult();
 		$yetanotherresult = new ValidationResult();
-		$anotherresult->error("Eat with your mouth closed", "EATING101");
+		$anotherresult->addError("Eat with your mouth closed", 'bad', "EATING101");
-		$yetanotherresult->error("You didn't wash your hands", "BECLEAN");
+		$yetanotherresult->addError("You didn't wash your hands", 'bad', "BECLEAN", false);
 
 		$this->assertTrue($result->valid());
 		$this->assertFalse($anotherresult->valid());
@@ -104,7 +107,53 @@ class ValidationExceptionTest extends SapphireTest
 		$this->assertEquals(array(
 			"EATING101" => "Eat with your mouth closed",
 			"BECLEAN" => "You didn't wash your hands"
-		),$result->messageList());
+		), $result->messageList());
+	}
+
+	/**
+	 * Test that a ValidationException created with no contained ValidationResult
+	 * will correctly populate itself with an inferred version
+	 */
+	public function testCreateForField() {
+		$exception = ValidationException::create_for_field('Content', 'Content is required');
+
+		$this->assertEquals('Content is required', $exception->getMessage());
+		$this->assertEquals(false, $exception->getResult()->valid());
+
+		$this->assertEquals(array(
+			'Content' => array(
+				'message' => 'Content is required',
+				'messageType' => 'bad',
+			),
+		), $exception->getResult()->fieldErrors());
+	}
+
+	/**
+	 * Test that a ValidationException created with no contained ValidationResult
+	 * will correctly populate itself with an inferred version
+	 */
+	public function testValidationResultAddMethods() {
+		$result = new ValidationResult();
+		$result->addMessage('A spork is not a knife', 'bad');
+		$result->addError('A knife is not a back scratcher');
+		$result->addFieldMessage('Title', 'Title is good', 'good');
+		$result->addFieldError('Content', 'Content is bad');
+
+
+		$this->assertEquals(array(
+			'Title' => array(
+				'message' => 'Title is good',
+				'messageType' => 'good'
+			),
+			'Content' => array(
+				'message' => 'Content is bad',
+				'messageType' => 'bad'
+			)
+		), $result->fieldErrors());
+
+		$this->assertEquals('A spork is not a knife; A knife is not a back scratcher', $result->overallMessage());
+
+		$exception = ValidationException::create_for_field('Content', 'Content is required');
 	}
 
 }

tests/php/Security/MemberAuthenticatorTest.php

@@ -139,6 +139,7 @@ class MemberAuthenticatorTest extends SapphireTest {
 			'tempid' => $tempID,
 			'Password' => 'mypassword'
 		), $form);
+		$form->setupFormErrors();
 		$this->assertNotEmpty($result);
 		$this->assertEquals($result->ID, $member->ID);
 		$this->assertEmpty($form->Message());
@@ -149,8 +150,9 @@ class MemberAuthenticatorTest extends SapphireTest {
 			'tempid' => $tempID,
 			'Password' => 'notmypassword'
 		), $form);
+		$form->setupFormErrors();
 		$this->assertEmpty($result);
-		$this->assertEquals('The provided details don't seem to be correct. Please try again.', $form->Message());
+		$this->assertEquals(Convert::raw2xml(_t('Member.ERRORWRONGCRED')), $form->Message());
 		$this->assertEquals('bad', $form->MessageType());
 	}
 
@@ -168,6 +170,7 @@ class MemberAuthenticatorTest extends SapphireTest {
 			'Email' => 'admin',
 			'Password' => 'password'
 		), $form);
+		$form->setupFormErrors();
 		$this->assertNotEmpty($result);
 		$this->assertEquals($result->Email, Security::default_admin_username());
 		$this->assertEmpty($form->Message());
@@ -178,6 +181,7 @@ class MemberAuthenticatorTest extends SapphireTest {
 			'Email' => 'admin',
 			'Password' => 'notmypassword'
 		), $form);
+		$form->setupFormErrors();
 		$this->assertEmpty($result);
 		$this->assertEquals('The provided details don't seem to be correct. Please try again.', $form->Message());
 		$this->assertEquals('bad', $form->MessageType());

tests/php/Security/SecurityTest.php

@@ -444,7 +444,7 @@ class SecurityTest extends FunctionalTest {
 					$member->LockedOutUntil,
 					'User does not have a lockout time set if under threshold for failed attempts'
 				);
-				$this->assertContains($this->loginErrorMessage(), Convert::raw2xml(_t('Member.ERRORWRONGCRED')));
+				$this->assertContains(Convert::raw2xml(_t('Member.ERRORWRONGCRED')), $this->loginErrorMessage());
 			} else {
 				// Fuzzy matching for time to avoid side effects from slow running tests
 				$this->assertGreaterThan(
@@ -644,7 +644,8 @@ class SecurityTest extends FunctionalTest {
 	 * Get the error message on the login form
 	 */
 	public function loginErrorMessage() {
-		return $this->session()->inst_get('FormInfo.MemberLoginForm_LoginForm.formError.message');
+		$result = $this->session()->inst_get('FormInfo.MemberLoginForm_LoginForm.result');
+		return $result->message();
 	}
 
 }