From 28eaacb491a13e49046988cc4092399b307cb94a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Sep 2013 15:58:35 -0700 Subject: [PATCH] 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 | 4 -- src/__phutil_library_map__.php | 4 +- .../PhabricatorSetupCheckExtraConfig.php | 4 ++ ...hestAuxiliaryFieldDefaultSpecification.php | 29 ++++++++++ .../ManiphestAuxiliaryFieldSpecification.php | 53 ++++++++++++++----- .../PhabricatorManiphestConfigOptions.php | 26 +++++---- .../ManiphestTaskDetailController.php | 16 +++++- .../ManiphestTaskEditController.php | 16 +++++- .../ManiphestDefaultTaskExtensions.php | 38 ------------- .../extensions/ManiphestTaskExtensions.php | 44 --------------- ...bricatorManiphestTaskTestDataGenerator.php | 15 +----- .../maniphest/storage/ManiphestTask.php | 25 ++++++++- .../user/userguide/maniphest_custom.diviner | 14 ----- .../field/PhabricatorCustomFieldList.php | 13 +++-- 14 files changed, 149 insertions(+), 152 deletions(-) delete mode 100644 src/applications/maniphest/extensions/ManiphestDefaultTaskExtensions.php delete mode 100644 src/applications/maniphest/extensions/ManiphestTaskExtensions.php diff --git a/conf/default.conf.php b/conf/default.conf.php index cf9f963d4e..47763d75e8 100644 --- a/conf/default.conf.php +++ b/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". diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f9dc3ae204..04466fadea 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php b/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php index 6d6096d4ac..a0634df705 100644 --- a/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php +++ b/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; diff --git a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php index 8a2ef852e9..eaf828555d 100644 --- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php +++ b/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; } diff --git a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php index afcbe1e2e6..471b8f5ad1 100644 --- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php +++ b/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) { - $this->task = $task; - return $this; - } - + // TODO: Remove; obsolete. public function getTask() { - return $this->task; - } - - public function setUser(PhabricatorUser $user) { - $this->user = $user; - return $this; + return $this->getObject(); } + // 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)); + } + } + } + } diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php index 11f70d1619..8a5a4903e7 100644 --- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php +++ b/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( - 'maniphest.custom-task-extensions-class', - 'class', - '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.fields', $custom_field_type, $default_fields) + ->setCustomData(id(new ManiphestTask())->getCustomFieldBaseClass()) + ->setDescription(pht("Select and reorder task fields.")), $this->newOption('maniphest.priorities', 'wild', $priority_defaults) ->setSummary(pht("Configure Maniphest priority names.")) ->setDescription( diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 58e9bd594c..79e6e32743 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/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(); - $aux_fields = $extensions->loadFields($task, $user); + $field_list = PhabricatorCustomField::getObjectFields( + $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; diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 4e773cdd35..388720770c 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -84,8 +84,20 @@ final class ManiphestTaskEditController extends ManiphestController { $errors = array(); $e_title = true; - $extensions = ManiphestTaskExtensions::newExtensions(); - $aux_fields = $extensions->loadFields($task, $user); + $field_list = PhabricatorCustomField::getObjectFields( + $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(); diff --git a/src/applications/maniphest/extensions/ManiphestDefaultTaskExtensions.php b/src/applications/maniphest/extensions/ManiphestDefaultTaskExtensions.php deleted file mode 100644 index 3183019805..0000000000 --- a/src/applications/maniphest/extensions/ManiphestDefaultTaskExtensions.php +++ /dev/null @@ -1,38 +0,0 @@ - $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; - } - -} diff --git a/src/applications/maniphest/extensions/ManiphestTaskExtensions.php b/src/applications/maniphest/extensions/ManiphestTaskExtensions.php deleted file mode 100644 index da40b25de2..0000000000 --- a/src/applications/maniphest/extensions/ManiphestTaskExtensions.php +++ /dev/null @@ -1,44 +0,0 @@ - - } - - 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; - } - -} diff --git a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php index 494213eb59..9e38f55b4f 100644 --- a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php +++ b/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) diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 5d3cdeb77e..9bab1223d1 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/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; + } + } diff --git a/src/docs/user/userguide/maniphest_custom.diviner b/src/docs/user/userguide/maniphest_custom.diviner index c6a262c034..09a000d862 100644 --- a/src/docs/user/userguide/maniphest_custom.diviner +++ b/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. diff --git a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php index d4b9f0747e..2a8a9b8341 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php @@ -45,11 +45,14 @@ final class PhabricatorCustomFieldList extends Phobject { $table = head($keys)->newStorageObject(); - $objects = $table->loadAllWhere( - 'objectPHID = %s AND fieldIndex IN (%Ls)', - $object->getPHID(), - array_keys($keys)); - $objects = mpull($objects, null, 'getFieldIndex'); + $objects = array(); + if ($object->getPHID()) { + $objects = $table->loadAllWhere( + 'objectPHID = %s AND fieldIndex IN (%Ls)', + $object->getPHID(), + array_keys($keys)); + $objects = mpull($objects, null, 'getFieldIndex'); + } foreach ($keys as $key => $field) { $storage = idx($objects, $key);