mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-18 19:40:55 +01:00
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
This commit is contained in:
parent
de77d5249b
commit
6dfc7e48ae
3 changed files with 89 additions and 44 deletions
|
@ -29,18 +29,13 @@ final class DifferentialParseCommitMessageConduitAPIMethod
|
||||||
$corpus = $request->getValue('corpus');
|
$corpus = $request->getValue('corpus');
|
||||||
$is_partial = $request->getValue('partial');
|
$is_partial = $request->getValue('partial');
|
||||||
|
|
||||||
$revision = new DifferentialRevision();
|
|
||||||
|
|
||||||
$field_list = PhabricatorCustomField::getObjectFields(
|
$field_list = PhabricatorCustomField::getObjectFields(
|
||||||
$revision,
|
new DifferentialRevision(),
|
||||||
DifferentialCustomField::ROLE_COMMITMESSAGE);
|
DifferentialCustomField::ROLE_COMMITMESSAGE);
|
||||||
$field_list->setViewer($viewer);
|
$field_list->setViewer($viewer);
|
||||||
$field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit');
|
$field_map = mpull($field_list, null, 'getFieldKeyForConduit');
|
||||||
|
|
||||||
$this->errors = array();
|
$corpus_map = $this->parseCommitMessage($corpus);
|
||||||
|
|
||||||
$label_map = $this->buildLabelMap($field_list);
|
|
||||||
$corpus_map = $this->parseCommitMessage($corpus, $label_map);
|
|
||||||
|
|
||||||
$values = array();
|
$values = array();
|
||||||
foreach ($corpus_map as $field_key => $text_value) {
|
foreach ($corpus_map as $field_key => $text_value) {
|
||||||
|
@ -94,44 +89,12 @@ final class DifferentialParseCommitMessageConduitAPIMethod
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
private function buildLabelMap(PhabricatorCustomFieldList $field_list) {
|
private function parseCommitMessage($corpus) {
|
||||||
$label_map = array();
|
$viewer = $this->getViewer();
|
||||||
|
$parser = DifferentialCommitMessageParser::newStandardParser($viewer);
|
||||||
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);
|
|
||||||
|
|
||||||
$result = $parser->parseCorpus($corpus);
|
$result = $parser->parseCorpus($corpus);
|
||||||
|
|
||||||
|
$this->errors = array();
|
||||||
foreach ($parser->getErrors() as $error) {
|
foreach ($parser->getErrors() as $error) {
|
||||||
$this->errors[] = $error;
|
$this->errors[] = $error;
|
||||||
}
|
}
|
||||||
|
|
|
@ -10,6 +10,7 @@ abstract class DifferentialCoreCustomField
|
||||||
|
|
||||||
private $value;
|
private $value;
|
||||||
private $fieldError;
|
private $fieldError;
|
||||||
|
private $fieldParser;
|
||||||
|
|
||||||
abstract protected function readValueFromRevision(
|
abstract protected function readValueFromRevision(
|
||||||
DifferentialRevision $revision);
|
DifferentialRevision $revision);
|
||||||
|
@ -60,6 +61,32 @@ abstract class DifferentialCoreCustomField
|
||||||
$error->setIsMissingFieldError(true);
|
$error->setIsMissingFieldError(true);
|
||||||
$errors[] = $error;
|
$errors[] = $error;
|
||||||
$this->setFieldError(pht('Required'));
|
$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;
|
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() {
|
public function canDisableField() {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
|
@ -27,6 +27,45 @@ final class DifferentialCommitMessageParser extends Phobject {
|
||||||
private $errors;
|
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 )--------------------------------------------- */
|
/* -( Configuring the Parser )--------------------------------------------- */
|
||||||
|
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue