From 467be5e53fe6f574082ab7c470834103b2647eef Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 26 Jun 2017 09:22:40 -0700 Subject: [PATCH] Convert the "list" and "list" Config option types Summary: Ref T12845. This updates the "list" and "list" 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 | 6 ++ .../PhabricatorConfigEditController.php | 22 ----- ...PhabricatorConfigManagementSetWorkflow.php | 25 +---- .../PhabricatorApplicationConfigOptions.php | 54 ---------- .../type/PhabricatorRegexListConfigType.php | 24 +++++ .../type/PhabricatorStringListConfigType.php | 8 ++ .../type/PhabricatorTextListConfigType.php | 98 +++++++++++++++++++ 7 files changed, 141 insertions(+), 96 deletions(-) create mode 100644 src/applications/config/type/PhabricatorRegexListConfigType.php create mode 100644 src/applications/config/type/PhabricatorStringListConfigType.php create mode 100644 src/applications/config/type/PhabricatorTextListConfigType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a0cfff88f3..7753782858 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index efdd64e92a..0d4aea3252 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -347,20 +347,6 @@ final class PhabricatorConfigEditController $set_value = null; switch ($type) { - case 'list': - case 'list': - $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': - case 'list': - 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': - case 'list': - $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.')); diff --git a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php index d0407291af..e1b54e4dcf 100644 --- a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php +++ b/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': - $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': - $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; diff --git a/src/applications/config/type/PhabricatorRegexListConfigType.php b/src/applications/config/type/PhabricatorRegexListConfigType.php new file mode 100644 index 0000000000..b37b440c7e --- /dev/null +++ b/src/applications/config/type/PhabricatorRegexListConfigType.php @@ -0,0 +1,24 @@ +'; + + 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)); + } + } + +} diff --git a/src/applications/config/type/PhabricatorStringListConfigType.php b/src/applications/config/type/PhabricatorStringListConfigType.php new file mode 100644 index 0000000000..70f4c04316 --- /dev/null +++ b/src/applications/config/type/PhabricatorStringListConfigType.php @@ -0,0 +1,8 @@ +'; + +} diff --git a/src/applications/config/type/PhabricatorTextListConfigType.php b/src/applications/config/type/PhabricatorTextListConfigType.php new file mode 100644 index 0000000000..20f5362ea6 --- /dev/null +++ b/src/applications/config/type/PhabricatorTextListConfigType.php @@ -0,0 +1,98 @@ +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; + } + +}