From 7a39ac43b48c4c40ccea93439968f93c7000bf94 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 13 Sep 2013 11:48:00 -0700 Subject: [PATCH] Add a "list" config option and move regex config to it Summary: Fixes T3807. Several issues: - Currently, we split config of type `list` on commas, which makes it impossible to enter a regex with a comma in it. - Split on newlines only. - Some of the examples are confusing (provided in JSON instead of the format you actually have to enter them). - Show examples in the same format you should enter text. - We didn't validate regexps. - Introduce `list` to validate regexes. @hlau: Note that the old config format for the bugtraq stuff implied the delimiters on the regular expression. They are no longer implied. The examples show the correct format. Test Plan: Viewed and edited affected config, hitting error and success cases. Reviewers: btrahan Reviewed By: btrahan CC: hlau, aran Maniphest Tasks: T3807 Differential Revision: https://secure.phabricator.com/D6969 --- .../PhabricatorConfigEditController.php | 18 +++++++++++- .../PhabricatorApplicationConfigOptions.php | 28 +++++++++++++++++++ .../PhabricatorDifferentialConfigOptions.php | 6 ++-- .../PhabricatorDiffusionConfigOptions.php | 8 +++--- .../storage/PhabricatorRepository.php | 4 +-- 5 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index fec7f39c37..f7ba0a54a4 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -282,7 +282,18 @@ final class PhabricatorConfigEditController $set_value = (string)$value; break; case 'list': - $set_value = $request->getStrList('value'); + case 'list': + $set_value = phutil_split_lines( + $request->getStr('value'), + $retain_endings = false); + + foreach ($set_value as $key => $v) { + if (!strlen($v)) { + unset($set_value[$key]); + } + } + $set_value = array_values($set_value); + break; case 'set': $set_value = array_fill_keys($request->getStrList('value'), true); @@ -364,6 +375,7 @@ final class PhabricatorConfigEditController case 'bool': return $value ? 'true' : 'false'; case 'list': + case 'list': return implode("\n", nonempty($value, array())); case 'set': return implode("\n", nonempty(array_keys($value), array())); @@ -424,6 +436,10 @@ final class PhabricatorConfigEditController ->setOptions($names); break; case 'list': + case 'list': + $control = id(new AphrontFormTextAreaControl()) + ->setCaption(pht('Separate values with newlines.')); + break; case 'set': $control = id(new AphrontFormTextAreaControl()) ->setCaption(pht('Separate values with newlines or commas.')); diff --git a/src/applications/config/option/PhabricatorApplicationConfigOptions.php b/src/applications/config/option/PhabricatorApplicationConfigOptions.php index afac82dd20..c7c768e81a 100644 --- a/src/applications/config/option/PhabricatorApplicationConfigOptions.php +++ b/src/applications/config/option/PhabricatorApplicationConfigOptions.php @@ -78,6 +78,34 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject { } } break; + case 'list': + $valid = true; + if (!is_array($value)) { + throw new PhabricatorConfigValidationException( + pht( + "Option '%s' must be a list of regular expressions, but value ". + "is not an array.", + $option->getKey())); + } + if ($value && array_keys($value) != range(0, count($value) - 1)) { + throw new PhabricatorConfigValidationException( + pht( + "Option '%s' must be a list of regular expressions, but the ". + "value is a map with unnatural keys.", + $option->getKey())); + } + foreach ($value as $v) { + $ok = @preg_match($v, ''); + if ($ok === false) { + throw new PhabricatorConfigValidationException( + pht( + "Option '%s' must be a list of regular expressions, but the ". + "value '%s' is not a valid regular expression.", + $option->getKey(), + $v)); + } + } + break; case 'list': $valid = true; if (!is_array($value)) { diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php index 4d3d9cc327..da50a026ec 100644 --- a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php +++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php @@ -40,7 +40,7 @@ final class PhabricatorDifferentialConfigOptions "PhutilRemarkupEngineBlockRule")), $this->newOption( 'differential.whitespace-matters', - 'list', + 'list', array( '/\.py$/', '/\.l?hs$/', @@ -127,14 +127,14 @@ final class PhabricatorDifferentialConfigOptions "If you set this to true, users won't need to login to view ". "Differential revisions. Anonymous users will have read-only ". "access and won't be able to interact with the revisions.")), - $this->newOption('differential.generated-paths', 'list', array()) + $this->newOption('differential.generated-paths', 'list', array()) ->setSummary(pht("File regexps to treat as automatically generated.")) ->setDescription( pht( "List of file regexps that should be treated as if they are ". "generated by an automatic process, and thus get hidden by ". "default in differential.")) - ->addExample('["/config\.h$/", "#/autobuilt/#"]', pht("Valid Setting")), + ->addExample("/config\.h$/\n#/autobuilt/#", pht("Valid Setting")), $this->newOption('differential.allow-self-accept', 'bool', false) ->setBoolOptions( array( diff --git a/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php b/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php index 7fff476070..453a9036aa 100644 --- a/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php +++ b/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php @@ -73,12 +73,12 @@ final class PhabricatorDiffusionConfigOptions 'URL of external bug tracker used by Diffusion. %s will be '. 'substituted by the bug ID.', '%BUGID%')), - $this->newOption('bugtraq.logregex', 'list', array()) - ->addExample(array('\B#([1-9]\d*)\b'), pht('Issue #123')) + $this->newOption('bugtraq.logregex', 'list', array()) + ->addExample(array('/\B#([1-9]\d*)\b/'), pht('Issue #123')) ->addExample( - array('[Ii]ssues?:?(\s*,?\s*#\d+)+', '(\d+)'), + array('/[Ii]ssues?:?(\s*,?\s*#\d+)+/', '/(\d+)/'), pht('Issue #123, #456')) - ->addExample(array('(?addExample(array('/(?setDescription(pht( 'Regular expression to link external bug tracker. See '. 'http://tortoisesvn.net/docs/release/TortoiseSVN_en/'. diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 96acf334e6..d1cc0a5ab5 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -717,13 +717,13 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $matches = null; $flags = PREG_SET_ORDER | PREG_OFFSET_CAPTURE; - preg_match_all('('.$bugtraq_re.')', $message, $matches, $flags); + preg_match_all($bugtraq_re, $message, $matches, $flags); foreach ($matches as $match) { list($all, $all_offset) = array_shift($match); if ($id_re != '') { // Match substrings with bug IDs - preg_match_all('('.$id_re.')', $all, $match, PREG_OFFSET_CAPTURE); + preg_match_all($id_re, $all, $match, PREG_OFFSET_CAPTURE); list(, $match) = $match; } else { $all_offset = 0;