From b852f213c36deef7f7a228a510d5cc3857347bc8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 30 Dec 2012 15:36:06 -0800 Subject: [PATCH] Begin moving Phabricator configuration into PHP Summary: Ref T2255. Ref T2221. Lay the groundwork to move configuration into PHP, so we can show descriptions in the web UI, do typechecking, disable application options when an application is uninstalled, etc. Test Plan: {F28421} {F28420} {F28422} Reviewers: codeblock, btrahan, vrana Reviewed By: codeblock CC: aran Maniphest Tasks: T2221, T2255 Differential Revision: https://secure.phabricator.com/D4306 --- src/__phutil_library_map__.php | 8 ++ .../PhabricatorApplicationConfig.php | 3 +- .../PhabricatorConfigController.php | 9 +-- .../PhabricatorConfigEditController.php | 50 +++++++----- .../PhabricatorConfigGroupController.php | 68 ++++++++++++++++ .../PhabricatorConfigIssueListController.php | 3 +- .../PhabricatorConfigListController.php | 34 ++++---- .../PhabricatorApplicationConfigOptions.php | 70 +++++++++++++++++ .../config/option/PhabricatorConfigOption.php | 77 +++++++++++++++++++ .../PhabricatorDifferentialConfigOptions.php | 67 ++++++++++++++++ 10 files changed, 345 insertions(+), 44 deletions(-) create mode 100644 src/applications/config/controller/PhabricatorConfigGroupController.php create mode 100644 src/applications/config/option/PhabricatorApplicationConfigOptions.php create mode 100644 src/applications/config/option/PhabricatorConfigOption.php create mode 100644 src/applications/differential/config/PhabricatorDifferentialConfigOptions.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5e23240701..16be2465c8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -581,6 +581,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationCalendar' => 'applications/calendar/application/PhabricatorApplicationCalendar.php', 'PhabricatorApplicationConduit' => 'applications/conduit/application/PhabricatorApplicationConduit.php', 'PhabricatorApplicationConfig' => 'applications/config/application/PhabricatorApplicationConfig.php', + 'PhabricatorApplicationConfigOptions' => 'applications/config/option/PhabricatorApplicationConfigOptions.php', 'PhabricatorApplicationCountdown' => 'applications/countdown/application/PhabricatorApplicationCountdown.php', 'PhabricatorApplicationDaemons' => 'applications/daemon/application/PhabricatorApplicationDaemons.php', 'PhabricatorApplicationDifferential' => 'applications/differential/application/PhabricatorApplicationDifferential.php', @@ -686,12 +687,14 @@ phutil_register_library_map(array( 'PhabricatorConfigEntry' => 'applications/config/storage/PhabricatorConfigEntry.php', 'PhabricatorConfigEntryDAO' => 'applications/config/storage/PhabricatorConfigEntryDAO.php', 'PhabricatorConfigFileSource' => 'infrastructure/env/PhabricatorConfigFileSource.php', + 'PhabricatorConfigGroupController' => 'applications/config/controller/PhabricatorConfigGroupController.php', 'PhabricatorConfigIssueListController' => 'applications/config/controller/PhabricatorConfigIssueListController.php', 'PhabricatorConfigIssueViewController' => 'applications/config/controller/PhabricatorConfigIssueViewController.php', 'PhabricatorConfigListController' => 'applications/config/controller/PhabricatorConfigListController.php', 'PhabricatorConfigLocalSource' => 'infrastructure/env/PhabricatorConfigLocalSource.php', 'PhabricatorConfigManagementSetWorkflow' => 'infrastructure/env/management/PhabricatorConfigManagementSetWorkflow.php', 'PhabricatorConfigManagementWorkflow' => 'infrastructure/env/management/PhabricatorConfigManagementWorkflow.php', + 'PhabricatorConfigOption' => 'applications/config/option/PhabricatorConfigOption.php', 'PhabricatorConfigProxySource' => 'infrastructure/env/PhabricatorConfigProxySource.php', 'PhabricatorConfigSource' => 'infrastructure/env/PhabricatorConfigSource.php', 'PhabricatorConfigStackSource' => 'infrastructure/env/PhabricatorConfigStackSource.php', @@ -725,6 +728,7 @@ phutil_register_library_map(array( 'PhabricatorDefaultFileStorageEngineSelector' => 'applications/files/engineselector/PhabricatorDefaultFileStorageEngineSelector.php', 'PhabricatorDefaultSearchEngineSelector' => 'applications/search/selector/PhabricatorDefaultSearchEngineSelector.php', 'PhabricatorDifferenceEngine' => 'infrastructure/diff/PhabricatorDifferenceEngine.php', + 'PhabricatorDifferentialConfigOptions' => 'applications/differential/config/PhabricatorDifferentialConfigOptions.php', 'PhabricatorDirectoryController' => 'applications/directory/controller/PhabricatorDirectoryController.php', 'PhabricatorDirectoryMainController' => 'applications/directory/controller/PhabricatorDirectoryMainController.php', 'PhabricatorDisabledUserController' => 'applications/auth/controller/PhabricatorDisabledUserController.php', @@ -1889,6 +1893,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationCalendar' => 'PhabricatorApplication', 'PhabricatorApplicationConduit' => 'PhabricatorApplication', 'PhabricatorApplicationConfig' => 'PhabricatorApplication', + 'PhabricatorApplicationConfigOptions' => 'Phobject', 'PhabricatorApplicationCountdown' => 'PhabricatorApplication', 'PhabricatorApplicationDaemons' => 'PhabricatorApplication', 'PhabricatorApplicationDifferential' => 'PhabricatorApplication', @@ -2008,12 +2013,14 @@ phutil_register_library_map(array( 'PhabricatorConfigEntry' => 'PhabricatorConfigEntryDAO', 'PhabricatorConfigEntryDAO' => 'PhabricatorLiskDAO', 'PhabricatorConfigFileSource' => 'PhabricatorConfigProxySource', + 'PhabricatorConfigGroupController' => 'PhabricatorConfigController', 'PhabricatorConfigIssueListController' => 'PhabricatorConfigController', 'PhabricatorConfigIssueViewController' => 'PhabricatorConfigController', 'PhabricatorConfigListController' => 'PhabricatorConfigController', 'PhabricatorConfigLocalSource' => 'PhabricatorConfigProxySource', 'PhabricatorConfigManagementSetWorkflow' => 'PhabricatorConfigManagementWorkflow', 'PhabricatorConfigManagementWorkflow' => 'PhutilArgumentWorkflow', + 'PhabricatorConfigOption' => 'Phobject', 'PhabricatorConfigProxySource' => 'PhabricatorConfigSource', 'PhabricatorConfigStackSource' => 'PhabricatorConfigSource', 'PhabricatorContentSourceView' => 'AphrontView', @@ -2042,6 +2049,7 @@ phutil_register_library_map(array( 'PhabricatorDaemonTimelineEventController' => 'PhabricatorDaemonController', 'PhabricatorDefaultFileStorageEngineSelector' => 'PhabricatorFileStorageEngineSelector', 'PhabricatorDefaultSearchEngineSelector' => 'PhabricatorSearchEngineSelector', + 'PhabricatorDifferentialConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorDirectoryController' => 'PhabricatorController', 'PhabricatorDirectoryMainController' => 'PhabricatorDirectoryController', 'PhabricatorDisabledUserController' => 'PhabricatorAuthController', diff --git a/src/applications/config/application/PhabricatorApplicationConfig.php b/src/applications/config/application/PhabricatorApplicationConfig.php index e069d1b602..cd1c1fc605 100644 --- a/src/applications/config/application/PhabricatorApplicationConfig.php +++ b/src/applications/config/application/PhabricatorApplicationConfig.php @@ -7,7 +7,7 @@ final class PhabricatorApplicationConfig extends PhabricatorApplication { } public function getIconName() { - return 'config'; + return 'setup'; } public function getTitleGlyph() { @@ -27,6 +27,7 @@ final class PhabricatorApplicationConfig extends PhabricatorApplication { '/config/' => array( '' => 'PhabricatorConfigListController', 'edit/(?P[\w\.\-]+)/' => 'PhabricatorConfigEditController', + 'group/(?P[^/]+)/' => 'PhabricatorConfigGroupController', 'issue/' => array( '' => 'PhabricatorConfigIssueListController', '(?P[^/]+)/' => 'PhabricatorConfigIssueViewController', diff --git a/src/applications/config/controller/PhabricatorConfigController.php b/src/applications/config/controller/PhabricatorConfigController.php index 01197efa5c..53e1a351c5 100644 --- a/src/applications/config/controller/PhabricatorConfigController.php +++ b/src/applications/config/controller/PhabricatorConfigController.php @@ -10,7 +10,9 @@ abstract class PhabricatorConfigController extends PhabricatorController { $user = $this->getRequest()->getUser(); $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI($this->getApplicationURI('filter/'))); + $nav->setBaseURI(new PhutilURI($this->getApplicationURI())); + $nav->addFilter('/', pht('Configuration')); + $nav->addFilter('issue/', pht('Setup Issues')); return $nav; } @@ -19,11 +21,6 @@ abstract class PhabricatorConfigController extends PhabricatorController { return $this->buildSideNavView(null, true)->getMenu(); } - public function buildApplicationCrumbs() { - $crumbs = parent::buildApplicationCrumbs(); - return $crumbs; - } - /** * Properly format a JSON value. * diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index a1b31b465e..59463c5aa4 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -6,27 +6,35 @@ final class PhabricatorConfigEditController private $key; public function willProcessRequest(array $data) { - $this->key = idx($data, 'key'); + $this->key = $data['key']; } public function processRequest() { $request = $this->getRequest(); $user = $request->getUser(); - $config = id(new PhabricatorConfigFileSource('default')) - ->getAllKeys(); - if (!$this->key || !array_key_exists($this->key, $config)) { + + $options = PhabricatorApplicationConfigOptions::loadAllOptions(); + if (empty($options[$this->key])) { return new Aphront404Response(); } + $option = $options[$this->key]; + $group = $option->getGroup(); + $group_uri = $this->getApplicationURI('group/'.$group->getKey().'/'); - $default = $this->prettyPrintJSON($config[$this->key]); + // TODO: This isn't quite correct -- we should read from the entire + // configuration stack, ignoring database configuration. For now, though, + // it's a reasonable approximation. + $default_value = $option->getDefault(); + + $default = $this->prettyPrintJSON($default_value); // Check if the config key is already stored in the database. // Grab the value if it is. $value = null; $config_entry = id(new PhabricatorConfigEntry()) ->loadOneWhere( - 'configKey = %s AND namespace=%s', + 'configKey = %s AND namespace = %s', $this->key, 'default'); if ($config_entry) { @@ -39,6 +47,7 @@ final class PhabricatorConfigEditController $e_value = null; $errors = array(); if ($request->isFormPost()) { + $new_value = $request->getStr('value'); if (strlen($new_value)) { $json = json_decode($new_value, true); @@ -53,8 +62,7 @@ final class PhabricatorConfigEditController } else { // TODO: When we do Transactions, make this just set isDeleted = 1 $config_entry->delete(); - return id(new AphrontRedirectResponse()) - ->setURI($config_entry->getURI()); + return id(new AphrontRedirectResponse())->setURI($group_uri); } $config_entry->setValue($value); @@ -62,8 +70,7 @@ final class PhabricatorConfigEditController if (!$errors) { $config_entry->save(); - return id(new AphrontRedirectResponse()) - ->setURI($config_entry->getURI()); + return id(new AphrontRedirectResponse())->setURI($group_uri); } } @@ -91,7 +98,7 @@ final class PhabricatorConfigEditController ->setName('value')) ->appendChild( id(new AphrontFormSubmitControl()) - ->addCancelButton($config_entry->getURI()) + ->addCancelButton($group_uri) ->setValue(pht('Save Config Entry'))) ->appendChild( phutil_render_tag( @@ -112,13 +119,20 @@ final class PhabricatorConfigEditController $title = pht('Edit %s', $this->key); $short = pht('Edit'); - $crumbs = $this->buildApplicationCrumbs($this->buildSideNavView()); - $crumbs->addCrumb( - id(new PhabricatorCrumbView()) - ->setName($this->key) - ->setHref('/config/edit/'.$this->key)); - $crumbs->addCrumb( - id(new PhabricatorCrumbView())->setName($short)); + $crumbs = $this->buildApplicationCrumbs(); + $crumbs + ->addCrumb( + id(new PhabricatorCrumbView()) + ->setName(pht('Config')) + ->setHref($this->getApplicationURI())) + ->addCrumb( + id(new PhabricatorCrumbView()) + ->setName($group->getName()) + ->setHref($group_uri)) + ->addCrumb( + id(new PhabricatorCrumbView()) + ->setName($this->key) + ->setHref('/config/edit/'.$this->key)); return $this->buildApplicationPage( array( diff --git a/src/applications/config/controller/PhabricatorConfigGroupController.php b/src/applications/config/controller/PhabricatorConfigGroupController.php new file mode 100644 index 0000000000..2ab468540c --- /dev/null +++ b/src/applications/config/controller/PhabricatorConfigGroupController.php @@ -0,0 +1,68 @@ +groupKey = $data['key']; + } + + public function processRequest() { + $request = $this->getRequest(); + $user = $request->getUser(); + + $groups = PhabricatorApplicationConfigOptions::loadAll(); + $options = idx($groups, $this->groupKey); + if (!$options) { + return new Aphront404Response(); + } + + $title = pht('%s Configuration', $options->getName()); + + $header = id(new PhabricatorHeaderView()) + ->setHeader($title); + + $list = $this->buildOptionList($options->getOptions()); + + $crumbs = $this + ->buildApplicationCrumbs() + ->addCrumb( + id(new PhabricatorCrumbView()) + ->setName(pht('Config')) + ->setHref($this->getApplicationURI())) + ->addCrumb( + id(new PhabricatorCrumbView()) + ->setName($options->getName()) + ->setHref($this->getApplicationURI())); + + return $this->buildApplicationPage( + array( + $crumbs, + $header, + $list, + ), + array( + 'title' => $title, + 'device' => true, + ) + ); + } + + private function buildOptionList(array $options) { + assert_instances_of($options, 'PhabricatorConfigOption'); + + $list = new PhabricatorObjectItemListView(); + foreach ($options as $option) { + $item = id(new PhabricatorObjectItemView()) + ->setHeader($option->getKey()) + ->setHref('/config/edit/'.$option->getKey().'/') + ->addAttribute(phutil_escape_html($option->getSummary())); + $list->addItem($item); + } + + return $list; + } + +} diff --git a/src/applications/config/controller/PhabricatorConfigIssueListController.php b/src/applications/config/controller/PhabricatorConfigIssueListController.php index fd794e3e71..eb4c9998da 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueListController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueListController.php @@ -8,6 +8,7 @@ final class PhabricatorConfigIssueListController $user = $request->getUser(); $nav = $this->buildSideNavView(); + $nav->selectFilter('issue/'); $issues = PhabricatorSetupCheck::runAllChecks(); PhabricatorSetupCheck::setOpenSetupIssueCount(count($issues)); @@ -30,7 +31,7 @@ final class PhabricatorConfigIssueListController ->buildApplicationCrumbs($nav) ->addCrumb( id(new PhabricatorCrumbView()) - ->setName($title) + ->setName(pht('Setup')) ->setHref($this->getApplicationURI('issue/'))); $nav->setCrumbs($crumbs); diff --git a/src/applications/config/controller/PhabricatorConfigListController.php b/src/applications/config/controller/PhabricatorConfigListController.php index d885436dd2..0c5c8b3274 100644 --- a/src/applications/config/controller/PhabricatorConfigListController.php +++ b/src/applications/config/controller/PhabricatorConfigListController.php @@ -8,18 +8,15 @@ final class PhabricatorConfigListController $user = $request->getUser(); $nav = $this->buildSideNavView(); + $nav->selectFilter('/'); - $pager = new AphrontCursorPagerView(); - $pager->readFromRequest($request); + $groups = PhabricatorApplicationConfigOptions::loadAll(); + $list = $this->buildConfigOptionsList($groups); - $config = new PhabricatorConfigFileSource('default'); - $list = $this->buildConfigList(array_keys($config->getAllKeys())); - $list->setPager($pager); - $list->setNoDataString( - 'No data. Something probably went wrong in reading the default config.'); + $title = pht('Phabricator Configuration'); $header = id(new PhabricatorHeaderView()) - ->setHeader(pht('Configuration')); + ->setHeader($title); $nav->appendChild( array( @@ -28,31 +25,32 @@ final class PhabricatorConfigListController )); $crumbs = $this - ->buildApplicationCrumbs($nav) + ->buildApplicationCrumbs() ->addCrumb( id(new PhabricatorCrumbView()) - ->setName(pht('Configuration')) - ->setHref($this->getApplicationURI('filter/'))); + ->setName(pht('Config')) + ->setHref($this->getApplicationURI())); $nav->setCrumbs($crumbs); return $this->buildApplicationPage( $nav, array( - 'title' => pht('Configuration'), + 'title' => $title, 'device' => true, ) ); } - private function buildConfigList(array $keys) { - $list = new PhabricatorObjectItemListView(); + private function buildConfigOptionsList(array $groups) { + assert_instances_of($groups, 'PhabricatorApplicationConfigOptions'); - foreach ($keys as $key) { + $list = new PhabricatorObjectItemListView(); + foreach ($groups as $group) { $item = id(new PhabricatorObjectItemView()) - ->setHeader($key) - ->setHref('/config/edit/'.$key) - ->setObject($key); + ->setHeader($group->getName()) + ->setHref('/config/group/'.$group->getKey().'/') + ->addAttribute(phutil_escape_html($group->getDescription())); $list->addItem($item); } diff --git a/src/applications/config/option/PhabricatorApplicationConfigOptions.php b/src/applications/config/option/PhabricatorApplicationConfigOptions.php new file mode 100644 index 0000000000..7a373ce927 --- /dev/null +++ b/src/applications/config/option/PhabricatorApplicationConfigOptions.php @@ -0,0 +1,70 @@ +setKey($key) + ->setType($type) + ->setDefault($default) + ->setGroup($this); + } + + final public static function loadAll() { + $symbols = id(new PhutilSymbolLoader()) + ->setAncestorClass('PhabricatorApplicationConfigOptions') + ->setConcreteOnly(true) + ->selectAndLoadSymbols(); + + $groups = array(); + foreach ($symbols as $symbol) { + $obj = newv($symbol['name'], array()); + $key = $obj->getKey(); + if (isset($groups[$key])) { + $pclass = get_class($groups[$key]); + $nclass = $symbol['name']; + + throw new Exception( + "Multiple PhabricatorApplicationConfigOptions subclasses have the ". + "same key ('{$key}'): {$pclass}, {$nclass}."); + } + $groups[$key] = $obj; + } + + return $groups; + } + + final public static function loadAllOptions() { + $groups = self::loadAll(); + + $options = array(); + foreach ($groups as $group) { + foreach ($group->getOptions() as $option) { + $key = $option->getKey(); + if (isset($options[$key])) { + throw new Exception( + "Mulitple PhabricatorApplicationConfigOptions subclasses contain ". + "an option named '{$key}'!"); + } + $options[$key] = $option; + } + } + + return $options; + } + + +} diff --git a/src/applications/config/option/PhabricatorConfigOption.php b/src/applications/config/option/PhabricatorConfigOption.php new file mode 100644 index 0000000000..cd120f078a --- /dev/null +++ b/src/applications/config/option/PhabricatorConfigOption.php @@ -0,0 +1,77 @@ +group = $group; + return $this; + } + + public function getGroup() { + return $this->group; + } + + public function setOptions(array $options) { + $this->options = $options; + return $this; + } + + public function getOptions() { + return $this->options; + } + + public function setKey($key) { + $this->key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + + public function setDefault($default) { + $this->default = $default; + return $this; + } + + public function getDefault() { + return $this->default; + } + + public function setSummary($summary) { + $this->summary = $summary; + return $this; + } + + public function getSummary() { + return $this->summary; + } + + public function setDescription($description) { + $this->description = $description; + return $this; + } + + public function getDescription() { + return $this->description; + } + + public function setType($type) { + $this->type = $type; + return $this; + } + + public function getType() { + return $this->type; + } + +} diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php new file mode 100644 index 0000000000..a92654af8d --- /dev/null +++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php @@ -0,0 +1,67 @@ +newOption('differential.show-host-field', 'bool', false) + ->setOptions( + array( + pht('Disable "Host" Fields'), + pht('Show "Host" Fields'), + )) + ->setSummary(pht('Show or hide the "Host" and "Path" fields.')) + ->setDescription( + pht( + 'Differential can show "Host" and "Path" fields on revisions, '. + 'with information about the machine and working directory where '. + 'the change came from. These fields are disabled by default '. + 'because they may occasionally have sensitive information, but '. + 'they can be useful if you work in an environment with shared '. + 'development machines. You can set this option to true to enable '. + 'these fields.')), + $this->newOption('differential.show-test-plan-field', 'bool', true) + ->setOptions( + array( + pht('Hide "Test Plan" Field'), + pht('Show "Test Plan" Field'), + )) + ->setSummary(pht('Show or hide the "Test Plan" field.')) + ->setDescription( + pht( + 'Differential has a required "Test Plan" field by default, which '. + 'requires authors to fill out information about how they verified '. + 'the correctness of their changes when they send code for review. '. + 'If you would prefer not to use this field, you can disable it '. + 'here. You can also make it optional (instead of required) by '. + 'setting {{differential.require-test-plan-field}}.')), + $this->newOption('differential.enable-email-accept', 'bool', false) + ->setOptions( + array( + pht('Disable Email "!accept" Action'), + pht('Enable Email "!accept" Action'), + )) + ->setSummary(pht('Enable or disable "!accept" action via email.')) + ->setDescription( + pht( + 'If inbound email is configured, users can interact with '. + 'revisions by using "!actions" in email replies (for example, '. + '"!resign" or "!rethink"). However, by default, users may not '. + '"!accept" revisions via email: email authentication can be '. + 'configured to be very weak, and email "!accept" is kind of '. + 'sketchy and implies the revision may not actually be receiving '. + 'thorough review. You can enable "!accept" by setting this '. + 'option to true.')), + ); + } + +}