Merge pull request #2537 from wilr/membervalidator

FIX: Use Injector API for managing Member_Validator instance.

admin/code/AdminRootController.php

@@ -1,5 +1,9 @@
 <?php
 
+/**
+ * @package framework
+ * @subpackage admin
+ */
 class AdminRootController extends Controller {
 
 	/**

admin/code/CMSBatchAction.php

@@ -10,8 +10,8 @@
  * 	_t('CMSBatchActions.PUBLISHED_PAGES', 'published %d pages')));
  * </code>
  * 
- * @package cms
- * @subpackage batchaction
+ * @package framework
+ * @subpackage admin
  */
 abstract class CMSBatchAction extends Object {
 	

admin/code/CMSBatchActionHandler.php

@@ -3,8 +3,8 @@
 /**
  * Special request handler for admin/batchaction
  *  
- * @package cms
- * @subpackage batchaction
+ * @package framework
+ * @subpackage admin
  */
 class CMSBatchActionHandler extends RequestHandler {
 	

admin/code/CMSForm.php

@@ -1,6 +1,11 @@
 <?php
+
 /**
- * Deals with special form handling in CMS, mainly around {@link PjaxResponseNegotiator}
+ * Deals with special form handling in CMS, mainly around
+ * {@link PjaxResponseNegotiator}
+ *
+ * @package framework
+ * @subpackage admin
  */
 class CMSForm extends Form {
 

admin/code/CMSPreviewable.php

@@ -1,12 +1,15 @@
 <?php
 /**
  * Interface to provide enough information about a record to make it previewable
- * through the CMS. It uses the record database ID, its "frontend" and "backend" links
- * to link up the edit form with its preview. 
+ * through the CMS. It uses the record database ID, its "frontend" and "backend"
+ * links to link up the edit form with its preview.
  * 
- * Also used by {@link SilverStripeNavigator} to generate links - 
- * both within the CMS preview, and as a frontend utility 
- * for logged-in CMS authors in custom themes (with the $SilverStripeNavigator template marker).
+ * Also used by {@link SilverStripeNavigator} to generate links -  both within
+ * the CMS preview, and as a frontend utility for logged-in CMS authors in
+ * custom themes (with the $SilverStripeNavigator template marker).
+ *
+ * @package framework
+ * @subpackage admin
  */
 interface CMSPreviewable {
 	
@@ -17,8 +20,9 @@ interface CMSPreviewable {
 	public function Link();
 	
 	/**
-	 * @return String Absolute URL to the CMS-author view. Should point to a controller subclassing {@link LeftAndMain}.
-	 * Example: http://mysite.com/admin/edit/6
+	 * @return String Absolute URL to the CMS-author view. Should point to a
+	 * controller subclassing {@link LeftAndMain}. Example:
+	 * http://mysite.com/admin/edit/6
 	 */
 	public function CMSEditLink();
 

admin/code/CMSProfileController.php

@@ -1,4 +1,9 @@
 <?php
+
+/**
+ * @package framework
+ * @subpackage admin
+ */
 class CMSProfileController extends LeftAndMain {
 
 	private static $url_segment = 'myprofile';
@@ -22,8 +27,11 @@ class CMSProfileController extends LeftAndMain {
 		$this->setCurrentPageID(Member::currentUserID());
 
 		$form = parent::getEditForm($id, $fields);
-		if($form instanceof SS_HTTPResponse) return $form;
 		
+		if($form instanceof SS_HTTPResponse) {
+			return $form;
+		}
+
 		$form->Fields()->removeByName('LastVisited');
 		$form->Fields()->push(new HiddenField('ID', null, Member::currentUserID()));
 		$form->Actions()->push(
@@ -32,11 +40,21 @@ class CMSProfileController extends LeftAndMain {
 				->setAttribute('data-icon', 'accept')
 				->setUseButtonTag(true)
 		);
+
 		$form->Actions()->removeByName('action_delete');
-		$form->setValidator(new Member_Validator());
 		$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;

admin/code/GroupImportForm.php

@@ -1,10 +1,11 @@
 <?php
+
 /**
  * Imports {@link Group} records by CSV upload, as defined in
  * {@link GroupCsvBulkLoader}.
  * 
- * @package cms
- * @subpackage batchactions
+ * @package framework
+ * @subpackage admin
  */
 class GroupImportForm extends Form {
 	

admin/code/LeftAndMainExtension.php

@@ -1,4 +1,5 @@
 <?php
+
 /**
  * Plug-ins for additional functionality in your LeftAndMain classes.
  * 

admin/code/MemberImportForm.php

@@ -1,10 +1,11 @@
 <?php
+
 /**
  * Imports {@link Member} records by CSV upload, as defined in
  * {@link MemberCsvBulkLoader}.
  * 
- * @package cms
- * @subpackage batchactions
+ * @package framework
+ * @subpackage admin
  */
 class MemberImportForm extends Form {
 	

admin/code/SecurityAdmin.php

@@ -1,8 +1,10 @@
 <?php
+
 /**
  * Security section of the CMS
- * @package cms
- * @subpackage security
+ *
+ * @package framework
+ * @subpackage admin
  */
 class SecurityAdmin extends LeftAndMain implements PermissionProvider {
 
@@ -60,7 +62,10 @@ class SecurityAdmin extends LeftAndMain implements PermissionProvider {
 		
 		// TODO Duplicate record fetching (see parent implementation)
 		$record = $this->getRecord($id);
-		if($record && !$record->canView()) return Security::permissionFailure($this);
+
+		if($record && !$record->canView()) {
+			return Security::permissionFailure($this);
+		}
 		
 		$memberList = GridField::create(
 			'Members',
@@ -70,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',

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);
 	}
 }
-
-

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)
 	 *

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) {

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();