Use CustomField, not AuxiliaryField, to power RevisionView

Summary: Ref T2222. This will probably have some rough edges for a bit (e.g., weird cases I didn't remember or think of), but there's no change to the underlying data and we can easily revert if things get too messy.

Test Plan: Looked at a variety of revisions and saw sensible output.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

conf/default.conf.php

@@ -787,8 +787,6 @@ return array(
 
 // -- Differential ---------------------------------------------------------- //
 
-  'differential.revision-custom-detail-renderer'  => null,
-
   // List of file regexps where whitespace is meaningful and should not
   // use 'ignore-all' by default
   'differential.whitespace-matters' => array(

src/applications/config/check/PhabricatorSetupCheckExtraConfig.php

@@ -180,6 +180,8 @@ final class PhabricatorSetupCheckExtraConfig extends PhabricatorSetupCheck {
       'auth.sessions.web' => $session_reason,
       'tokenizer.ondemand' => pht(
         'Phabricator now manages typeahead strategies automatically.'),
+      'differential.revision-custom-detail-renderer' => pht(
+        'Obsolete; use standard rendering events instead.'),
     );
 
     return $ancient_config;

src/applications/differential/config/PhabricatorDifferentialConfigOptions.php

@@ -13,12 +13,6 @@ final class PhabricatorDifferentialConfigOptions
 
   public function getOptions() {
     return array(
-      $this->newOption(
-        'differential.revision-custom-detail-renderer',
-        'class',
-        null)
-        ->setBaseClass('DifferentialRevisionDetailRenderer')
-        ->setDescription(pht("Custom revision detail renderer.")),
       $this->newOption(
         'differential.whitespace-matters',
         'list<regex>',

src/applications/differential/controller/DifferentialRevisionDetailRenderer.php

@@ -1,46 +0,0 @@
-<?php
-
-abstract class DifferentialRevisionDetailRenderer {
-  private $user;
-  private $diff;
-  private $vsDiff;
-
-  final public function setUser(PhabricatorUser $user) {
-    $this->user = $user;
-    return $this;
-  }
-
-  final protected function getUser() {
-    return $this->user;
-  }
-
-  final public function setDiff(DifferentialDiff $diff) {
-    $this->diff = $diff;
-    return $this;
-  }
-
-  final protected function getDiff() {
-    return $this->diff;
-  }
-
-  final public function setVSDiff(DifferentialDiff $diff) {
-    $this->vsDiff = $diff;
-    return $this;
-  }
-
-  final protected function getVSDiff() {
-    return $this->vsDiff;
-  }
-
-  /**
-   * This function must return an array of action links that will be
-   * added to the end of action links on the differential revision
-   * page. Each element in the array must be an array which must
-   * contain 'name' and 'href' fields. 'name' will be the name of the
-   * link and 'href' will be the address where the link points
-   * to. 'class' is optional and can be used for specifying a CSS
-   * class.
-   */
-  abstract public function generateActionLinks(DifferentialRevision $revision,
-                                               DifferentialDiff $diff);
-}

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -41,6 +41,8 @@ final class DifferentialRevisionViewController extends DifferentialController {
         "This revision has no diffs. Something has gone quite wrong.");
     }
 
+    $revision->attachActiveDiff(last($diffs));
+
     $diff_vs = $request->getInt('vs');
 
     $target_id = $request->getInt('id');
@@ -82,8 +84,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $target_manual->getID());
     $props = mpull($props, 'getData', 'getName');
 
-    $aux_fields = $this->loadAuxiliaryFields($revision);
-
     $comments = $revision->loadComments();
 
     $all_changesets = $changesets;
@@ -113,25 +113,8 @@ final class DifferentialRevisionViewController extends DifferentialController {
       }
     }
 
-    $aux_phids = array();
-    foreach ($aux_fields as $key => $aux_field) {
-      $aux_field->setDiff($target);
-      $aux_field->setManualDiff($target_manual);
-      $aux_field->setDiffProperties($props);
-      $aux_phids[$key] = $aux_field->getRequiredHandlePHIDsForRevisionView();
-    }
-    $object_phids = array_merge($object_phids, array_mergev($aux_phids));
-    $object_phids = array_unique($object_phids);
-
     $handles = $this->loadViewerHandles($object_phids);
 
-    foreach ($aux_fields as $key => $aux_field) {
-      // Make sure each field only has access to handles it specifically
-      // requested, not all handles. Otherwise you can get a field which works
-      // only in the presence of other fields.
-      $aux_field->setHandles(array_select_keys($handles, $aux_phids[$key]));
-    }
-
     $request_uri = $request->getRequestURI();
 
     $limit = 100;
@@ -184,32 +167,22 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $visible_changesets = $changesets;
     }
 
+    $field_list = PhabricatorCustomField::getObjectFields(
+      $revision,
+      PhabricatorCustomField::ROLE_VIEW);
+
+    $field_list->setViewer($user);
+    $field_list->readFieldsFromStorage($revision);
+
     $revision_detail = id(new DifferentialRevisionDetailView())
       ->setUser($user)
       ->setRevision($revision)
       ->setDiff(end($diffs))
-      ->setAuxiliaryFields($aux_fields)
+      ->setCustomFields($field_list)
       ->setURI($request->getRequestURI());
 
     $actions = $this->getRevisionActions($revision);
 
-    $custom_renderer_class = PhabricatorEnv::getEnvConfig(
-      'differential.revision-custom-detail-renderer');
-    if ($custom_renderer_class) {
-
-      // TODO: build a better version of the action links and deprecate the
-      // whole DifferentialRevisionDetailRenderer class.
-      $custom_renderer = newv($custom_renderer_class, array());
-      $custom_renderer->setUser($user);
-      $custom_renderer->setDiff($target);
-      if ($diff_vs) {
-        $custom_renderer->setVSDiff($diffs[$diff_vs]);
-      }
-      $actions = array_merge(
-        $actions,
-        $custom_renderer->generateActionLinks($revision, $target_manual));
-    }
-
     $whitespace = $request->getStr(
       'whitespace',
       DifferentialChangesetParser::WHITESPACE_IGNORE_ALL);
@@ -340,7 +313,9 @@ final class DifferentialRevisionViewController extends DifferentialController {
 
       $comment_form = new DifferentialAddCommentView();
       $comment_form->setRevision($revision);
-      $comment_form->setAuxFields($aux_fields);
+
+      // TODO: Restore the ability for fields to add accept warnings.
+
       $comment_form->setActions($this->getRevisionCommentActions($revision));
 
       $action_uri = '/differential/comment/save/';
@@ -450,46 +425,42 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $revision,
       PhabricatorPolicyCapability::CAN_EDIT);
 
-    $links = array();
+    $actions = array();
 
-    $links[] = array(
+    $actions[] = id(new PhabricatorActionView())
-      'icon'  =>  'edit',
+      ->setIcon('edit')
-      'href'  => "/differential/revision/edit/{$revision_id}/",
+      ->setHref("/differential/revision/edit/{$revision_id}/")
-      'name'  => pht('Edit Revision'),
+      ->setName(pht('Edit Revision'))
-      'disabled' => !$can_edit,
+      ->setDisabled(!$can_edit)
-      'sigil' => $can_edit ? null : 'workflow',
+      ->setWorkflow(!$can_edit);
-    );
 
     $this->requireResource('phabricator-object-selector-css');
     $this->requireResource('javelin-behavior-phabricator-object-selector');
 
-    $links[] = array(
+    $actions[] = id(new PhabricatorActionView())
-      'icon'  => 'link',
+      ->setIcon('link')
-      'name'  => pht('Edit Dependencies'),
+      ->setName(pht('Edit Dependencies'))
-      'href'  => "/search/attach/{$revision_phid}/DREV/dependencies/",
+      ->setHref("/search/attach/{$revision_phid}/DREV/dependencies/")
-      'sigil' => 'workflow',
+      ->setWorkflow(true)
-      'disabled' => !$can_edit,
+      ->setDisabled(!$can_edit);
-    );
 
     $maniphest = 'PhabricatorApplicationManiphest';
     if (PhabricatorApplication::isClassInstalled($maniphest)) {
-      $links[] = array(
+      $actions[] = id(new PhabricatorActionView())
-        'icon'  => 'attach',
+        ->setIcon('attach')
-        'name'  => pht('Edit Maniphest Tasks'),
+        ->setName(pht('Edit Maniphest Tasks'))
-        'href'  => "/search/attach/{$revision_phid}/TASK/",
+        ->setHref("/search/attach/{$revision_phid}/TASK/")
-        'sigil' => 'workflow',
+        ->setWorkflow(true)
-        'disabled' => !$can_edit,
+        ->setDisabled(!$can_edit);
-      );
     }
 
     $request_uri = $this->getRequest()->getRequestURI();
-    $links[] = array(
+    $actions[] = id(new PhabricatorActionView())
-      'icon'  => 'download',
+      ->setIcon('download')
-      'name'  => pht('Download Raw Diff'),
+      ->setName(pht('Download Raw Diff'))
-      'href'  => $request_uri->alter('download', 'true')
+      ->setHref($request_uri->alter('download', 'true'));
-    );
 
-    return $links;
+    return $actions;
   }
 
   private function getRevisionCommentActions(DifferentialRevision $revision) {
@@ -689,25 +660,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
     return array($changesets, $vs_map, $vs_changesets, $refs);
   }
 
-  private function loadAuxiliaryFields(DifferentialRevision $revision) {
-
-    $aux_fields = DifferentialFieldSelector::newSelector()
-      ->getFieldSpecifications();
-    foreach ($aux_fields as $key => $aux_field) {
-      if (!$aux_field->shouldAppearOnRevisionView()) {
-        unset($aux_fields[$key]);
-      } else {
-        $aux_field->setUser($this->getRequest()->getUser());
-      }
-    }
-
-    $aux_fields = DifferentialAuxiliaryField::loadFromStorage(
-      $revision,
-      $aux_fields);
-
-    return $aux_fields;
-  }
-
   private function buildSymbolIndexes(
     PhabricatorRepositoryArcanistProject $arc_project,
     array $visible_changesets) {

src/applications/differential/customfield/DifferentialBlameRevisionField.php

@@ -15,6 +15,10 @@ final class DifferentialBlameRevisionField
     return pht('Stores a reference to what this fixes.');
   }
 
+  public function shouldDisableByDefault() {
+    return true;
+  }
+
   public function shouldAppearInPropertyView() {
     return true;
   }

src/applications/differential/customfield/DifferentialHostField.php

@@ -15,6 +15,10 @@ final class DifferentialHostField
     return pht('Shows the local host where the diff came from.');
   }
 
+  public function shouldDisableByDefault() {
+    return true;
+  }
+
   public function shouldAppearInPropertyView() {
     return true;
   }

src/applications/differential/customfield/DifferentialPathField.php

@@ -15,6 +15,10 @@ final class DifferentialPathField
     return pht('Shows the local path where the diff came from.');
   }
 
+  public function shouldDisableByDefault() {
+    return true;
+  }
+
   public function shouldAppearInPropertyView() {
     return true;
   }

src/applications/differential/customfield/DifferentialRevertPlanField.php

@@ -15,6 +15,10 @@ final class DifferentialRevertPlanField
     return pht('Instructions for reverting/undoing this change.');
   }
 
+  public function shouldDisableByDefault() {
+    return true;
+  }
+
   public function shouldAppearInPropertyView() {
     return true;
   }

src/applications/differential/storage/DifferentialRevision.php

@@ -469,8 +469,7 @@ final class DifferentialRevision extends DifferentialDAO
   }
 
   public function shouldShowSubscribersProperty() {
-    // TODO: Differential does its own thing for now.
+    return true;
-    return false;
   }
 
   public function shouldAllowSubscription($phid) {
@@ -483,19 +482,42 @@ final class DifferentialRevision extends DifferentialDAO
 
   public function getCustomFieldSpecificationForRole($role) {
     $fields = array(
+      new DifferentialAuthorField(),
+
       new DifferentialTitleField(),
       new DifferentialSummaryField(),
       new DifferentialTestPlanField(),
       new DifferentialReviewersField(),
+      new DifferentialProjectReviewersField(),
       new DifferentialSubscribersField(),
       new DifferentialRepositoryField(),
       new DifferentialViewPolicyField(),
       new DifferentialEditPolicyField(),
+
+      new DifferentialDependsOnField(),
+      new DifferentialDependenciesField(),
+      new DifferentialManiphestTasksField(),
+      new DifferentialCommitsField(),
+
+      new DifferentialJIRAIssuesField(),
+      new DifferentialAsanaRepresentationField(),
+
+      new DifferentialBlameRevisionField(),
+      new DifferentialPathField(),
+      new DifferentialHostField(),
+      new DifferentialRevertPlanField(),
+
+      new DifferentialApplyPatchField(),
     );
 
-    return array_fill_keys(
+    $result = array();
-      mpull($fields, 'getFieldKey'),
+    foreach ($fields as $field) {
-      array('disabled' => false));
+      $result[$field->getFieldKey()] = array(
+        'disabled' => $field->shouldDisableByDefault(),
+      );
+    }
+
+    return $result;
   }
 
   public function getCustomFieldBaseClass() {

src/applications/differential/view/DifferentialAddCommentView.php

@@ -6,7 +6,6 @@ final class DifferentialAddCommentView extends AphrontView {
   private $actions;
   private $actionURI;
   private $draft;
-  private $auxFields;
   private $reviewers = array();
   private $ccs = array();
 
@@ -15,12 +14,6 @@ final class DifferentialAddCommentView extends AphrontView {
     return $this;
   }
 
-  public function setAuxFields(array $aux_fields) {
-    assert_instances_of($aux_fields, 'DifferentialFieldSpecification');
-    $this->auxFields = $aux_fields;
-    return $this;
-  }
-
   public function setActions(array $actions) {
     $this->actions = $actions;
     return $this;
@@ -136,7 +129,7 @@ final class DifferentialAddCommentView extends AphrontView {
       ));
 
     $diff = $revision->loadActiveDiff();
-    $warnings = mpull($this->auxFields, 'renderWarningBoxForRevisionAccept');
+    $warnings = array();
 
     Javelin::initBehavior(
       'differential-accept-with-errors',

src/applications/differential/view/DifferentialRevisionDetailView.php

@@ -4,7 +4,7 @@ final class DifferentialRevisionDetailView extends AphrontView {
 
   private $revision;
   private $actions;
-  private $auxiliaryFields = array();
+  private $customFields;
   private $diff;
   private $uri;
 
@@ -37,9 +37,8 @@ final class DifferentialRevisionDetailView extends AphrontView {
     return $this->actions;
   }
 
-  public function setAuxiliaryFields(array $fields) {
+  public function setCustomFields(PhabricatorCustomFieldList $list) {
-    assert_instances_of($fields, 'DifferentialFieldSpecification');
+    $this->customFields = $list;
-    $this->auxiliaryFields = $fields;
     return $this;
   }
 
@@ -57,15 +56,7 @@ final class DifferentialRevisionDetailView extends AphrontView {
       ->setObject($revision)
       ->setObjectURI($this->getURI());
     foreach ($this->getActions() as $action) {
-      $obj = id(new PhabricatorActionView())
+      $actions->addAction($action);
-        ->setIcon(idx($action, 'icon', 'edit'))
-        ->setName($action['name'])
-        ->setHref(idx($action, 'href'))
-        ->setWorkflow(idx($action, 'sigil') == 'workflow')
-        ->setRenderAsForm(!empty($action['instant']))
-        ->setUser($user)
-        ->setDisabled(idx($action, 'disabled', false));
-      $actions->addAction($obj);
     }
 
     $properties = id(new PHUIPropertyListView())
@@ -102,42 +93,15 @@ final class DifferentialRevisionDetailView extends AphrontView {
       $properties->addProperty(pht('Next Step'), $next_step);
     }
 
-    foreach ($this->auxiliaryFields as $field) {
-      $value = $field->renderValueForRevisionView();
-      if ($value !== null) {
-        $label = rtrim($field->renderLabelForRevisionView(), ':');
-        $properties->addProperty($label, $value);
-      }
-    }
     $properties->setHasKeyboardShortcuts(true);
     $properties->setActionList($actions);
 
-    $properties->invokeWillRenderEvent();
+    $field_list = $this->customFields;
-
+    if ($field_list) {
-    if (strlen($revision->getSummary())) {
+      $field_list->appendFieldsToPropertyList(
-      $properties->addSectionHeader(
+        $revision,
-        pht('Summary'),
+        $user,
-        PHUIPropertyListView::ICON_SUMMARY);
+        $properties);
-      $properties->addTextContent(
-        PhabricatorMarkupEngine::renderOneObject(
-          id(new PhabricatorMarkupOneOff())
-            ->setPreserveLinebreaks(true)
-            ->setContent($revision->getSummary()),
-          'default',
-          $user));
-    }
-
-    if (strlen($revision->getTestPlan())) {
-      $properties->addSectionHeader(
-        pht('Test Plan'),
-        PHUIPropertyListView::ICON_TESTPLAN);
-      $properties->addTextContent(
-        PhabricatorMarkupEngine::renderOneObject(
-          id(new PhabricatorMarkupOneOff())
-            ->setPreserveLinebreaks(true)
-            ->setContent($revision->getTestPlan()),
-          'default',
-          $user));
     }
 
     $object_box = id(new PHUIObjectBoxView())

src/applications/releeph/differential/ReleephDifferentialRevisionDetailRenderer.php

@@ -1,39 +0,0 @@
-<?php
-
-final class ReleephDifferentialRevisionDetailRenderer {
-
-  public static function generateActionLink(DifferentialRevision $revision,
-                                            DifferentialDiff $diff) {
-
-    $arc_project = $diff->loadArcanistProject(); // 93us
-    if (!$arc_project) {
-      return;
-    }
-
-    $releeph_projects = id(new ReleephProject())->loadAllWhere(
-      'arcanistProjectID = %d AND isActive = 1',
-      $arc_project->getID());
-
-    if (!$releeph_projects) {
-      return;
-    }
-
-    $releeph_branches = id(new ReleephBranch())->loadAllWhere(
-      'releephProjectID IN (%Ld) AND isActive = 1',
-      mpull($releeph_projects, 'getID'));
-
-    if (!$releeph_branches) {
-      return;
-    }
-
-    $uri = new PhutilURI(
-      '/releeph/request/differentialcreate/D'.$revision->getID());
-    return array(
-      'name'  => 'Releeph Request',
-      'sigil' => 'workflow',
-      'href'  => $uri,
-      'icon'  => 'fork',
-    );
-  }
-
-}