revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 05:50:55 +01:00
Code Issues Releases Wiki Activity

Begin modularizing config options in a more modern way

Browse source 
Summary:
Ref T12845. Config options are "modular", but the modularity is very old, half-implemented, and doesn't use modern patterns.

Half the types are hard-coded, while half the types are semi-modular but in a weird hacky way where you prefix the type with `custom:...`.

The actual API is also weird and requires types to return a lot of `array($stuff, $thing, $other_thing, $more_stuff)` sorts of tuples.

Instead:

  - Add a new replacement layer which uses modern modularity patterns and overrides the older stuff if available, so we can migrate things one at a time.
  - New layer uses a more modern API -- no `return array($thing, $other_thing, ...)`, and more modern building blocks (like AphrontHTTPParameterType).
  - New layer allows custom types to be deleted, which will ultimately let us deal with T12845.

Then, convert the `'int'` type to use the new layer.

Test Plan:
  - Set, edited, tried-to-change-in-an-invalid-way, and deleted an `'int'` option from the web UI.
  - Same from the CLI.
  - Edited `config.json` to have an invalid value, verified that the error was detected and config was repaired.

Reviewers: chad, amckinley

Reviewed By: amckinley

Maniphest Tasks: T12845

Differential Revision: https://secure.phabricator.com/D18155
This commit is contained in:
epriestley 2017-06-26 08:45:31 -07:00
parent d6450bf7d2
commit 03b6bdde19
8 changed files with 322 additions and 84 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
src/__phutil_library_map__.php
View file

@ -2448,6 +2448,7 @@ phutil_register_library_map(array(
'PhabricatorConfigTableSchema' => 'applications/config/schema/PhabricatorConfigTableSchema.php',
'PhabricatorConfigTransaction' => 'applications/config/storage/PhabricatorConfigTransaction.php',
'PhabricatorConfigTransactionQuery' => 'applications/config/query/PhabricatorConfigTransactionQuery.php',
'PhabricatorConfigType' => 'applications/config/type/PhabricatorConfigType.php',
'PhabricatorConfigValidationException' => 'applications/config/exception/PhabricatorConfigValidationException.php',
'PhabricatorConfigVersionController' => 'applications/config/controller/PhabricatorConfigVersionController.php',
'PhabricatorConpherenceApplication' => 'applications/conpherence/application/PhabricatorConpherenceApplication.php',
@ -2997,6 +2998,7 @@ phutil_register_library_map(array(
'PhabricatorInlineCommentPreviewController' => 'infrastructure/diff/PhabricatorInlineCommentPreviewController.php',
'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/PhabricatorInlineSummaryView.php',
'PhabricatorInstructionsEditField' => 'applications/transactions/editfield/PhabricatorInstructionsEditField.php',
'PhabricatorIntConfigType' => 'applications/config/type/PhabricatorIntConfigType.php',
'PhabricatorInternalSetting' => 'applications/settings/setting/PhabricatorInternalSetting.php',
'PhabricatorInternationalizationManagementExtractWorkflow' => 'infrastructure/internationalization/management/PhabricatorInternationalizationManagementExtractWorkflow.php',
'PhabricatorInternationalizationManagementWorkflow' => 'infrastructure/internationalization/management/PhabricatorInternationalizationManagementWorkflow.php',
@ -4127,6 +4129,7 @@ phutil_register_library_map(array(
'PhabricatorTestStorageEngine' => 'applications/files/engine/PhabricatorTestStorageEngine.php',
'PhabricatorTestWorker' => 'infrastructure/daemon/workers/__tests__/PhabricatorTestWorker.php',
'PhabricatorTextAreaEditField' => 'applications/transactions/editfield/PhabricatorTextAreaEditField.php',
'PhabricatorTextConfigType' => 'applications/config/type/PhabricatorTextConfigType.php',
'PhabricatorTextEditField' => 'applications/transactions/editfield/PhabricatorTextEditField.php',
'PhabricatorTime' => 'infrastructure/time/PhabricatorTime.php',
'PhabricatorTimeFormatSetting' => 'applications/settings/setting/PhabricatorTimeFormatSetting.php',
@ -7697,6 +7700,7 @@ phutil_register_library_map(array(
'PhabricatorConfigTableSchema' => 'PhabricatorConfigStorageSchema',
'PhabricatorConfigTransaction' => 'PhabricatorApplicationTransaction',
'PhabricatorConfigTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
'PhabricatorConfigType' => 'Phobject',
'PhabricatorConfigValidationException' => 'Exception',
'PhabricatorConfigVersionController' => 'PhabricatorConfigController',
'PhabricatorConpherenceApplication' => 'PhabricatorApplication',
@ -8324,6 +8328,7 @@ phutil_register_library_map(array(
'PhabricatorInlineCommentPreviewController' => 'PhabricatorController',
'PhabricatorInlineSummaryView' => 'AphrontView',
'PhabricatorInstructionsEditField' => 'PhabricatorEditField',
'PhabricatorIntConfigType' => 'PhabricatorTextConfigType',
'PhabricatorInternalSetting' => 'PhabricatorSetting',
'PhabricatorInternationalizationManagementExtractWorkflow' => 'PhabricatorInternationalizationManagementWorkflow',
'PhabricatorInternationalizationManagementWorkflow' => 'PhabricatorManagementWorkflow',
@ -9659,6 +9664,7 @@ phutil_register_library_map(array(
'PhabricatorTestStorageEngine' => 'PhabricatorFileStorageEngine',
'PhabricatorTestWorker' => 'PhabricatorWorker',
'PhabricatorTextAreaEditField' => 'PhabricatorEditField',
'PhabricatorTextConfigType' => 'PhabricatorConfigType',
'PhabricatorTextEditField' => 'PhabricatorEditField',
'PhabricatorTime' => 'Phobject',
'PhabricatorTimeFormatSetting' => 'PhabricatorSelectSetting',

69
src/applications/config/controller/PhabricatorConfigEditController.php
View file

@ -274,12 +274,58 @@ final class PhabricatorConfigEditController
PhabricatorConfigOption $option,
AphrontRequest $request) {
$type = $option->newOptionType();
if ($type) {
$is_set = $type->isValuePresentInRequest($option, $request);
if ($is_set) {
$value = $type->readValueFromRequest($option, $request);
$errors = array();
try {
$canonical_value = $type->newValueFromRequestValue(
$option,
$value);
$type->validateStoredValue($canonical_value);
$xaction = $type->newTransaction($option, $canonical_value);
} catch (PhabricatorConfigValidationException $ex) {
$errors[] = $ex->getMessage();
$xaction = null;
}
return array(
$errors ? pht('Invalid') : null,
$errors,
$value,
$xaction,
);
} else {
$delete_xaction = id(new PhabricatorConfigTransaction())
->setTransactionType(PhabricatorConfigTransaction::TYPE_EDIT)
->setNewValue(
array(
'deleted' => true,
'value' => null,
));
return array(
null,
array(),
null,
$delete_xaction,
);
}
}
// TODO: If we missed on the new `PhabricatorConfigType` map, fall back
// to the old semi-modular, semi-hacky way of doing things.
$xaction = new PhabricatorConfigTransaction();
$xaction->setTransactionType(PhabricatorConfigTransaction::TYPE_EDIT);
$e_value = null;
$errors = array();
if ($option->isCustomType()) {
$info = $option->getCustomObject()->readRequest($option, $request);
list($e_value, $errors, $set_value, $value) = $info;
@ -301,14 +347,6 @@ final class PhabricatorConfigEditController
$set_value = null;
switch ($type) {
case 'int':
if (preg_match('/^-?[0-9]+$/', trim($value))) {
$set_value = (int)$value;
} else {
$e_value = pht('Invalid');
$errors[] = pht('Value must be an integer.');
}
break;
case 'string':
case 'enum':
$set_value = (string)$value;
@ -390,6 +428,11 @@ final class PhabricatorConfigEditController
PhabricatorConfigEntry $entry,
$value) {
$type = $option->newOptionType();
if ($type) {
return $type->newDisplayValue($option, $value);
}
if ($option->isCustomType()) {
return $option->getCustomObject()->getDisplayValue(
$option,
@ -398,7 +441,6 @@ final class PhabricatorConfigEditController
} else {
$type = $option->getType();
switch ($type) {
case 'int':
case 'string':
case 'enum':
case 'class':
@ -421,6 +463,14 @@ final class PhabricatorConfigEditController
$display_value,
$e_value) {
$type = $option->newOptionType();
if ($type) {
return $type->newControls(
$option,
$display_value,
$e_value);
}
if ($option->isCustomType()) {
$controls = $option->getCustomObject()->renderControls(
$option,
@ -429,7 +479,6 @@ final class PhabricatorConfigEditController
} else {
$type = $option->getType();
switch ($type) {
case 'int':
case 'string':
$control = id(new AphrontFormTextControl());
break;

128
src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php
View file

@ -59,63 +59,47 @@ final class PhabricatorConfigManagementSetWorkflow
$option = $options[$key];
$type = $option->getType();
switch ($type) {
case 'string':
case 'class':
case 'enum':
$value = (string)$value;
break;
case 'int':
if (!ctype_digit($value)) {
throw new PhutilArgumentUsageException(
pht(
"Config key '%s' is of type '%s'. Specify an integer.",
$key,
$type));
}
$value = (int)$value;
break;
case 'bool':
if ($value == 'true') {
$value = true;
} else if ($value == 'false') {
$value = false;
} else {
throw new PhutilArgumentUsageException(
pht(
"Config key '%s' is of type '%s'. Specify '%s' or '%s'.",
$key,
$type,
'true',
'false'));
}
break;
default:
$value = json_decode($value, true);
if (!is_array($value)) {
switch ($type) {
case 'set':
$command = csprintf(
'./bin/config set %R %s',
$type = $option->newOptionType();
if ($type) {
try {
$value = $type->newValueFromCommandLineValue(
$option,
$value);
} catch (PhabricatorConfigValidationException $ex) {
throw new PhutilArgumentUsageException($ex->getMessage());
}
} else {
$type = $option->getType();
switch ($type) {
case 'string':
case 'class':
case 'enum':
$value = (string)$value;
break;
case 'bool':
if ($value == 'true') {
$value = true;
} else if ($value == 'false') {
$value = false;
} else {
throw new PhutilArgumentUsageException(
pht(
"Config key '%s' is of type '%s'. Specify '%s' or '%s'.",
$key,
'{"value1": true, "value2": true}');
$message = sprintf(
"%s\n\n %s\n",
pht(
'Config key "%s" is of type "%s". Specify it in JSON. '.
'For example:',
$key,
$type),
$command);
break;
default:
if (preg_match('/^list</', $type)) {
$type,
'true',
'false'));
}
break;
default:
$value = json_decode($value, true);
if (!is_array($value)) {
switch ($type) {
case 'set':
$command = csprintf(
'./bin/config set %R %s',
$key,
'["a", "b", "c"]');
'{"value1": true, "value2": true}');
$message = sprintf(
"%s\n\n %s\n",
@ -125,18 +109,36 @@ final class PhabricatorConfigManagementSetWorkflow
$key,
$type),
$command);
} else {
$message = pht(
'Config key "%s" is of type "%s". Specify it in JSON.',
$key,
$type);
}
break;
break;
default:
if (preg_match('/^list</', $type)) {
$command = csprintf(
'./bin/config set %R %s',
$key,
'["a", "b", "c"]');
$message = sprintf(
"%s\n\n %s\n",
pht(
'Config key "%s" is of type "%s". Specify it in JSON. '.
'For example:',
$key,
$type),
$command);
} else {
$message = pht(
'Config key "%s" is of type "%s". Specify it in JSON.',
$key,
$type);
}
break;
}
throw new PhutilArgumentUsageException($message);
}
throw new PhutilArgumentUsageException($message);
}
break;
break;
}
}
$use_database = $args->getArg('database');
if ($option->getLocked() && $use_database) {
throw new PhutilArgumentUsageException(

25
src/applications/config/option/PhabricatorApplicationConfigOptions.php
View file

@ -20,13 +20,24 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject {
return;
}
$type = $option->newOptionType();
if ($type) {
try {
$type->validateStoredValue($option, $value);
} catch (PhabricatorConfigValidationException $ex) {
throw $ex;
} catch (Exception $ex) {
// If custom validators threw exceptions other than validation
// exceptions, convert them to validation exceptions so we repair the
// configuration and raise an error.
throw new PhabricatorConfigValidationException($ex->getMessage());
}
}
if ($option->isCustomType()) {
try {
return $option->getCustomObject()->validateOption($option, $value);
} catch (Exception $ex) {
// If custom validators threw exceptions, convert them to configuation
// validation exceptions so we repair the configuration and raise
// an error.
throw new PhabricatorConfigValidationException($ex->getMessage());
}
}
@ -41,14 +52,6 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject {
$option->getKey()));
}
break;
case 'int':
if (!is_int($value)) {
throw new PhabricatorConfigValidationException(
pht(
"Option '%s' is of type int, but value is not an integer.",
$option->getKey()));
}
break;
case 'string':
if (!is_string($value)) {
throw new PhabricatorConfigValidationException(

6
src/applications/config/option/PhabricatorConfigOption.php
View file

@ -175,6 +175,12 @@ final class PhabricatorConfigOption
return $this->type;
}
public function newOptionType() {
$type_key = $this->getType();
$type_map = PhabricatorConfigType::getAllTypes();
return idx($type_map, $type_key);
}
public function isCustomType() {
return !strncmp($this->getType(), 'custom:', 7);
}

115
src/applications/config/type/PhabricatorConfigType.php Normal file
View file

@ -0,0 +1,115 @@
<?php
abstract class PhabricatorConfigType extends Phobject {
final public function getTypeKey() {
return $this->getPhobjectClassConstant('TYPEKEY');
}
final public static function getAllTypes() {
return id(new PhutilClassMapQuery())
->setAncestorClass(__CLASS__)
->setUniqueMethod('getTypeKey')
->execute();
}
public function isValuePresentInRequest(
PhabricatorConfigOption $option,
AphrontRequest $request) {
$http_type = $this->newHTTPParameterType();
return $http_type->getExists($request, 'value');
}
public function readValueFromRequest(
PhabricatorConfigOption $option,
AphrontRequest $request) {
$http_type = $this->newHTTPParameterType();
return $http_type->getValue($request, 'value');
}
abstract protected function newHTTPParameterType();
public function validateValue(PhabricatorConfigOption $option, $value) {
return array();
}
public function newTransaction(
PhabricatorConfigOption $option,
$value) {
$xaction_value = $this->newTransactionValue($option, $value);
return id(new PhabricatorConfigTransaction())
->setTransactionType(PhabricatorConfigTransaction::TYPE_EDIT)
->setNewValue(
array(
'deleted' => false,
'value' => $xaction_value,
));
}
protected function newTransactionValue(
PhabricatorConfigOption $option,
$value) {
return $value;
}
public function newDisplayValue(
PhabricatorConfigOption $option,
$value) {
return $value;
}
public function newControls(
PhabricatorConfigOption $option,
$value,
$error) {
$control = $this->newControl($option)
->setError($error)
->setLabel(pht('Database Value'))
->setName('value');
$value = $this->newControlValue($option, $value);
$control->setValue($value);
return array(
$control,
);
}
abstract protected function newControl(PhabricatorConfigOption $option);
protected function newControlValue(
PhabricatorConfigOption $option,
$value) {
return $value;
}
protected function newException($message) {
return new PhabricatorConfigValidationException($message);
}
public function newValueFromRequestValue(
PhabricatorConfigOption $option,
$value) {
return $this->newCanonicalValue($option, $value);
}
public function newValueFromCommandLineValue(
PhabricatorConfigOption $option,
$value) {
return $this->newCanonicalValue($option, $value);
}
protected function newCanonicalValue(
PhabricatorConfigOption $option,
$value) {
return $value;
}
abstract public function validateStoredValue(
PhabricatorConfigOption $option,
$value);
}

36
src/applications/config/type/PhabricatorIntConfigType.php Normal file
View file

@ -0,0 +1,36 @@
<?php
final class PhabricatorIntConfigType
extends PhabricatorTextConfigType {
const TYPEKEY = 'int';
protected function newCanonicalValue(
PhabricatorConfigOption $option,
$value) {
if (!preg_match('/^-?[0-9]+\z/', $value)) {
throw $this->newException(
pht(
'Value for option "%s" must be an integer.',
$option->getKey()));
}
return (int)$value;
}
public function validateStoredValue(
PhabricatorConfigOption $option,
$value) {
if (!is_int($value)) {
throw $this->newException(
pht(
'Option "%s" is of type "%s", but the configured value is not '.
'an integer.',
$option->getKey(),
$this->getTypeKey()));
}
}
}

21
src/applications/config/type/PhabricatorTextConfigType.php Normal file
View file

@ -0,0 +1,21 @@
<?php
abstract class PhabricatorTextConfigType
extends PhabricatorConfigType {
public function isValuePresentInRequest(
PhabricatorConfigOption $option,
AphrontRequest $request) {
$value = parent::readValueFromRequest($option, $request);
return (bool)strlen($value);
}
protected function newHTTPParameterType() {
return new AphrontStringHTTPParameterType();
}
protected function newControl(PhabricatorConfigOption $option) {
return new AphrontFormTextControl();
}
}