Drive CLI-based revision edits through "differential.revision.edit" API + EditEngine

Summary:
Ref T11114. This creates `differential.revision.edit` (a modern, v3 API method) and redefines the existing methods in terms of it.

Both `differential.createrevision` and `differential.updaterevision` are now internally implemented by building a `differential.revision.edit` API call and then executing it.

I //think// this covers everything except custom fields, which need some tweaking to work with EditEngine. I'll clean that up in the next change.

Test Plan:
  - Created, updated, and edited revisions via `arc`.
  - Called APIs manually via test console.
  - Stored custom fields ("Blame Rev", "Revert Plan") aren't exposed yet.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

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

src/__phutil_library_map__.php

@@ -527,6 +527,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionControlSystem' => 'applications/differential/constants/DifferentialRevisionControlSystem.php',
     'DifferentialRevisionDependedOnByRevisionEdgeType' => 'applications/differential/edge/DifferentialRevisionDependedOnByRevisionEdgeType.php',
     'DifferentialRevisionDependsOnRevisionEdgeType' => 'applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php',
+    'DifferentialRevisionEditConduitAPIMethod' => 'applications/differential/conduit/DifferentialRevisionEditConduitAPIMethod.php',
     'DifferentialRevisionEditEngine' => 'applications/differential/editor/DifferentialRevisionEditEngine.php',
     'DifferentialRevisionEditProController' => 'applications/differential/controller/DifferentialRevisionEditProController.php',
     'DifferentialRevisionFulltextEngine' => 'applications/differential/search/DifferentialRevisionFulltextEngine.php',
@@ -5203,6 +5204,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionControlSystem' => 'Phobject',
     'DifferentialRevisionDependedOnByRevisionEdgeType' => 'PhabricatorEdgeType',
     'DifferentialRevisionDependsOnRevisionEdgeType' => 'PhabricatorEdgeType',
+    'DifferentialRevisionEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod',
     'DifferentialRevisionEditEngine' => 'PhabricatorEditEngine',
     'DifferentialRevisionEditProController' => 'DifferentialController',
     'DifferentialRevisionFulltextEngine' => 'PhabricatorFulltextEngine',

src/applications/differential/conduit/DifferentialConduitAPIMethod.php

@@ -53,21 +53,16 @@ abstract class DifferentialConduitAPIMethod extends ConduitAPIMethod {
 
     $viewer = $request->getUser();
 
-    $field_list = PhabricatorCustomField::getObjectFields(
+    // We're going to build the body of a "differential.revision.edit" API
-      $revision,
+    // request, then just call that code directly.
-      DifferentialCustomField::ROLE_COMMITMESSAGEEDIT);
-
-    $field_list
-      ->setViewer($viewer)
-      ->readFieldsFromStorage($revision);
-    $field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit');
 
     $xactions = array();
+    $xactions[] = array(
+      'type' => DifferentialRevisionEditEngine::KEY_UPDATE,
+      'value' => $diff->getPHID(),
+    );
 
-    $xactions[] = id(new DifferentialTransaction())
+    $field_map = DifferentialCommitMessageField::newEnabledFields($viewer);
-      ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
-      ->setNewValue($diff->getPHID());
-
     $values = $request->getValue('fields', array());
     foreach ($values as $key => $value) {
       $field = idx($field_map, $key);
@@ -79,53 +74,20 @@ abstract class DifferentialConduitAPIMethod extends ConduitAPIMethod {
         continue;
       }
 
-      $role = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS;
-      if (!$field->shouldEnableForRole($role)) {
-        continue;
-      }
-
-      // TODO: This is fairly similar to PhabricatorCustomField's
-      // buildFieldTransactionsFromRequest() method, but that's currently not
-      // easy to reuse.
-
-      $transaction_type = $field->getApplicationTransactionType();
-      $xaction = id(new DifferentialTransaction())
-        ->setTransactionType($transaction_type);
-
-      if ($transaction_type == PhabricatorTransactions::TYPE_CUSTOMFIELD) {
-        // For TYPE_CUSTOMFIELD transactions only, we provide the old value
-        // as an input.
-        $old_value = $field->getOldValueForApplicationTransactions();
-        $xaction->setOldValue($old_value);
-      }
-
       // The transaction itself will be validated so this is somewhat
       // redundant, but this validator will sometimes give us a better error
       // message or a better reaction to a bad value type.
-      $field->validateCommitMessageValue($value);
+      $value = $field->readFieldValueFromConduit($value);
-      $field->readValueFromCommitMessage($value);
 
-      $xaction
+      foreach ($field->getFieldTransactions($value) as $xaction) {
-        ->setNewValue($field->getNewValueForApplicationTransactions());
+        $xactions[] = $xaction;
-
-      if ($transaction_type == PhabricatorTransactions::TYPE_CUSTOMFIELD) {
-        // For TYPE_CUSTOMFIELD transactions, add the field key in metadata.
-        $xaction->setMetadataValue('customfield:key', $field->getFieldKey());
       }
-
-      $metadata = $field->getApplicationTransactionMetadata();
-      foreach ($metadata as $meta_key => $meta_value) {
-        $xaction->setMetadataValue($meta_key, $meta_value);
-      }
-
-      $xactions[] = $xaction;
     }
 
     $message = $request->getValue('message');
     if (strlen($message)) {
-      // This is a little awkward, and should maybe move inside the transaction
+      // This is a little awkward, and should move elsewhere or be removed. It
-      // editor. It largely exists for legacy reasons. See some discussion in
+      // largely exists for legacy reasons. See some discussion in T7899.
-      // T7899.
       $first_line = head(phutil_split_lines($message, false));
 
       $first_line = id(new PhutilUTF8StringTruncator())
@@ -136,20 +98,24 @@ abstract class DifferentialConduitAPIMethod extends ConduitAPIMethod {
       $diff->setDescription($first_line);
       $diff->save();
 
-      $xactions[] = id(new DifferentialTransaction())
+      $xactions[] = array(
-        ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
+        'type' => PhabricatorCommentEditEngineExtension::EDITKEY,
-        ->attachComment(
+        'value' => $message,
-          id(new DifferentialTransactionComment())
+      );
-            ->setContent($message));
     }
 
-    $editor = id(new DifferentialTransactionEditor())
+    $method = 'differential.revision.edit';
-      ->setActor($viewer)
+    $params = array(
-      ->setContentSource($request->newContentSource())
+      'transactions' => $xactions,
-      ->setContinueOnNoEffect(true)
+    );
-      ->setContinueOnMissingFields(true);
 
-    $editor->applyTransactions($revision, $xactions);
+    if ($revision->getID()) {
+      $params['objectIdentifier'] = $revision->getID();
+    }
+
+    return id(new ConduitCall($method, $params, $strict = true))
+      ->setUser($viewer)
+      ->execute();
   }
 
   protected function loadCustomFieldsForRevisions(

src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php

@@ -45,16 +45,18 @@ final class DifferentialCreateRevisionConduitAPIMethod
     $revision = DifferentialRevision::initializeNewRevision($viewer);
     $revision->attachReviewerStatus(array());
 
-    $this->applyFieldEdit(
+    $result = $this->applyFieldEdit(
       $request,
       $revision,
       $diff,
       $request->getValue('fields', array()),
       $message = null);
 
+    $revision_id = $result['object']['id'];
+
     return array(
-      'revisionid'  => $revision->getID(),
+      'revisionid'  => $revision_id,
-      'uri'         => PhabricatorEnv::getURI('/D'.$revision->getID()),
+      'uri'         => PhabricatorEnv::getURI('/D'.$revision_id),
     );
   }
 

src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php

@@ -49,9 +49,15 @@ final class DifferentialGetCommitMessageConduitAPIMethod
       $revision = DifferentialRevision::initializeNewRevision($viewer);
     }
 
+    // There are three modes here: "edit", "create", and "read" (which has
+    // no value for the "edit" parameter).
+
+    // In edit or create mode, we hide read-only fields. In create mode, we
+    // show "Field:" templates for some fields even if they are empty.
     $edit_mode = $request->getValue('edit');
+
+    $is_any_edit = (bool)strlen($edit_mode);
     $is_create = ($edit_mode == 'create');
-    $is_edit = ($edit_mode && !$is_create);
 
     $field_list = DifferentialCommitMessageField::newEnabledFields($viewer);
 
@@ -62,7 +68,7 @@ final class DifferentialGetCommitMessageConduitAPIMethod
 
     // If we're editing the message, remove fields like "Conflicts" and
     // "git-svn-id" which should not be presented to the user for editing.
-    if ($is_edit) {
+    if ($is_any_edit) {
       foreach ($field_list as $field_key => $field) {
         if (!$field->isFieldEditable()) {
           unset($field_list[$field_key]);
@@ -96,7 +102,7 @@ final class DifferentialGetCommitMessageConduitAPIMethod
       $wire_value = $value_map[$field_key];
       $value = $field->renderFieldValue($wire_value);
 
-      $is_template = ($is_edit && $field->isTemplateField());
+      $is_template = ($is_create && $field->isTemplateField());
 
       if (!is_string($value) && !is_null($value)) {
         throw new Exception(

src/applications/differential/conduit/DifferentialRevisionEditConduitAPIMethod.php

@@ -0,0 +1,18 @@
+<?php
+
+final class DifferentialRevisionEditConduitAPIMethod
+  extends PhabricatorEditEngineAPIMethod {
+
+  public function getAPIMethodName() {
+    return 'differential.revision.edit';
+  }
+
+  public function newEditEngine() {
+    return new DifferentialRevisionEditEngine();
+  }
+
+  public function getMethodSummary() {
+    return pht('Apply transactions to create or update a revision.');
+  }
+
+}

src/applications/differential/editor/DifferentialRevisionEditEngine.php

@@ -7,6 +7,8 @@ final class DifferentialRevisionEditEngine
 
   const ENGINECONST = 'differential.revision';
 
+  const KEY_UPDATE = 'update';
+
   public function getEngineName() {
     return pht('Revisions');
   }
@@ -100,21 +102,20 @@ final class DifferentialRevisionEditEngine
 
     $fields = array();
 
-    if ($diff || $is_create) {
+    $fields[] = id(new PhabricatorHandlesEditField())
-      $fields[] = id(new PhabricatorHandlesEditField())
+      ->setKey(self::KEY_UPDATE)
-        ->setKey('update')
+      ->setLabel(pht('Update Diff'))
-        ->setLabel(pht('Update Diff'))
+      ->setDescription(pht('New diff to create or update the revision with.'))
-        ->setDescription(pht('New diff to create or update the revision with.'))
+      ->setConduitDescription(pht('Create or update a revision with a diff.'))
-        ->setConduitDescription(pht('Create or update a revision with a diff.'))
+      ->setConduitTypeDescription(pht('PHID of the diff.'))
-        ->setConduitTypeDescription(pht('PHID of the diff.'))
+      ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
-        ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
+      ->setHandleParameterType(new AphrontPHIDListHTTPParameterType())
-        ->setHandleParameterType(new AphrontPHIDListHTTPParameterType())
+      ->setSingleValue($diff_phid)
-        ->setSingleValue($diff_phid)
+      ->setIsConduitOnly(!$diff)
-        ->setIsReorderable(false)
+      ->setIsReorderable(false)
-        ->setIsDefaultable(false)
+      ->setIsDefaultable(false)
-        ->setIsInvisible(true)
+      ->setIsInvisible(true)
-        ->setIsLockable(false);
+      ->setIsLockable(false);
-    }
 
     if ($is_update) {
       $fields[] = id(new PhabricatorInstructionsEditField())
@@ -134,7 +135,7 @@ final class DifferentialRevisionEditEngine
     }
 
     $fields[] = id(new PhabricatorTextEditField())
-      ->setKey('title')
+      ->setKey(DifferentialRevisionTitleTransaction::EDITKEY)
       ->setLabel(pht('Title'))
       ->setIsRequired(true)
       ->setTransactionType(
@@ -145,7 +146,7 @@ final class DifferentialRevisionEditEngine
       ->setValue($object->getTitle());
 
     $fields[] = id(new PhabricatorRemarkupEditField())
-      ->setKey('summary')
+      ->setKey(DifferentialRevisionSummaryTransaction::EDITKEY)
       ->setLabel(pht('Summary'))
       ->setTransactionType(
         DifferentialRevisionSummaryTransaction::TRANSACTIONTYPE)
@@ -156,7 +157,7 @@ final class DifferentialRevisionEditEngine
 
     if ($plan_enabled) {
       $fields[] = id(new PhabricatorRemarkupEditField())
-        ->setKey('testPlan')
+        ->setKey(DifferentialRevisionTestPlanTransaction::EDITKEY)
         ->setLabel(pht('Test Plan'))
         ->setIsRequired($plan_required)
         ->setTransactionType(
@@ -169,7 +170,7 @@ final class DifferentialRevisionEditEngine
     }
 
     $fields[] = id(new PhabricatorDatasourceEditField())
-      ->setKey('reviewerPHIDs')
+      ->setKey(DifferentialRevisionReviewersTransaction::EDITKEY)
       ->setLabel(pht('Reviewers'))
       ->setDatasource(new DifferentialReviewerDatasource())
       ->setUseEdgeTransactions(true)

src/applications/differential/field/DifferentialCommitMessageCustomField.php

@@ -60,4 +60,9 @@ abstract class DifferentialCommitMessageCustomField
     return $idx;
   }
 
+  public function getFieldTransactions($value) {
+    // TODO: Implement this!
+    return array();
+  }
+
 }

src/applications/differential/field/DifferentialCommitMessageField.php

@@ -67,6 +67,13 @@ abstract class DifferentialCommitMessageField
     return $value;
   }
 
+  public function getFieldTransactions($value) {
+    if (!$this->isFieldEditable()) {
+      return array();
+    }
+    throw new PhutilMethodNotImplementedException();
+  }
+
   final public function getCommitMessageFieldKey() {
     return $this->getPhobjectClassConstant('FIELDKEY', 64);
   }

src/applications/differential/field/DifferentialConflictsCommitMessageField.php

@@ -17,4 +17,8 @@ final class DifferentialConflictsCommitMessageField
     return false;
   }
 
+  public function isTemplateField() {
+    return false;
+  }
+
 }

src/applications/differential/field/DifferentialGitSVNIDCommitMessageField.php

@@ -17,4 +17,8 @@ final class DifferentialGitSVNIDCommitMessageField
     return false;
   }
 
+  public function isTemplateField() {
+    return false;
+  }
+
 }

src/applications/differential/field/DifferentialReviewedByCommitMessageField.php

@@ -27,6 +27,10 @@ final class DifferentialReviewedByCommitMessageField
     return false;
   }
 
+  public function isTemplateField() {
+    return false;
+  }
+
   public function readFieldValueFromObject(DifferentialRevision $revision) {
     if (!$revision->getPHID()) {
       return array();

src/applications/differential/field/DifferentialReviewersCommitMessageField.php

@@ -77,6 +77,30 @@ final class DifferentialReviewersCommitMessageField
     return $this->renderHandleList($phid_list, $suffix_map);
   }
 
+  public function getFieldTransactions($value) {
+    $value = $this->inflateReviewers($value);
+
+    $reviewer_list = array();
+    foreach ($value as $reviewer) {
+      $phid = $reviewer['phid'];
+      if (isset($reviewer['suffixes']['!'])) {
+        $reviewer_list[] = 'blocking('.$phid.')';
+      } else {
+        $reviewer_list[] = $phid;
+      }
+    }
+
+    $xaction_key = DifferentialRevisionReviewersTransaction::EDITKEY;
+    $xaction_type = "{$xaction_key}.set";
+
+    return array(
+      array(
+        'type' => $xaction_type,
+        'value' => $reviewer_list,
+      ),
+    );
+  }
+
   private function flattenReviewers(array $values) {
     // NOTE: For now, `arc` relies on this field returning only scalars, so we
     // need to reduce the results into scalars. See T10981.

src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php

@@ -76,4 +76,8 @@ final class DifferentialRevisionIDCommitMessageField
     return PhabricatorEnv::getProductionURI('/D'.$value);
   }
 
+  public function getFieldTransactions($value) {
+    return array();
+  }
+
 }

src/applications/differential/field/DifferentialSubscribersCommitMessageField.php

@@ -48,4 +48,13 @@ final class DifferentialSubscribersCommitMessageField
     return $this->renderHandleList($value);
   }
 
+  public function getFieldTransactions($value) {
+    return array(
+      array(
+        'type' => PhabricatorSubscriptionsEditEngineExtension::EDITKEY_SET,
+        'value' => $value,
+      ),
+    );
+  }
+
 }

src/applications/differential/field/DifferentialSummaryCommitMessageField.php

@@ -17,4 +17,13 @@ final class DifferentialSummaryCommitMessageField
     return $revision->getSummary();
   }
 
+  public function getFieldTransactions($value) {
+    return array(
+      array(
+        'type' => DifferentialRevisionSummaryTransaction::EDITKEY,
+        'value' => $value,
+      ),
+    );
+  }
+
 }

src/applications/differential/field/DifferentialTagsCommitMessageField.php

@@ -54,5 +54,13 @@ final class DifferentialTagsCommitMessageField
     return $this->renderHandleList($value);
   }
 
+  public function getFieldTransactions($value) {
+    return array(
+      array(
+        'type' => PhabricatorProjectsEditEngineExtension::EDITKEY_SET,
+        'value' => $value,
+      ),
+    );
+  }
 
 }

src/applications/differential/field/DifferentialTasksCommitMessageField.php

@@ -54,4 +54,8 @@ final class DifferentialTasksCommitMessageField
     return $this->renderHandleList($value);
   }
 
+  public function getFieldTransactions($value) {
+    // TODO: Implement this!
+    return array();
+  }
 }

src/applications/differential/field/DifferentialTestPlanCommitMessageField.php

@@ -41,4 +41,13 @@ final class DifferentialTestPlanCommitMessageField
     return $revision->getTestPlan();
   }
 
+  public function getFieldTransactions($value) {
+    return array(
+      array(
+        'type' => DifferentialRevisionTestPlanTransaction::EDITKEY,
+        'value' => $value,
+      ),
+    );
+  }
+
 }

src/applications/differential/field/DifferentialTitleCommitMessageField.php

@@ -47,4 +47,13 @@ final class DifferentialTitleCommitMessageField
     return $value;
   }
 
+  public function getFieldTransactions($value) {
+    return array(
+      array(
+        'type' => DifferentialRevisionTitleTransaction::EDITKEY,
+        'value' => $value,
+      ),
+    );
+  }
+
 }

src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php

@@ -4,6 +4,7 @@ final class DifferentialRevisionReviewersTransaction
   extends DifferentialRevisionTransactionType {
 
   const TRANSACTIONTYPE = 'differential.revision.reviewers';
+  const EDITKEY = 'reviewers';
 
   public function generateOldValue($object) {
     $reviewers = $object->getReviewerStatus();
@@ -18,6 +19,7 @@ final class DifferentialRevisionReviewersTransaction
       ->setViewer($actor);
 
     $reviewers = $this->generateOldValue($object);
+    $old_reviewers = $reviewers;
 
     // First, remove any reviewers we're getting rid of.
     $rem = idx($value, '-', array());
@@ -63,7 +65,7 @@ final class DifferentialRevisionReviewersTransaction
 
     $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING;
     foreach ($add_map as $phid => $new_status) {
-      $old_status = idx($reviewers, $phid);
+      $old_status = idx($old_reviewers, $phid);
 
       // If we have an old status and this didn't make the reviewer blocking
       // or nonblocking, just retain the old status. This makes sure we don't
@@ -76,6 +78,7 @@ final class DifferentialRevisionReviewersTransaction
         $is_unblock = (!$now_blocking && $was_blocking);
 
         if (!$is_block && !$is_unblock) {
+          $reviewers[$phid] = $old_status;
           continue;
         }
       }
@@ -86,6 +89,14 @@ final class DifferentialRevisionReviewersTransaction
     return $reviewers;
   }
 
+  public function getTransactionHasEffect($object, $old, $new) {
+    // At least for now, we ignore transactions which ONLY reorder reviewers
+    // without making any actual changes.
+    ksort($old);
+    ksort($new);
+    return ($old !== $new);
+  }
+
   public function applyExternalEffects($object, $value) {
     $src_phid = $object->getPHID();
 

src/applications/differential/xaction/DifferentialRevisionSummaryTransaction.php

@@ -4,6 +4,7 @@ final class DifferentialRevisionSummaryTransaction
   extends DifferentialRevisionTransactionType {
 
   const TRANSACTIONTYPE = 'differential.revision.summary';
+  const EDITKEY = 'summary';
 
   public function generateOldValue($object) {
     return $object->getSummary();

src/applications/differential/xaction/DifferentialRevisionTestPlanTransaction.php

@@ -4,6 +4,7 @@ final class DifferentialRevisionTestPlanTransaction
   extends DifferentialRevisionTransactionType {
 
   const TRANSACTIONTYPE = 'differential.revision.testplan';
+  const EDITKEY = 'testPlan';
 
   public function generateOldValue($object) {
     return $object->getTestPlan();

src/applications/differential/xaction/DifferentialRevisionTitleTransaction.php

@@ -4,6 +4,7 @@ final class DifferentialRevisionTitleTransaction
   extends DifferentialRevisionTransactionType {
 
   const TRANSACTIONTYPE = 'differential.revision.title';
+  const EDITKEY = 'title';
 
   public function generateOldValue($object) {
     return $object->getTitle();

src/applications/project/engineextension/PhabricatorProjectsEditEngineExtension.php

@@ -5,6 +5,10 @@ final class PhabricatorProjectsEditEngineExtension
 
   const EXTENSIONKEY = 'projects.projects';
 
+  const EDITKEY_ADD = 'projects.add';
+  const EDITKEY_SET = 'projects.set';
+  const EDITKEY_REMOVE = 'projects.remove';
+
   public function getExtensionPriority() {
     return 500;
   }
@@ -58,14 +62,14 @@ final class PhabricatorProjectsEditEngineExtension
 
     $projects_field->setViewer($engine->getViewer());
 
-    $edit_add = $projects_field->getConduitEditType('projects.add')
+    $edit_add = $projects_field->getConduitEditType(self::EDITKEY_ADD)
       ->setConduitDescription(pht('Add project tags.'));
 
-    $edit_set = $projects_field->getConduitEditType('projects.set')
+    $edit_set = $projects_field->getConduitEditType(self::EDITKEY_SET)
       ->setConduitDescription(
         pht('Set project tags, overwriting current value.'));
 
-    $edit_rem = $projects_field->getConduitEditType('projects.remove')
+    $edit_rem = $projects_field->getConduitEditType(self::EDITKEY_REMOVE)
       ->setConduitDescription(pht('Remove project tags.'));
 
     return array(

src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php

@@ -5,6 +5,10 @@ final class PhabricatorSubscriptionsEditEngineExtension
 
   const EXTENSIONKEY = 'subscriptions.subscribers';
 
+  const EDITKEY_ADD = 'subscribers.add';
+  const EDITKEY_SET = 'subscribers.set';
+  const EDITKEY_REMOVE = 'subscribers.remove';
+
   public function getExtensionPriority() {
     return 750;
   }
@@ -52,14 +56,14 @@ final class PhabricatorSubscriptionsEditEngineExtension
 
     $subscribers_field->setViewer($engine->getViewer());
 
-    $edit_add = $subscribers_field->getConduitEditType('subscribers.add')
+    $edit_add = $subscribers_field->getConduitEditType(self::EDITKEY_ADD)
       ->setConduitDescription(pht('Add subscribers.'));
 
-    $edit_set = $subscribers_field->getConduitEditType('subscribers.set')
+    $edit_set = $subscribers_field->getConduitEditType(self::EDITKEY_SET)
       ->setConduitDescription(
         pht('Set subscribers, overwriting current value.'));
 
-    $edit_rem = $subscribers_field->getConduitEditType('subscribers.remove')
+    $edit_rem = $subscribers_field->getConduitEditType(self::EDITKEY_REMOVE)
       ->setConduitDescription(pht('Remove subscribers.'));
 
     return array(

src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php

@@ -4,6 +4,7 @@ final class PhabricatorCommentEditEngineExtension
   extends PhabricatorEditEngineExtension {
 
   const EXTENSIONKEY = 'transactions.comment';
+  const EDITKEY = 'comment';
 
   public function getExtensionPriority() {
     return 9000;
@@ -39,7 +40,7 @@ final class PhabricatorCommentEditEngineExtension
     $comment_type = PhabricatorTransactions::TYPE_COMMENT;
 
     $comment_field = id(new PhabricatorCommentEditField())
-      ->setKey('comment')
+      ->setKey(self::EDITKEY)
       ->setLabel(pht('Comments'))
       ->setAliases(array('comments'))
       ->setIsHidden(true)

src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php

@@ -396,13 +396,16 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject {
       if (!self::isFunctionToken($token)) {
         $results[] = $token;
       } else {
-        $evaluate[] = $token;
+        // Put a placeholder in the result list so that we retain token order
+        // when possible. We'll overwrite this below.
+        $results[] = null;
+        $evaluate[last_key($results)] = $token;
       }
     }
 
     $results = $this->evaluateValues($results);
 
-    foreach ($evaluate as $function) {
+    foreach ($evaluate as $result_key => $function) {
       $function = self::parseFunction($function);
       if (!$function) {
         throw new PhabricatorTypeaheadInvalidTokenException();
@@ -411,11 +414,23 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject {
       $name = $function['name'];
       $argv = $function['argv'];
 
-      foreach ($this->evaluateFunction($name, array($argv)) as $phid) {
+      $evaluated_tokens = $this->evaluateFunction($name, array($argv));
-        $results[] = $phid;
+      if (!$evaluated_tokens) {
+        unset($results[$result_key]);
+      } else {
+        $is_first = true;
+        foreach ($evaluated_tokens as $phid) {
+          if ($is_first) {
+            $results[$result_key] = $phid;
+            $is_first = false;
+          } else {
+            $results[] = $phid;
+          }
+        }
       }
     }
 
+    $results = array_values($results);
     $results = $this->didEvaluateTokens($results);
 
     return $results;