From df939f1337fdb4a709f06040b8bcf16f0bbcd2bb Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 25 Jan 2017 11:23:26 -0800 Subject: [PATCH] Fix two issues with embedding other fields inside "Summary" or "Test Plan" in Differential with the web UI Summary: Ref T11114. Converting to EditEngine caused us to stop running this validation, since these fields no longer subclass this parent. Restore the validation. Also, make sure we check the //first// line of the value, too. After the change to make "Tests: xyz" a valid title, you could write silly summaries / test plans and escape the check if the first line was bogus. Test Plan: {F2493228} Reviewers: chad Reviewed By: chad Maniphest Tasks: T11114 Differential Revision: https://secure.phabricator.com/D17248 --- .../DifferentialCoreCustomField.php | 41 ------------- ...DifferentialTestPlanCommitMessageField.php | 7 +++ ...DifferentialRevisionSummaryTransaction.php | 7 +++ .../DifferentialRevisionTransactionType.php | 58 ++++++++++++++++++- 4 files changed, 71 insertions(+), 42 deletions(-) diff --git a/src/applications/differential/customfield/DifferentialCoreCustomField.php b/src/applications/differential/customfield/DifferentialCoreCustomField.php index 6d956ac367..f88ac8836c 100644 --- a/src/applications/differential/customfield/DifferentialCoreCustomField.php +++ b/src/applications/differential/customfield/DifferentialCoreCustomField.php @@ -64,52 +64,11 @@ abstract class DifferentialCoreCustomField 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; - } - } } 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/field/DifferentialTestPlanCommitMessageField.php b/src/applications/differential/field/DifferentialTestPlanCommitMessageField.php index a477a9036e..178e51d985 100644 --- a/src/applications/differential/field/DifferentialTestPlanCommitMessageField.php +++ b/src/applications/differential/field/DifferentialTestPlanCommitMessageField.php @@ -50,4 +50,11 @@ final class DifferentialTestPlanCommitMessageField ); } + public function validateTransactions($object, array $xactions) { + return $this->validateCommitMessageCorpusTransactions( + $object, + $xactions, + pht('Test Plan')); + } + } diff --git a/src/applications/differential/xaction/DifferentialRevisionSummaryTransaction.php b/src/applications/differential/xaction/DifferentialRevisionSummaryTransaction.php index a4f1dcafa5..238eb53903 100644 --- a/src/applications/differential/xaction/DifferentialRevisionSummaryTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionSummaryTransaction.php @@ -54,4 +54,11 @@ final class DifferentialRevisionSummaryTransaction return $changes; } + public function validateTransactions($object, array $xactions) { + return $this->validateCommitMessageCorpusTransactions( + $object, + $xactions, + pht('Summary')); + } + } diff --git a/src/applications/differential/xaction/DifferentialRevisionTransactionType.php b/src/applications/differential/xaction/DifferentialRevisionTransactionType.php index f740d97c74..59b0c7510d 100644 --- a/src/applications/differential/xaction/DifferentialRevisionTransactionType.php +++ b/src/applications/differential/xaction/DifferentialRevisionTransactionType.php @@ -1,4 +1,60 @@ validateMessageCorpus($xaction, $field_name); + if ($error) { + $errors[] = $error; + } + } + + return $errors; + } + + private function validateMessageCorpus($xaction, $field_name) { + $value = $xaction->getNewValue(); + if (!strlen($value)) { + return null; + } + + // Put a placeholder title on the message, because the first line of a + // message is now always parsed as a title. + $value = "\n".$value; + + $viewer = $this->getActor(); + $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__'); + + $result = $parser->parseCorpus($value); + + unset($result['__title__']); + unset($result['__summary__']); + + if (!$result) { + return null; + } + + return $this->newInvalidError( + 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.', + $field_name, + implode(', ', array_keys($result))), + $xaction); + } + +}