Validation refactoring in Form.php

gistfile1.diff

diff --git a/forms/Form.php b/forms/Form.php
index 3bf9cbe..ff3c1cf 100644
--- a/forms/Form.php
+++ b/forms/Form.php
@@ -195,31 +195,33 @@ class Form extends RequestHandler {
 	);
 	
 	/**
-	 * Set up current form errors in session to
-	 * the current form if appropriate.
+	 * Take errors from a ValidationResult and populate the form with the appropriate message.
+	 * 
+	 * @param ValidationResult $result The erroneous ValidationResult.  If none passed, this will be atken
+	 * from the session
 	 */
-	public function setupFormErrors() {
-		$errorInfo = Session::get("FormInfo.{$this->FormName()}");
-
-		if(isset($errorInfo['errors']) && is_array($errorInfo['errors'])) {
-			foreach($errorInfo['errors'] as $error) {
-				$field = $this->fields->dataFieldByName($error['fieldName']);
+	public function setupFormErrors($result = null, $data = null) {
+		if(!$result) $result = Session::get("FormInfo.{$this->FormName()}.result");
+		if(!$result || $result->valid()) return;
+		
+		foreach($result->fieldErrors() as $fieldName => $fieldError) {
+			$field = $this->fields->dataFieldByName($fieldName);
+			$field->setError($fieldError['message'], $fieldError['messageType']);
+		}
 
-				if(!$field) {
-					$errorInfo['message'] = $error['message'];
-					$errorInfo['type'] = $error['messageType'];
-				} else {
-					$field->setError($error['message'], $error['messageType']);
-				}
-			}
+		$this->setMessage($result->overallMessage(), 'bad');
 
-			// load data in from previous submission upon error
-			if(isset($errorInfo['data'])) $this->loadDataFrom($errorInfo['data']);
-		}
+		// load data in from previous submission upon error
+		if(!$data) $data = Session::get("FormInfo.{$this->FormName()}.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) {
+		Session::set("FormInfo.{$this->FormName()}.result", $result);
+		Session::set("FormInfo.{$this->FormName()}.data", $data);
 	}
 	
 	/**
@@ -313,16 +315,48 @@ class Form extends RequestHandler {
 		}*/
 		
 		// Validate the form
-		if(!$this->validate()) {
+		
+		try {
+			// This will throw a ValidationException is appropraite
+			$this->validate();
+
+			// 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);
+			}
+
+		} catch(ValidationException $e) {
+			// The ValdiationResult contains all the relevant metadata
+			$result = $e->getResult();
+
 			if(Director::is_ajax()) {
+				// TO DO: Remove this
+				if($this->validator->getJavascriptValidationHandler() == 'prototype') {
+					FormResponse::status_message(_t('Form.VALIDATIONFAILED', 'Validation failed'), 'bad');
+					foreach($errors as $error) {
+						FormResponse::add(sprintf(
+							"validationError('%s', '%s', '%s');\n",
+							Convert::raw2js($error['fieldName']),
+							Convert::raw2js($error['message']),
+							Convert::raw2js($error['messageType'])
+						));
+					}
+				}
+
 				// Special case for legacy Validator.js implementation (assumes eval'ed javascript collected through FormResponse)
 				$acceptType = $request->getHeader('Accept');
 				if(strpos($acceptType, 'application/json') !== FALSE) {
+					// TO DO: This JSON format has changed
 					// Send validation errors back as JSON with a flag at the start
-					$response = new SS_HTTPResponse(Convert::array2json($this->validator->getErrors()));
+					$response = new SS_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 SS_HTTPResponse($this->forTemplate());
 					$response->addHeader('Content-Type', 'text/html');
@@ -330,6 +364,10 @@ class Form extends RequestHandler {
 				
 				return $response;
 			} else {
+				// Save the relevant information in the session
+				$this->saveFormErrorsToSession($result, $this->getData());
+				
+				// Redirect back to the form
 				if($this->getRedirectToFormOnValidationError()) {
 					if($pageURL = $request->getHeader('Referer')) {
 						if(Director::is_site_url($pageURL)) {
@@ -343,16 +381,6 @@ class Form extends RequestHandler {
 			}
 		}
 		
-		// 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);
-		}
-		
 		return $this->httpError(404);
 	}
 	
@@ -429,11 +457,7 @@ class Form extends RequestHandler {
 	 * and used the next time this form is displayed.
 	 */
 	public function addErrorMessage($fieldName, $message, $messageType) {
-		Session::add_to_array("FormInfo.{$this->FormName()}.errors",  array(
-			'fieldName' => $fieldName,
-			'message' => $message,
-			'messageType' => $messageType,
-		));
+		Deprecation::notice('3.0', 'Throw a ValidationException instead.');
 	}
 
 	public function transform(FormTransformation $trans) {
@@ -962,11 +986,13 @@ class Form extends RequestHandler {
 	 * @param type Should be set to good, bad, or warning.
 	 */
 	public function sessionMessage($message, $type) {
+		// TO DO: Deprecate and replace with something else, perhaps based on a ValidationResult?
 		Session::set("FormInfo.{$this->FormName()}.formError.message", $message);
 		Session::set("FormInfo.{$this->FormName()}.formError.type", $type);
 	}
 
 	public static function messageForForm( $formName, $message, $type ) {
+		// TO DO: Deprecate and replace with something else, perhaps based on a ValidationResult?
 		Session::set("FormInfo.{$formName}.formError.message", $message);
 		Session::set("FormInfo.{$formName}.formError.type", $type);
 	}
@@ -974,10 +1000,12 @@ class Form extends RequestHandler {
 	public function clearMessage() {
 		$this->message  = null;
 		Session::clear("FormInfo.{$this->FormName()}.errors");
+		Session::clear("FormInfo.{$this->FormName()}.result");
 		Session::clear("FormInfo.{$this->FormName()}.formError");
 	}
 	public function resetValidation() {
 		Session::clear("FormInfo.{$this->FormName()}.errors");
+		Session::clear("FormInfo.{$this->FormName()}.result");
 	}
 
 	/**
@@ -1002,40 +1030,27 @@ class Form extends RequestHandler {
 
 	/**
 	 * Processing that occurs before a form is executed.
-	 * This includes form validation, if it fails, we redirect back
-	 * to the form with appropriate error messages.
+	 * 
+	 * This includes form validation, if it fails, we throw a ValidationException
 	 * 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
 	 */
-	 function validate(){
+	function validate(){
 		if($this->validator){
 			$errors = $this->validator->validate();
 
-			if($errors){
-				if(Director::is_ajax() && $this->validator->getJavascriptValidationHandler() == 'prototype') {
-					FormResponse::status_message(_t('Form.VALIDATIONFAILED', 'Validation failed'), 'bad');
-					foreach($errors as $error) {
-						FormResponse::add(sprintf(
-							"validationError('%s', '%s', '%s');\n",
-							Convert::raw2js($error['fieldName']),
-							Convert::raw2js($error['message']),
-							Convert::raw2js($error['messageType'])
-						));
-					}
-				} else {
-					$data = $this->getData();
-
-					// Load errors into session and post back
-					Session::set("FormInfo.{$this->FormName()}.errors", $errors); 
-					Session::set("FormInfo.{$this->FormName()}.data", $data);
+			if($errors) {
+				$result = new ValidationResult;
+				foreach($errors as $error) {
+					$result->error($error['message'], null, $error['fieldName'], $error['messageType']);
 				}
-				return false;
+				throw new ValidationException($result);
 			}
 		}
-		return true;
 	}
 
 	/**
diff --git a/forms/gridfield/GridFieldDeleteAction.php b/forms/gridfield/GridFieldDeleteAction.php
index 62e32f0..e815afb 100644
--- a/forms/gridfield/GridFieldDeleteAction.php
+++ b/forms/gridfield/GridFieldDeleteAction.php
@@ -130,7 +130,7 @@ class GridFieldDeleteAction implements GridField_ColumnProvider, GridField_Actio
 				return;
 			}
 			if($actionName == 'deleterecord' && !$item->canDelete()) {
-				throw new ValidationException(_t('GridFieldAction_Delete.DeletePermissionsFailure',"No delete permissions"),0);
+				throw new ValidationException(_t('GridFieldAction_Delete.DeletePermissionsFailure',"No delete permissions"));
 			}
 			$gridField->getList()->remove($item);
 		} 
diff --git a/forms/gridfield/GridFieldDetailForm.php b/forms/gridfield/GridFieldDetailForm.php
index cc9a595..6f0ab12 100755
--- a/forms/gridfield/GridFieldDetailForm.php
+++ b/forms/gridfield/GridFieldDetailForm.php
@@ -316,7 +316,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler {
 		try {
 			$toDelete = $this->record;
 			if (!$toDelete->canDelete()) {
-				throw new ValidationException(_t('GridFieldDetailsForm.DeletePermissionsFailure',"No delete permissions"),0);
+				throw new ValidationException(_t('GridFieldDetailsForm.DeletePermissionsFailure',"No delete permissions"));
 			}
 
 			$toDelete->delete();
diff --git a/model/ValidationException.php b/model/ValidationException.php
index d6a5bff..d307c90 100644
--- a/model/ValidationException.php
+++ b/model/ValidationException.php
@@ -10,16 +10,24 @@
 class ValidationException extends Exception {
 
 	/**
-	 * @var {@link ValidationResult} or string
+	 * @var {@link ValidationResult} or string.  Omit for a default message.
 	 */
 	protected $result;
 
-	public function __construct($result = null, $message = null, $code = 0) {
+	/**
+	 * @param $result {@link ValidationResult} or string.  Omit for a default message.
+	 * @param $message Message argument for Exception constructor
+	 * @param $code Code argument for Exception constructor
+	 */
+	public function __construct($result = null, $message = null, $code = null) {
 		if($result instanceof ValidationResult) {
 			$this->result = $result;
+		} else if(is_string($result)) {
+			$this->result = new ValidationResult(false, $result);
+		} else if(!$result) {
+			$this->result = new ValidationResult(false, _t("ValdiationExcetpion.DEFAULT_ERROR", "Validation error"));
 		} else {
-			$code = $message;
-			$message = $result;
+			throw new InvalidArgumentException("ValidationExceptions must be passed a ValdiationResult, a string, or nothign at all");
 		}
 		
 		parent::__construct($message, $code);
diff --git a/model/ValidationResult.php b/model/ValidationResult.php
index 0ed834d..8058d08 100644
--- a/model/ValidationResult.php
+++ b/model/ValidationResult.php
@@ -24,27 +24,40 @@ class ValidationResult extends Object {
 	 */
 	function __construct($valid = true, $message = null) {
 		$this->isValid = $valid;
-		if($message) $this->errorList[] = $message;
+		if($message) $this->error($message);
 		parent::__construct();
 	}
 	
 	/**
+	 * Return the full error meta-data, suitable for combining with another ValidationResult.
+	 */
+	function getErrorMetaData() {
+		return $this->errorList;
+	}
+	
+	/**
 	 * Record an error against this validation result,
 	 * @param $message The validation error message
 	 * @param $code An optional error code string, that can be accessed with {@link $this->codeList()}.
 	 */
-	function error($message, $code = null) {
+	function error($message, $code = null, $fieldName = null, $errorType = null) {
 		$this->isValid = false;
 		
+		$metadata = array(
+			'message' => $message,
+			'fieldName' => $fieldName,
+			'errorType' => $errorType,
+		);
+		
 		if($code) {
 			if(!is_numeric($code)) {
-				$this->errorList[$code] = $message;
+				$this->errorList[$code] = $metadata;
 			} else {
-				user_error("ValidationResult::error() - Don't use a numeric code '$code'.  Use a string.  I'm going to ignore it.", E_USER_WARNING);
-				$this->errorList[$code] = $message;
+				throw new InvalidArgumentException("ValidationResult::error() - Don't use a numeric code '$code'.  Use a string.");
+				$this->errorList[$code] = $metadata;
 			}
 		} else {
-			$this->errorList[] = $message;
+			$this->errorList[] = $metadata;
 		}
 	}
 	
@@ -59,9 +72,28 @@ class ValidationResult extends Object {
 	 * Get an array of errors
 	 */
 	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 'errorType'
+	 */
+	function fieldErrors() {
+		$output = array();
+		foreach($this->errorList as $key => $item) {
+			if($item['fieldName']) {
+				$output[$item['fieldName']] = array('message' => $item['message'], 'errorType' => $item['errorType']);
+			}
+		}
+		return $output;
+	}
+	
 	/**
 	 * Get an array of error codes
 	 */
@@ -75,14 +107,25 @@ class ValidationResult extends Object {
 	 * Get the error message as a string.
 	 */
 	function message() {
-		return implode("; ", $this->errorList);
+		return implode("; ", $this->messageList());
+	}
+	
+	/**
+	 * The the error message that's not related to a field as a string
+	 */
+	function overallMessage() {
+		$messages = array();
+		foreach($this->errorList as $item) {
+			if(!$item['fieldName']) $messages[] = $item['message'];
+		}
+		return implode("; ", $messages);
 	}
 	
 	/**
 	 * Get a starred list of all messages
 	 */
 	function starredList() {
-		return " * " . implode("\n * ", $this->errorList);
+		return " * " . implode("\n * ", $this->messageList());
 	}
 	
 	/**
@@ -92,8 +135,6 @@ class ValidationResult extends Object {
 	 */
 	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());
 	}
-	
-	
 }
diff --git a/security/Member.php b/security/Member.php
index 6f8e59b..6770afc 100644
--- a/security/Member.php
+++ b/security/Member.php
@@ -626,7 +626,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 				)
 			);
 			if($existingRecord) {
-				throw new ValidationException(new ValidationResult(false, sprintf(
+				throw new ValidationException(sprintf(
 					_t(
 						'Member.ValidationIdentifierFailed', 
 						'Can\'t overwrite existing member #%d with identical identifier (%s = %s))', 
@@ -636,7 +636,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 					$existingRecord->ID,
 					$identifierField,
 					$this->$identifierField
-				)));
+				));
 			}
 		}
 
@@ -1463,7 +1463,7 @@ class Member_ProfileForm extends Form {
 		$member = DataObject::get_by_id("Member", $SQL_data['ID']);
 		
 		if($SQL_data['Locale'] != $member->Locale) {
-			$form->addErrorMessage("Generic", _t('Member.REFRESHLANG'),"good");
+			$form->sessionMessage( _t('Member.REFRESHLANG'),"good");
 		}
 		
 		$form->saveInto($member);
diff --git a/security/MemberAuthenticator.php b/security/MemberAuthenticator.php
index feade8c..7c17910 100644
--- a/security/MemberAuthenticator.php
+++ b/security/MemberAuthenticator.php
@@ -30,6 +30,8 @@ class MemberAuthenticator extends Authenticator {
    * @see Security::setDefaultAdmin()
    */
   public static function authenticate($RAW_data, Form $form = null) {
+	throw new ValidationException('This is a test');
+	
 	if(array_key_exists('Email', $RAW_data) && $RAW_data['Email']){
 		$SQL_user = Convert::raw2sql($RAW_data['Email']);
 	} else {