From 6dfc7e48ae7bc49115b986409a47a7835e45b56e Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 12 Nov 2016 07:04:01 -0800 Subject: [PATCH] Don't let users write summaries or test plans which will become ambiguous in commit messages Summary: Ref T11085. To recreate the issue: - From the web UI, click "Edit Revision". - Write something like this as your "Summary" (i.e., put another field marker, like "Test Plan:", into the summary): > This is a test of the > Test Plan: field to see > if it works. - Save changes. Later, when the summary is amended into a commit message, the parser will see two "Test Plan:" fields and fail to parse the message. Instead, prevent users from making this edit. Test Plan: {F1917640} Reviewers: chad Reviewed By: chad Maniphest Tasks: T11085 Differential Revision: https://secure.phabricator.com/D16846 --- ...tialParseCommitMessageConduitAPIMethod.php | 51 +++---------------- .../DifferentialCoreCustomField.php | 43 ++++++++++++++++ .../DifferentialCommitMessageParser.php | 39 ++++++++++++++ 3 files changed, 89 insertions(+), 44 deletions(-) diff --git a/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php index 107d22d2a9..4527983311 100644 --- a/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php @@ -29,18 +29,13 @@ final class DifferentialParseCommitMessageConduitAPIMethod $corpus = $request->getValue('corpus'); $is_partial = $request->getValue('partial'); - $revision = new DifferentialRevision(); - $field_list = PhabricatorCustomField::getObjectFields( - $revision, + new DifferentialRevision(), DifferentialCustomField::ROLE_COMMITMESSAGE); $field_list->setViewer($viewer); - $field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit'); + $field_map = mpull($field_list, null, 'getFieldKeyForConduit'); - $this->errors = array(); - - $label_map = $this->buildLabelMap($field_list); - $corpus_map = $this->parseCommitMessage($corpus, $label_map); + $corpus_map = $this->parseCommitMessage($corpus); $values = array(); foreach ($corpus_map as $field_key => $text_value) { @@ -94,44 +89,12 @@ final class DifferentialParseCommitMessageConduitAPIMethod ); } - private function buildLabelMap(PhabricatorCustomFieldList $field_list) { - $label_map = array(); - - foreach ($field_list->getFields() as $key => $field) { - $labels = $field->getCommitMessageLabels(); - $key = $field->getFieldKeyForConduit(); - - foreach ($labels as $label) { - $normal_label = DifferentialCommitMessageParser::normalizeFieldLabel( - $label); - if (!empty($label_map[$normal_label])) { - throw new Exception( - pht( - 'Field label "%s" is parsed by two custom fields: "%s" and '. - '"%s". Each label must be parsed by only one field.', - $label, - $key, - $label_map[$normal_label])); - } - $label_map[$normal_label] = $key; - } - } - - return $label_map; - } - - - private function parseCommitMessage($corpus, array $label_map) { - $key_title = id(new DifferentialTitleField())->getFieldKeyForConduit(); - $key_summary = id(new DifferentialSummaryField())->getFieldKeyForConduit(); - - $parser = id(new DifferentialCommitMessageParser()) - ->setLabelMap($label_map) - ->setTitleKey($key_title) - ->setSummaryKey($key_summary); - + private function parseCommitMessage($corpus) { + $viewer = $this->getViewer(); + $parser = DifferentialCommitMessageParser::newStandardParser($viewer); $result = $parser->parseCorpus($corpus); + $this->errors = array(); foreach ($parser->getErrors() as $error) { $this->errors[] = $error; } diff --git a/src/applications/differential/customfield/DifferentialCoreCustomField.php b/src/applications/differential/customfield/DifferentialCoreCustomField.php index 87d18553ad..7b6d02276d 100644 --- a/src/applications/differential/customfield/DifferentialCoreCustomField.php +++ b/src/applications/differential/customfield/DifferentialCoreCustomField.php @@ -10,6 +10,7 @@ abstract class DifferentialCoreCustomField private $value; private $fieldError; + private $fieldParser; abstract protected function readValueFromRevision( DifferentialRevision $revision); @@ -60,6 +61,32 @@ abstract class DifferentialCoreCustomField $error->setIsMissingFieldError(true); $errors[] = $error; $this->setFieldError(pht('Required')); + continue; + } + } + + if (is_string($value)) { + $parser = $this->getFieldParser(); + $result = $parser->parseCorpus($value); + + unset($result['__title__']); + unset($result['__summary__']); + + if ($result) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'The value you have entered in "%s" can not be parsed '. + 'unambiguously when rendered in a commit message. Edit the '. + 'message so that keywords like "Summary:" and "Test Plan:" do '. + 'not appear at the beginning of lines. Parsed keys: %s.', + $this->getFieldName(), + implode(', ', array_keys($result))), + $xaction); + $errors[] = $error; + $this->setFieldError(pht('Invalid')); + continue; } } } @@ -67,6 +94,22 @@ abstract class DifferentialCoreCustomField return $errors; } + private function getFieldParser() { + if (!$this->fieldParser) { + $viewer = $this->getViewer(); + $parser = DifferentialCommitMessageParser::newStandardParser($viewer); + + // Set custom title and summary keys so we can detect the presence of + // "Summary:" in, e.g., a test plan. + $parser->setTitleKey('__title__'); + $parser->setSummaryKey('__summary__'); + + $this->fieldParser = $parser; + } + + return $this->fieldParser; + } + public function canDisableField() { return false; } diff --git a/src/applications/differential/parser/DifferentialCommitMessageParser.php b/src/applications/differential/parser/DifferentialCommitMessageParser.php index fda7ae05b6..ff76b30e51 100644 --- a/src/applications/differential/parser/DifferentialCommitMessageParser.php +++ b/src/applications/differential/parser/DifferentialCommitMessageParser.php @@ -27,6 +27,45 @@ final class DifferentialCommitMessageParser extends Phobject { private $errors; + public static function newStandardParser(PhabricatorUser $viewer) { + + $key_title = id(new DifferentialTitleField())->getFieldKeyForConduit(); + $key_summary = id(new DifferentialSummaryField())->getFieldKeyForConduit(); + + $field_list = PhabricatorCustomField::getObjectFields( + new DifferentialRevision(), + DifferentialCustomField::ROLE_COMMITMESSAGE); + $field_list->setViewer($viewer); + + $label_map = array(); + + foreach ($field_list->getFields() as $field) { + $labels = $field->getCommitMessageLabels(); + $key = $field->getFieldKeyForConduit(); + + foreach ($labels as $label) { + $normal_label = self::normalizeFieldLabel( + $label); + if (!empty($label_map[$normal_label])) { + throw new Exception( + pht( + 'Field label "%s" is parsed by two custom fields: "%s" and '. + '"%s". Each label must be parsed by only one field.', + $label, + $key, + $label_map[$normal_label])); + } + $label_map[$normal_label] = $key; + } + } + + return id(new self()) + ->setLabelMap($label_map) + ->setTitleKey($key_title) + ->setSummaryKey($key_summary); + } + + /* -( Configuring the Parser )--------------------------------------------- */