Remove ManiphestTaskExtensions

Summary: Ref T418. Maniphest has an obsolete class-based field selector. Replace it with CustomField-based selectors, which use the nice config UI and are generally way easier to use.

Test Plan: Added custom fields; edited and viewed custom fields on tasks. Everything worked as expected.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418

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

conf/default.conf.php

@@ -933,10 +933,6 @@ return array(
   // fields to Maniphest, see "Maniphest User Guide: Adding Custom Fields".
   'maniphest.custom-fields' => array(),
 
-  // Class which drives custom field construction. See "Maniphest User Guide:
-  // Adding Custom Fields" in the documentation for more information.
-  'maniphest.custom-task-extensions-class' => 'ManiphestDefaultTaskExtensions',
-
   // What should the default task priority be in create flows?
   // See the constants in @{class:ManiphestTaskPriority} for valid values.
   // Defaults to "needs triage".

src/__phutil_library_map__.php

@@ -695,7 +695,6 @@ phutil_register_library_map(array(
     'ManiphestCustomFieldStorage' => 'applications/maniphest/storage/ManiphestCustomFieldStorage.php',
     'ManiphestCustomFieldStringIndex' => 'applications/maniphest/storage/ManiphestCustomFieldStringIndex.php',
     'ManiphestDAO' => 'applications/maniphest/storage/ManiphestDAO.php',
-    'ManiphestDefaultTaskExtensions' => 'applications/maniphest/extensions/ManiphestDefaultTaskExtensions.php',
     'ManiphestEdgeEventListener' => 'applications/maniphest/event/ManiphestEdgeEventListener.php',
     'ManiphestExcelDefaultFormat' => 'applications/maniphest/export/ManiphestExcelDefaultFormat.php',
     'ManiphestExcelFormat' => 'applications/maniphest/export/ManiphestExcelFormat.php',
@@ -717,7 +716,6 @@ phutil_register_library_map(array(
     'ManiphestTaskDescriptionPreviewController' => 'applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php',
     'ManiphestTaskDetailController' => 'applications/maniphest/controller/ManiphestTaskDetailController.php',
     'ManiphestTaskEditController' => 'applications/maniphest/controller/ManiphestTaskEditController.php',
-    'ManiphestTaskExtensions' => 'applications/maniphest/extensions/ManiphestTaskExtensions.php',
     'ManiphestTaskListController' => 'applications/maniphest/controller/ManiphestTaskListController.php',
     'ManiphestTaskListView' => 'applications/maniphest/view/ManiphestTaskListView.php',
     'ManiphestTaskMailReceiver' => 'applications/maniphest/mail/ManiphestTaskMailReceiver.php',
@@ -2762,7 +2760,6 @@ phutil_register_library_map(array(
     'ManiphestCustomFieldStorage' => 'PhabricatorCustomFieldStorage',
     'ManiphestCustomFieldStringIndex' => 'PhabricatorCustomFieldStringIndexStorage',
     'ManiphestDAO' => 'PhabricatorLiskDAO',
-    'ManiphestDefaultTaskExtensions' => 'ManiphestTaskExtensions',
     'ManiphestEdgeEventListener' => 'PhutilEventListener',
     'ManiphestExcelDefaultFormat' => 'ManiphestExcelFormat',
     'ManiphestExportController' => 'ManiphestController',
@@ -2784,6 +2781,7 @@ phutil_register_library_map(array(
       2 => 'PhabricatorPolicyInterface',
       3 => 'PhabricatorTokenReceiverInterface',
       4 => 'PhrequentTrackableInterface',
+      5 => 'PhabricatorCustomFieldInterface',
     ),
     'ManiphestTaskAuxiliaryStorage' => 'ManiphestDAO',
     'ManiphestTaskDescriptionChangeController' => 'ManiphestController',

src/applications/config/check/PhabricatorSetupCheckExtraConfig.php

@@ -146,6 +146,10 @@ final class PhabricatorSetupCheckExtraConfig extends PhabricatorSetupCheck {
         pht(
           'External loaders have been replaced. Extend `PhabricatorPHIDType` '.
           'to implement new PHID and handle types.'),
+      'maniphest.custom-task-extensions-class' =>
+        pht(
+          'Maniphest fields are now loaded automatically. You can configure '.
+          'them with `maniphest.fields`.'),
     );
 
     return $ancient_config;

src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php

@@ -25,6 +25,35 @@ class ManiphestAuxiliaryFieldDefaultSpecification
   const TYPE_USERS    = 'users';
   const TYPE_HEADER   = 'header';
 
+  public function createFields() {
+    $fields = PhabricatorEnv::getEnvConfig('maniphest.custom-fields');
+    $specs = array();
+    foreach ($fields as $aux => $info) {
+      $spec = new ManiphestAuxiliaryFieldDefaultSpecification();
+      $spec->setAuxiliaryKey($aux);
+      $spec->setLabel(idx($info, 'label'));
+      $spec->setCaption(idx($info, 'caption'));
+      $spec->setFieldType(idx($info, 'type'));
+      $spec->setRequired(idx($info, 'required'));
+
+      $spec->setCheckboxLabel(idx($info, 'checkbox-label'));
+      $spec->setCheckboxValue(idx($info, 'checkbox-value', 1));
+
+      if ($spec->getFieldType() ==
+        ManiphestAuxiliaryFieldDefaultSpecification::TYPE_SELECT) {
+        $spec->setSelectOptions(idx($info, 'options'));
+      }
+
+      $spec->setShouldCopyWhenCreatingSimilarTask(idx($info, 'copy'));
+
+      $spec->setDefaultValue(idx($info, 'default'));
+
+      $specs[] = $spec;
+    }
+
+    return $specs;
+  }
+
   public function getFieldType() {
     return $this->fieldType;
   }

src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php

@@ -14,27 +14,17 @@ abstract class ManiphestAuxiliaryFieldSpecification
   private $auxiliaryKey;
   private $caption;
   private $value;
-  private $user;
-  private $task;
   private $markupEngine;
   private $handles;
 
-  public function setTask(ManiphestTask $task) {
+  // TODO: Remove; obsolete.
-    $this->task = $task;
-    return $this;
-  }
-
   public function getTask() {
-    return $this->task;
+    return $this->getObject();
-  }
-
-  public function setUser(PhabricatorUser $user) {
-    $this->user = $user;
-    return $this;
   }
 
+  // TODO: Remove; obsolete.
   public function getUser() {
-    return $this->user;
+    return $this->getViewer();
   }
 
   public function setLabel($val) {
@@ -229,4 +219,39 @@ abstract class ManiphestAuxiliaryFieldSpecification
     return true;
   }
 
+
+/* -(  API Compatibility With New Custom Fields  )--------------------------- */
+
+
+  public function getFieldKey() {
+    return $this->getAuxiliaryKey();
+  }
+
+  public function shouldAppearInEditView() {
+    return true;
+  }
+
+  public function shouldAppearInPropertyView() {
+    return true;
+  }
+
+
+/* -(  Legacy Migration Support  )------------------------------------------- */
+
+
+  // TODO: Migrate to common storage and remove this.
+  public static function loadLegacyDataFromStorage(
+    ManiphestTask $task,
+    PhabricatorCustomFieldList $list) {
+
+    $task->loadAndAttachAuxiliaryAttributes();
+
+    foreach ($list->getFields() as $field) {
+      if ($task->getID()) {
+        $key = $field->getAuxiliaryKey();
+        $field->setValueFromStorage($task->getAuxiliaryAttribute($key));
+      }
+    }
+  }
+
 }

src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php

@@ -46,6 +46,18 @@ final class PhabricatorManiphestConfigOptions
       ),
     );
 
+    // This is intentionally blank for now, until we can move more Maniphest
+    // logic to custom fields.
+    $default_fields = array();
+
+    foreach ($default_fields as $key => $enabled) {
+      $default_fields[$key] = array(
+        'disabled' => !$enabled,
+      );
+    }
+
+    $custom_field_type = 'custom:PhabricatorCustomFieldConfigOptionType';
+
     return array(
       $this->newOption('maniphest.custom-fields', 'wild', array())
         ->setSummary(pht("Custom Maniphest fields."))
@@ -59,17 +71,9 @@ final class PhabricatorManiphestConfigOptions
           '"type": "int", "caption": "Estimated number of hours this will '.
           'take.", "required": false}}',
           pht('Valid Setting')),
-      $this->newOption(
+      $this->newOption('maniphest.fields', $custom_field_type, $default_fields)
-        'maniphest.custom-task-extensions-class',
+        ->setCustomData(id(new ManiphestTask())->getCustomFieldBaseClass())
-        'class',
+        ->setDescription(pht("Select and reorder task fields.")),
-        'ManiphestDefaultTaskExtensions')
-        ->setBaseClass('ManiphestTaskExtensions')
-        ->setSummary(pht("Class which drives custom field construction."))
-        ->setDescription(
-          pht(
-            "Class which drives custom field construction. See 'Maniphest ".
-            "User Guide: Adding Custom Fields' in the documentation for more ".
-            "information.")),
       $this->newOption('maniphest.priorities', 'wild', $priority_defaults)
         ->setSummary(pht("Configure Maniphest priority names."))
         ->setDescription(

src/applications/maniphest/controller/ManiphestTaskDetailController.php

@@ -35,8 +35,20 @@ final class ManiphestTaskDetailController extends ManiphestController {
       'taskID = %d ORDER BY id ASC',
       $task->getID());
 
-    $extensions = ManiphestTaskExtensions::newExtensions();
+    $field_list = PhabricatorCustomField::getObjectFields(
-    $aux_fields = $extensions->loadFields($task, $user);
+      $task,
+      PhabricatorCustomField::ROLE_VIEW);
+
+    foreach ($field_list->getFields() as $field) {
+      $field->setObject($task);
+      $field->setViewer($user);
+    }
+
+    ManiphestAuxiliaryFieldSpecification::loadLegacyDataFromStorage(
+      $task,
+      $field_list);
+
+    $aux_fields = $field_list->getFields();
 
     $e_commit = PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT;
     $e_dep_on = PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK;

src/applications/maniphest/controller/ManiphestTaskEditController.php

@@ -84,8 +84,20 @@ final class ManiphestTaskEditController extends ManiphestController {
     $errors = array();
     $e_title = true;
 
-    $extensions = ManiphestTaskExtensions::newExtensions();
+    $field_list = PhabricatorCustomField::getObjectFields(
-    $aux_fields = $extensions->loadFields($task, $user);
+      $task,
+      PhabricatorCustomField::ROLE_EDIT);
+
+    foreach ($field_list->getFields() as $field) {
+      $field->setObject($task);
+      $field->setViewer($user);
+    }
+
+    ManiphestAuxiliaryFieldSpecification::loadLegacyDataFromStorage(
+      $task,
+      $field_list);
+
+    $aux_fields = $field_list->getFields();
 
     if ($request->isFormPost()) {
       $changes = array();

src/applications/maniphest/extensions/ManiphestDefaultTaskExtensions.php

@@ -1,38 +0,0 @@
-<?php
-
-/**
- * @group maniphest
- */
-final class ManiphestDefaultTaskExtensions
-  extends ManiphestTaskExtensions {
-
-  public function getAuxiliaryFieldSpecifications() {
-    $fields = PhabricatorEnv::getEnvConfig('maniphest.custom-fields');
-    $specs = array();
-    foreach ($fields as $aux => $info) {
-      $spec = new ManiphestAuxiliaryFieldDefaultSpecification();
-      $spec->setAuxiliaryKey($aux);
-      $spec->setLabel(idx($info, 'label'));
-      $spec->setCaption(idx($info, 'caption'));
-      $spec->setFieldType(idx($info, 'type'));
-      $spec->setRequired(idx($info, 'required'));
-
-      $spec->setCheckboxLabel(idx($info, 'checkbox-label'));
-      $spec->setCheckboxValue(idx($info, 'checkbox-value', 1));
-
-      if ($spec->getFieldType() ==
-        ManiphestAuxiliaryFieldDefaultSpecification::TYPE_SELECT) {
-        $spec->setSelectOptions(idx($info, 'options'));
-      }
-
-      $spec->setShouldCopyWhenCreatingSimilarTask(idx($info, 'copy'));
-
-      $spec->setDefaultValue(idx($info, 'default'));
-
-      $specs[] = $spec;
-    }
-
-    return $specs;
-  }
-
-}

src/applications/maniphest/extensions/ManiphestTaskExtensions.php

@@ -1,44 +0,0 @@
-<?php
-
-/**
- * @group maniphest
- */
-abstract class ManiphestTaskExtensions {
-
-  final public function __construct() {
-    // <empty>
-  }
-
-  abstract public function getAuxiliaryFieldSpecifications();
-
-
-  final public static function newExtensions() {
-    $key = 'maniphest.custom-task-extensions-class';
-    return PhabricatorEnv::newObjectFromConfig($key);
-  }
-
-  public function loadFields(ManiphestTask $task, PhabricatorUser $viewer) {
-    $aux_fields = $this->getAuxiliaryFieldSpecifications();
-    if (!$aux_fields) {
-      return array();
-    }
-
-    $task->loadAndAttachAuxiliaryAttributes();
-
-    foreach ($aux_fields as $aux) {
-      $aux->setUser($viewer);
-      $aux->setTask($task);
-
-      // If we're creating a new task, we don't bother loading any stored data.
-      // This allows any defaults configured by the Extensions object to
-      // survive.
-      if ($task->getID()) {
-        $key = $aux->getAuxiliaryKey();
-        $aux->setValueFromStorage($task->getAuxiliaryAttribute($key));
-      }
-    }
-
-    return $aux_fields;
-  }
-
-}

src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php

@@ -40,20 +40,7 @@ final class PhabricatorManiphestTaskTestDataGenerator
       $transaction->setNewValue($value);
       $transactions[] = $transaction;
     }
-    // Accumulate Auxiliary Transactions
+
-    $aux_fields = id(ManiphestTaskExtensions::newExtensions())
-      ->loadFields($task, $author);
-    if ($aux_fields) {
-      foreach ($aux_fields as $aux_field) {
-        $transaction = clone $template;
-        $transaction->setTransactionType(
-          ManiphestTransactionType::TYPE_AUXILIARY);
-        $aux_key = $aux_field->getAuxiliaryKey();
-        $transaction->setMetadataValue('aux:key', $aux_key);
-        $transaction->setNewValue($aux_field->getValueForStorage());
-        $transactions[] = $transaction;
-      }
-    }
     // Apply Transactions
     $editor = id(new ManiphestTransactionEditor())
       ->setActor($author)

src/applications/maniphest/storage/ManiphestTask.php

@@ -8,7 +8,8 @@ final class ManiphestTask extends ManiphestDAO
     PhabricatorMarkupInterface,
     PhabricatorPolicyInterface,
     PhabricatorTokenReceiverInterface,
-    PhrequentTrackableInterface {
+    PhrequentTrackableInterface,
+    PhabricatorCustomFieldInterface {
 
   const MARKUP_FIELD_DESCRIPTION = 'markup:desc';
 
@@ -37,6 +38,7 @@ final class ManiphestTask extends ManiphestDAO
   private $auxiliaryAttributes = self::ATTACHABLE;
   private $auxiliaryDirty = array();
   private $groupByProjectPHID = self::ATTACHABLE;
+  private $customFields = self::ATTACHABLE;
 
   public function getConfiguration() {
     return array(
@@ -305,4 +307,25 @@ final class ManiphestTask extends ManiphestDAO
         )));
   }
 
+
+/* -(  PhabricatorCustomFieldInterface  )------------------------------------ */
+
+
+  public function getCustomFieldSpecificationForRole($role) {
+    return PhabricatorEnv::getEnvConfig('maniphest.fields');
+  }
+
+  public function getCustomFieldBaseClass() {
+    return 'ManiphestCustomField';
+  }
+
+  public function getCustomFields() {
+    return $this->assertAttached($this->customFields);
+  }
+
+  public function attachCustomFields(PhabricatorCustomFieldAttachment $fields) {
+    $this->customFields = $fields;
+    return $this;
+  }
+
 }

src/docs/user/userguide/maniphest_custom.diviner

@@ -72,17 +72,3 @@ the field. These options are available:
     into the new task, while others are not; by default, fields are not copied.
     If you want this field to be copied, specify `true` for the `copy` property.
 
-= Advanced Field Customization =
-
-If you want to add fields with more specialized validation, storage, or
-rendering logic, you can do so with a little work:
-
-  - Extend @{class:ManiphestAuxiliaryFieldSpecification} and implement
-    your specialized rendering, validation, storage, etc., logic.
-  - Extend @{class:ManiphestTaskExtensions} and return a list of fields which
-    includes your custom field objects.
-  - Set `maniphest.custom-extensions` to the name of your new extensions
-    class.
-
-This is relatively advanced but should give you significant flexibility in
-defining custom fields.

src/infrastructure/customfield/field/PhabricatorCustomFieldList.php

@@ -45,11 +45,14 @@ final class PhabricatorCustomFieldList extends Phobject {
 
     $table = head($keys)->newStorageObject();
 
-    $objects = $table->loadAllWhere(
+    $objects = array();
-      'objectPHID = %s AND fieldIndex IN (%Ls)',
+    if ($object->getPHID()) {
-      $object->getPHID(),
+      $objects = $table->loadAllWhere(
-      array_keys($keys));
+        'objectPHID = %s AND fieldIndex IN (%Ls)',
-    $objects = mpull($objects, null, 'getFieldIndex');
+        $object->getPHID(),
+        array_keys($keys));
+      $objects = mpull($objects, null, 'getFieldIndex');
+    }
 
     foreach ($keys as $key => $field) {
       $storage = idx($objects, $key);