Page MenuHomePhorge

Add a Before-Destruction Engine
AcceptedPublic

Authored by valerio.bozzolan on Mon, May 19, 21:48.

Details

Summary

Allow extensions to run hooks *before* an object is destroyed.

This would easily allow to do things like:

  • Before you actually destroy a profile picture, find its active usages and set the builtin profile image. This is our core need to be able to fix T16074.
  • Before you actually destroy something at midnight, allow extensions to throw violently and effectively impede destruction saying for example "please go to sleep first" in the exception message, visible from command line. This specific thing is not currently required but theoretically works.
  • Before you actually destroy a generic object, you can still query its dependent items, before the destruction of the object. This is particularly useful if you want to use Query methods, which often require a fully functioning parent object. This last specific thing is not currently required but theoretically works.
  • Before you actually destroy a generic object, you can still destroy something else using the PhabricatorDestructionEngine. This last specific thing is not currently required but theoretically works.

This new engine is to be considered unstable.
If you implement this class please contact Phorge at T16079 even if the task is closed.

Closes T16079

Ref T16074

Test Plan

Try to destroy something (./bin/remove destroy). Absolutely no regressions.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D26026_3
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 2016
Build 2016: arc lint + arc unit

Event Timeline

mainframe98 added inline comments.
src/applications/system/engine/PhabricatorBeforeDestructionEngineExtension.php
37
45–46
valerio.bozzolan added inline comments.
src/applications/system/engine/PhabricatorBeforeDestructionEngineExtension.php
45–46

PHPStan-man @aklapper: is it self[] or array<self>? or same?

valerio.bozzolan marked an inline comment as not done.
  • fix typos
  • add PHPDoc @return
valerio.bozzolan added inline comments.
src/applications/system/engine/PhabricatorBeforeDestructionEngineExtension.php
45–46

I ask since I cannot find none of them in the code 🤔

grep -R -F -- 'array<self>' .
grep -R -F -- 'self[]' .
src/applications/system/engine/PhabricatorBeforeDestructionEngineExtension.php
45–46

@return list<PhabricatorDestructionEngineExtensions> or @return list<PhabricatorDestructionEngine> I'd guess (as Phorge loves to use non-standard types)

  • @return list<PhabricatorDestructionEngineExtension>
valerio.bozzolan marked 2 inline comments as done.
  • fix last typo

Some small nitpicks only. Makes sense to me, didn't spot any logic flaws, keeps close to already existing code in that very directory

src/applications/system/engine/PhabricatorBeforeDestructionEngineExtension.php
5–6

should be "a hook" instead of "an hook"

27

Did you mean "weather" or really "whenever" here?

31

Var is $destruction_engine in this signature and $engine in next signature, may want to just call it $engine for consistency?

42

Note to myself: Right, this should be PhabricatorDestructionEngine because PhabricatorBeforeDestructionEngine makes no sense

This revision is now accepted and ready to land.Sun, May 25, 23:43
valerio.bozzolan marked 4 inline comments as done.

add more and more PHPDoc, and fix typos - thanks

src/applications/system/engine/PhabricatorBeforeDestructionEngineExtension.php
27

Uhm I try to avoid "weather" since it can be confused with sunny/cloudy/rainy lol.

Maybe I can set "if"

31

Uhm I see. I think that the name $destruction_engine is better in both points to avoid confusion between the two engines.

src/applications/system/engine/PhabricatorBeforeDestructionEngineExtension.php
27

Oh god, I wanted to write "whether" of course. Sigh...

valerio.bozzolan added inline comments.
src/applications/system/engine/PhabricatorBeforeDestructionEngineExtension.php
27

LOL OK probably resolved with "Check if this extension supports" - so no risk to talk about the sun or the rain asd