Skip to content

Commit 7fcc0f9

Browse files
author
epriestley
committedMay 19, 2022
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
1 parent cfa42c5 commit 7fcc0f9

File tree

5 files changed

+7
-106
lines changed

5 files changed

+7
-106
lines changed
 

‎src/applications/files/storage/PhabricatorFile.php

-17
Original file line numberDiff line numberDiff line change
@@ -1426,23 +1426,6 @@ public function attachToObject($phid) {
14261426
}
14271427

14281428

1429-
/**
1430-
* Remove the policy edge between this file and some object.
1431-
*
1432-
* @param phid Object PHID to detach from.
1433-
* @return this
1434-
*/
1435-
public function detachFromObject($phid) {
1436-
$edge_type = PhabricatorObjectHasFileEdgeType::EDGECONST;
1437-
1438-
id(new PhabricatorEdgeEditor())
1439-
->removeEdge($phid, $edge_type, $this->getPHID())
1440-
->save();
1441-
1442-
return $this;
1443-
}
1444-
1445-
14461429
/**
14471430
* Configure a newly created file object according to specified parameters.
14481431
*

‎src/applications/files/storage/__tests__/PhabricatorFileTestCase.php

-12
Original file line numberDiff line numberDiff line change
@@ -224,18 +224,6 @@ public function testFileVisibility() {
224224
),
225225
$this->canViewFile($users, $xform),
226226
pht('Attached Thumbnail Visibility'));
227-
228-
// Detach the object and make sure it affects the thumbnail.
229-
$file->detachFromObject($object->getPHID());
230-
231-
// Test the detached thumbnail's visibility.
232-
$this->assertEqual(
233-
array(
234-
true,
235-
false,
236-
),
237-
$this->canViewFile($users, $xform),
238-
pht('Detached Thumbnail Visibility'));
239227
}
240228

241229
private function canViewFile(array $users, PhabricatorFile $file) {

‎src/applications/macro/xaction/PhabricatorMacroAudioTransaction.php

+5-23
Original file line numberDiff line numberDiff line change
@@ -13,32 +13,14 @@ public function applyInternalEffects($object, $value) {
1313
$object->setAudioPHID($value);
1414
}
1515

16-
public function applyExternalEffects($object, $value) {
17-
$old = $this->generateOldValue($object);
18-
$new = $value;
19-
$all = array();
20-
if ($old) {
21-
$all[] = $old;
22-
}
23-
if ($new) {
24-
$all[] = $new;
25-
}
16+
public function extractFilePHIDs($object, $value) {
17+
$file_phids = array();
2618

27-
$files = id(new PhabricatorFileQuery())
28-
->setViewer($this->getActor())
29-
->withPHIDs($all)
30-
->execute();
31-
$files = mpull($files, null, 'getPHID');
32-
33-
$old_file = idx($files, $old);
34-
if ($old_file) {
35-
$old_file->detachFromObject($object->getPHID());
19+
if ($value) {
20+
$file_phids[] = $value;
3621
}
3722

38-
$new_file = idx($files, $new);
39-
if ($new_file) {
40-
$new_file->attachToObject($object->getPHID());
41-
}
23+
return $file_phids;
4224
}
4325

4426
public function getTitle() {

‎src/applications/macro/xaction/PhabricatorMacroFileTransaction.php

+2-26
Original file line numberDiff line numberDiff line change
@@ -13,32 +13,8 @@ public function applyInternalEffects($object, $value) {
1313
$object->setFilePHID($value);
1414
}
1515

16-
public function applyExternalEffects($object, $value) {
17-
$old = $this->generateOldValue($object);
18-
$new = $value;
19-
$all = array();
20-
if ($old) {
21-
$all[] = $old;
22-
}
23-
if ($new) {
24-
$all[] = $new;
25-
}
26-
27-
$files = id(new PhabricatorFileQuery())
28-
->setViewer($this->getActor())
29-
->withPHIDs($all)
30-
->execute();
31-
$files = mpull($files, null, 'getPHID');
32-
33-
$old_file = idx($files, $old);
34-
if ($old_file) {
35-
$old_file->detachFromObject($object->getPHID());
36-
}
37-
38-
$new_file = idx($files, $new);
39-
if ($new_file) {
40-
$new_file->attachToObject($object->getPHID());
41-
}
16+
public function extractFilePHIDs($object, $value) {
17+
return array($value);
4218
}
4319

4420
public function getTitle() {

‎src/applications/project/xaction/PhabricatorProjectImageTransaction.php

-28
Original file line numberDiff line numberDiff line change
@@ -13,34 +13,6 @@ public function applyInternalEffects($object, $value) {
1313
$object->setProfileImagePHID($value);
1414
}
1515

16-
public function applyExternalEffects($object, $value) {
17-
$old = $this->getOldValue();
18-
$new = $value;
19-
$all = array();
20-
if ($old) {
21-
$all[] = $old;
22-
}
23-
if ($new) {
24-
$all[] = $new;
25-
}
26-
27-
$files = id(new PhabricatorFileQuery())
28-
->setViewer($this->getActor())
29-
->withPHIDs($all)
30-
->execute();
31-
$files = mpull($files, null, 'getPHID');
32-
33-
$old_file = idx($files, $old);
34-
if ($old_file) {
35-
$old_file->detachFromObject($object->getPHID());
36-
}
37-
38-
$new_file = idx($files, $new);
39-
if ($new_file) {
40-
$new_file->attachToObject($object->getPHID());
41-
}
42-
}
43-
4416
public function getTitle() {
4517
$old = $this->getOldValue();
4618
$new = $this->getNewValue();

0 commit comments

Comments
 (0)
Failed to load comments.