From f79320e64e8f2d470a017a65d477f9bfae8d96ab Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 8 Apr 2014 12:09:43 -0700 Subject: [PATCH] Render default/current settings in the same format they'll be edited Summary: Fixes T4773. For config settings of type `list`, `set`, or `list`, the "defaults" table and "examples" aren't always in the same format you should actually use when changing the setting. This is pretty confusing. Instead, always show the settings in the desired format. For example, if the user should enter a newline-separated list, show them a newline separated list. Test Plan: - Grepped for `list`, `list`, and `'set'`; verified all the config had the right example format (most already did). - Viewed config settings of various kinds, including custom settings. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4773 Differential Revision: https://secure.phabricator.com/D8725 --- .../PhabricatorConfigEditController.php | 33 ++++++++++++------- .../PhabricatorConfigJSONOptionType.php | 12 ------- .../custom/PhabricatorConfigOptionType.php | 12 +++++-- .../option/PhabricatorCoreConfigOptions.php | 4 +-- .../PhabricatorSecurityConfigOptions.php | 3 +- .../CelerityManagementMapWorkflow.php | 2 +- 6 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index bd0079750f..18815ab83b 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -94,7 +94,14 @@ final class PhabricatorConfigEditController } } } else { - $display_value = $this->getDisplayValue($option, $config_entry); + if ($config_entry->getIsDeleted()) { + $display_value = null; + } else { + $display_value = $this->getDisplayValue( + $option, + $config_entry, + $config_entry->getValue()); + } } $form = new AphrontFormView(); @@ -186,7 +193,7 @@ final class PhabricatorConfigEditController $form->appendChild( id(new AphrontFormMarkupControl()) ->setLabel(pht('Default')) - ->setValue($this->renderDefaults($option))); + ->setValue($this->renderDefaults($option, $config_entry))); } $title = pht('Edit %s', $this->key); @@ -348,17 +355,16 @@ final class PhabricatorConfigEditController private function getDisplayValue( PhabricatorConfigOption $option, - PhabricatorConfigEntry $entry) { - - if ($entry->getIsDeleted()) { - return null; - } + PhabricatorConfigEntry $entry, + $value) { if ($option->isCustomType()) { - return $option->getCustomObject()->getDisplayValue($option, $entry); + return $option->getCustomObject()->getDisplayValue( + $option, + $entry, + $value); } else { $type = $option->getType(); - $value = $entry->getValue(); switch ($type) { case 'int': case 'string': @@ -497,7 +503,10 @@ final class PhabricatorConfigEditController $table); } - private function renderDefaults(PhabricatorConfigOption $option) { + private function renderDefaults( + PhabricatorConfigOption $option, + PhabricatorConfigEntry $entry) { + $stack = PhabricatorEnv::getConfigSourceStack(); $stack = $stack->getStack(); @@ -515,7 +524,9 @@ final class PhabricatorConfigEditController if (!array_key_exists($option->getKey(), $value)) { $value = phutil_tag('em', array(), pht('(empty)')); } else { - $value = PhabricatorConfigJSON::prettyPrintJSON( + $value = $this->getDisplayValue( + $option, + $entry, $value[$option->getKey()]); } diff --git a/src/applications/config/custom/PhabricatorConfigJSONOptionType.php b/src/applications/config/custom/PhabricatorConfigJSONOptionType.php index 91250e05c4..84d4ad2441 100644 --- a/src/applications/config/custom/PhabricatorConfigJSONOptionType.php +++ b/src/applications/config/custom/PhabricatorConfigJSONOptionType.php @@ -34,18 +34,6 @@ abstract class PhabricatorConfigJSONOptionType return array($e_value, $errors, $storage_value, $display_value); } - public function getDisplayValue( - PhabricatorConfigOption $option, - PhabricatorConfigEntry $entry) { - $value = $entry->getValue(); - if (!$value) { - return ''; - } - - $json = new PhutilJSON(); - return $json->encodeFormatted($value); - } - public function renderControl( PhabricatorConfigOption $option, $display_value, diff --git a/src/applications/config/custom/PhabricatorConfigOptionType.php b/src/applications/config/custom/PhabricatorConfigOptionType.php index 6cd4877b48..c50315a140 100644 --- a/src/applications/config/custom/PhabricatorConfigOptionType.php +++ b/src/applications/config/custom/PhabricatorConfigOptionType.php @@ -20,8 +20,16 @@ abstract class PhabricatorConfigOptionType { public function getDisplayValue( PhabricatorConfigOption $option, - PhabricatorConfigEntry $entry) { - return $entry->getValue(); + PhabricatorConfigEntry $entry, + $value) { + + if (is_array($value)) { + $json = new PhutilJSON(); + return $json->encodeFormatted($value); + } else { + return $value; + } + } public function renderControl( diff --git a/src/applications/config/option/PhabricatorCoreConfigOptions.php b/src/applications/config/option/PhabricatorCoreConfigOptions.php index c99abe0419..dda46f82c6 100644 --- a/src/applications/config/option/PhabricatorCoreConfigOptions.php +++ b/src/applications/config/option/PhabricatorCoreConfigOptions.php @@ -59,8 +59,8 @@ final class PhabricatorCoreConfigOptions "won't work. The major use case for this is moving installs ". "across domains.")) ->addExample( - '["http://phabricator2.example.com/", '. - '"http://phabricator3.example.com/"]', + "http://phabricator2.example.com/\n". + "http://phabricator3.example.com/", pht('Valid Setting')), $this->newOption('phabricator.timezone', 'string', null) ->setSummary( diff --git a/src/applications/config/option/PhabricatorSecurityConfigOptions.php b/src/applications/config/option/PhabricatorSecurityConfigOptions.php index 556d213df6..5c5235074e 100644 --- a/src/applications/config/option/PhabricatorSecurityConfigOptions.php +++ b/src/applications/config/option/PhabricatorSecurityConfigOptions.php @@ -124,8 +124,7 @@ final class PhabricatorSecurityConfigOptions "automatically linked if the protocol appears in this set. This ". "whitelist is primarily to prevent security issues like ". "javascript:// URIs.")) - ->addExample( - '{"http": true, "https": true"}', pht('Valid Setting')) + ->addExample("http\nhttps", pht('Valid Setting')) ->setLocked(true), $this->newOption( 'uri.allowed-editor-protocols', diff --git a/src/infrastructure/celerity/management/CelerityManagementMapWorkflow.php b/src/infrastructure/celerity/management/CelerityManagementMapWorkflow.php index 0795ac5467..a904c4fea2 100644 --- a/src/infrastructure/celerity/management/CelerityManagementMapWorkflow.php +++ b/src/infrastructure/celerity/management/CelerityManagementMapWorkflow.php @@ -196,7 +196,7 @@ final class CelerityManagementMapWorkflow * * @param string Resource name. * @param string Resource data. - * @return pair|nul> The `@provides` symbol and the + * @return pair|null> The `@provides` symbol and the * list of `@requires` symbols. If the resource is not part of the * dependency graph, both are null. */