Improve code structure of PHID fields in EditEngine

Summary: Ref T9132. I had some hacks in place for dealing with Edge/Subscribers stuff. Clean that up so it's structured a little better.

Test Plan:
  - Edited subscribers and projects.
  - Verified things still show up in Conduit.
  - Made concurrent edits (added a project in one window, removed it in another window, got a clean result with a correct merge of the two edits).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132

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

src/__phutil_library_map__.php

@@ -145,6 +145,7 @@ phutil_register_library_map(array(
     'AphrontJavelinView' => 'view/AphrontJavelinView.php',
     'AphrontKeyboardShortcutsAvailableView' => 'view/widget/AphrontKeyboardShortcutsAvailableView.php',
     'AphrontListFilterView' => 'view/layout/AphrontListFilterView.php',
+    'AphrontListHTTPParameterType' => 'aphront/httpparametertype/AphrontListHTTPParameterType.php',
     'AphrontMalformedRequestException' => 'aphront/exception/AphrontMalformedRequestException.php',
     'AphrontMoreView' => 'view/layout/AphrontMoreView.php',
     'AphrontMultiColumnView' => 'view/layout/AphrontMultiColumnView.php',
@@ -2580,6 +2581,7 @@ phutil_register_library_map(array(
     'PhabricatorPHID' => 'applications/phid/storage/PhabricatorPHID.php',
     'PhabricatorPHIDConstants' => 'applications/phid/PhabricatorPHIDConstants.php',
     'PhabricatorPHIDInterface' => 'applications/phid/interface/PhabricatorPHIDInterface.php',
+    'PhabricatorPHIDListEditField' => 'applications/transactions/editfield/PhabricatorPHIDListEditField.php',
     'PhabricatorPHIDResolver' => 'applications/phid/resolver/PhabricatorPHIDResolver.php',
     'PhabricatorPHIDType' => 'applications/phid/type/PhabricatorPHIDType.php',
     'PhabricatorPHIDTypeTestCase' => 'applications/phid/type/__tests__/PhabricatorPHIDTypeTestCase.php',
@@ -3943,18 +3945,19 @@ phutil_register_library_map(array(
     'AphrontJavelinView' => 'AphrontView',
     'AphrontKeyboardShortcutsAvailableView' => 'AphrontView',
     'AphrontListFilterView' => 'AphrontView',
+    'AphrontListHTTPParameterType' => 'AphrontHTTPParameterType',
     'AphrontMalformedRequestException' => 'AphrontException',
     'AphrontMoreView' => 'AphrontView',
     'AphrontMultiColumnView' => 'AphrontView',
     'AphrontMySQLDatabaseConnectionTestCase' => 'PhabricatorTestCase',
     'AphrontNullView' => 'AphrontView',
     'AphrontPHIDHTTPParameterType' => 'AphrontHTTPParameterType',
-    'AphrontPHIDListHTTPParameterType' => 'AphrontHTTPParameterType',
+    'AphrontPHIDListHTTPParameterType' => 'AphrontListHTTPParameterType',
     'AphrontPHPHTTPSink' => 'AphrontHTTPSink',
     'AphrontPageView' => 'AphrontView',
     'AphrontPlainTextResponse' => 'AphrontResponse',
     'AphrontProgressBarView' => 'AphrontBarView',
-    'AphrontProjectListHTTPParameterType' => 'AphrontHTTPParameterType',
+    'AphrontProjectListHTTPParameterType' => 'AphrontListHTTPParameterType',
     'AphrontProxyResponse' => array(
       'AphrontResponse',
       'AphrontResponseProducerInterface',
@@ -3974,13 +3977,13 @@ phutil_register_library_map(array(
     'AphrontStackTraceView' => 'AphrontView',
     'AphrontStandaloneHTMLResponse' => 'AphrontHTMLResponse',
     'AphrontStringHTTPParameterType' => 'AphrontHTTPParameterType',
-    'AphrontStringListHTTPParameterType' => 'AphrontHTTPParameterType',
+    'AphrontStringListHTTPParameterType' => 'AphrontListHTTPParameterType',
     'AphrontTableView' => 'AphrontView',
     'AphrontTagView' => 'AphrontView',
     'AphrontTokenizerTemplateView' => 'AphrontView',
     'AphrontTypeaheadTemplateView' => 'AphrontView',
     'AphrontUnhandledExceptionResponse' => 'AphrontStandaloneHTMLResponse',
-    'AphrontUserListHTTPParameterType' => 'AphrontHTTPParameterType',
+    'AphrontUserListHTTPParameterType' => 'AphrontListHTTPParameterType',
     'AphrontView' => array(
       'Phobject',
       'PhutilSafeHTMLProducerInterface',
@@ -6752,6 +6755,7 @@ phutil_register_library_map(array(
     'PhabricatorPHDConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorPHID' => 'Phobject',
     'PhabricatorPHIDConstants' => 'Phobject',
+    'PhabricatorPHIDListEditField' => 'PhabricatorEditField',
     'PhabricatorPHIDResolver' => 'Phobject',
     'PhabricatorPHIDType' => 'Phobject',
     'PhabricatorPHIDTypeTestCase' => 'PhutilTestCase',
@@ -7437,7 +7441,7 @@ phutil_register_library_map(array(
     'PhabricatorTokenReceiverQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorTokenTokenPHIDType' => 'PhabricatorPHIDType',
     'PhabricatorTokenUIEventListener' => 'PhabricatorEventListener',
-    'PhabricatorTokenizerEditField' => 'PhabricatorEditField',
+    'PhabricatorTokenizerEditField' => 'PhabricatorPHIDListEditField',
     'PhabricatorTokensApplication' => 'PhabricatorApplication',
     'PhabricatorTokensSettingsPanel' => 'PhabricatorSettingsPanel',
     'PhabricatorTooltipUIExample' => 'PhabricatorUIExample',

src/aphront/httpparametertype/AphrontListHTTPParameterType.php

@@ -0,0 +1,10 @@
+<?php
+
+abstract class AphrontListHTTPParameterType
+  extends AphrontHTTPParameterType {
+
+  protected function getParameterDefault() {
+    return array();
+  }
+
+}

src/aphront/httpparametertype/AphrontPHIDListHTTPParameterType.php

@@ -1,7 +1,7 @@
 <?php
 
 final class AphrontPHIDListHTTPParameterType
-  extends AphrontHTTPParameterType {
+  extends AphrontListHTTPParameterType {
 
   protected function getParameterValue(AphrontRequest $request, $key) {
     $type = new AphrontStringListHTTPParameterType();

src/aphront/httpparametertype/AphrontProjectListHTTPParameterType.php

@@ -1,7 +1,7 @@
 <?php
 
 final class AphrontProjectListHTTPParameterType
-  extends AphrontHTTPParameterType {
+  extends AphrontListHTTPParameterType {
 
   protected function getParameterValue(AphrontRequest $request, $key) {
     $type = new AphrontStringListHTTPParameterType();

src/aphront/httpparametertype/AphrontStringListHTTPParameterType.php

@@ -1,7 +1,7 @@
 <?php
 
 final class AphrontStringListHTTPParameterType
-  extends AphrontHTTPParameterType {
+  extends AphrontListHTTPParameterType {
 
   protected function getParameterValue(AphrontRequest $request, $key) {
     $list = $request->getArr($key, null);

src/aphront/httpparametertype/AphrontUserListHTTPParameterType.php

@@ -1,7 +1,7 @@
 <?php
 
 final class AphrontUserListHTTPParameterType
-  extends AphrontHTTPParameterType {
+  extends AphrontListHTTPParameterType {
 
   protected function getParameterValue(AphrontRequest $request, $key) {
     $type = new AphrontStringListHTTPParameterType();

src/applications/project/editor/PhabricatorProjectsEditEngineExtension.php

@@ -48,6 +48,11 @@ final class PhabricatorProjectsEditEngineExtension
       ->setEditTypeKey('projects')
       ->setDescription(pht('Add or remove associated projects.'))
       ->setAliases(array('project', 'projects'))
+      ->setUseEdgeTransactions(true)
+      ->setEdgeTransactionDescriptions(
+        pht('Add projects.'),
+        pht('Remove projects.'),
+        pht('Set associated projects, overwriting current value.'))
       ->setTransactionType($edge_type)
       ->setMetadataValue('edge:type', $project_edge_type)
       ->setValue($project_phids);

src/applications/subscriptions/editor/PhabricatorSubscriptionsEditEngineExtension.php

@@ -45,6 +45,11 @@ final class PhabricatorSubscriptionsEditEngineExtension
       ->setEditTypeKey('subscribers')
       ->setDescription(pht('Manage subscribers.'))
       ->setAliases(array('subscriber', 'subscribers'))
+      ->setUseEdgeTransactions(true)
+      ->setEdgeTransactionDescriptions(
+        pht('Add subscribers.'),
+        pht('Remove subscribers.'),
+        pht('Set subscribers, overwriting current value.'))
       ->setTransactionType($subscribers_type)
       ->setValue($sub_phids);
 

src/applications/transactions/editengine/PhabricatorEditEngine.php

@@ -1048,6 +1048,11 @@ abstract class PhabricatorEditEngine
     $types = array();
     foreach ($fields as $field) {
       $field_types = $field->getEditTransactionTypes();
+
+      if ($field_types === null) {
+        continue;
+      }
+
       foreach ($field_types as $field_type) {
         $field_type->setField($field);
         $types[$field_type->getEditType()] = $field_type;

src/applications/transactions/editfield/PhabricatorEditField.php

@@ -7,6 +7,7 @@ abstract class PhabricatorEditField extends Phobject {
   private $label;
   private $aliases = array();
   private $value;
+  private $initialValue;
   private $hasValue = false;
   private $object;
   private $transactionType;
@@ -262,6 +263,7 @@ abstract class PhabricatorEditField extends Phobject {
 
   public function setValue($value) {
     $this->hasValue = true;
+    $this->initialValue = $value;
     $this->value = $value;
     return $this;
   }
@@ -289,6 +291,10 @@ abstract class PhabricatorEditField extends Phobject {
     return $this;
   }
 
+  public function getMetadata() {
+    return $this->metadata;
+  }
+
   protected function getValueForTransaction() {
     return $this->getValue();
   }
@@ -348,6 +354,31 @@ abstract class PhabricatorEditField extends Phobject {
     return $this->getValueFromSubmit($request, $key);
   }
 
+
+  /**
+   * Read and return the value the object had when the user first loaded the
+   * form.
+   *
+   * This is the initial value from the user's point of view when they started
+   * the edit process, and used primarily to prevent race conditions for fields
+   * like "Projects" and "Subscribers" that use tokenizers and support edge
+   * transactions.
+   *
+   * Most fields do not need to store these values or deal with initial value
+   * handling.
+   *
+   * @param AphrontRequest Request to read from.
+   * @param string Key to read.
+   * @return wild Value read from request.
+   */
+  protected function getInitialValueFromSubmit(AphrontRequest $request, $key) {
+    return null;
+  }
+
+  public function getInitialValue() {
+    return $this->initialValue;
+  }
+
   public function readValueFromSubmit(AphrontRequest $request) {
     $key = $this->getKey();
     if ($this->getValueExistsInSubmit($request, $key)) {
@@ -356,6 +387,10 @@ abstract class PhabricatorEditField extends Phobject {
       $value = $this->getDefaultValue();
     }
     $this->value = $value;
+
+    $initial_value = $this->getInitialValueFromSubmit($request, $key);
+    $this->initialValue = $initial_value;
+
     return $this;
   }
 
@@ -414,66 +449,30 @@ abstract class PhabricatorEditField extends Phobject {
       ->setValueType($this->getHTTPParameterType()->getTypeName());
   }
 
-  public function getEditTransactionTypes() {
+  protected function getEditTransactionType() {
     $transaction_type = $this->getTransactionType();
+
     if ($transaction_type === null) {
-      return array();
+      return null;
     }
 
     $type_key = $this->getEditTypeKey();
 
-    // TODO: This is a pretty big pile of hard-coded hacks for now.
-
-    $edge_types = array(
-      PhabricatorTransactions::TYPE_EDGE => array(
-        '+' => pht('Add projects.'),
-        '-' => pht('Remove projects.'),
-        '=' => pht('Set associated projects, overwriting current value.'),
-      ),
-      PhabricatorTransactions::TYPE_SUBSCRIBERS => array(
-        '+' => pht('Add subscribers.'),
-        '-' => pht('Remove subscribers.'),
-        '=' => pht('Set subscribers, overwriting current value.'),
-      ),
-    );
-
-    if (isset($edge_types[$transaction_type])) {
-      $base = id(new PhabricatorEdgeEditType())
-        ->setTransactionType($transaction_type)
-        ->setMetadata($this->metadata);
-
-      $strings = $edge_types[$transaction_type];
-
-      $add = id(clone $base)
-        ->setEditType($type_key.'.add')
-        ->setEdgeOperation('+')
-        ->setDescription($strings['+'])
-        ->setValueDescription(pht('List of PHIDs to add.'));
-      $rem = id(clone $base)
-        ->setEditType($type_key.'.remove')
-        ->setEdgeOperation('-')
-        ->setDescription($strings['-'])
-        ->setValueDescription(pht('List of PHIDs to remove.'));
-      $set = id(clone $base)
-        ->setEditType($type_key.'.set')
-        ->setEdgeOperation('=')
-        ->setDescription($strings['='])
-        ->setValueDescription(pht('List of PHIDs to set.'));
-
-      return array(
-        $add,
-        $rem,
-        $set,
-      );
-    }
-
-    return array(
-      $this->newEditType()
+    return $this->newEditType()
       ->setEditType($type_key)
       ->setTransactionType($transaction_type)
       ->setDescription($this->getDescription())
-        ->setMetadata($this->metadata),
-    );
+      ->setMetadata($this->getMetadata());
+  }
+
+  public function getEditTransactionTypes() {
+    $edit_type = $this->getEditTransactionType();
+
+    if ($edit_type === null) {
+      return null;
+    }
+
+    return array($edit_type);
   }
 
 }

src/applications/transactions/editfield/PhabricatorPHIDListEditField.php

@@ -0,0 +1,114 @@
+<?php
+
+abstract class PhabricatorPHIDListEditField
+  extends PhabricatorEditField {
+
+  private $useEdgeTransactions;
+  private $transactionDescriptions = array();
+
+  public function setUseEdgeTransactions($use_edge_transactions) {
+    $this->useEdgeTransactions = $use_edge_transactions;
+    return $this;
+  }
+
+  public function getUseEdgeTransactions() {
+    return $this->useEdgeTransactions;
+  }
+
+  public function setEdgeTransactionDescriptions($add, $rem, $set) {
+    $this->transactionDescriptions = array(
+      '+' => $add,
+      '-' => $rem,
+      '=' => $set,
+    );
+    return $this;
+  }
+
+  protected function newHTTPParameterType() {
+    return new AphrontPHIDListHTTPParameterType();
+  }
+
+  protected function getValueForTransaction() {
+    $new = parent::getValueForTransaction();
+
+    if (!$this->getUseEdgeTransactions()) {
+      return $new;
+    }
+
+    $old = $this->getInitialValue();
+    if ($old === null) {
+      return array(
+        '=' => array_fuse($new),
+      );
+    }
+
+    // If we're building an edge transaction and the request has data about the
+    // original value the user saw when they loaded the form, interpret the
+    // edit as a mixture of "+" and "-" operations instead of a single "="
+    // operation. This limits our exposure to race conditions by making most
+    // concurrent edits merge correctly.
+
+    $add = array_diff($new, $old);
+    $rem = array_diff($old, $new);
+
+    $value = array();
+
+    if ($add) {
+      $value['+'] = array_fuse($add);
+    }
+    if ($rem) {
+      $value['-'] = array_fuse($rem);
+    }
+
+    return $value;
+  }
+
+  protected function newEditType() {
+    if ($this->getUseEdgeTransactions()) {
+      return new PhabricatorEdgeEditType();
+    }
+
+    return parent::newEditType();
+  }
+
+  public function getEditTransactionTypes() {
+    if (!$this->getUseEdgeTransactions()) {
+      return parent::getEditTransactionTypes();
+    }
+
+    $transaction_type = $this->getTransactionType();
+    if ($transaction_type === null) {
+      return array();
+    }
+
+    $type_key = $this->getEditTypeKey();
+    $strings = $this->transactionDescriptions;
+
+    $base = $this->getEditTransactionType();
+
+    $add = id(clone $base)
+      ->setEditType($type_key.'.add')
+      ->setEdgeOperation('+')
+      ->setDescription(idx($strings, '+'))
+      ->setValueDescription(pht('List of PHIDs to add.'));
+
+    $rem = id(clone $base)
+      ->setEditType($type_key.'.remove')
+      ->setEdgeOperation('-')
+      ->setDescription(idx($strings, '-'))
+      ->setValueDescription(pht('List of PHIDs to remove.'));
+
+    $set = id(clone $base)
+      ->setEditType($type_key.'.set')
+      ->setEdgeOperation('=')
+      ->setDescription(idx($strings, '='))
+      ->setValueDescription(pht('List of PHIDs to set.'));
+
+    return array(
+      $add,
+      $rem,
+      $set,
+    );
+  }
+
+}

src/applications/transactions/editfield/PhabricatorTokenizerEditField.php

@@ -1,9 +1,7 @@
 <?php
 
 abstract class PhabricatorTokenizerEditField
-  extends PhabricatorEditField {
-
-  private $originalValue;
+  extends PhabricatorPHIDListEditField {
 
   abstract protected function newDatasource();
 
@@ -11,75 +9,16 @@ abstract class PhabricatorTokenizerEditField
     $control = id(new AphrontFormTokenizerControl())
       ->setDatasource($this->newDatasource());
 
-    if ($this->originalValue !== null) {
-      $control->setOriginalValue($this->originalValue);
+    $initial_value = $this->getInitialValue();
+    if ($initial_value !== null) {
+      $control->setOriginalValue($initial_value);
     }
 
     return $control;
   }
 
-  public function setValue($value) {
-    $this->originalValue = $value;
-    return parent::setValue($value);
-  }
-
-  protected function getValueFromSubmit(AphrontRequest $request, $key) {
-    // TODO: Maybe move this unusual read somewhere else so subclassing this
-    // correctly is easier?
-    $this->originalValue = $request->getArr($key.'.original');
-
-    return parent::getValueFromSubmit($request, $key);
-  }
-
-  protected function getValueForTransaction() {
-    $new = parent::getValueForTransaction();
-
-    $edge_types = array(
-      PhabricatorTransactions::TYPE_EDGE => true,
-      PhabricatorTransactions::TYPE_SUBSCRIBERS => true,
-    );
-
-    if (isset($edge_types[$this->getTransactionType()])) {
-      if ($this->originalValue !== null) {
-        // If we're building an edge transaction and the request has data
-        // about the original value the user saw when they loaded the form,
-        // interpret the edit as a mixture of "+" and "-" operations instead
-        // of a single "=" operation. This limits our exposure to race
-        // conditions by making most concurrent edits merge correctly.
-
-        $new = parent::getValueForTransaction();
-        $old = $this->originalValue;
-
-        $add = array_diff($new, $old);
-        $rem = array_diff($old, $new);
-
-        $value = array();
-
-        if ($add) {
-          $value['+'] = array_fuse($add);
-        }
-        if ($rem) {
-          $value['-'] = array_fuse($rem);
-        }
-
-        return $value;
-      } else {
-
-        if (!is_array($new)) {
-          throw new Exception(print_r($new, true));
-        }
-
-        return array(
-          '=' => array_fuse($new),
-        );
-      }
-    }
-
-    return $new;
-  }
-
-  protected function newHTTPParameterType() {
-    return new AphrontPHIDListHTTPParameterType();
+  protected function getInitialValueFromSubmit(AphrontRequest $request, $key) {
+    return $request->getArr($key.'.original');
   }
 
 }