From 106e90dcf0863785d3b1ace08190f750e872f3e6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Dec 2018 14:27:39 -0800 Subject: [PATCH] Remove the "willApplyTransactions()" hook from ApplicationTransactionEditor Summary: Depends on D19908. Ref T13222. In D19897, I reordered some transaction code and affected the call order of `willApplyTransactions()`. It turns out that we use this call for only one thing, and that thing is pretty silly: naming the raw paste data file when editing paste content. This is only user-visible in the URL when you click "View Raw Paste" and seems exceptionally low-value, so remove the hook and pick a consistent name for the paste datafiles. (We could retain the name behavior in other ways, but it hardly seems worthwhile.) Test Plan: - Created and edited a paste. - Grepped for `willApplyTransactions()`. Note that `EditEngine` (vs `ApplicationTransacitonEditor`) still has a `willApplyTransactions()`, which has one callsite in Phabricator (in Calendar) and a couple in Instances. That's untouched and still works. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222 Differential Revision: https://secure.phabricator.com/D19909 --- .../PhabricatorPasteContentTransaction.php | 28 ++----------------- ...habricatorApplicationTransactionEditor.php | 15 ---------- .../storage/PhabricatorModularTransaction.php | 5 ---- .../PhabricatorModularTransactionType.php | 4 --- 4 files changed, 3 insertions(+), 49 deletions(-) diff --git a/src/applications/paste/xaction/PhabricatorPasteContentTransaction.php b/src/applications/paste/xaction/PhabricatorPasteContentTransaction.php index c90892345d..1e4ad2d26c 100644 --- a/src/applications/paste/xaction/PhabricatorPasteContentTransaction.php +++ b/src/applications/paste/xaction/PhabricatorPasteContentTransaction.php @@ -5,8 +5,6 @@ final class PhabricatorPasteContentTransaction const TRANSACTIONTYPE = 'paste.create'; - private $fileName; - public function generateOldValue($object) { return $object->getFilePHID(); } @@ -32,26 +30,6 @@ final class PhabricatorPasteContentTransaction return array($error); } - public function willApplyTransactions($object, array $xactions) { - // Find the most user-friendly filename we can by examining the title of - // the paste and the pending transactions. We'll use this if we create a - // new file to store raw content later. - - $name = $object->getTitle(); - if (!strlen($name)) { - $name = 'paste.raw'; - } - - $type_title = PhabricatorPasteTitleTransaction::TRANSACTIONTYPE; - foreach ($xactions as $xaction) { - if ($xaction->getTransactionType() == $type_title) { - $name = $xaction->getNewValue(); - } - } - - $this->fileName = $name; - } - public function generateNewValue($object, $value) { // If this transaction does not really change the paste content, return // the current file PHID so this transaction no-ops. @@ -66,16 +44,16 @@ final class PhabricatorPasteContentTransaction $editor = $this->getEditor(); $actor = $editor->getActor(); - $file = $this->newFileForPaste($actor, $this->fileName, $value); + $file = $this->newFileForPaste($actor, $value); return $file->getPHID(); } - private function newFileForPaste(PhabricatorUser $actor, $name, $data) { + private function newFileForPaste(PhabricatorUser $actor, $data) { return PhabricatorFile::newFromFileData( $data, array( - 'name' => $name, + 'name' => 'raw.txt', 'mime-type' => 'text/plain; charset=utf-8', 'authorPHID' => $actor->getPHID(), 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index e58a3618a3..854c361614 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1026,8 +1026,6 @@ abstract class PhabricatorApplicationTransactionEditor $xactions = $this->filterTransactions($object, $xactions); if (!$is_preview) { - $this->willApplyTransactions($object, $xactions); - $this->hasRequiredMFA = true; if ($this->getShouldRequireMFA()) { $this->requireMFA($object, $xactions); @@ -4374,19 +4372,6 @@ abstract class PhabricatorApplicationTransactionEditor return idx($types, $type); } - private function willApplyTransactions($object, array $xactions) { - foreach ($xactions as $xaction) { - $type = $xaction->getTransactionType(); - - $xtype = $this->getModularTransactionType($type); - if (!$xtype) { - continue; - } - - $xtype->willApplyTransactions($object, $xactions); - } - } - public function getCreateObjectTitle($author, $object) { return pht('%s created this object.', $author); } diff --git a/src/applications/transactions/storage/PhabricatorModularTransaction.php b/src/applications/transactions/storage/PhabricatorModularTransaction.php index f6aece2a7f..38b9d1835d 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransaction.php +++ b/src/applications/transactions/storage/PhabricatorModularTransaction.php @@ -69,11 +69,6 @@ abstract class PhabricatorModularTransaction ->generateNewValue($object, $this->getNewValue()); } - final public function willApplyTransactions($object, array $xactions) { - return $this->getTransactionImplementation() - ->willApplyTransactions($object, $xactions); - } - final public function applyInternalEffects($object) { return $this->getTransactionImplementation() ->applyInternalEffects($object); diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index 35dd3ac197..3d2efe0501 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -23,10 +23,6 @@ abstract class PhabricatorModularTransactionType return array(); } - public function willApplyTransactions($object, array $xactions) { - return; - } - public function applyInternalEffects($object, $value) { return; }