From 813d34b15ef43167022454a3a99ecd331833836e Mon Sep 17 00:00:00 2001
From: Will Rossiter <will.rossiter@gmail.com>
Date: Wed, 16 Oct 2013 11:29:43 +1300
Subject: [PATCH] FIX: Use Injector API for managing Member_Validator instance.

Updates the CMS profile page and SecurityAdmin to give developers a few ways to customise the required fields.

Added extension hook updateValidator for getValidator for things like modules to inject required fields to go along with Injector for replacing the entire class for project specific use.
---
 admin/code/CMSProfileController.php        |  14 ++-
 admin/code/SecurityAdmin.php               |  11 ++-
 forms/RequiredFields.php                   | 108 +++++++++++++++------
 security/Member.php                        |  42 ++++++--
 tests/control/CMSProfileControllerTest.php |  10 ++
 tests/security/MemberTest.php              | 100 ++++++++++++++++++-
 6 files changed, 244 insertions(+), 41 deletions(-)

diff --git a/admin/code/CMSProfileController.php b/admin/code/CMSProfileController.php
index 2d751dbd1..6b03f105e 100644
--- a/admin/code/CMSProfileController.php
+++ b/admin/code/CMSProfileController.php
@@ -31,9 +31,9 @@ class CMSProfileController extends LeftAndMain {
 		if($form instanceof SS_HTTPResponse) {
 			return $form;
 		}
+
 		$form->Fields()->removeByName('LastVisited');
 		$form->Fields()->push(new HiddenField('ID', null, Member::currentUserID()));
-		$form->setValidator(new Member_Validator());
 		$form->Actions()->push(
 			FormAction::create('save',_t('CMSMain.SAVE', 'Save'))
 				->addExtraClass('ss-ui-button ss-ui-action-constructive')
@@ -44,7 +44,17 @@ class CMSProfileController extends LeftAndMain {
 		$form->Actions()->removeByName('action_delete');
 		$form->setTemplate('Form');
 		$form->setAttribute('data-pjax-fragment', null);
-		if($form->Fields()->hasTabset()) $form->Fields()->findOrMakeTab('Root')->setTemplate('CMSTabSet');
+
+		if($member = Member::currentUser()) {
+			$form->setValidator($member->getValidator());
+		} else {
+			$form->setValidator(Injector::inst()->get('Member')->getValidator());
+		}
+
+		if($form->Fields()->hasTabset()) {
+			$form->Fields()->findOrMakeTab('Root')->setTemplate('CMSTabSet');
+		}
+
 		$form->addExtraClass('member-profile-form root-form cms-edit-form cms-panel-padded center');
 		
 		return $form;
diff --git a/admin/code/SecurityAdmin.php b/admin/code/SecurityAdmin.php
index 9a89badd1..2e71de0bc 100755
--- a/admin/code/SecurityAdmin.php
+++ b/admin/code/SecurityAdmin.php
@@ -75,7 +75,16 @@ class SecurityAdmin extends LeftAndMain implements PermissionProvider {
 				->addComponent(new GridFieldButtonRow('after'))
 				->addComponent(new GridFieldExportButton('buttons-after-left'))
 		)->addExtraClass("members_grid");
-		$memberListConfig->getComponentByType('GridFieldDetailForm')->setValidator(new Member_Validator());
+
+		if($record && method_exists($record, 'getValidator')) {
+			$validator = $record->getValidator();
+		} else {
+			$validator = Injector::inst()->get('Member')->getValidator();
+		}
+
+		$memberListConfig
+			->getComponentByType('GridFieldDetailForm')
+			->setValidator($validator);
 
 		$groupList = GridField::create(
 			'Groups',
diff --git a/forms/RequiredFields.php b/forms/RequiredFields.php
index 1e3adbd08..960a2bcde 100644
--- a/forms/RequiredFields.php
+++ b/forms/RequiredFields.php
@@ -1,10 +1,12 @@
 <?php
+
 /**
- * Required Fields allows you to set which fields
- * need to be present before submitting the form
- * Submit an array of arguments or each field as a
- * seperate argument. Validation is performed on a name by
- * name basis.
+ * Required Fields allows you to set which fields need to be present before
+ * submitting the form. Submit an array of arguments or each field as a separate
+ * argument.
+ *
+ * Validation is performed on a field by field basis through
+ * {@link FormField::validate}.
  *
  * @package forms
  * @subpackage validators
@@ -15,8 +17,8 @@ class RequiredFields extends Validator {
 	protected $useLabels = true;
 
 	/**
-	 * Pass each field to be validated as a seperate argument
-	 * to the constructor of this object. (an array of elements are ok)
+	 * Pass each field to be validated as a seperate argument to the constructor
+	 * of this object. (an array of elements are ok).
 	 */
 	public function __construct() {
 		$required = func_get_args();
@@ -40,9 +42,12 @@ class RequiredFields extends Validator {
 
 	/**
 	 * Clears all the validation from this object.
+	 *
+	 * @return RequiredFields
 	 */
-	public function removeValidation(){
+	public function removeValidation() {
 		$this->required = array();
+
 		return $this;
 	}
 
@@ -50,7 +55,9 @@ class RequiredFields extends Validator {
 	 * Debug helper
 	 */
 	public function debug() {
-		if(!is_array($this->required)) return false;
+		if(!is_array($this->required)) {
+			return false;
+		}
 
 		$result = "<ul>";
 		foreach( $this->required as $name ){
@@ -62,25 +69,40 @@ class RequiredFields extends Validator {
 	}
 
 	/**
-	* Allows validation of fields via specification of a php function for validation which is executed after
-	* the form is submitted
-	*/
+	 * Allows validation of fields via specification of a php function for
+	 * validation which is executed after the form is submitted.
+	 *
+	 * @param array $data
+	 *
+	 * @return boolean
+	 */
 	public function php($data) {
 		$valid = true;
-
 		$fields = $this->form->Fields();
+
 		foreach($fields as $field) {
 			$valid = ($field->validate($this) && $valid);
 		}
+
 		if($this->required) {
 			foreach($this->required as $fieldName) {
-				if(!$fieldName) continue;
+				if(!$fieldName) {
+					continue;
+				}
 
-				$formField = $fields->dataFieldByName($fieldName);
+				if($fieldName instanceof FormField) {
+					$formField = $fieldName;
+					$fieldName = $fieldName->getName();
+				}
+				else {
+					$formField = $fields->dataFieldByName($fieldName);
+				}
 
 				$error = true;
+				
 				// submitted data for file upload fields come back as an array
 				$value = isset($data[$fieldName]) ? $data[$fieldName] : null;
+
 				if(is_array($value)) {
 					if($formField instanceof FileField && isset($value['error']) && $value['error']) {
 						$error = true;
@@ -106,11 +128,13 @@ class RequiredFields extends Validator {
 					if($msg = $formField->getCustomValidationMessage()) {
 						$errorMessage = $msg;
 					}
+
 					$this->validationError(
 						$fieldName,
 						$errorMessage,
 						"required"
 					);
+
 					$valid = false;
 				}
 			}
@@ -120,40 +144,66 @@ class RequiredFields extends Validator {
 	}
 
 	/**
-	 * Add's a single required field to requiredfields stack
+	 * Adds a single required field to required fields stack.
+	 *
+	 * @param string $field
+	 *
+	 * @return RequiredFields
 	 */
-	public function addRequiredField( $field ) {
+	public function addRequiredField($field) {
 		$this->required[$field] = $field;
-		return $this;
-	}
 
-	public function removeRequiredField($field) {
-		unset($this->required[$field]);
 		return $this;
 	}
 
 	/**
-	 * allows you too add more required fields to this object after construction.
+	 * Removes a required field
+	 *
+	 * @param string $field
+	 *
+	 * @return RequiredFields
 	 */
-	public function appendRequiredFields($requiredFields){
-		$this->required = $this->required + ArrayLib::valuekey($requiredFields->getRequired());
+	public function removeRequiredField($field) {
+		unset($this->required[$field]);
+
+		return $this;
+	}
+
+	/**
+	 * Add {@link RequiredField} objects together
+	 *
+	 * @param RequiredFields
+	 *
+	 * @return RequiredFields
+	 */
+	public function appendRequiredFields($requiredFields) {
+		$this->required = $this->required + ArrayLib::valuekey(
+			$requiredFields->getRequired()
+		);
+
 		return $this;
 	}
 
 	/**
 	 * Returns true if the named field is "required".
-	 * Used by FormField to return a value for FormField::Required(), to do things like show *s on the form template.
+	 *
+	 * Used by {@link FormField} to return a value for FormField::Required(),
+	 * to do things like show *s on the form template.
+	 *
+	 * @param string $fieldName
+	 *
+	 * @return boolean
 	 */
 	public function fieldIsRequired($fieldName) {
 		return isset($this->required[$fieldName]);
 	}
 
 	/**
-	 * getter function for append
+	 * Return the required fields
+	 *
+	 * @return array
 	 */
-	public function getRequired(){
+	public function getRequired() {
 		return array_values($this->required);
 	}
 }
-
-
diff --git a/security/Member.php b/security/Member.php
index a4f212a9f..8dac6ff40 100644
--- a/security/Member.php
+++ b/security/Member.php
@@ -611,8 +611,21 @@ class Member extends DataObject implements TemplateGlobalProvider {
 		return $fields;
 	}
 
+	/**
+	 * Returns the {@link RequiredFields} instance for the Member object. This
+	 * Validator is used when saving a {@link CMSProfileController} or added to
+	 * any form responsible for saving a users data.
+	 *
+	 * To customize the required fields, add a {@link DataExtension} to member
+	 * calling the `updateValidator()` method.
+	 *
+	 * @return Member_Validator
+	 */
 	public function getValidator() {
-		return new Member_Validator();
+		$validator = Injector::inst()->create('Member_Validator');
+		$this->extend('updateValidator', $validator);
+
+		return $validator;
 	}
 
 
@@ -624,6 +637,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 	 */
 	public static function currentUser() {
 		$id = Member::currentUserID();
+
 		if($id) {
 			return DataObject::get_one("Member", "\"Member\".\"ID\" = $id", true, 1);
 		}
@@ -1525,18 +1539,21 @@ class Member_GroupSet extends ManyManyList {
 
 /**
  * Class used as template to send an email saying that the password has been
- * changed
+ * changed.
+ *
  * @package framework
  * @subpackage security
  */
 class Member_ChangePasswordEmail extends Email {
+
 	protected $from = '';   // setting a blank from address uses the site's default administrator email
 	protected $subject = '';
 	protected $ss_template = 'ChangePasswordEmail';
 	
 	public function __construct() {
 		parent::__construct();
-	$this->subject = _t('Member.SUBJECTPASSWORDCHANGED', "Your password has been changed", 'Email subject');
+
+		$this->subject = _t('Member.SUBJECTPASSWORDCHANGED', "Your password has been changed", 'Email subject');
 	}
 }
 
@@ -1544,6 +1561,7 @@ class Member_ChangePasswordEmail extends Email {
 
 /**
  * Class used as template to send the forgot password email
+ *
  * @package framework
  * @subpackage security
  */
@@ -1554,18 +1572,29 @@ class Member_ForgotPasswordEmail extends Email {
 	
 	public function __construct() {
 		parent::__construct();
-	$this->subject = _t('Member.SUBJECTPASSWORDRESET', "Your password reset link", 'Email subject');
+
+		$this->subject = _t('Member.SUBJECTPASSWORDRESET', "Your password reset link", 'Email subject');
 	}
 }
 
 /**
  * Member Validator
+ *
+ * Custom validation for the Member object can be achieved either through an
+ * {@link DataExtension} on the Member object or, by specifying a subclass of
+ * {@link Member_Validator} through the {@link Injector} API.
+ *
+ * {@see Member::getValidator()}
+ *
  * @package framework
  * @subpackage security
  */
 class Member_Validator extends RequiredFields {
 
-	protected $customRequired = array('FirstName', 'Email'); //, 'Password');
+	protected $customRequired = array(
+		'FirstName',
+		'Email'
+	);
 
 
 	/**
@@ -1573,15 +1602,16 @@ class Member_Validator extends RequiredFields {
 	 */
 	public function __construct() {
 		$required = func_get_args();
+
 		if(isset($required[0]) && is_array($required[0])) {
 			$required = $required[0];
 		}
+
 		$required = array_merge($required, $this->customRequired);
 
 		parent::__construct($required);
 	}
 
-
 	/**
 	 * Check if the submitted member data is valid (server-side)
 	 *
diff --git a/tests/control/CMSProfileControllerTest.php b/tests/control/CMSProfileControllerTest.php
index d5611c3f3..9545a2c9a 100644
--- a/tests/control/CMSProfileControllerTest.php
+++ b/tests/control/CMSProfileControllerTest.php
@@ -1,4 +1,9 @@
 <?php
+
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class CMSProfileControllerTest extends FunctionalTest {
 
 	protected static $fixture_file = 'CMSProfileControllerTest.yml';
@@ -74,6 +79,11 @@ class CMSProfileControllerTest extends FunctionalTest {
 	}
 
 }
+
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class CMSProfileControllerTestExtension extends DataExtension {
 
 	public function canEdit($member = null) {
diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php
index 279a16848..86aa4e61c 100644
--- a/tests/security/MemberTest.php
+++ b/tests/security/MemberTest.php
@@ -42,10 +42,11 @@ class MemberTest extends FunctionalTest {
 	
 	public function tearDown() {
 		Member::config()->unique_identifier_field = $this->orig['Member_unique_identifier_field'];
-
 		parent::tearDown();
 	}
 
+
+
 	/**
 	 * @expectedException ValidationException
 	 */
@@ -698,21 +699,110 @@ class MemberTest extends FunctionalTest {
 		);
 	}
 
+
+	public function testCustomMemberValidator() {
+		$member = $this->objFromFixture('Member', 'admin');
+
+		$form = new MemberTest_ValidatorForm();
+		$form->loadDataFrom($member);
+
+		$validator = new Member_Validator();
+		$validator->setForm($form);
+
+		$pass = $validator->php(array(
+			'FirstName' => 'Borris',
+			'Email' => 'borris@silverstripe.com'
+		));
+
+		$fail = $validator->php(array(
+			'Email' => 'borris@silverstripe.com',
+			'Surname' => ''
+		));
+
+		$this->assertTrue($pass, 'Validator requires on FirstName and Email');
+		$this->assertFalse($fail, 'Missing FirstName');
+
+		$ext = new MemberTest_ValidatorExtension();
+		$ext->updateValidator($validator);
+
+		$pass = $validator->php(array(
+			'FirstName' => 'Borris',
+			'Email' => 'borris@silverstripe.com'
+		));
+
+		$fail = $validator->php(array(
+			'Email' => 'borris@silverstripe.com'
+		));
+
+		$this->assertFalse($pass, 'Missing surname');
+		$this->assertFalse($fail, 'Missing surname value');
+
+		$fail = $validator->php(array(
+			'Email' => 'borris@silverstripe.com',
+			'Surname' => 'Silverman'
+		));
+
+		$this->assertTrue($fail, 'Passes with email and surname now (no firstname)');
+	}
+
 }
+
+/**
+ * @package framework
+ * @subpackage tests
+ */
+class MemberTest_ValidatorForm extends Form implements TestOnly {
+
+	public function __construct() {
+		parent::__construct(Controller::curr(), __CLASS__, new FieldList(
+			new TextField('Email'),
+			new TextField('Surname'),
+			new TextField('ID'),
+			new TextField('FirstName')
+		), new FieldList(
+			new FormAction('someAction')
+		));
+	}
+}
+
+/**
+ * @package framework
+ * @subpackage tests
+ */
+class MemberTest_ValidatorExtension extends DataExtension implements TestOnly {
+
+	public function updateValidator(&$validator) {
+		$validator->addRequiredField('Surname');
+		$validator->removeRequiredField('FirstName');
+	}
+}
+
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class MemberTest_ViewingAllowedExtension extends DataExtension implements TestOnly {
 
 	public function canView($member = null) {
 		return true;
 	}
-
 }
+
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class MemberTest_ViewingDeniedExtension extends DataExtension implements TestOnly {
 
 	public function canView($member = null) {
 		return false;
 	}
-
 }
+
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class MemberTest_EditingAllowedDeletingDeniedExtension extends DataExtension implements TestOnly {
 
 	public function canView($member = null) {
@@ -729,6 +819,10 @@ class MemberTest_EditingAllowedDeletingDeniedExtension extends DataExtension imp
 
 }
 
+/**
+ * @package framework
+ * @subpackage tests
+ */
 class MemberTest_PasswordValidator extends PasswordValidator {
 	public function __construct() {
 		parent::__construct();