From a14b82d4f466ec1efee5e97d771ac56622ea7112 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 27 Jun 2017 06:30:19 -0700 Subject: [PATCH] Move "wild" config types to new code Summary: Ref T12845. This is the last of the hard-coded types. These are mostly used for values which users don't directly edit, so it's largely OK that they aren't carefully validated. In some cases, it would be good to introduce a separate validator eventually. Test Plan: Edited, deleted and mangled these values via the web UI and CLI. Reviewers: chad, amckinley Reviewed By: amckinley Maniphest Tasks: T12845 Differential Revision: https://secure.phabricator.com/D18164 --- src/__phutil_library_map__.php | 4 ++ .../PhabricatorConfigEditController.php | 66 ++++--------------- ...PhabricatorConfigManagementSetWorkflow.php | 1 + .../PhabricatorApplicationConfigOptions.php | 14 ++-- .../config/type/PhabricatorJSONConfigType.php | 38 +++++++++++ .../config/type/PhabricatorWildConfigType.php | 39 +++++++++++ .../config/PhabricatorUserConfigOptions.php | 2 +- 7 files changed, 104 insertions(+), 60 deletions(-) create mode 100644 src/applications/config/type/PhabricatorJSONConfigType.php create mode 100644 src/applications/config/type/PhabricatorWildConfigType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ac19fcfbe5..e10b5fceef 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3010,6 +3010,7 @@ phutil_register_library_map(array( 'PhabricatorIteratedMD5PasswordHasherTestCase' => 'infrastructure/util/password/__tests__/PhabricatorIteratedMD5PasswordHasherTestCase.php', 'PhabricatorIteratorFileUploadSource' => 'applications/files/uploadsource/PhabricatorIteratorFileUploadSource.php', 'PhabricatorJIRAAuthProvider' => 'applications/auth/provider/PhabricatorJIRAAuthProvider.php', + 'PhabricatorJSONConfigType' => 'applications/config/type/PhabricatorJSONConfigType.php', 'PhabricatorJavelinLinter' => 'infrastructure/lint/linter/PhabricatorJavelinLinter.php', 'PhabricatorJiraIssueHasObjectEdgeType' => 'applications/doorkeeper/edge/PhabricatorJiraIssueHasObjectEdgeType.php', 'PhabricatorJumpNavHandler' => 'applications/search/engine/PhabricatorJumpNavHandler.php', @@ -4263,6 +4264,7 @@ phutil_register_library_map(array( 'PhabricatorWebContentSource' => 'infrastructure/contentsource/PhabricatorWebContentSource.php', 'PhabricatorWebServerSetupCheck' => 'applications/config/check/PhabricatorWebServerSetupCheck.php', 'PhabricatorWeekStartDaySetting' => 'applications/settings/setting/PhabricatorWeekStartDaySetting.php', + 'PhabricatorWildConfigType' => 'applications/config/type/PhabricatorWildConfigType.php', 'PhabricatorWordPressAuthProvider' => 'applications/auth/provider/PhabricatorWordPressAuthProvider.php', 'PhabricatorWorker' => 'infrastructure/daemon/workers/PhabricatorWorker.php', 'PhabricatorWorkerActiveTask' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php', @@ -8348,6 +8350,7 @@ phutil_register_library_map(array( 'PhabricatorIteratedMD5PasswordHasherTestCase' => 'PhabricatorTestCase', 'PhabricatorIteratorFileUploadSource' => 'PhabricatorFileUploadSource', 'PhabricatorJIRAAuthProvider' => 'PhabricatorOAuth1AuthProvider', + 'PhabricatorJSONConfigType' => 'PhabricatorTextConfigType', 'PhabricatorJavelinLinter' => 'ArcanistLinter', 'PhabricatorJiraIssueHasObjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorJumpNavHandler' => 'Phobject', @@ -9839,6 +9842,7 @@ phutil_register_library_map(array( 'PhabricatorWebContentSource' => 'PhabricatorContentSource', 'PhabricatorWebServerSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorWeekStartDaySetting' => 'PhabricatorSelectSetting', + 'PhabricatorWildConfigType' => 'PhabricatorJSONConfigType', 'PhabricatorWordPressAuthProvider' => 'PhabricatorOAuth2AuthProvider', 'PhabricatorWorker' => 'Phobject', 'PhabricatorWorkerActiveTask' => 'PhabricatorWorkerTask', diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index 7b0227540a..6ab4ccc253 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -325,40 +325,14 @@ final class PhabricatorConfigEditController $e_value = null; $errors = array(); - if ($option->isCustomType()) { $info = $option->getCustomObject()->readRequest($option, $request); list($e_value, $errors, $set_value, $value) = $info; } else { - $value = $request->getStr('value'); - if (!strlen($value)) { - $value = null; - - $xaction->setNewValue( - array( - 'deleted' => true, - 'value' => null, - )); - - return array($e_value, $errors, $value, $xaction); - } - - $type = $option->getType(); - $set_value = null; - - switch ($type) { - default: - $json = json_decode($value, true); - if ($json === null && strtolower($value) != 'null') { - $e_value = pht('Invalid'); - $errors[] = pht( - 'The given value must be valid JSON. This means, among '. - 'other things, that you must wrap strings in double-quotes.'); - } else { - $set_value = $json; - } - break; - } + throw new Exception( + pht( + 'Unknown configuration option type "%s".', + $option->getType())); } if (!$errors) { @@ -389,13 +363,12 @@ final class PhabricatorConfigEditController $option, $entry, $value); - } else { - $type = $option->getType(); - switch ($type) { - default: - return PhabricatorConfigJSON::prettyPrintJSON($value); - } } + + throw new Exception( + pht( + 'Unknown configuration option type "%s".', + $option->getType())); } private function renderControls( @@ -417,23 +390,10 @@ final class PhabricatorConfigEditController $display_value, $e_value); } else { - $type = $option->getType(); - switch ($type) { - default: - $control = id(new AphrontFormTextAreaControl()) - ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_TALL) - ->setCustomClass('PhabricatorMonospaced') - ->setCaption(pht('Enter value in JSON.')); - break; - } - - $control - ->setLabel(pht('Database Value')) - ->setError($e_value) - ->setValue($display_value) - ->setName('value'); - - $controls = array($control); + throw new Exception( + pht( + 'Unknown configuration option type "%s".', + $option->getType())); } return $controls; diff --git a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php index b29ef8c8ba..9f50d2eeed 100644 --- a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php +++ b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php @@ -70,6 +70,7 @@ final class PhabricatorConfigManagementSetWorkflow throw new PhutilArgumentUsageException($ex->getMessage()); } } else { + // NOTE: For now, this handles both "wild" values and custom types. $type = $option->getType(); switch ($type) { default: diff --git a/src/applications/config/option/PhabricatorApplicationConfigOptions.php b/src/applications/config/option/PhabricatorApplicationConfigOptions.php index 89c9409aba..49cd9eb17e 100644 --- a/src/applications/config/option/PhabricatorApplicationConfigOptions.php +++ b/src/applications/config/option/PhabricatorApplicationConfigOptions.php @@ -24,6 +24,7 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject { if ($type) { try { $type->validateStoredValue($option, $value); + $this->didValidateOption($option, $value); } catch (PhabricatorConfigValidationException $ex) { throw $ex; } catch (Exception $ex) { @@ -32,6 +33,8 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject { // configuration and raise an error. throw new PhabricatorConfigValidationException($ex->getMessage()); } + + return; } if ($option->isCustomType()) { @@ -40,12 +43,11 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject { } catch (Exception $ex) { throw new PhabricatorConfigValidationException($ex->getMessage()); } - } - - switch ($option->getType()) { - case 'wild': - default: - break; + } else { + throw new Exception( + pht( + 'Unknown configuration option type "%s".', + $option->getType())); } $this->didValidateOption($option, $value); diff --git a/src/applications/config/type/PhabricatorJSONConfigType.php b/src/applications/config/type/PhabricatorJSONConfigType.php new file mode 100644 index 0000000000..b1226c6e53 --- /dev/null +++ b/src/applications/config/type/PhabricatorJSONConfigType.php @@ -0,0 +1,38 @@ +newException( + pht( + 'Value for option "%s" (of type "%s") must be specified in JSON, '. + 'but input could not be decoded: %s', + $option->getKey(), + $this->getTypeKey(), + $ex->getMessage())); + } + + return $value; + } + + protected function newControl(PhabricatorConfigOption $option) { + return id(new AphrontFormTextAreaControl()) + ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_TALL) + ->setCustomClass('PhabricatorMonospaced') + ->setCaption(pht('Enter value in JSON.')); + } + + public function newDisplayValue( + PhabricatorConfigOption $option, + $value) { + return PhabricatorConfigJSON::prettyPrintJSON($value); + } + +} diff --git a/src/applications/config/type/PhabricatorWildConfigType.php b/src/applications/config/type/PhabricatorWildConfigType.php new file mode 100644 index 0000000000..3f278ca4e9 --- /dev/null +++ b/src/applications/config/type/PhabricatorWildConfigType.php @@ -0,0 +1,39 @@ +newException( + pht( + 'Value for option "%s" (of type "%s") must be specified in JSON, '. + 'but input could not be decoded. (Did you forget to quote a string?)', + $option->getKey(), + $this->getTypeKey())); + } + + return $value; + } + + public function validateStoredValue( + PhabricatorConfigOption $option, + $value) { + return; + } + +} diff --git a/src/applications/people/config/PhabricatorUserConfigOptions.php b/src/applications/people/config/PhabricatorUserConfigOptions.php index 79be912699..3ef736c8ca 100644 --- a/src/applications/people/config/PhabricatorUserConfigOptions.php +++ b/src/applications/people/config/PhabricatorUserConfigOptions.php @@ -43,7 +43,7 @@ final class PhabricatorUserConfigOptions $this->newOption('user.fields', $custom_field_type, $default) ->setCustomData(id(new PhabricatorUser())->getCustomFieldBaseClass()) ->setDescription(pht('Select and reorder user profile fields.')), - $this->newOption('user.custom-field-definitions', 'map', array()) + $this->newOption('user.custom-field-definitions', 'wild', array()) ->setDescription(pht('Add new simple fields to user profiles.')), $this->newOption('user.require-real-name', 'bool', true) ->setDescription(pht('Always require real name for user profiles.'))