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)
Mon, Dec 16, 4:04 PM
Unknown Object (File)
Sun, Dec 15, 10:24 AM
Unknown Object (File)
Tue, Dec 10, 7:16 PM
Unknown Object (File)
Mon, Dec 9, 1:48 PM
Unknown Object (File)
Mon, Dec 9, 1:48 PM
Unknown Object (File)
Mon, Dec 9, 1:48 PM
Unknown Object (File)
Mon, Dec 9, 1:48 PM
Unknown Object (File)
Mon, Dec 9, 1:46 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.