Convert Maniphest custom config to new config types

Summary:
Fixes T12870. Ref T12845.

Technically, this addresses the core issue in T12845 too, but I'm going to convert the rest of the `custom:...` types before closing that.

In particular, for T12870:

  - Validates that keywords are unique across priorities.
  - Fixes missing newline in documentation.
  - Updates documentation to note that keywords are now mandatory and must be unique across priorities.

Test Plan: Edited, deleted and mangled all the Maniphest custom options (priorities, statuses, points, subtypes).

Reviewers: chad, amckinley

Reviewed By: amckinley

Maniphest Tasks: T12870, T12845

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

src/__phutil_library_map__.php

@@ -1488,8 +1488,8 @@ phutil_register_library_map(array(
     'ManiphestHovercardEngineExtension' => 'applications/maniphest/engineextension/ManiphestHovercardEngineExtension.php',
     'ManiphestInfoConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php',
     'ManiphestNameIndex' => 'applications/maniphest/storage/ManiphestNameIndex.php',
-    'ManiphestPointsConfigOptionType' => 'applications/maniphest/config/ManiphestPointsConfigOptionType.php',
+    'ManiphestPointsConfigType' => 'applications/maniphest/config/ManiphestPointsConfigType.php',
-    'ManiphestPriorityConfigOptionType' => 'applications/maniphest/config/ManiphestPriorityConfigOptionType.php',
+    'ManiphestPrioritiesConfigType' => 'applications/maniphest/config/ManiphestPrioritiesConfigType.php',
     'ManiphestPriorityEmailCommand' => 'applications/maniphest/command/ManiphestPriorityEmailCommand.php',
     'ManiphestPrioritySearchConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestPrioritySearchConduitAPIMethod.php',
     'ManiphestProjectNameFulltextEngineExtension' => 'applications/maniphest/engineextension/ManiphestProjectNameFulltextEngineExtension.php',
@@ -1500,11 +1500,11 @@ phutil_register_library_map(array(
     'ManiphestReportController' => 'applications/maniphest/controller/ManiphestReportController.php',
     'ManiphestSchemaSpec' => 'applications/maniphest/storage/ManiphestSchemaSpec.php',
     'ManiphestSearchConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestSearchConduitAPIMethod.php',
-    'ManiphestStatusConfigOptionType' => 'applications/maniphest/config/ManiphestStatusConfigOptionType.php',
     'ManiphestStatusEmailCommand' => 'applications/maniphest/command/ManiphestStatusEmailCommand.php',
     'ManiphestStatusSearchConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestStatusSearchConduitAPIMethod.php',
+    'ManiphestStatusesConfigType' => 'applications/maniphest/config/ManiphestStatusesConfigType.php',
     'ManiphestSubpriorityController' => 'applications/maniphest/controller/ManiphestSubpriorityController.php',
-    'ManiphestSubtypesConfigOptionsType' => 'applications/maniphest/config/ManiphestSubtypesConfigOptionsType.php',
+    'ManiphestSubtypesConfigType' => 'applications/maniphest/config/ManiphestSubtypesConfigType.php',
     'ManiphestTask' => 'applications/maniphest/storage/ManiphestTask.php',
     'ManiphestTaskAssignHeraldAction' => 'applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php',
     'ManiphestTaskAssignOtherHeraldAction' => 'applications/maniphest/herald/ManiphestTaskAssignOtherHeraldAction.php',
@@ -6603,8 +6603,8 @@ phutil_register_library_map(array(
     'ManiphestHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension',
     'ManiphestInfoConduitAPIMethod' => 'ManiphestConduitAPIMethod',
     'ManiphestNameIndex' => 'ManiphestDAO',
-    'ManiphestPointsConfigOptionType' => 'PhabricatorConfigJSONOptionType',
+    'ManiphestPointsConfigType' => 'PhabricatorJSONConfigType',
-    'ManiphestPriorityConfigOptionType' => 'PhabricatorConfigJSONOptionType',
+    'ManiphestPrioritiesConfigType' => 'PhabricatorJSONConfigType',
     'ManiphestPriorityEmailCommand' => 'ManiphestEmailCommand',
     'ManiphestPrioritySearchConduitAPIMethod' => 'ManiphestConduitAPIMethod',
     'ManiphestProjectNameFulltextEngineExtension' => 'PhabricatorFulltextEngineExtension',
@@ -6615,11 +6615,11 @@ phutil_register_library_map(array(
     'ManiphestReportController' => 'ManiphestController',
     'ManiphestSchemaSpec' => 'PhabricatorConfigSchemaSpec',
     'ManiphestSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod',
-    'ManiphestStatusConfigOptionType' => 'PhabricatorConfigJSONOptionType',
     'ManiphestStatusEmailCommand' => 'ManiphestEmailCommand',
     'ManiphestStatusSearchConduitAPIMethod' => 'ManiphestConduitAPIMethod',
+    'ManiphestStatusesConfigType' => 'PhabricatorJSONConfigType',
     'ManiphestSubpriorityController' => 'ManiphestController',
-    'ManiphestSubtypesConfigOptionsType' => 'PhabricatorConfigJSONOptionType',
+    'ManiphestSubtypesConfigType' => 'PhabricatorJSONConfigType',
     'ManiphestTask' => array(
       'ManiphestDAO',
       'PhabricatorSubscribableInterface',

src/applications/config/controller/PhabricatorConfigEditController.php

@@ -290,6 +290,11 @@ final class PhabricatorConfigEditController
         } catch (PhabricatorConfigValidationException $ex) {
           $errors[] = $ex->getMessage();
           $xaction = null;
+        } catch (Exception $ex) {
+          // NOTE: Some older validators throw bare exceptions. Purely in good
+          // taste, it would be nice to convert these at some point.
+          $errors[] = $ex->getMessage();
+          $xaction = null;
         }
 
         return array(

src/applications/maniphest/config/ManiphestPointsConfigOptionType.php

@@ -1,10 +0,0 @@
-<?php
-
-final class ManiphestPointsConfigOptionType
-  extends PhabricatorConfigJSONOptionType {
-
-  public function validateOption(PhabricatorConfigOption $option, $value) {
-    ManiphestTaskPoints::validateConfiguration($value);
-  }
-
-}

src/applications/maniphest/config/ManiphestPointsConfigType.php

@@ -0,0 +1,14 @@
+<?php
+
+final class ManiphestPointsConfigType
+  extends PhabricatorJSONConfigType {
+
+  const TYPEKEY = 'maniphest.points';
+
+  public function validateStoredValue(
+    PhabricatorConfigOption $option,
+    $value) {
+    ManiphestTaskPoints::validateConfiguration($value);
+  }
+
+}

src/applications/maniphest/config/ManiphestPrioritiesConfigType.php

@@ -0,0 +1,14 @@
+<?php
+
+final class ManiphestPrioritiesConfigType
+  extends PhabricatorJSONConfigType {
+
+  const TYPEKEY = 'maniphest.priorities';
+
+  public function validateStoredValue(
+    PhabricatorConfigOption $option,
+    $value) {
+    ManiphestTaskPriority::validateConfiguration($value);
+  }
+
+}

src/applications/maniphest/config/ManiphestPriorityConfigOptionType.php

@@ -1,10 +0,0 @@
-<?php
-
-final class ManiphestPriorityConfigOptionType
-  extends PhabricatorConfigJSONOptionType {
-
-  public function validateOption(PhabricatorConfigOption $option, $value) {
-    ManiphestTaskPriority::validateConfiguration($value);
-  }
-
-}

src/applications/maniphest/config/ManiphestStatusConfigOptionType.php

@@ -1,10 +0,0 @@
-<?php
-
-final class ManiphestStatusConfigOptionType
-  extends PhabricatorConfigJSONOptionType {
-
-  public function validateOption(PhabricatorConfigOption $option, $value) {
-    ManiphestTaskStatus::validateConfiguration($value);
-  }
-
-}

src/applications/maniphest/config/ManiphestStatusesConfigType.php

@@ -0,0 +1,14 @@
+<?php
+
+final class ManiphestStatusesConfigType
+  extends PhabricatorJSONConfigType {
+
+  const TYPEKEY = 'maniphest.statuses';
+
+  public function validateStoredValue(
+    PhabricatorConfigOption $option,
+    $value) {
+    ManiphestTaskStatus::validateConfiguration($value);
+  }
+
+}

src/applications/maniphest/config/ManiphestSubtypesConfigOptionsType.php

@@ -1,10 +0,0 @@
-<?php
-
-final class ManiphestSubtypesConfigOptionsType
-  extends PhabricatorConfigJSONOptionType {
-
-  public function validateOption(PhabricatorConfigOption $option, $value) {
-    PhabricatorEditEngineSubtype::validateConfiguration($value);
-  }
-
-}

src/applications/maniphest/config/ManiphestSubtypesConfigType.php

@@ -0,0 +1,14 @@
+<?php
+
+final class ManiphestSubtypesConfigType
+  extends PhabricatorJSONConfigType {
+
+  const TYPEKEY = 'maniphest.subtypes';
+
+  public function validateStoredValue(
+    PhabricatorConfigOption $option,
+    $value) {
+    PhabricatorEditEngineSubtype::validateConfiguration($value);
+  }
+
+}

src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php

@@ -20,47 +20,47 @@ final class PhabricatorManiphestConfigOptions
   }
 
   public function getOptions() {
-    $priority_type = 'custom:ManiphestPriorityConfigOptionType';
+    $priority_type = 'maniphest.priorities';
     $priority_defaults = array(
       100 => array(
         'name'  => pht('Unbreak Now!'),
+        'keywords' => array('unbreak'),
         'short' => pht('Unbreak!'),
         'color' => 'pink',
-        'keywords' => array('unbreak'),
       ),
       90 => array(
         'name' => pht('Needs Triage'),
+        'keywords' => array('triage'),
         'short' => pht('Triage'),
         'color' => 'violet',
-        'keywords' => array('triage'),
       ),
       80 => array(
         'name' => pht('High'),
+        'keywords' => array('high'),
         'short' => pht('High'),
         'color' => 'red',
-        'keywords' => array('high'),
       ),
       50 => array(
         'name' => pht('Normal'),
+        'keywords' => array('normal'),
         'short' => pht('Normal'),
         'color' => 'orange',
-        'keywords' => array('normal'),
       ),
       25 => array(
         'name' => pht('Low'),
+        'keywords' => array('low'),
         'short' => pht('Low'),
         'color' => 'yellow',
-        'keywords' => array('low'),
       ),
       0 => array(
         'name' => pht('Wishlist'),
+        'keywords' => array('wish', 'wishlist'),
         'short' => pht('Wish'),
         'color' => 'sky',
-        'keywords' => array('wish', 'wishlist'),
       ),
     );
 
-    $status_type = 'custom:ManiphestStatusConfigOptionType';
+    $status_type = 'maniphest.statuses';
     $status_defaults = array(
       'open' => array(
         'name' => pht('Open'),
@@ -265,7 +265,7 @@ EOTEXT
     );
     $fields_json = id(new PhutilJSON())->encodeFormatted($fields_example);
 
-    $points_type = 'custom:ManiphestPointsConfigOptionType';
+    $points_type = 'maniphest.points';
 
     $points_example_1 = array(
       'enabled' => true,
@@ -299,7 +299,7 @@ See the example below for a starting point.
 EOTEXT
 ));
 
-    $subtype_type = 'custom:ManiphestSubtypesConfigOptionsType';
+    $subtype_type = 'maniphest.subtypes';
     $subtype_default_key = PhabricatorEditEngineSubtype::SUBTYPE_DEFAULT;
     $subtype_example = array(
       array(
@@ -349,6 +349,32 @@ EOTEXT
       ,
       $subtype_default_key));
 
+    $priorities_description = $this->deformat(pht(<<<EOTEXT
+Allows you to edit or override the default priorities available in Maniphest,
+like "High", "Normal" and "Low". The configuration should contain a map of
+numeric priority values (where larger numbers correspond to higher priorities)
+to priority specifications (see defaults below for examples).
+
+The keys you can define for a priority are:
+
+  - `name` //Required string.// Name of the priority.
+  - `keywords` //Required list<string>.// List of unique keywords which identify
+    this priority, like "high" or "low". Each priority must have at least one
+    keyword and two priorities may not share the same keyword.
+  - `short` //Optional string.// Alternate shorter name, used in UIs where
+    there is less space available.
+  - `color` //Optional string.// Color for this priority, like "red" or
+    "blue".
+  - `disabled` //Optional bool.// Set to true to prevent users from choosing
+    this priority when creating or editing tasks. Existing tasks will not be
+    affected, and can be batch edited to a different priority or left to
+    eventually die out.
+
+You can choose the default priority for newly created tasks with
+"maniphest.default-priority".
+EOTEXT
+      ));
+
 
     return array(
       $this->newOption('maniphest.custom-field-definitions', 'wild', array())
@@ -367,30 +393,7 @@ EOTEXT
         $priority_type,
         $priority_defaults)
         ->setSummary(pht('Configure Maniphest priority names.'))
-        ->setDescription(
+        ->setDescription($priorities_description),
-          pht(
-            'Allows you to edit or override the default priorities available '.
-            'in Maniphest, like "High", "Normal" and "Low". The configuration '.
-            'should contain a map of priority constants to priority '.
-            'specifications (see defaults below for examples).'.
-            "\n\n".
-            'The keys you can define for a priority are:'.
-            "\n\n".
-            '  - `name` Name of the priority.'."\n".
-            '  - `short` Alternate shorter name, used in UIs where there is '.
-            '    not much space available.'."\n".
-            '  - `color` A color for this priority, like "red" or "blue".'.
-            '  - `keywords` An optional list of keywords which can '.
-            '     be used to select this priority when using `!priority` '.
-            '     commands in email.'."\n".
-            '  - `disabled` Optional boolean to prevent users from choosing '.
-            '     this priority when creating or editing tasks. Existing '.
-            '     tasks will be unaffected, and can be batch edited to a '.
-            '     different priority or left to eventually die out.'.
-            "\n\n".
-            'You can choose which priority is the default for newly created '.
-            'tasks with `%s`.',
-            'maniphest.default-priority')),
       $this->newOption('maniphest.statuses', $status_type, $status_defaults)
         ->setSummary(pht('Configure Maniphest task statuses.'))
         ->setDescription($status_description)

src/applications/maniphest/constants/ManiphestTaskPriority.php

@@ -203,6 +203,7 @@ final class ManiphestTaskPriority extends ManiphestConstants {
           $config));
     }
 
+    $all_keywords = array();
     foreach ($config as $key => $value) {
       if (!ctype_digit((string)$key)) {
         throw new Exception(
@@ -223,9 +224,9 @@ final class ManiphestTaskPriority extends ManiphestConstants {
         $value,
         array(
           'name' => 'string',
+          'keywords' => 'list<string>',
           'short' => 'optional string',
           'color' => 'optional string',
-          'keywords' => 'list<string>',
           'disabled' => 'optional bool',
         ));
 
@@ -242,6 +243,18 @@ final class ManiphestTaskPriority extends ManiphestConstants {
               'low',
               'critical'));
         }
+
+        if (isset($all_keywords[$keyword])) {
+          throw new Exception(
+            pht(
+              'Two different task priorities ("%s" and "%s") have the same '.
+              'keyword ("%s"). Keywords must uniquely identify priorities.',
+              $value['name'],
+              $all_keywords[$keyword],
+              $keyword));
+        }
+
+        $all_keywords[$keyword] = $value['name'];
       }
     }
   }