Simplify and improve PhabricatorCustomField APIs

Summary:
Ref T1703. Ref T3718. The `PhabricatorCustomFieldList` seems like a pretty good idea. Move more code into it to make it harder to get wrong.

Also the sequencing on old/new values for these transactions was a bit off; fix that up.

Test Plan: Edited standard and custom profile fields.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1703, T3718

Differential Revision: https://secure.phabricator.com/D6751

src/applications/people/controller/PhabricatorPeopleProfileController.php

@@ -97,10 +97,9 @@ final class PhabricatorPeopleProfileController
       ->setUser($viewer)
       ->setObject($user);
 
-    $fields = PhabricatorCustomField::getObjectFields(
+    $field_list = PhabricatorCustomField::getObjectFields(
       $user,
       PhabricatorCustomField::ROLE_VIEW);
-    $field_list = new PhabricatorCustomFieldList($fields);
     $field_list->appendFieldsToPropertyList($user, $viewer, $view);
 
     return $view;

src/applications/people/controller/PhabricatorPeopleProfileEditController.php

@@ -32,20 +32,15 @@ final class PhabricatorPeopleProfileEditController
 
     $profile_uri = '/p/'.$user->getUsername().'/';
 
-    $fields = PhabricatorCustomField::getObjectFields(
+    $field_list = PhabricatorCustomField::getObjectFields(
       $user,
       PhabricatorCustomField::ROLE_EDIT);
-    $field_list = new PhabricatorCustomFieldList($fields);
+    $field_list->readFieldsFromStorage($user);
 
     if ($request->isFormPost()) {
-      $xactions = array();
-      foreach ($fields as $field) {
-        $field->readValueFromRequest($request);
-        $xactions[] = id(new PhabricatorUserTransaction())
-          ->setTransactionType(PhabricatorTransactions::TYPE_CUSTOMFIELD)
-          ->setMetadataValue('customfield:key', $field->getFieldKey())
-          ->setNewValue($field->getNewValueForApplicationTransactions());
-      }
+      $xactions = $field_list->buildFieldTransactionsFromRequest(
+        new PhabricatorUserTransaction(),
+        $request);
 
       $editor = id(new PhabricatorUserProfileEditor())
         ->setActor($viewer)
@@ -56,8 +51,6 @@ final class PhabricatorPeopleProfileEditController
       $editor->applyTransactions($user, $xactions);
 
       return id(new AphrontRedirectResponse())->setURI($profile_uri);
-    } else {
-      $field_list->readFieldsFromStorage($user);
     }
 
     $title = pht('Edit Profile');

src/applications/people/storage/PhabricatorUser.php

@@ -850,8 +850,8 @@ EOBODY;
     return $this->customFields[$role];
   }
 
-  public function attachCustomFields($role, array $fields) {
-    $this->customFields[$role] = $fields;
+  public function attachCustomFields($role, PhabricatorCustomFieldList $list) {
+    $this->customFields[$role] = $list;
     return $this;
   }
 

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -125,8 +125,9 @@ abstract class PhabricatorApplicationTransactionEditor
         }
         return $old_edges;
       case PhabricatorTransactions::TYPE_CUSTOMFIELD:
-        $field = $this->getCustomFieldForTransaction($object, $xaction);
-        return $field->getOldValueForApplicationTransactions();
+        // NOTE: Custom fields have their old value pre-populated when they are
+        // built by PhabricatorCustomFieldList.
+        return $xaction->getOldValue();
       default:
         return $this->getCustomTransactionOldValue($object, $xaction);
     }
@@ -585,9 +586,13 @@ abstract class PhabricatorApplicationTransactionEditor
         throw new Exception(
           "You can not apply transactions which already have commentVersions!");
       }
-      if ($xaction->getOldValue() !== null) {
-        throw new Exception(
-          "You can not apply transactions which already have oldValue!");
+
+      $custom_field_type = PhabricatorTransactions::TYPE_CUSTOMFIELD;
+      if ($xaction->getTransactionType() != $custom_field_type) {
+        if ($xaction->getOldValue() !== null) {
+          throw new Exception(
+            "You can not apply transactions which already have oldValue!");
+        }
       }
 
       $type = $xaction->getTransactionType();

src/infrastructure/customfield/field/PhabricatorCustomField.php

@@ -49,7 +49,7 @@ abstract class PhabricatorCustomField {
     $role) {
 
     try {
-      $fields = $object->getCustomFields($role);
+      $field_list = $object->getCustomFields($role);
     } catch (PhabricatorCustomFieldNotAttachedException $ex) {
       $base_class = $object->getCustomFieldBaseClass();
 
@@ -73,10 +73,11 @@ abstract class PhabricatorCustomField {
         $field->setObject($object);
       }
 
-      $object->attachCustomFields($role, $fields);
+      $field_list = new PhabricatorCustomFieldList($fields);
+      $object->attachCustomFields($role, $field_list);
     }
 
-    return $fields;
+    return $field_list;
   }
 
 
@@ -87,7 +88,7 @@ abstract class PhabricatorCustomField {
     PhabricatorCustomFieldInterface $object,
     $role,
     $field_key) {
-    return idx(self::getObjectFields($object, $role), $field_key);
+    return idx(self::getObjectFields($object, $role)->getFields(), $field_key);
   }
 
 

src/infrastructure/customfield/field/PhabricatorCustomFieldList.php

@@ -16,6 +16,9 @@ final class PhabricatorCustomFieldList extends Phobject {
     $this->fields = $fields;
   }
 
+  public function getFields() {
+    return $this->fields;
+  }
 
   /**
    * Read stored values for all fields which support storage.
@@ -120,4 +123,32 @@ final class PhabricatorCustomFieldList extends Phobject {
     }
   }
 
+  public function buildFieldTransactionsFromRequest(
+    PhabricatorApplicationTransaction $template,
+    AphrontRequest $request) {
+
+    $xactions = array();
+
+    $role = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS;
+    foreach ($this->fields as $field) {
+      if (!$field->shouldEnableForRole($role)) {
+        continue;
+      }
+
+      $old_value = $field->getOldValueForApplicationTransactions();
+
+      $field->readValueFromRequest($request);
+
+      $xaction = id(clone $template)
+        ->setTransactionType(PhabricatorTransactions::TYPE_CUSTOMFIELD)
+        ->setMetadataValue('customfield:key', $field->getFieldKey())
+        ->setOldValue($old_value)
+        ->setNewValue($field->getNewValueForApplicationTransactions());
+
+      $xactions[] = $xaction;
+    }
+
+    return $xactions;
+  }
+
 }

src/infrastructure/customfield/interface/PhabricatorCustomFieldInterface.php

@@ -5,7 +5,7 @@ interface PhabricatorCustomFieldInterface {
   public function getCustomFieldBaseClass();
   public function getCustomFieldSpecificationForRole($role);
   public function getCustomFields($role);
-  public function attachCustomFields($role, array $fields);
+  public function attachCustomFields($role, PhabricatorCustomFieldList $list);
 
 }
 
@@ -33,8 +33,8 @@ interface PhabricatorCustomFieldInterface {
     return $this->customFields[$role];
   }
 
-  public function attachCustomFields($role, array $fields) {
-    $this->customFields[$role] = $fields;
+  public function attachCustomFields($role, PhabricatorCustomFieldList $list) {
+    $this->customFields[$role] = $list;
     return $this;
   }