Load all diff properties in revision view

Summary:
We need to load all properties with some prefix in one field.
We can't merge them in one property because there will be a race condition for update (we don't have API for load+update+save).

Instead of providing API for this and complicating the code even more, just load everything unconditionally.
It shouldn't waste much bandwith or memory because we use most of the properties anyway.
It also looked overengineered to me.

Test Plan: Displayed revision with fields using diff properties.

Reviewers: epriestley, royw

Reviewed By: royw

CC: aran, royw, Korvin

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

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -82,16 +82,12 @@ public function processRequest() {
                                          $repository);
     }
 
-    list($aux_fields, $props) = $this->loadAuxiliaryFieldsAndProperties(
-      $revision,
-      $target,
-      $target_manual,
-      array(
-        'local:commits',
-        'arc:lint',
-        'arc:unit',
-      ));
+    $props = id(new DifferentialDiffProperty())->loadAllWhere(
+      'diffID = %d',
+      $target_manual->getID());
+    $props = mpull($props, 'getData', 'getName');
 
+    $aux_fields = $this->loadAuxiliaryFields($revision);
 
     $comments = $revision->loadComments();
     $comments = array_merge(
@@ -139,6 +135,9 @@ public function processRequest() {
 
     $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));
@@ -740,11 +739,7 @@ private function loadChangesetsAndVsMap(
     return array($changesets, $vs_map, $vs_changesets, $refs);
   }
 
-  private function loadAuxiliaryFieldsAndProperties(
-    DifferentialRevision $revision,
-    DifferentialDiff $diff,
-    DifferentialDiff $manual_diff,
-    array $special_properties) {
+  private function loadAuxiliaryFields(DifferentialRevision $revision) {
 
     $aux_fields = DifferentialFieldSelector::newSelector()
       ->getFieldSpecifications();
@@ -760,46 +755,7 @@ private function loadAuxiliaryFieldsAndProperties(
       $revision,
       $aux_fields);
 
-    $aux_props = array();
-    foreach ($aux_fields as $key => $aux_field) {
-      $aux_field->setDiff($diff);
-      $aux_field->setManualDiff($manual_diff);
-      $aux_props[$key] = $aux_field->getRequiredDiffProperties();
-    }
-
-    $required_properties = array_mergev($aux_props);
-    $required_properties = array_merge(
-      $required_properties,
-      $special_properties);
-
-    $property_map = array();
-    if ($required_properties) {
-      $properties = id(new DifferentialDiffProperty())->loadAllWhere(
-        'diffID = %d AND name IN (%Ls)',
-        $manual_diff->getID(),
-        $required_properties);
-      $property_map = mpull($properties, 'getData', 'getName');
-    }
-
-    foreach ($aux_fields as $key => $aux_field) {
-      // Give each field only the properties it specifically required, and
-      // set 'null' for each requested key which we didn't actually load a
-      // value for (otherwise, getDiffProperty() will throw).
-      if ($aux_props[$key]) {
-        $props = array_select_keys($property_map, $aux_props[$key]) +
-                 array_fill_keys($aux_props[$key], null);
-      } else {
-        $props = array();
-      }
-
-      $aux_field->setDiffProperties($props);
-    }
-
-    return array(
-      $aux_fields,
-      array_select_keys(
-        $property_map,
-        $special_properties));
+    return $aux_fields;
   }
 
   private function buildSymbolIndexes(

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

@@ -676,16 +676,6 @@ public function getRequiredHandlePHIDsForCommitMessage() {
     return $this->getRequiredHandlePHIDs();
   }
 
-  /**
-   * Specify which diff properties this field needs to load.
-   *
-   * @return list List of diff property keys this field requires.
-   * @task load
-   */
-  public function getRequiredDiffProperties() {
-    return array();
-  }
-
   /**
    * Parse a list of users into a canonical PHID list.
    *
@@ -904,8 +894,7 @@ final protected function getHandle($phid) {
   }
 
   /**
-   * Get a diff property which this field previously requested by returning
-   * the key from @{method:getRequiredDiffProperties}.
+   * Get a property of a diff set by @{method:setManualDiff}.
    *
    * @param  string      Diff property key.
    * @return mixed|null  Diff property, or null if the property does not have
@@ -919,14 +908,7 @@ final public function getDiffProperty($key) {
       // context.
       throw new DifferentialFieldDataNotAvailableException($this);
     }
-    if (!array_key_exists($key, $this->diffProperties)) {
-      $class = get_class($this);
-      throw new Exception(
-        "A differential field (of class '{$class}') is attempting to retrieve ".
-        "a diff property ('{$key}') which it did not request. Return all ".
-        "diff property keys you need from getRequiredDiffProperties().");
-    }
-    return $this->diffProperties[$key];
+    return idx($this->diffProperties, $key);
   }
 
 }

src/applications/differential/field/specification/DifferentialLintFieldSpecification.php

@@ -27,10 +27,6 @@ public function renderLabelForRevisionView() {
     return 'Lint:';
   }
 
-  public function getRequiredDiffProperties() {
-    return array('arc:lint', 'arc:lint-excuse', 'arc:lint-postponed');
-  }
-
   private function getLintExcuse() {
     return $this->getDiffProperty('arc:lint-excuse');
   }

src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php

@@ -27,10 +27,6 @@ public function renderLabelForRevisionView() {
     return 'Unit:';
   }
 
-  public function getRequiredDiffProperties() {
-    return array('arc:unit', 'arc:unit-excuse');
-  }
-
   private function getUnitExcuse() {
     return $this->getDiffProperty('arc:unit-excuse');
   }