Convert the "list<string>" and "list<regex>" Config option types

Summary: Ref T12845. This updates the "list<string>" and "list<regex>" options.

Test Plan: Set, deleted, and mangled options of these types from the web UI and CLI.

Reviewers: chad, amckinley

Reviewed By: amckinley

Maniphest Tasks: T12845

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

src/__phutil_library_map__.php

@@ -3746,6 +3746,7 @@ phutil_register_library_map(array(
     'PhabricatorRecaptchaConfigOptions' => 'applications/config/option/PhabricatorRecaptchaConfigOptions.php',
     'PhabricatorRedirectController' => 'applications/base/controller/PhabricatorRedirectController.php',
     'PhabricatorRefreshCSRFController' => 'applications/auth/controller/PhabricatorRefreshCSRFController.php',
+    'PhabricatorRegexListConfigType' => 'applications/config/type/PhabricatorRegexListConfigType.php',
     'PhabricatorRegistrationProfile' => 'applications/people/storage/PhabricatorRegistrationProfile.php',
     'PhabricatorReleephApplication' => 'applications/releeph/application/PhabricatorReleephApplication.php',
     'PhabricatorReleephApplicationConfigOptions' => 'applications/releeph/config/PhabricatorReleephApplicationConfigOptions.php',
@@ -4071,6 +4072,7 @@ phutil_register_library_map(array(
     'PhabricatorStorageSchemaSpec' => 'infrastructure/storage/schema/PhabricatorStorageSchemaSpec.php',
     'PhabricatorStorageSetupCheck' => 'applications/config/check/PhabricatorStorageSetupCheck.php',
     'PhabricatorStringConfigType' => 'applications/config/type/PhabricatorStringConfigType.php',
+    'PhabricatorStringListConfigType' => 'applications/config/type/PhabricatorStringListConfigType.php',
     'PhabricatorStringListEditField' => 'applications/transactions/editfield/PhabricatorStringListEditField.php',
     'PhabricatorStringSetting' => 'applications/settings/setting/PhabricatorStringSetting.php',
     'PhabricatorSubmitEditField' => 'applications/transactions/editfield/PhabricatorSubmitEditField.php',
@@ -4133,6 +4135,7 @@ phutil_register_library_map(array(
     'PhabricatorTextAreaEditField' => 'applications/transactions/editfield/PhabricatorTextAreaEditField.php',
     'PhabricatorTextConfigType' => 'applications/config/type/PhabricatorTextConfigType.php',
     'PhabricatorTextEditField' => 'applications/transactions/editfield/PhabricatorTextEditField.php',
+    'PhabricatorTextListConfigType' => 'applications/config/type/PhabricatorTextListConfigType.php',
     'PhabricatorTime' => 'infrastructure/time/PhabricatorTime.php',
     'PhabricatorTimeFormatSetting' => 'applications/settings/setting/PhabricatorTimeFormatSetting.php',
     'PhabricatorTimeGuard' => 'infrastructure/time/PhabricatorTimeGuard.php',
@@ -9207,6 +9210,7 @@ phutil_register_library_map(array(
     'PhabricatorRecaptchaConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorRedirectController' => 'PhabricatorController',
     'PhabricatorRefreshCSRFController' => 'PhabricatorAuthController',
+    'PhabricatorRegexListConfigType' => 'PhabricatorTextListConfigType',
     'PhabricatorRegistrationProfile' => 'Phobject',
     'PhabricatorReleephApplication' => 'PhabricatorApplication',
     'PhabricatorReleephApplicationConfigOptions' => 'PhabricatorApplicationConfigOptions',
@@ -9609,6 +9613,7 @@ phutil_register_library_map(array(
     'PhabricatorStorageSchemaSpec' => 'PhabricatorConfigSchemaSpec',
     'PhabricatorStorageSetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorStringConfigType' => 'PhabricatorTextConfigType',
+    'PhabricatorStringListConfigType' => 'PhabricatorTextListConfigType',
     'PhabricatorStringListEditField' => 'PhabricatorEditField',
     'PhabricatorStringSetting' => 'PhabricatorSetting',
     'PhabricatorSubmitEditField' => 'PhabricatorEditField',
@@ -9670,6 +9675,7 @@ phutil_register_library_map(array(
     'PhabricatorTextAreaEditField' => 'PhabricatorEditField',
     'PhabricatorTextConfigType' => 'PhabricatorConfigType',
     'PhabricatorTextEditField' => 'PhabricatorEditField',
+    'PhabricatorTextListConfigType' => 'PhabricatorTextConfigType',
     'PhabricatorTime' => 'Phobject',
     'PhabricatorTimeFormatSetting' => 'PhabricatorSelectSetting',
     'PhabricatorTimeGuard' => 'Phobject',

src/applications/config/controller/PhabricatorConfigEditController.php

@@ -347,20 +347,6 @@ final class PhabricatorConfigEditController
       $set_value = null;
 
       switch ($type) {
-        case 'list<string>':
-        case 'list<regex>':
-          $set_value = phutil_split_lines(
-            $request->getStr('value'),
-            $retain_endings = false);
-
-          foreach ($set_value as $key => $v) {
-            if (!strlen($v)) {
-              unset($set_value[$key]);
-            }
-          }
-          $set_value = array_values($set_value);
-
-          break;
         case 'set':
           $set_value = array_fill_keys($request->getStrList('value'), true);
           break;
@@ -441,9 +427,6 @@ final class PhabricatorConfigEditController
           return $value;
         case 'bool':
           return $value ? 'true' : 'false';
-        case 'list<string>':
-        case 'list<regex>':
-          return implode("\n", nonempty($value, array()));
         case 'set':
           return implode("\n", nonempty(array_keys($value), array()));
         default:
@@ -497,11 +480,6 @@ final class PhabricatorConfigEditController
           $control = id(new AphrontFormSelectControl())
             ->setOptions($names);
           break;
-        case 'list<string>':
-        case 'list<regex>':
-          $control = id(new AphrontFormTextAreaControl())
-            ->setCaption(pht('Separate values with newlines.'));
-          break;
         case 'set':
           $control = id(new AphrontFormTextAreaControl())
             ->setCaption(pht('Separate values with newlines or commas.'));

src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php

@@ -65,6 +65,7 @@ final class PhabricatorConfigManagementSetWorkflow
         $value = $type->newValueFromCommandLineValue(
           $option,
           $value);
+        $type->validateStoredValue($option, $value);
       } catch (PhabricatorConfigValidationException $ex) {
         throw new PhutilArgumentUsageException($ex->getMessage());
       }
@@ -109,26 +110,10 @@ final class PhabricatorConfigManagementSetWorkflow
                   $command);
                 break;
               default:
-                if (preg_match('/^list</', $type)) {
+                $message = pht(
-                  $command = csprintf(
+                  'Config key "%s" is of type "%s". Specify it in JSON.',
-                    './bin/config set %R %s',
+                  $key,
-                    $key,
+                  $type);
-                    '["a", "b", "c"]');
-
-                  $message = sprintf(
-                    "%s\n\n    %s\n",
-                    pht(
-                      'Config key "%s" is of type "%s". Specify it in JSON. '.
-                      'For example:',
-                      $key,
-                      $type),
-                    $command);
-                } else {
-                  $message = pht(
-                    'Config key "%s" is of type "%s". Specify it in JSON.',
-                    $key,
-                    $type);
-                }
                 break;
             }
             throw new PhutilArgumentUsageException($message);

src/applications/config/option/PhabricatorApplicationConfigOptions.php

@@ -85,60 +85,6 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject {
           }
         }
         break;
-      case 'list<regex>':
-        $valid = true;
-        if (!is_array($value)) {
-          throw new PhabricatorConfigValidationException(
-            pht(
-              "Option '%s' must be a list of regular expressions, but value ".
-              "is not an array.",
-              $option->getKey()));
-        }
-        if ($value && array_keys($value) != range(0, count($value) - 1)) {
-          throw new PhabricatorConfigValidationException(
-            pht(
-              "Option '%s' must be a list of regular expressions, but the ".
-              "value is a map with unnatural keys.",
-              $option->getKey()));
-        }
-        foreach ($value as $v) {
-          $ok = @preg_match($v, '');
-          if ($ok === false) {
-            throw new PhabricatorConfigValidationException(
-              pht(
-                "Option '%s' must be a list of regular expressions, but the ".
-                "value '%s' is not a valid regular expression.",
-                $option->getKey(),
-                $v));
-          }
-        }
-        break;
-      case 'list<string>':
-        $valid = true;
-        if (!is_array($value)) {
-          throw new PhabricatorConfigValidationException(
-            pht(
-              "Option '%s' must be a list of strings, but value is not ".
-              "an array.",
-              $option->getKey()));
-        }
-        if ($value && array_keys($value) != range(0, count($value) - 1)) {
-          throw new PhabricatorConfigValidationException(
-            pht(
-              "Option '%s' must be a list of strings, but the value is a ".
-              "map with unnatural keys.",
-              $option->getKey()));
-        }
-        foreach ($value as $v) {
-          if (!is_string($v)) {
-            throw new PhabricatorConfigValidationException(
-              pht(
-                "Option '%s' must be a list of strings, but it contains one ".
-                "or more non-strings.",
-                $option->getKey()));
-          }
-        }
-        break;
       case 'wild':
       default:
         break;

src/applications/config/type/PhabricatorRegexListConfigType.php

@@ -0,0 +1,24 @@
+<?php
+
+final class PhabricatorRegexListConfigType
+  extends PhabricatorTextListConfigType {
+
+  const TYPEKEY = 'list<regex>';
+
+  protected function validateStoredItem(
+    PhabricatorConfigOption $option,
+    $value) {
+
+    $ok = @preg_match($value, '');
+    if ($ok === false) {
+      throw $this->newException(
+        pht(
+          'Option "%s" is of type "%s" and must be set to a list of valid '.
+          'regular expressions, but "%s" is not a valid regular expression.',
+          $option->getKey(),
+          $this->getTypeKey(),
+          $value));
+    }
+  }
+
+}

src/applications/config/type/PhabricatorStringListConfigType.php

@@ -0,0 +1,8 @@
+<?php
+
+final class PhabricatorStringListConfigType
+  extends PhabricatorTextListConfigType {
+
+  const TYPEKEY = 'list<string>';
+
+}

src/applications/config/type/PhabricatorTextListConfigType.php

@@ -0,0 +1,98 @@
+<?php
+
+abstract class PhabricatorTextListConfigType
+  extends PhabricatorTextConfigType {
+
+  protected function newControl(PhabricatorConfigOption $option) {
+    return id(new AphrontFormTextAreaControl())
+      ->setCaption(pht('Separate values with newlines.'));
+  }
+
+  protected function newCanonicalValue(
+    PhabricatorConfigOption $option,
+    $value) {
+
+    $value = phutil_split_lines($value, $retain_endings = false);
+    foreach ($value as $k => $v) {
+      if (!strlen($v)) {
+        unset($value[$k]);
+      }
+    }
+
+    return array_values($value);
+  }
+
+  public function newValueFromCommandLineValue(
+    PhabricatorConfigOption $option,
+    $value) {
+
+    try {
+      $value = phutil_json_decode($value);
+    } catch (Exception $ex) {
+      throw $this->newException(
+        pht(
+          'Option "%s" is of type "%s", but the value you provided is not a '.
+          'valid JSON list. When setting a list option from the command '.
+          'line, specify the value in JSON. You may need to quote the '.
+          'value for your shell (for example: \'["a", "b", ...]\').',
+          $option->getKey(),
+          $this->getTypeKey()));
+    }
+
+    return $value;
+  }
+
+  public function newDisplayValue(
+    PhabricatorConfigOption $option,
+    $value) {
+    return implode("\n", $value);
+  }
+
+  public function validateStoredValue(
+    PhabricatorConfigOption $option,
+    $value) {
+
+    if (!is_array($value)) {
+      throw $this->newException(
+        pht(
+          'Option "%s" is of type "%s", but the configured value is not '.
+          'a list.',
+          $option->getKey(),
+          $this->getTypeKey()));
+    }
+
+    $expect_key = 0;
+    foreach ($value as $k => $v) {
+      if (!is_string($v)) {
+        throw $this->newException(
+          pht(
+            'Option "%s" is of type "%s", but the item at index "%s" of the '.
+            'list is not a string.',
+            $option->getKey(),
+            $this->getTypeKey(),
+            $k));
+      }
+
+      // Make sure this is a list with keys "0, 1, 2, ...", not a map with
+      // arbitrary keys.
+      if ($k != $expect_key) {
+        throw $this->newException(
+          pht(
+            'Option "%s" is of type "%s", but the value is not a list: it '.
+            'is a map with unnatural or sparse keys.',
+            $option->getKey(),
+            $this->getTypeKey()));
+      }
+      $expect_key++;
+
+      $this->validateStoredItem($option, $v);
+    }
+  }
+
+  protected function validateStoredItem(
+    PhabricatorConfigOption $option,
+    $value) {
+    return;
+  }
+
+}