Page MenuHomePhabricator

Remove "PhabricatorFile->detachFromObject()"
Needs ReviewPublic

Authored by epriestley on Thu, May 12, 9:15 PM.
This revision needs review, but there are no reviewers specified.


Maniphest Tasks
Restricted Maniphest Task

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.

Diff Detail

rP Phabricator
Lint Passed
Test Failures
Build Status
Buildable 25713
Build 35541: arc lint + arc unit

Unit TestsFailed

152 msPhabricatorFileTestCase::testFileIndirectScramble
Assertion failed, expected 'true' (at PhabricatorFileTestCase.php:135): Changing attached object view policy should scramble secret. ACTUAL VALUE
286 msPhabricatorCelerityTestCase::testCelerityMaps
3 assertions passed.
11 msPhabricatorConduitTestCase::testConduitMethods
1 assertion passed.
18 msPhabricatorFileTestCase::testFileDirectScramble
2 assertions passed.
7 msPhabricatorFileTestCase::testFileStorageDelete
1 assertion passed.
View Full Test Results (1 Failed · 40 Passed)

Event Timeline

epriestley created this revision.