From 9d30d49cfc7311dc84bd3ec446c5123e5aaf0497 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 26 Jun 2017 09:05:26 -0700 Subject: [PATCH] Convert "enum" and "string" config options to new modular option types Summary: Ref T12845. This moves the "enum" and "string" types to the new code. Test Plan: Set, deleted, and tried to set invalid values for various enum and string config values (header color, mail prefixes, etc) from the CLI and web. Reviewers: chad, amckinley Reviewed By: amckinley Maniphest Tasks: T12845 Differential Revision: https://secure.phabricator.com/D18156 --- src/__phutil_library_map__.php | 4 ++ .../PhabricatorConfigEditController.php | 20 +-------- ...PhabricatorConfigManagementSetWorkflow.php | 2 - .../PhabricatorApplicationConfigOptions.php | 8 ---- .../PhabricatorMetaMTAConfigOptions.php | 6 +-- .../option/PhabricatorUIConfigOptions.php | 12 +++--- .../config/type/PhabricatorEnumConfigType.php | 43 +++++++++++++++++++ .../type/PhabricatorStringConfigType.php | 22 ++++++++++ .../config/type/PhabricatorTextConfigType.php | 6 +++ .../PhabricatorDifferentialConfigOptions.php | 5 ++- 10 files changed, 89 insertions(+), 39 deletions(-) create mode 100644 src/applications/config/type/PhabricatorEnumConfigType.php create mode 100644 src/applications/config/type/PhabricatorStringConfigType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6ad24c6af3..a0cfff88f3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2755,6 +2755,7 @@ phutil_register_library_map(array( 'PhabricatorEmojiRemarkupRule' => 'applications/macro/markup/PhabricatorEmojiRemarkupRule.php', 'PhabricatorEmojiTranslation' => 'infrastructure/internationalization/translation/PhabricatorEmojiTranslation.php', 'PhabricatorEmptyQueryException' => 'infrastructure/query/PhabricatorEmptyQueryException.php', + 'PhabricatorEnumConfigType' => 'applications/config/type/PhabricatorEnumConfigType.php', 'PhabricatorEnv' => 'infrastructure/env/PhabricatorEnv.php', 'PhabricatorEnvTestCase' => 'infrastructure/env/__tests__/PhabricatorEnvTestCase.php', 'PhabricatorEpochEditField' => 'applications/transactions/editfield/PhabricatorEpochEditField.php', @@ -4069,6 +4070,7 @@ phutil_register_library_map(array( 'PhabricatorStoragePatch' => 'infrastructure/storage/management/PhabricatorStoragePatch.php', 'PhabricatorStorageSchemaSpec' => 'infrastructure/storage/schema/PhabricatorStorageSchemaSpec.php', 'PhabricatorStorageSetupCheck' => 'applications/config/check/PhabricatorStorageSetupCheck.php', + 'PhabricatorStringConfigType' => 'applications/config/type/PhabricatorStringConfigType.php', 'PhabricatorStringListEditField' => 'applications/transactions/editfield/PhabricatorStringListEditField.php', 'PhabricatorStringSetting' => 'applications/settings/setting/PhabricatorStringSetting.php', 'PhabricatorSubmitEditField' => 'applications/transactions/editfield/PhabricatorSubmitEditField.php', @@ -8044,6 +8046,7 @@ phutil_register_library_map(array( 'PhabricatorEmojiRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorEmojiTranslation' => 'PhutilTranslation', 'PhabricatorEmptyQueryException' => 'Exception', + 'PhabricatorEnumConfigType' => 'PhabricatorTextConfigType', 'PhabricatorEnv' => 'Phobject', 'PhabricatorEnvTestCase' => 'PhabricatorTestCase', 'PhabricatorEpochEditField' => 'PhabricatorEditField', @@ -9605,6 +9608,7 @@ phutil_register_library_map(array( 'PhabricatorStoragePatch' => 'Phobject', 'PhabricatorStorageSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhabricatorStorageSetupCheck' => 'PhabricatorSetupCheck', + 'PhabricatorStringConfigType' => 'PhabricatorTextConfigType', 'PhabricatorStringListEditField' => 'PhabricatorEditField', 'PhabricatorStringSetting' => 'PhabricatorSetting', 'PhabricatorSubmitEditField' => 'PhabricatorEditField', diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index 33e796200b..efdd64e92a 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -285,7 +285,7 @@ final class PhabricatorConfigEditController $canonical_value = $type->newValueFromRequestValue( $option, $value); - $type->validateStoredValue($canonical_value); + $type->validateStoredValue($option, $canonical_value); $xaction = $type->newTransaction($option, $canonical_value); } catch (PhabricatorConfigValidationException $ex) { $errors[] = $ex->getMessage(); @@ -347,10 +347,6 @@ final class PhabricatorConfigEditController $set_value = null; switch ($type) { - case 'string': - case 'enum': - $set_value = (string)$value; - break; case 'list': case 'list': $set_value = phutil_split_lines( @@ -441,8 +437,6 @@ final class PhabricatorConfigEditController } else { $type = $option->getType(); switch ($type) { - case 'string': - case 'enum': case 'class': return $value; case 'bool': @@ -479,9 +473,6 @@ final class PhabricatorConfigEditController } else { $type = $option->getType(); switch ($type) { - case 'string': - $control = id(new AphrontFormTextControl()); - break; case 'bool': $control = id(new AphrontFormSelectControl()) ->setOptions( @@ -491,15 +482,6 @@ final class PhabricatorConfigEditController 'false' => idx($option->getBoolOptions(), 1), )); break; - case 'enum': - $options = array_mergev( - array( - array('' => pht('(Use Default)')), - $option->getEnumOptions(), - )); - $control = id(new AphrontFormSelectControl()) - ->setOptions($options); - break; case 'class': $symbols = id(new PhutilSymbolLoader()) ->setType('class') diff --git a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php index 823f1db9b3..d0407291af 100644 --- a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php +++ b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php @@ -71,9 +71,7 @@ final class PhabricatorConfigManagementSetWorkflow } else { $type = $option->getType(); switch ($type) { - case 'string': case 'class': - case 'enum': $value = (string)$value; break; case 'bool': diff --git a/src/applications/config/option/PhabricatorApplicationConfigOptions.php b/src/applications/config/option/PhabricatorApplicationConfigOptions.php index 39538a2af5..1742169fde 100644 --- a/src/applications/config/option/PhabricatorApplicationConfigOptions.php +++ b/src/applications/config/option/PhabricatorApplicationConfigOptions.php @@ -52,14 +52,6 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject { $option->getKey())); } break; - case 'string': - if (!is_string($value)) { - throw new PhabricatorConfigValidationException( - pht( - "Option '%s' is of type string, but value is not a string.", - $option->getKey())); - } - break; case 'class': $symbols = id(new PhutilSymbolLoader()) ->setType('class') diff --git a/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php b/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php index 556b1b169a..cc9732dc70 100644 --- a/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php +++ b/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php @@ -297,9 +297,9 @@ EODOC $this->newOption('metamta.user-address-format', 'enum', 'full') ->setEnumOptions( array( - 'short' => 'short', - 'real' => 'real', - 'full' => 'full', + 'short' => pht('Short'), + 'real' => pht('Real'), + 'full' => pht('Full'), )) ->setSummary(pht('Control how Phabricator renders user names in mail.')) ->setDescription($address_description) diff --git a/src/applications/config/option/PhabricatorUIConfigOptions.php b/src/applications/config/option/PhabricatorUIConfigOptions.php index cef3fbd342..7b2823f6ba 100644 --- a/src/applications/config/option/PhabricatorUIConfigOptions.php +++ b/src/applications/config/option/PhabricatorUIConfigOptions.php @@ -21,12 +21,12 @@ final class PhabricatorUIConfigOptions public function getOptions() { $options = array( - 'blindigo' => 'blindigo', - 'red' => 'red', - 'blue' => 'blue', - 'green' => 'green', - 'indigo' => 'indigo', - 'dark' => 'dark', + 'blindigo' => pht('Blindigo'), + 'red' => pht('Red'), + 'blue' => pht('Blue'), + 'green' => pht('Green'), + 'indigo' => pht('Indigo'), + 'dark' => pht('Dark'), ); $example = <<newException( + pht( + 'Option "%s" is of type "%s", but the configured value is not '. + 'a string.', + $option->getKey(), + $this->getTypeKey())); + } + + $map = $option->getEnumOptions(); + if (!isset($map[$value])) { + throw $this->newException( + pht( + 'Option "%s" is of type "%s", but the current value ("%s") is not '. + 'among the set of valid values: %s.', + $option->getKey(), + $this->getTypeKey(), + $value, + implode(', ', array_keys($map)))); + } + } + + protected function newControl(PhabricatorConfigOption $option) { + $map = array( + '' => pht('(Use Default)'), + ) + $option->getEnumOptions(); + + return id(new AphrontFormSelectControl()) + ->setOptions($map); + } + +} diff --git a/src/applications/config/type/PhabricatorStringConfigType.php b/src/applications/config/type/PhabricatorStringConfigType.php new file mode 100644 index 0000000000..da05b75780 --- /dev/null +++ b/src/applications/config/type/PhabricatorStringConfigType.php @@ -0,0 +1,22 @@ +newException( + pht( + 'Option "%s" is of type "%s", but the configured value is not '. + 'a string.', + $option->getKey(), + $this->getTypeKey())); + } + } + +} diff --git a/src/applications/config/type/PhabricatorTextConfigType.php b/src/applications/config/type/PhabricatorTextConfigType.php index fee3977487..8e67d65b7a 100644 --- a/src/applications/config/type/PhabricatorTextConfigType.php +++ b/src/applications/config/type/PhabricatorTextConfigType.php @@ -10,6 +10,12 @@ abstract class PhabricatorTextConfigType return (bool)strlen($value); } + protected function newCanonicalValue( + PhabricatorConfigOption $option, + $value) { + return (string)$value; + } + protected function newHTTPParameterType() { return new AphrontStringHTTPParameterType(); } diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php index a3db26bef6..7ff886664f 100644 --- a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php +++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php @@ -260,7 +260,10 @@ EOHELP ->setDescription( pht('Format for inlined or attached patches.')) ->setEnumOptions( - array('unified' => 'unified', 'git' => 'git')), + array( + 'unified' => pht('Unified'), + 'git' => pht('Git'), + )), ); }