Move Differential's read-only fields to the extensible field schema

Summary:
Move additional fields (which rely on loading handles) to the extensible field
classes and out of hardcoding in the controller.

Depends on D807.

Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 808

src/__phutil_library_map__.php

@@ -134,6 +134,8 @@ phutil_register_library_map(array(
     'DifferentialAction' => 'applications/differential/constants/action',
     'DifferentialAddCommentView' => 'applications/differential/view/addcomment',
     'DifferentialApplyPatchFieldSpecification' => 'applications/differential/field/specification/applypatch',
+    'DifferentialArcanistProjectFieldSpecification' => 'applications/differential/field/specification/arcanistproject',
+    'DifferentialAuthorFieldSpecification' => 'applications/differential/field/specification/author',
     'DifferentialAuxiliaryField' => 'applications/differential/storage/auxiliaryfield',
     'DifferentialBlameRevisionFieldSpecification' => 'applications/differential/field/specification/blamerev',
     'DifferentialCCWelcomeMail' => 'applications/differential/mail/ccwelcome',
@@ -153,9 +155,11 @@ phutil_register_library_map(array(
     'DifferentialCommitMessageField' => 'applications/differential/data/commitmessage',
     'DifferentialCommitMessageModifier' => 'applications/differential/data/commitmessage',
     'DifferentialCommitMessageParserException' => 'applications/differential/parser/commitmessage/exception',
+    'DifferentialCommitsFieldSpecification' => 'applications/differential/field/specification/commits',
     'DifferentialController' => 'applications/differential/controller/base',
     'DifferentialDAO' => 'applications/differential/storage/base',
     'DifferentialDefaultFieldSelector' => 'applications/differential/field/selector/default',
+    'DifferentialDependenciesFieldSpecification' => 'applications/differential/field/specification/dependencies',
     'DifferentialDiff' => 'applications/differential/storage/diff',
     'DifferentialDiffContentMail' => 'applications/differential/mail/diffcontent',
     'DifferentialDiffCreateController' => 'applications/differential/controller/diffcreate',
@@ -178,6 +182,7 @@ phutil_register_library_map(array(
     'DifferentialLinesFieldSpecification' => 'applications/differential/field/specification/lines',
     'DifferentialLintStatus' => 'applications/differential/constants/lintstatus',
     'DifferentialMail' => 'applications/differential/mail/base',
+    'DifferentialManiphestTasksFieldSpecification' => 'applications/differential/field/specification/maniphesttasks',
     'DifferentialNewDiffMail' => 'applications/differential/mail/newdiff',
     'DifferentialPathFieldSpecification' => 'applications/differential/field/specification/path',
     'DifferentialPrimaryPaneView' => 'applications/differential/view/primarypane',
@@ -782,6 +787,8 @@ phutil_register_library_map(array(
     'DarkConsoleXHProfPlugin' => 'DarkConsolePlugin',
     'DifferentialAddCommentView' => 'AphrontView',
     'DifferentialApplyPatchFieldSpecification' => 'DifferentialFieldSpecification',
+    'DifferentialArcanistProjectFieldSpecification' => 'DifferentialFieldSpecification',
+    'DifferentialAuthorFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialAuxiliaryField' => 'DifferentialDAO',
     'DifferentialBlameRevisionFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialCCWelcomeMail' => 'DifferentialReviewRequestMail',
@@ -793,9 +800,11 @@ phutil_register_library_map(array(
     'DifferentialCommentMail' => 'DifferentialMail',
     'DifferentialCommentPreviewController' => 'DifferentialController',
     'DifferentialCommentSaveController' => 'DifferentialController',
+    'DifferentialCommitsFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialController' => 'PhabricatorController',
     'DifferentialDAO' => 'PhabricatorLiskDAO',
     'DifferentialDefaultFieldSelector' => 'DifferentialFieldSelector',
+    'DifferentialDependenciesFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialDiff' => 'DifferentialDAO',
     'DifferentialDiffContentMail' => 'DifferentialMail',
     'DifferentialDiffCreateController' => 'DifferentialController',
@@ -811,6 +820,7 @@ phutil_register_library_map(array(
     'DifferentialInlineCommentPreviewController' => 'DifferentialController',
     'DifferentialInlineCommentView' => 'AphrontView',
     'DifferentialLinesFieldSpecification' => 'DifferentialFieldSpecification',
+    'DifferentialManiphestTasksFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialNewDiffMail' => 'DifferentialReviewRequestMail',
     'DifferentialPathFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialPrimaryPaneView' => 'AphrontView',

src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php

@@ -104,20 +104,29 @@ class DifferentialRevisionViewController extends DifferentialController {
       }
     }
 
-    if ($target->getArcanistProjectPHID()) {
-      $object_phids[] = $target->getArcanistProjectPHID();
-    }
-
     foreach ($revision->getAttached() as $type => $phids) {
       foreach ($phids as $phid => $info) {
         $object_phids[] = $phid;
       }
     }
+
+    $aux_phids = array();
+    foreach ($aux_fields as $key => $aux_field) {
+      $aux_phids[$key] = $aux_field->getRequiredHandlePHIDsForRevisionView();
+    }
+    $object_phids = array_merge($object_phids, array_mergev($aux_phids));
     $object_phids = array_unique($object_phids);
 
     $handles = id(new PhabricatorObjectHandleData($object_phids))
       ->loadHandles();
 
+    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;
@@ -326,9 +335,6 @@ class DifferentialRevisionViewController extends DifferentialController {
     $status = DifferentialRevisionStatus::getNameForRevisionStatus($status);
     $properties['Revision Status'] = '<strong>'.$status.'</strong>'.$next_step;
 
-    $author = $handles[$revision->getAuthorPHID()];
-    $properties['Author'] = $author->renderLink();
-
     $properties['Reviewers'] = $this->renderHandleLinkList(
       array_select_keys(
         $handles,
@@ -435,43 +441,6 @@ class DifferentialRevisionViewController extends DifferentialController {
 
     $properties['Unit'] = $ustar.' '.$umsg.$utail;
 
-    $drevs = $revision->getAttachedPHIDs(
-      PhabricatorPHIDConstants::PHID_TYPE_DREV);
-    if ($drevs) {
-      $links = array();
-      foreach ($drevs as $drev_phid) {
-        $links[] = $handles[$drev_phid]->renderLink();
-      }
-      $properties['Depends On'] = implode('<br />', $links);
-    }
-
-    if (PhabricatorEnv::getEnvConfig('maniphest.enabled')) {
-      $tasks = $revision->getAttachedPHIDs(
-        PhabricatorPHIDConstants::PHID_TYPE_TASK);
-      if ($tasks) {
-        $links = array();
-        foreach ($tasks as $task_phid) {
-          $links[] = $handles[$task_phid]->renderLink();
-        }
-        $properties['Maniphest Tasks'] = implode('<br />', $links);
-      }
-    }
-
-    $commit_phids = $revision->getCommitPHIDs();
-    if ($commit_phids) {
-      $links = array();
-      foreach ($commit_phids as $commit_phid) {
-        $links[] = $handles[$commit_phid]->renderLink();
-      }
-      $properties['Commits'] = implode('<br />', $links);
-    }
-
-    $arcanist_phid = $diff->getArcanistProjectPHID();
-    if ($arcanist_phid) {
-      $properties['Arcanist Project'] = phutil_escape_html(
-        $handles[$arcanist_phid]->getName());
-    }
-
     return $properties;
   }
 

src/applications/differential/controller/revisionview/__init__.php

@@ -29,7 +29,6 @@ phutil_require_module('phabricator', 'applications/differential/view/revisioncom
 phutil_require_module('phabricator', 'applications/differential/view/revisiondetail');
 phutil_require_module('phabricator', 'applications/differential/view/revisionupdatehistory');
 phutil_require_module('phabricator', 'applications/draft/storage/draft');
-phutil_require_module('phabricator', 'applications/phid/constants');
 phutil_require_module('phabricator', 'applications/phid/handle/data');
 phutil_require_module('phabricator', 'applications/repository/constants/repositorytype');
 phutil_require_module('phabricator', 'infrastructure/celerity/api');

src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php

@@ -21,9 +21,14 @@ final class DifferentialDefaultFieldSelector
 
   public function getFieldSpecifications() {
     return array(
+      new DifferentialAuthorFieldSpecification(),
+      new DifferentialDependenciesFieldSpecification(),
+      new DifferentialManiphestTasksFieldSpecification(),
+      new DifferentialCommitsFieldSpecification(),
       new DifferentialHostFieldSpecification(),
       new DifferentialPathFieldSpecification(),
       new DifferentialLinesFieldSpecification(),
+      new DifferentialArcanistProjectFieldSpecification(),
       new DifferentialApplyPatchFieldSpecification(),
       new DifferentialExportPatchFieldSpecification(),
     );

src/applications/differential/field/selector/default/__init__.php

@@ -8,9 +8,14 @@
 
 phutil_require_module('phabricator', 'applications/differential/field/selector/base');
 phutil_require_module('phabricator', 'applications/differential/field/specification/applypatch');
+phutil_require_module('phabricator', 'applications/differential/field/specification/arcanistproject');
+phutil_require_module('phabricator', 'applications/differential/field/specification/author');
+phutil_require_module('phabricator', 'applications/differential/field/specification/commits');
+phutil_require_module('phabricator', 'applications/differential/field/specification/dependencies');
 phutil_require_module('phabricator', 'applications/differential/field/specification/exportpatch');
 phutil_require_module('phabricator', 'applications/differential/field/specification/host');
 phutil_require_module('phabricator', 'applications/differential/field/specification/lines');
+phutil_require_module('phabricator', 'applications/differential/field/specification/maniphesttasks');
 phutil_require_module('phabricator', 'applications/differential/field/specification/path');
 
 

src/applications/differential/field/specification/arcanistproject/DifferentialArcanistProjectFieldSpecification.php

@@ -0,0 +1,54 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialArcanistProjectFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  public function shouldAppearOnRevisionView() {
+    return true;
+  }
+
+  public function getRequiredHandlePHIDs() {
+    $arcanist_phid = $this->getArcanistProjectPHID();
+    if (!$arcanist_phid) {
+      return array();
+    }
+
+    return array($arcanist_phid);
+  }
+
+  public function renderLabelForRevisionView() {
+    return 'Arcanist Project:';
+  }
+
+  public function renderValueForRevisionView() {
+    $arcanist_phid = $this->getArcanistProjectPHID();
+    if (!$arcanist_phid) {
+      return null;
+    }
+
+    $handle = $this->getHandle($arcanist_phid);
+    return phutil_escape_html($handle->getName());
+  }
+
+  private function getArcanistProjectPHID() {
+    $diff = $this->getDiff();
+    return $diff->getArcanistProjectPHID();
+  }
+
+}

src/applications/differential/field/specification/arcanistproject/__init__.php

@@ -0,0 +1,14 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+
+phutil_require_module('phutil', 'markup');
+
+
+phutil_require_source('DifferentialArcanistProjectFieldSpecification.php');

src/applications/differential/field/specification/author/DifferentialAuthorFieldSpecification.php

@@ -0,0 +1,46 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialAuthorFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  public function shouldAppearOnRevisionView() {
+    return true;
+  }
+
+  public function getRequiredHandlePHIDsForRevisionView() {
+    return array($this->getAuthorPHID());
+  }
+
+  public function renderLabelForRevisionView() {
+    return 'Author:';
+  }
+
+  public function renderValueForRevisionView() {
+    $author_phid = $this->getAuthorPHID();
+    $handle = $this->getHandle($author_phid);
+
+    return $handle->renderLink();
+  }
+
+  private function getAuthorPHID() {
+    $revision = $this->getRevision();
+    return $revision->getAuthorPHID();
+  }
+
+}

src/applications/differential/field/specification/author/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+
+
+phutil_require_source('DifferentialAuthorFieldSpecification.php');

src/applications/differential/field/specification/base/DifferentialFieldSpecification.php

@@ -28,12 +28,14 @@
  * @task edit Extending the Revision Edit Interface
  * @task view Extending the Revision View Interface
  * @task conduit Extending the Conduit View Interface
+ * @task handles Loading Handles
  * @task context Contextual Data
  */
 abstract class DifferentialFieldSpecification {
 
   private $revision;
   private $diff;
+  private $handles;
 
 
 /* -(  Storage  )------------------------------------------------------------ */
@@ -229,8 +231,63 @@ abstract class DifferentialFieldSpecification {
   }
 
 
+/* -(  Loading Handles  )---------------------------------------------------- */
+
+
+  /**
+   * Specify which @{class:PhabricatorObjectHandles} need to be loaded for your
+   * field to render correctly.
+   *
+   * This is a convenience method which makes the handles available on all
+   * interfaces where the field appears. If your field needs handles on only
+   * some interfaces (or needs different handles on different interfaces) you
+   * can overload the more specific methods to customize which interfaces you
+   * retrieve handles for. Requesting only the handles you need will improve
+   * the performance of your field.
+   *
+   * You can later retrieve these handles by calling @{method:getHandle}.
+   *
+   * @return list List of PHIDs to load handles for.
+   * @task handles
+   */
+  protected function getRequiredHandlePHIDs() {
+    return array();
+  }
+
+  /**
+   * Specify which @{class:PhabricatorObjectHandles} need to be loaded for your
+   * field to render correctly on the view interface.
+   *
+   * This is a more specific version of @{method:getRequiredHandlePHIDs} which
+   * can be overridden to improve field performance by loading only data you
+   * need.
+   *
+   * @return list List of PHIDs to load handles for.
+   * @task handles
+   */
+  public function getRequiredHandlePHIDsForRevisionView() {
+    return $this->getRequiredHandlePHIDs();
+  }
+
+  /**
+   * Specify which @{class:PhabricatorObjectHandles} need to be loaded for your
+   * field to render correctly on the edit interface.
+   *
+   * This is a more specific version of @{method:getRequiredHandlePHIDs} which
+   * can be overridden to improve field performance by loading only data you
+   * need.
+   *
+   * @return list List of PHIDs to load handles for.
+   * @task handles
+   */
+  public function getRequiredHandlePHIDsForEdit() {
+    return $this->getRequiredHandlePHIDs();
+  }
+
+
 /* -(  Contextual Data  )---------------------------------------------------- */
 
+
   /**
    * @task context
    */
@@ -247,6 +304,14 @@ abstract class DifferentialFieldSpecification {
     return $this;
   }
 
+  /**
+   * @task context
+   */
+  final public function setHandles(array $handles) {
+    $this->handles = $handles;
+    return $this;
+  }
+
   /**
    * @task context
    */
@@ -267,4 +332,23 @@ abstract class DifferentialFieldSpecification {
     return $this->diff;
   }
 
+  /**
+   * Get the handle for an object PHID. You must overload
+   * @{method:getRequiredHandlePHIDs} (or a more specific version thereof)
+   * and include the PHID you want in the list for it to be available here.
+   *
+   * @return PhabricatorObjectHandle Handle to the object.
+   * @task context
+   */
+  final protected function getHandle($phid) {
+    if (empty($this->handles[$phid])) {
+      $class = get_class($this);
+      throw new Exception(
+        "A differential field (of class '{$class}') is attempting to retrieve ".
+        "a handle ('{$phid}') which it did not request. Return all handle ".
+        "PHIDs you need from getRequiredHandlePHIDs().");
+    }
+    return $this->handles[$phid];
+  }
+
 }

src/applications/differential/field/specification/commits/DifferentialCommitsFieldSpecification.php

@@ -0,0 +1,53 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialCommitsFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  public function shouldAppearOnRevisionView() {
+    return true;
+  }
+
+  public function getRequiredHandlePHIDsForRevisionView() {
+    return $this->getCommitPHIDs();
+  }
+
+  public function renderLabelForRevisionView() {
+    return 'Commits:';
+  }
+
+  public function renderValueForRevisionView() {
+    $commit_phids = $this->getCommitPHIDs();
+    if (!$commit_phids) {
+      return null;
+    }
+
+    $links = array();
+    foreach ($commit_phids as $commit_phid) {
+      $links[] = $this->getHandle($commit_phid)->renderLink();
+    }
+
+    return implode('<br />', $links);
+  }
+
+  private function getCommitPHIDs() {
+    $revision = $this->getRevision();
+    return $revision->getCommitPHIDs();
+  }
+
+}

src/applications/differential/field/specification/commits/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+
+
+phutil_require_source('DifferentialCommitsFieldSpecification.php');

src/applications/differential/field/specification/dependencies/DifferentialDependenciesFieldSpecification.php

@@ -0,0 +1,54 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialDependenciesFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  public function shouldAppearOnRevisionView() {
+    return true;
+  }
+
+  public function getRequiredHandlePHIDsForRevisionView() {
+    return $this->getDependentRevisionPHIDs();
+  }
+
+  public function renderLabelForRevisionView() {
+    return 'Depends On:';
+  }
+
+  public function renderValueForRevisionView() {
+    $revision_phids = $this->getDependentRevisionPHIDs();
+    if (!$revision_phids) {
+      return null;
+    }
+
+    $links = array();
+    foreach ($revision_phids as $revision_phids) {
+      $links[] = $this->getHandle($revision_phids)->renderLink();
+    }
+
+    return implode('<br />', $links);
+  }
+
+  private function getDependentRevisionPHIDs() {
+    $revision = $this->getRevision();
+    return $revision->getAttachedPHIDs(
+      PhabricatorPHIDConstants::PHID_TYPE_DREV);
+  }
+
+}

src/applications/differential/field/specification/dependencies/__init__.php

@@ -0,0 +1,13 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+phutil_require_module('phabricator', 'applications/phid/constants');
+
+
+phutil_require_source('DifferentialDependenciesFieldSpecification.php');

src/applications/differential/field/specification/maniphesttasks/DifferentialManiphestTasksFieldSpecification.php

@@ -0,0 +1,54 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialManiphestTasksFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  public function shouldAppearOnRevisionView() {
+    return PhabricatorEnv::getEnvConfig('maniphest.enabled');
+  }
+
+  public function getRequiredHandlePHIDsForRevisionView() {
+    return $this->getManiphestTaskPHIDs();
+  }
+
+  public function renderLabelForRevisionView() {
+    return 'Maniphest Tasks:';
+  }
+
+  public function renderValueForRevisionView() {
+    $task_phids = $this->getManiphestTaskPHIDs();
+    if (!$task_phids) {
+      return null;
+    }
+
+    $links = array();
+    foreach ($task_phids as $task_phid) {
+      $links[] = $this->getHandle($task_phid)->renderLink();
+    }
+
+    return implode('<br />', $links);
+  }
+
+  private function getManiphestTaskPHIDs() {
+    $revision = $this->getRevision();
+    return $revision->getAttachedPHIDs(
+      PhabricatorPHIDConstants::PHID_TYPE_TASK);
+  }
+
+}

src/applications/differential/field/specification/maniphesttasks/__init__.php

@@ -0,0 +1,14 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+phutil_require_module('phabricator', 'applications/phid/constants');
+phutil_require_module('phabricator', 'infrastructure/env');
+
+
+phutil_require_source('DifferentialManiphestTasksFieldSpecification.php');