Merge pull request #2850 from kinglozzer/2827-member-extend

FIX: Rewrite Member getCMSFields to ensure updateCMSFields is only run once (fixes #2827)

security/Member.php

@@ -1202,10 +1202,10 @@ class Member extends DataObject implements TemplateGlobalProvider {
 	 *                   editing this member.
 	 */
 	public function getCMSFields() {
-		require_once('Zend/Date.php');
+		require_once 'Zend/Date.php';
-		
-		$fields = parent::getCMSFields();
 
+		$self = $this;
+		$this->beforeUpdateCMSFields(function($fields) use ($self) {
 			$mainFields = $fields->fieldByName("Root")->fieldByName("Main")->Children;
 
 			$password = new ConfirmedPasswordField(
@@ -1216,7 +1216,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 				true // showOnClick
 			);
 			$password->setCanBeEmpty(true);
-		if(!$this->ID) $password->showOnClick = false;
+			if( ! $self->ID) $password->showOnClick = false;
 			$mainFields->replaceField('Password', $password);
 
 			$mainFields->replaceField('Locale', new DropdownField(
@@ -1232,7 +1232,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 			$mainFields->removeByName('PasswordExpiry');
 			$mainFields->removeByName('LockedOutUntil');
 			
-		if(!self::config()->lock_out_after_incorrect_logins) {
+			if( ! $self->config()->lock_out_after_incorrect_logins) {
 				$mainFields->removeByName('FailedLoginCount');
 			}
 			
@@ -1267,14 +1267,14 @@ 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($this->ID) {
+				if($self->ID) {
 					$permissionsField = new PermissionCheckboxSetField_Readonly(
 						'Permissions',
 						false,
 						'Permission',
 						'GroupID',
 						// we don't want parent relationships, they're automatically resolved in the field
-					$this->getManyManyComponents('Groups')
+						$self->getManyManyComponents('Groups')
 					);
 					$fields->findOrMakeTab('Root.Permissions', singleton('Permission')->i18n_plural_name());
 					$fields->addFieldToTab('Root.Permissions', $permissionsField);
@@ -1284,7 +1284,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 			$permissionsTab = $fields->fieldByName("Root")->fieldByName('Permissions');
 			if($permissionsTab) $permissionsTab->addExtraClass('readonly');
 			
-		$defaultDateFormat = Zend_Locale_Format::getDateFormat(new Zend_Locale($this->Locale));
+			$defaultDateFormat = Zend_Locale_Format::getDateFormat(new Zend_Locale($self->Locale));
 			$dateFormatMap = array(
 				'MMM d, yyyy' => Zend_Date::now()->toString('MMM d, yyyy'),
 				'yyyy/MM/dd' => Zend_Date::now()->toString('yyyy/MM/dd'),
@@ -1296,13 +1296,13 @@ class Member extends DataObject implements TemplateGlobalProvider {
 			$mainFields->push(
 				$dateFormatField = new MemberDatetimeOptionsetField(
 					'DateFormat',
-				$this->fieldLabel('DateFormat'),
+					$self->fieldLabel('DateFormat'),
 					$dateFormatMap
 				)
 			);
-		$dateFormatField->setValue($this->DateFormat);
+			$dateFormatField->setValue($self->DateFormat);
 			
-		$defaultTimeFormat = Zend_Locale_Format::getTimeFormat(new Zend_Locale($this->Locale));
+			$defaultTimeFormat = Zend_Locale_Format::getTimeFormat(new Zend_Locale($self->Locale));
 			$timeFormatMap = array(
 				'h:mm a' => Zend_Date::now()->toString('h:mm a'),
 				'H:mm' => Zend_Date::now()->toString('H:mm'),
@@ -1312,15 +1312,14 @@ class Member extends DataObject implements TemplateGlobalProvider {
 			$mainFields->push(
 				$timeFormatField = new MemberDatetimeOptionsetField(
 					'TimeFormat',
-				$this->fieldLabel('TimeFormat'),
+					$self->fieldLabel('TimeFormat'),
 					$timeFormatMap
 				)
 			);
-		$timeFormatField->setValue($this->TimeFormat);
+			$timeFormatField->setValue($self->TimeFormat);
+		});
 		
-		$this->extend('updateCMSFields', $fields);
+		return parent::getCMSFields();
-		
-		return $fields;
 	}
 	
 	/**

tests/security/MemberTest.php

@@ -614,6 +614,22 @@ class MemberTest extends FunctionalTest {
 		);
 	}
 
+	/**
+	 * Test that extensions using updateCMSFields() are applied correctly
+	 */
+	public function testUpdateCMSFields() {
+		Member::add_extension('MemberTest_FieldsExtension');
+
+		$member = singleton('Member');
+		$fields = $member->getCMSFields();
+
+		$this->assertNotNull($fields->dataFieldByName('Email'), 'Scaffolded fields are retained');
+		$this->assertNull($fields->dataFieldByName('Salt'), 'Field modifications run correctly');
+		$this->assertNotNull($fields->dataFieldByName('TestMemberField'), 'Extension is applied correctly');
+
+		Member::remove_extension('MemberTest_FieldsExtension');
+	}
+	
 	/**
 	 * Test that all members are returned
 	 */
@@ -827,6 +843,18 @@ class MemberTest_ViewingDeniedExtension extends DataExtension implements TestOnl
 	}
 }
 
+/**
+ * @package framework
+ * @subpackage tests
+ */
+class MemberTest_FieldsExtension extends DataExtension implements TestOnly {
+
+	public function updateCMSFields(FieldList $fields) {
+		$fields->addFieldToTab('Root.Main', new TextField('TestMemberField', 'Test'));
+	}
+
+}
+
 /**
  * @package framework
  * @subpackage tests