diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3591,6 +3591,8 @@ 'PhabricatorTokensCurtainExtension' => 'applications/tokens/engineextension/PhabricatorTokensCurtainExtension.php', 'PhabricatorTokensSettingsPanel' => 'applications/settings/panel/PhabricatorTokensSettingsPanel.php', 'PhabricatorTooltipUIExample' => 'applications/uiexample/examples/PhabricatorTooltipUIExample.php', + 'PhabricatorTransactionChange' => 'applications/transactions/data/PhabricatorTransactionChange.php', + 'PhabricatorTransactionRemarkupChange' => 'applications/transactions/data/PhabricatorTransactionRemarkupChange.php', 'PhabricatorTransactions' => 'applications/transactions/constants/PhabricatorTransactions.php', 'PhabricatorTransactionsApplication' => 'applications/transactions/application/PhabricatorTransactionsApplication.php', 'PhabricatorTransactionsDestructionEngineExtension' => 'applications/transactions/engineextension/PhabricatorTransactionsDestructionEngineExtension.php', @@ -8397,6 +8399,8 @@ 'PhabricatorTokensCurtainExtension' => 'PHUICurtainExtension', 'PhabricatorTokensSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorTooltipUIExample' => 'PhabricatorUIExample', + 'PhabricatorTransactionChange' => 'Phobject', + 'PhabricatorTransactionRemarkupChange' => 'PhabricatorTransactionChange', 'PhabricatorTransactions' => 'Phobject', 'PhabricatorTransactionsApplication' => 'PhabricatorApplication', 'PhabricatorTransactionsDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension', diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -542,7 +542,7 @@ protected function expandCustomRemarkupBlockTransactions( PhabricatorLiskDAO $object, array $xactions, - $blocks, + array $changes, PhutilMarkupEngine $engine) { // we are only really trying to find unmentionable phids here... @@ -563,7 +563,7 @@ return $result; } - $flat_blocks = array_mergev($blocks); + $flat_blocks = mpull($changes, 'getNewValue'); $huge_block = implode("\n\n", $flat_blocks); $phid_map = array(); $phid_map[] = $this->getUnmentionablePHIDMap(); diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1303,10 +1303,10 @@ protected function expandCustomRemarkupBlockTransactions( PhabricatorLiskDAO $object, array $xactions, - $blocks, + array $changes, PhutilMarkupEngine $engine) { - $flat_blocks = array_mergev($blocks); + $flat_blocks = mpull($changes, 'getNewValue'); $huge_block = implode("\n\n", $flat_blocks); $task_map = array(); diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -54,16 +54,18 @@ return parent::shouldGenerateOldValue(); } - public function getRemarkupBlocks() { - $blocks = parent::getRemarkupBlocks(); + protected function newRemarkupChanges() { + $changes = array(); switch ($this->getTransactionType()) { case self::TYPE_DESCRIPTION: - $blocks[] = $this->getNewValue(); + $changes[] = $this->newRemarkupChange() + ->setOldValue($this->getOldValue()) + ->setNewValue($this->getNewValue()); break; } - return $blocks; + return $changes; } public function getRequiredHandlePHIDs() { diff --git a/src/applications/transactions/data/PhabricatorTransactionChange.php b/src/applications/transactions/data/PhabricatorTransactionChange.php new file mode 100644 --- /dev/null +++ b/src/applications/transactions/data/PhabricatorTransactionChange.php @@ -0,0 +1,37 @@ +transaction = $xaction; + return $this; + } + + final public function getTransaction() { + return $this->transaction; + } + + final public function setOldValue($old_value) { + $this->oldValue = $old_value; + return $this; + } + + final public function getOldValue() { + return $this->oldValue; + } + + final public function setNewValue($new_value) { + $this->newValue = $new_value; + return $this; + } + + final public function getNewValue() { + return $this->newValue; + } + +} diff --git a/src/applications/transactions/data/PhabricatorTransactionRemarkupChange.php b/src/applications/transactions/data/PhabricatorTransactionRemarkupChange.php new file mode 100644 --- /dev/null +++ b/src/applications/transactions/data/PhabricatorTransactionRemarkupChange.php @@ -0,0 +1,4 @@ +setTransactionPHID($xaction->getPHID()); $comment->save(); + $old_comment = $xaction->getComment(); + $comment->attachOldComment($old_comment); + $xaction->setCommentVersion($new_version); $xaction->setCommentPHID($comment->getPHID()); $xaction->setViewPolicy($comment->getViewPolicy()); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1320,17 +1320,28 @@ private function buildSubscribeTransaction( PhabricatorLiskDAO $object, array $xactions, - array $blocks) { + array $changes) { if (!($object instanceof PhabricatorSubscribableInterface)) { return null; } if ($this->shouldEnableMentions($object, $xactions)) { - $texts = array_mergev($blocks); - $phids = PhabricatorMarkupEngine::extractPHIDsFromMentions( + // Identify newly mentioned users. We ignore users who were previously + // mentioned so that we don't re-subscribe users after an edit of text + // which mentions them. + $old_texts = mpull($changes, 'getOldValue'); + $new_texts = mpull($changes, 'getNewValue'); + + $old_phids = PhabricatorMarkupEngine::extractPHIDsFromMentions( + $this->getActor(), + $old_texts); + + $new_phids = PhabricatorMarkupEngine::extractPHIDsFromMentions( $this->getActor(), - $texts); + $new_texts); + + $phids = array_diff($new_phids, $old_phids); } else { $phids = array(); } @@ -1381,11 +1392,6 @@ return $xaction; } - protected function getRemarkupBlocksFromTransaction( - PhabricatorApplicationTransaction $transaction) { - return $transaction->getRemarkupBlocks(); - } - protected function mergeTransactions( PhabricatorApplicationTransaction $u, PhabricatorApplicationTransaction $v) { @@ -1464,15 +1470,12 @@ $xactions = $this->applyImplicitCC($object, $xactions); - $blocks = array(); - foreach ($xactions as $key => $xaction) { - $blocks[$key] = $this->getRemarkupBlocksFromTransaction($xaction); - } + $changes = $this->getRemarkupChanges($xactions); $subscribe_xaction = $this->buildSubscribeTransaction( $object, $xactions, - $blocks); + $changes); if ($subscribe_xaction) { $xactions[] = $subscribe_xaction; } @@ -1484,7 +1487,7 @@ $block_xactions = $this->expandRemarkupBlockTransactions( $object, $xactions, - $blocks, + $changes, $engine); foreach ($block_xactions as $xaction) { @@ -1494,27 +1497,46 @@ return $xactions; } + private function getRemarkupChanges(array $xactions) { + $changes = array(); + + foreach ($xactions as $key => $xaction) { + foreach ($this->getRemarkupChangesFromTransaction($xaction) as $change) { + $changes[] = $change; + } + } + + return $changes; + } + + private function getRemarkupChangesFromTransaction( + PhabricatorApplicationTransaction $transaction) { + return $transaction->getRemarkupChanges(); + } + private function expandRemarkupBlockTransactions( PhabricatorLiskDAO $object, array $xactions, - $blocks, + array $changes, PhutilMarkupEngine $engine) { $block_xactions = $this->expandCustomRemarkupBlockTransactions( $object, $xactions, - $blocks, + $changes, $engine); $mentioned_phids = array(); if ($this->shouldEnableMentions($object, $xactions)) { - foreach ($blocks as $key => $xaction_blocks) { - foreach ($xaction_blocks as $block) { - $engine->markupText($block); - $mentioned_phids += $engine->getTextMetadata( - PhabricatorObjectRemarkupRule::KEY_MENTIONED_OBJECTS, - array()); - } + foreach ($changes as $change) { + // Here, we don't care about processing only new mentions after an edit + // because there is no way for an object to ever "unmention" itself on + // another object, so we can ignore the old value. + $engine->markupText($change->getNewValue()); + + $mentioned_phids += $engine->getTextMetadata( + PhabricatorObjectRemarkupRule::KEY_MENTIONED_OBJECTS, + array()); } } @@ -1559,7 +1581,7 @@ protected function expandCustomRemarkupBlockTransactions( PhabricatorLiskDAO $object, array $xactions, - $blocks, + array $changes, PhutilMarkupEngine $engine) { return array(); } @@ -3096,11 +3118,8 @@ PhabricatorLiskDAO $object, array $xactions) { - $blocks = array(); - foreach ($xactions as $xaction) { - $blocks[] = $this->getRemarkupBlocksFromTransaction($xaction); - } - $blocks = array_mergev($blocks); + $changes = $this->getRemarkupChanges($xactions); + $blocks = mpull($changes, 'getNewValue'); $phids = array(); if ($blocks) { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -179,6 +179,50 @@ return $this->assertAttached($this->object); } + public function getRemarkupChanges() { + $changes = $this->newRemarkupChanges(); + assert_instances_of($changes, 'PhabricatorTransactionRemarkupChange'); + + // Convert older-style remarkup blocks into newer-style remarkup changes. + // This builds changes that do not have the correct "old value", so rules + // that operate differently against edits (like @user mentions) won't work + // properly. + foreach ($this->getRemarkupBlocks() as $block) { + $changes[] = $this->newRemarkupChange() + ->setOldValue(null) + ->setNewValue($block); + } + + $comment = $this->getComment(); + if ($comment) { + if ($comment->hasOldComment()) { + $old_value = $comment->getOldComment()->getContent(); + } else { + $old_value = null; + } + + $new_value = $comment->getContent(); + + $changes[] = $this->newRemarkupChange() + ->setOldValue($old_value) + ->setNewValue($new_value); + } + + return $changes; + } + + protected function newRemarkupChanges() { + return array(); + } + + protected function newRemarkupChange() { + return id(new PhabricatorTransactionRemarkupChange()) + ->setTransaction($this); + } + + /** + * @deprecated + */ public function getRemarkupBlocks() { $blocks = array(); @@ -195,10 +239,6 @@ break; } - if ($this->getComment()) { - $blocks[] = $this->getComment()->getContent(); - } - return $blocks; } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php --- a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php @@ -18,6 +18,8 @@ protected $contentSource; protected $isDeleted = 0; + private $oldComment = self::ATTACHABLE; + abstract public function getApplicationTransactionObject(); public function generatePHID() { @@ -85,6 +87,20 @@ return $this; } + public function attachOldComment( + PhabricatorApplicationTransactionComment $old_comment) { + $this->oldComment = $old_comment; + return $this; + } + + public function getOldComment() { + return $this->assertAttached($this->oldComment); + } + + public function hasOldComment() { + return ($this->oldComment !== self::ATTACHABLE); + } + /* -( PhabricatorMarkupInterface )----------------------------------------- */