Separate reading object values out of didSetObject() in CustomField

Summary:
Ref T3886. Broadly, fields break down into two types right now: fields which store data on the object (like `DifferentialTitleField`) and fields which store data in custom field storage.

The former type generally reads data from the object into local storage prior to editing, then writes it back afterward. Currently, this happens in `didSetObject()`.

However, now that we load and set objects from ApplicationTransactionQuery, we'll do this extra read-field-values on view interfaces too. There, it's unnecessary and sometimes throws data-attached exceptions.

Instead, separate these concepts, and do all the read-from-object / read-from-storage in one logical chunk, separate from `didSetObject()`.

Test Plan:
  - Edited Differential revision.
  - Edited Maniphest task.
  - Edited Project.
  - Edited user profile.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

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

src/applications/differential/customfield/DifferentialCoreCustomField.php

@@ -79,7 +79,7 @@ abstract class DifferentialCoreCustomField
     return true;
   }
 
-  protected function didSetObject(PhabricatorCustomFieldInterface $object) {
+  public function readValueFromObject(PhabricatorCustomFieldInterface $object) {
     if ($this->isCoreFieldRequired()) {
       $this->setFieldError(true);
     }

src/applications/maniphest/controller/ManiphestTaskDetailController.php

@@ -47,15 +47,9 @@ final class ManiphestTaskDetailController extends ManiphestController {
     $field_list = PhabricatorCustomField::getObjectFields(
       $task,
       PhabricatorCustomField::ROLE_VIEW);
-
-    foreach ($field_list->getFields() as $field) {
-      $field->setObject($task);
-      $field->setViewer($user);
-    }
-
-    $field_list->readFieldsFromStorage($task);
-
-    $aux_fields = $field_list->getFields();
+    $field_list
+      ->setViewer($user)
+      ->readFieldsFromStorage($task);
 
     $e_commit = PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT;
     $e_dep_on = PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK;

src/applications/maniphest/controller/ManiphestTaskEditController.php

@@ -158,11 +158,6 @@ final class ManiphestTaskEditController extends ManiphestController {
       $task,
       PhabricatorCustomField::ROLE_EDIT);
     $field_list->setViewer($user);
-
-    foreach ($field_list->getFields() as $field) {
-      $field->setObject($task);
-    }
-
     $field_list->readFieldsFromStorage($task);
 
     $aux_fields = $field_list->getFields();
@@ -389,6 +384,7 @@ final class ManiphestTaskEditController extends ManiphestController {
 
             if ($fields) {
               id(new PhabricatorCustomFieldList($fields))
+                ->setViewer($user)
                 ->readFieldsFromStorage($template_task);
 
               foreach ($fields as $key => $field) {

src/applications/people/customfield/PhabricatorUserBlurbField.php

@@ -29,7 +29,7 @@ final class PhabricatorUserBlurbField
     return true;
   }
 
-  protected function didSetObject(PhabricatorCustomFieldInterface $object) {
+  public function readValueFromObject(PhabricatorCustomFieldInterface $object) {
     $this->value = $object->loadUserProfile()->getBlurb();
   }
 

src/applications/people/customfield/PhabricatorUserRealNameField.php

@@ -29,7 +29,7 @@ final class PhabricatorUserRealNameField
     return true;
   }
 
-  protected function didSetObject(PhabricatorCustomFieldInterface $object) {
+  public function readValueFromObject(PhabricatorCustomFieldInterface $object) {
     $this->value = $object->getRealName();
   }
 

src/applications/people/customfield/PhabricatorUserTitleField.php

@@ -29,7 +29,7 @@ final class PhabricatorUserTitleField
     return true;
   }
 
-  protected function didSetObject(PhabricatorCustomFieldInterface $object) {
+  public function readValueFromObject(PhabricatorCustomFieldInterface $object) {
     $this->value = $object->loadUserProfile()->getTitle();
   }
 

src/infrastructure/customfield/field/PhabricatorCustomField.php

@@ -347,6 +347,7 @@ abstract class PhabricatorCustomField {
    * Sets the object this field belongs to.
    *
    * @param PhabricatorCustomFieldInterface The object this field belongs to.
+   * @return this
    * @task context
    */
   final public function setObject(PhabricatorCustomFieldInterface $object) {
@@ -361,6 +362,21 @@ abstract class PhabricatorCustomField {
   }
 
 
+  /**
+   * Read object data into local field storage, if applicable.
+   *
+   * @param PhabricatorCustomFieldInterface The object this field belongs to.
+   * @return this
+   * @task context
+   */
+  public function readValueFromObject(PhabricatorCustomFieldInterface $object) {
+    if ($this->proxy) {
+      $this->proxy->readValueFromObject($object);
+    }
+    return $this;
+  }
+
+
   /**
    * Get the object this field belongs to.
    *

src/infrastructure/customfield/field/PhabricatorCustomFieldList.php

@@ -39,6 +39,11 @@ final class PhabricatorCustomFieldList extends Phobject {
   public function readFieldsFromStorage(
     PhabricatorCustomFieldInterface $object) {
 
+    foreach ($this->fields as $field) {
+      $field->setObject($object);
+      $field->readValueFromObject($object);
+    }
+
     $keys = array();
     foreach ($this->fields as $field) {
       if ($field->shouldEnableForRole(PhabricatorCustomField::ROLE_STORAGE)) {

src/view/form/control/AphrontFormPolicyControl.php

@@ -180,7 +180,9 @@ final class AphrontFormPolicyControl extends AphrontFormControl {
         'customPlaceholder' => $this->getCustomPolicyPlaceholder(),
       ));
 
-    $selected = $flat_options[$this->getValue()];
+    $selected = idx($flat_options, $this->getValue(), array());
+    $selected_icon = idx($selected, 'icon');
+    $selected_name = idx($selected, 'name');
 
     return phutil_tag(
       'div',
@@ -205,8 +207,8 @@ final class AphrontFormPolicyControl extends AphrontFormControl {
                 'class' => 'phui-button-text',
               ),
               array(
-                $icons[$selected['icon']],
-                $selected['name'],
+                idx($icons, $selected_icon),
+                $selected_name,
               )),
           )),
         $input,