Page MenuHomePhabricator

Switch File deletion to use ModularTransactions
ClosedPublic

Authored by amckinley on Apr 18 2017, 7:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 17, 5:33 AM
Unknown Object (File)
Wed, Nov 13, 9:02 PM
Unknown Object (File)
Thu, Oct 31, 10:56 PM
Unknown Object (File)
Thu, Oct 31, 10:56 PM
Unknown Object (File)
Thu, Oct 31, 10:56 PM
Unknown Object (File)
Thu, Oct 31, 10:56 PM
Unknown Object (File)
Wed, Oct 30, 10:59 PM
Unknown Object (File)
Sun, Oct 27, 7:45 PM

Details

Summary

Fixes T12587. Adds a new PhabricatorFileDeleteTransaction that enqueues File delete tasks.

Test Plan
  • hack PhabricatorFileQuery to ignore isDeleted state
  • stop daemons
  • upload a file, delete it from the UI
  • check that the DB has updated isDeleted = 1
  • check timeline rendering in File detail view
  • start daemons
  • confirm rows are deleted from DB

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley added inline comments.
src/applications/files/controller/PhabricatorFileDeleteController.php
33

Maybe just use true to make the API a little more obvious later, unless you can come up with states we might have in the future other than "deleted" or "not deleted"?

src/applications/files/xaction/PhabricatorFileDeleteTransaction.php
17

You can omit save() here since the Editor calls it after calling all the applyInternalEffects() resolve -- basically it's doing:

foreach ($transactions as $xaction) {
  $xaction->applyInternalEffects($object, ...);
}
$object->save();
This revision is now accepted and ready to land.Apr 18 2017, 7:28 PM
amckinley added inline comments.
src/applications/files/controller/PhabricatorFileDeleteController.php
33

"Delete in progress"? in re: discussion in T12587

src/applications/files/controller/PhabricatorFileDeleteController.php
33

We might store a flag for that internally, but any API client would only ever say "delete this" or "cancel deleting this", I think -- there aren't three different values they could meaningfully pass that I can come up with, at least.

This revision was automatically updated to reflect the committed changes.