From 207131d14f8f3d724598a0f5c646d71392069003 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 27 Jan 2018 05:22:09 -0800 Subject: [PATCH] (stable) Wrap edge transaction readers in a translation layer Summary: Ref T13051. This puts a translation layer between the raw edge data in the transaction table and the UI that uses it. The intent is to start writing new, more compact data soon. This class give us a consistent API for interacting with either the new or old data format, so we don't have to migrate everything upfront. Test Plan: Browsed around, saw existing edge transactions render properly in transactions and feed. Added and removed subscribers and projects, saw good transaction rendering. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13051 Differential Revision: https://secure.phabricator.com/D18946 --- src/__phutil_library_map__.php | 2 + .../storage/DifferentialTransaction.php | 13 ---- .../PhabricatorApplicationTransaction.php | 25 ++++--- .../util/PhabricatorEdgeChangeRecord.php | 68 +++++++++++++++++++ 4 files changed, 82 insertions(+), 26 deletions(-) create mode 100644 src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 44d67c6259..c40f79b052 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2747,6 +2747,7 @@ phutil_register_library_map(array( 'PhabricatorDraftEngine' => 'applications/transactions/draft/PhabricatorDraftEngine.php', 'PhabricatorDraftInterface' => 'applications/transactions/draft/PhabricatorDraftInterface.php', 'PhabricatorDrydockApplication' => 'applications/drydock/application/PhabricatorDrydockApplication.php', + 'PhabricatorEdgeChangeRecord' => 'infrastructure/edges/util/PhabricatorEdgeChangeRecord.php', 'PhabricatorEdgeConfig' => 'infrastructure/edges/constants/PhabricatorEdgeConfig.php', 'PhabricatorEdgeConstants' => 'infrastructure/edges/constants/PhabricatorEdgeConstants.php', 'PhabricatorEdgeCycleException' => 'infrastructure/edges/exception/PhabricatorEdgeCycleException.php', @@ -8170,6 +8171,7 @@ phutil_register_library_map(array( 'PhabricatorDraftDAO' => 'PhabricatorLiskDAO', 'PhabricatorDraftEngine' => 'Phobject', 'PhabricatorDrydockApplication' => 'PhabricatorApplication', + 'PhabricatorEdgeChangeRecord' => 'Phobject', 'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants', 'PhabricatorEdgeConstants' => 'Phobject', 'PhabricatorEdgeCycleException' => 'Exception', diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index 667341cef4..f7dcf29860 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -87,19 +87,6 @@ final class DifferentialTransaction } break; - case PhabricatorTransactions::TYPE_EDGE: - $add = array_diff_key($new, $old); - $rem = array_diff_key($old, $new); - - // Hide metadata-only edge transactions. These correspond to users - // accepting or rejecting revisions, but the change is always explicit - // because of the TYPE_ACTION transaction. Rendering these transactions - // just creates clutter. - - if (!$add && !$rem) { - return true; - } - break; case DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE: // Don't hide the initial "X requested review: ..." transaction from // mail or feed even when it occurs during creation. We need this diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index d49c3bf675..97b57009ec 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -302,8 +302,8 @@ abstract class PhabricatorApplicationTransaction $phids[] = $new; break; case PhabricatorTransactions::TYPE_EDGE: - $phids[] = ipull($old, 'dst'); - $phids[] = ipull($new, 'dst'); + $record = PhabricatorEdgeChangeRecord::newFromTransaction($this); + $phids[] = $record->getChangedPHIDs(); break; case PhabricatorTransactions::TYPE_COLUMNS: foreach ($new as $move) { @@ -632,9 +632,8 @@ abstract class PhabricatorApplicationTransaction return true; break; case PhabricatorObjectMentionedByObjectEdgeType::EDGECONST: - $new = ipull($this->getNewValue(), 'dst'); - $old = ipull($this->getOldValue(), 'dst'); - $add = array_diff($new, $old); + $record = PhabricatorEdgeChangeRecord::newFromTransaction($this); + $add = $record->getAddedPHIDs(); $add_value = reset($add); $add_handle = $this->getHandle($add_value); if ($add_handle->getPolicyFiltered()) { @@ -933,10 +932,10 @@ abstract class PhabricatorApplicationTransaction } break; case PhabricatorTransactions::TYPE_EDGE: - $new = ipull($new, 'dst'); - $old = ipull($old, 'dst'); - $add = array_diff($new, $old); - $rem = array_diff($old, $new); + $record = PhabricatorEdgeChangeRecord::newFromTransaction($this); + $add = $record->getAddedPHIDs(); + $rem = $record->getRemovedPHIDs(); + $type = $this->getMetadata('edge:type'); $type = head($type); @@ -1172,10 +1171,10 @@ abstract class PhabricatorApplicationTransaction $this->renderHandleLink($new)); } case PhabricatorTransactions::TYPE_EDGE: - $new = ipull($new, 'dst'); - $old = ipull($old, 'dst'); - $add = array_diff($new, $old); - $rem = array_diff($old, $new); + $record = PhabricatorEdgeChangeRecord::newFromTransaction($this); + $add = $record->getAddedPHIDs(); + $rem = $record->getRemovedPHIDs(); + $type = $this->getMetadata('edge:type'); $type = head($type); diff --git a/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php b/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php new file mode 100644 index 0000000000..6e0cb1d3e0 --- /dev/null +++ b/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php @@ -0,0 +1,68 @@ +xaction = $xaction; + return $record; + } + + public function getChangedPHIDs() { + $add = $this->getAddedPHIDs(); + $rem = $this->getRemovedPHIDs(); + + $add = array_fuse($add); + $rem = array_fuse($rem); + + return array_keys($add + $rem); + } + + public function getAddedPHIDs() { + $old = $this->getOldDestinationPHIDs(); + $new = $this->getNewDestinationPHIDs(); + + $old = array_fuse($old); + $new = array_fuse($new); + + $add = array_diff_key($new, $old); + return array_keys($add); + } + + public function getRemovedPHIDs() { + $old = $this->getOldDestinationPHIDs(); + $new = $this->getNewDestinationPHIDs(); + + $old = array_fuse($old); + $new = array_fuse($new); + + $rem = array_diff_key($old, $new); + return array_keys($rem); + } + + private function getOldDestinationPHIDs() { + if ($this->xaction) { + $old = $this->xaction->getOldValue(); + return ipull($old, 'dst'); + } + + throw new Exception( + pht('Edge change record is not configured with any change data.')); + } + + private function getNewDestinationPHIDs() { + if ($this->xaction) { + $new = $this->xaction->getNewValue(); + return ipull($new, 'dst'); + } + + throw new Exception( + pht('Edge change record is not configured with any change data.')); + } + + +}