From 7fcc0f9ebd91a61b8c19801a0d578e08c96f9197 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 May 2022 14:05:26 -0700 Subject: [PATCH] Remove "PhabricatorFile->detachFromObject()" Summary: Ref T13603. Currently, files are sometimes detached from objects. For example, when you change the image for a Macro, the old image is detached. This is wrong: the image should remain attached so users who can view the macro can view the complete "alice change the image from X to Y" transaction. To be able to understand the change that was applied, you need to be able to view both files. All workflows which currently detach files aren't conistent with the modern way applications behave, except maybe one callsite in a unit test, and that one's kind of moot. Get rid of this stuff and just use PHID extraction to perform file attachment in all cases. Test Plan: Created and edited macros, verified files were properly attached and remained attached across edits. Maniphest Tasks: T13603 Differential Revision: https://secure.phabricator.com/D21815 --- .../files/storage/PhabricatorFile.php | 17 ----------- .../__tests__/PhabricatorFileTestCase.php | 12 -------- .../PhabricatorMacroAudioTransaction.php | 30 ++++--------------- .../PhabricatorMacroFileTransaction.php | 28 ++--------------- .../PhabricatorProjectImageTransaction.php | 28 ----------------- 5 files changed, 8 insertions(+), 107 deletions(-) diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index c78d4e1941..c38350d444 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -1426,23 +1426,6 @@ final class PhabricatorFile extends PhabricatorFileDAO } - /** - * Remove the policy edge between this file and some object. - * - * @param phid Object PHID to detach from. - * @return this - */ - public function detachFromObject($phid) { - $edge_type = PhabricatorObjectHasFileEdgeType::EDGECONST; - - id(new PhabricatorEdgeEditor()) - ->removeEdge($phid, $edge_type, $this->getPHID()) - ->save(); - - return $this; - } - - /** * Configure a newly created file object according to specified parameters. * diff --git a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php index 65dc0a12a2..a27e3b091a 100644 --- a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php +++ b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php @@ -224,18 +224,6 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase { ), $this->canViewFile($users, $xform), pht('Attached Thumbnail Visibility')); - - // Detach the object and make sure it affects the thumbnail. - $file->detachFromObject($object->getPHID()); - - // Test the detached thumbnail's visibility. - $this->assertEqual( - array( - true, - false, - ), - $this->canViewFile($users, $xform), - pht('Detached Thumbnail Visibility')); } private function canViewFile(array $users, PhabricatorFile $file) { diff --git a/src/applications/macro/xaction/PhabricatorMacroAudioTransaction.php b/src/applications/macro/xaction/PhabricatorMacroAudioTransaction.php index 26dc64c1f3..bb966a16cb 100644 --- a/src/applications/macro/xaction/PhabricatorMacroAudioTransaction.php +++ b/src/applications/macro/xaction/PhabricatorMacroAudioTransaction.php @@ -13,32 +13,14 @@ final class PhabricatorMacroAudioTransaction $object->setAudioPHID($value); } - public function applyExternalEffects($object, $value) { - $old = $this->generateOldValue($object); - $new = $value; - $all = array(); - if ($old) { - $all[] = $old; - } - if ($new) { - $all[] = $new; + public function extractFilePHIDs($object, $value) { + $file_phids = array(); + + if ($value) { + $file_phids[] = $value; } - $files = id(new PhabricatorFileQuery()) - ->setViewer($this->getActor()) - ->withPHIDs($all) - ->execute(); - $files = mpull($files, null, 'getPHID'); - - $old_file = idx($files, $old); - if ($old_file) { - $old_file->detachFromObject($object->getPHID()); - } - - $new_file = idx($files, $new); - if ($new_file) { - $new_file->attachToObject($object->getPHID()); - } + return $file_phids; } public function getTitle() { diff --git a/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php b/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php index fb0c56f1c1..4e9f019071 100644 --- a/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php +++ b/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php @@ -13,32 +13,8 @@ final class PhabricatorMacroFileTransaction $object->setFilePHID($value); } - public function applyExternalEffects($object, $value) { - $old = $this->generateOldValue($object); - $new = $value; - $all = array(); - if ($old) { - $all[] = $old; - } - if ($new) { - $all[] = $new; - } - - $files = id(new PhabricatorFileQuery()) - ->setViewer($this->getActor()) - ->withPHIDs($all) - ->execute(); - $files = mpull($files, null, 'getPHID'); - - $old_file = idx($files, $old); - if ($old_file) { - $old_file->detachFromObject($object->getPHID()); - } - - $new_file = idx($files, $new); - if ($new_file) { - $new_file->attachToObject($object->getPHID()); - } + public function extractFilePHIDs($object, $value) { + return array($value); } public function getTitle() { diff --git a/src/applications/project/xaction/PhabricatorProjectImageTransaction.php b/src/applications/project/xaction/PhabricatorProjectImageTransaction.php index f6d10f0961..9ed506d299 100644 --- a/src/applications/project/xaction/PhabricatorProjectImageTransaction.php +++ b/src/applications/project/xaction/PhabricatorProjectImageTransaction.php @@ -13,34 +13,6 @@ final class PhabricatorProjectImageTransaction $object->setProfileImagePHID($value); } - public function applyExternalEffects($object, $value) { - $old = $this->getOldValue(); - $new = $value; - $all = array(); - if ($old) { - $all[] = $old; - } - if ($new) { - $all[] = $new; - } - - $files = id(new PhabricatorFileQuery()) - ->setViewer($this->getActor()) - ->withPHIDs($all) - ->execute(); - $files = mpull($files, null, 'getPHID'); - - $old_file = idx($files, $old); - if ($old_file) { - $old_file->detachFromObject($object->getPHID()); - } - - $new_file = idx($files, $new); - if ($new_file) { - $new_file->attachToObject($object->getPHID()); - } - } - public function getTitle() { $old = $this->getOldValue(); $new = $this->getNewValue();