Merge pull request #5098 from bummzack/5086-fix-member-validator

Fix for issue #5086

security/Group.php

@@ -115,7 +115,7 @@ class Group extends DataObject {
 			$config->getComponentByType('GridFieldAddExistingAutocompleter')
 				->setResultsFormat('$Title ($Email)')->setSearchFields(array('FirstName', 'Surname', 'Email'));
 			$config->getComponentByType('GridFieldDetailForm')
-				->setValidator(new Member_Validator())
+				->setValidator(Member_Validator::create())
 				->setItemEditFormCallback(function($form, $component) use($group) {
 					$record = $form->getRecord();
 					$groupsField = $form->Fields()->dataFieldByName('DirectGroups');

security/Member.php

@@ -766,6 +766,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 	 */
 	public function getValidator() {
 		$validator = Injector::inst()->create('Member_Validator');
+		$validator->setForMember($this);
 		$this->extend('updateValidator', $validator);
 
 		return $validator;
@@ -1801,21 +1802,39 @@ class Member_ForgotPasswordEmail extends Email {
  * 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 DataExtension} on the Member_Validator object or, by specifying a subclass of
  * {@link Member_Validator} through the {@link Injector} API.
- *
+ * The Validator can also be modified by adding an Extension to Member and implement the
+ * <code>updateValidator</code> hook.
  * {@see Member::getValidator()}
  *
+ * Additional required fields can also be set via config API, eg.
+ * <code>
+ * Member_Validator:
+ *   customRequired:
+ *     - Surname
+ * </code>
+ *
  * @package framework
  * @subpackage security
  */
-class Member_Validator extends RequiredFields {
+class Member_Validator extends RequiredFields
-
+{
+	/**
+	 * Fields that are required by this validator
+	 * @config
+	 * @var array
+	 */
 	protected $customRequired = array(
 		'FirstName',
 		'Email'
 	);
 
+	/**
+	 * Determine what member this validator is meant for
+	 * @var Member
+	 */
+	protected $forMember = null;
 
 	/**
 	 * Constructor
@@ -1829,7 +1848,33 @@ class Member_Validator extends RequiredFields {
 
 		$required = array_merge($required, $this->customRequired);
 
-		parent::__construct($required);
+		// check for config API values and merge them in
+		$config = $this->config()->customRequired;
+		if(is_array($config)){
+			$required = array_merge($required, $config);
+		}
+
+		parent::__construct(array_unique($required));
+	}
+
+	/**
+	 * Get the member this validator applies to.
+	 * @return Member
+	 */
+	public function getForMember()
+	{
+		return $this->forMember;
+	}
+
+	/**
+	 * Set the Member this validator applies to.
+	 * @param Member $value
+	 * @return $this
+	 */
+	public function setForMember(Member $value)
+	{
+		$this->forMember = $value;
+		return $this;
 	}
 
 	/**
@@ -1842,47 +1887,54 @@ class Member_Validator extends RequiredFields {
 	 * @return bool Returns TRUE if the submitted data is valid, otherwise
 	 *              FALSE.
 	 */
-	public function php($data) {
+	public function php($data)
+	{
 		$valid = parent::php($data);
 
-		$identifierField = Member::config()->unique_identifier_field;
+		$identifierField = (string)Member::config()->unique_identifier_field;
-		$member = DataObject::get_one('Member', array(
-			"\"$identifierField\"" => $data[$identifierField]
-		));
 
-		// if we are in a complex table field popup, use ctf[childID], else use ID
+		// Only validate identifier field if it's actually set. This could be the case if
-		if(isset($_REQUEST['ctf']['childID'])) {
+		// somebody removes `Email` from the list of required fields.
-			$id = $_REQUEST['ctf']['childID'];
+		if(isset($data[$identifierField])){
-		} elseif(isset($_REQUEST['ID'])) {
+			$id = isset($data['ID']) ? (int)$data['ID'] : 0;
-			$id = $_REQUEST['ID'];
+			if(!$id && ($ctrl = $this->form->getController())){
-		} else {
+				// get the record when within GridField (Member editing page in CMS)
-			$id = null;
+				if($ctrl instanceof GridFieldDetailForm_ItemRequest && $record = $ctrl->getRecord()){
-		}
+					$id = $record->ID;
-
-		if($id && is_object($member) && $member->ID != $id) {
-			$uniqueField = $this->form->Fields()->dataFieldByName($identifierField);
-			$this->validationError(
-				$uniqueField->id(),
-				_t(
-					'Member.VALIDATIONMEMBEREXISTS',
-					'A member already exists with the same %s',
-					array('identifier' => strtolower($identifierField))
-				),
-				'required'
-			);
-			$valid = false;
-		}
-
-		// Execute the validators on the extensions
-		if($this->extension_instances) {
-			foreach($this->extension_instances as $extension) {
-				if(method_exists($extension, 'hasMethod') && $extension->hasMethod('updatePHP')) {
-					$valid &= $extension->updatePHP($data, $this->form);
 				}
 			}
+
+			// If there's no ID passed via controller or form-data, use the assigned member (if available)
+			if(!$id && ($member = $this->getForMember())){
+				$id = $member->exists() ? $member->ID : 0;
+			}
+
+			// set the found ID to the data array, so that extensions can also use it
+			$data['ID'] = $id;
+
+			$members = Member::get()->filter($identifierField, $data[$identifierField]);
+			if($id) {
+				$members = $members->exclude('ID', $id);
+			}
+
+			if($members->count() > 0) {
+				$this->validationError(
+					$identifierField,
+					_t(
+						'Member.VALIDATIONMEMBEREXISTS',
+						'A member already exists with the same {identifier}',
+						array('identifier' => Member::singleton()->fieldLabel($identifierField))
+					),
+					'required'
+				);
+				$valid = false;
+			}
 		}
 
-		return $valid;
-	}
 
+		// Execute the validators on the extensions
+		$results = $this->extend('updatePHP', $data, $this->form);
+		$results[] = $valid;
+		return min($results);
+	}
 }

tests/security/MemberTest.php

@@ -842,7 +842,135 @@ class MemberTest extends FunctionalTest {
 		}
 	}
 
-	public function testCustomMemberValidator() {
+	public function testMemberValidator()
+	{
+		// clear custom requirements for this test
+		Config::inst()->update('Member_Validator', 'customRequired', null);
+		$memberA = $this->objFromFixture('Member', 'admin');
+		$memberB = $this->objFromFixture('Member', 'test');
+
+		// create a blank form
+		$form = new MemberTest_ValidatorForm();
+
+		$validator = new Member_Validator();
+		$validator->setForm($form);
+
+		// Simulate creation of a new member via form, but use an existing member identifier
+		$fail = $validator->php(array(
+			'FirstName' => 'Test',
+			'Email' => $memberA->Email
+		));
+
+		$this->assertFalse(
+			$fail,
+			'Member_Validator must fail when trying to create new Member with existing Email.'
+		);
+
+		// populate the form with values from another member
+		$form->loadDataFrom($memberB);
+
+		// Assign the validator to an existing member
+		// (this is basically the same as passing the member ID with the form data)
+		$validator->setForMember($memberB);
+
+		// Simulate update of a member via form and use an existing member Email
+		$fail = $validator->php(array(
+			'FirstName' => 'Test',
+			'Email' => $memberA->Email
+		));
+
+		// Simulate update to a new Email address
+		$pass1 = $validator->php(array(
+			'FirstName' => 'Test',
+			'Email' => 'membervalidatortest@testing.com'
+		));
+
+		// Pass in the same Email address that the member already has. Ensure that case is valid
+		$pass2 = $validator->php(array(
+			'FirstName' => 'Test',
+			'Surname' => 'User',
+			'Email' => $memberB->Email
+		));
+
+		$this->assertFalse(
+			$fail,
+			'Member_Validator must fail when trying to update existing member with existing Email.'
+		);
+
+		$this->assertTrue(
+			$pass1,
+			'Member_Validator must pass when Email is updated to a value that\'s not in use.'
+		);
+
+		$this->assertTrue(
+			$pass2,
+			'Member_Validator must pass when Member updates his own Email to the already existing value.'
+		);
+	}
+
+	public function testMemberValidatorWithExtensions()
+	{
+		// clear custom requirements for this test
+		Config::inst()->update('Member_Validator', 'customRequired', null);
+
+		// create a blank form
+		$form = new MemberTest_ValidatorForm();
+
+		// Test extensions
+		Member_Validator::add_extension('MemberTest_MemberValidator_SurnameMustMatchFirstNameExtension');
+		$validator = new Member_Validator();
+		$validator->setForm($form);
+
+		// This test should fail, since the extension enforces FirstName == Surname
+		$fail = $validator->php(array(
+			'FirstName' => 'Test',
+			'Surname' => 'User',
+			'Email' => 'test-member-validator-extension@testing.com'
+		));
+
+		$pass = $validator->php(array(
+			'FirstName' => 'Test',
+			'Surname' => 'Test',
+			'Email' => 'test-member-validator-extension@testing.com'
+		));
+
+		$this->assertFalse(
+			$fail,
+			'Member_Validator must fail because of added extension.'
+		);
+
+		$this->assertTrue(
+			$pass,
+			'Member_Validator must succeed, since it meets all requirements.'
+		);
+
+		// Add another extension that always fails. This ensures that all extensions are considered in the validation
+		Member_Validator::add_extension('MemberTest_MemberValidator_AlwaysFailsExtension');
+		$validator = new Member_Validator();
+		$validator->setForm($form);
+
+		// Even though the data is valid, This test should still fail, since one extension always returns false
+		$fail = $validator->php(array(
+			'FirstName' => 'Test',
+			'Surname' => 'Test',
+			'Email' => 'test-member-validator-extension@testing.com'
+		));
+
+		$this->assertFalse(
+			$fail,
+			'Member_Validator must fail because of added extensions.'
+		);
+
+		// Remove added extensions
+		Member_Validator::remove_extension('MemberTest_MemberValidator_AlwaysFailsExtension');
+		Member_Validator::remove_extension('MemberTest_MemberValidator_SurnameMustMatchFirstNameExtension');
+	}
+
+	public function testCustomMemberValidator()
+	{
+		// clear custom requirements for this test
+		Config::inst()->update('Member_Validator', 'customRequired', null);
+
 		$member = $this->objFromFixture('Member', 'admin');
 
 		$form = new MemberTest_ValidatorForm();
@@ -861,7 +989,7 @@ class MemberTest extends FunctionalTest {
 			'Surname' => ''
 		));
 
-		$this->assertTrue($pass, 'Validator requires on FirstName and Email');
+		$this->assertTrue($pass, 'Validator requires a FirstName and Email');
 		$this->assertFalse($fail, 'Missing FirstName');
 
 		$ext = new MemberTest_ValidatorExtension();
@@ -919,6 +1047,30 @@ class MemberTest_ValidatorExtension extends DataExtension implements TestOnly {
 	}
 }
 
+/**
+ * Extension that adds additional validation criteria
+ * @package framework
+ * @subpackage tests
+ */
+class MemberTest_MemberValidator_SurnameMustMatchFirstNameExtension extends DataExtension implements TestOnly
+{
+	public function updatePHP($data, $form) {
+		return $data['FirstName'] == $data['Surname'];
+	}
+}
+
+/**
+ * Extension that adds additional validation criteria
+ * @package framework
+ * @subpackage tests
+ */
+class MemberTest_MemberValidator_AlwaysFailsExtension extends DataExtension implements TestOnly
+{
+	public function updatePHP($data, $form) {
+		return false;
+	}
+}
+
 /**
  * @package framework
  * @subpackage tests