You grep like a rockstar

Unlocking patch

Flagging as "andre is merging stuff https://we.phorge.it/D26007#26996"

Very good job as usual 👍

Nice. I'm 99% sure about the test plan, please see https://we.phorge.it/T16068#22703 before landing to adjust it, eventually.

Questions about the steps:

• fix last typo

• @return list<PhabricatorDestructionEngineExtension>

• fix typos
• add PHPDoc @return

Thanks! Additionally, it seems it's not possible to use these through {icon NAME} since it only supports font awesome AFAIK

Seems very useful thanks, I would like to find small extra time to expand the test plan a bit more and run these extra tests, since this sounds very "core"

Is getDisplayIconColor() related to these? (spoiler: probably it's not)

Yet another one strlen(null)...

Love this. Feel free to evaluate the addition of thus, dramatically increasing your corporate security (at the price of slowing you down even more). as stated here lol https://we.phorge.it/D26028#27452 or whatever small thing could help to better visualize this alien behavior that is very peculiar of Phorge (that is, as already said, not intuitive).

Should we also add some PHPDoc @deprecate (or whatever its name) to most CalendarColors?

Maybe better to just expand dialog, so, less code to maintain (?)

(I just mean the feature comes from Phabricator, not from Phorge; sorry I always pick random words to express myself)

Awesome. Optional bonus point, since this legacy feature thing is indeed veeeery confusing and very-Phorgi, maybe we may want to add a "Why" Phorge decided this, and highlight potential pitfalls.

Seems nice. Maybe we can generalize the text a bit to avoid "both" (since you can have multiple of them).

fix PHPDoc - thanks andre

The scenario in which this index would be needed is far from being normal. The patch D26027 that would benefit for it, is able to avoid that query in 99% of cases (it only needs such query when you want to destroy a profile picture, AND that picture was manually un-attached... why was it manually un-attached? by a spam click? by a faulty mouse?). So, the normal scenario is too small and unclear to justify a new index.

add extra inline comments about T16080

• implement review tips from @mainframe98 - thanks
• cover the edge case of a profile picture without attachments

Again, expanding a PhabricatorFileAttachment to support a destruction engine to post-pone its destruction does NOT work as intended (as it's still destroyed very after the file) since the destroyObjectPermanently() is always supposed to be executed BEFORE the extensions.

OK I've explored the proposal n. 1 about expanding PhabricatorFileAttachment to support PhabricatorDestructibleInterface, but I'm just moving the problem there. The attachment is still immediately nuclearized BEFORE the extensions can do things.

When you destroy a PhabricatorFile, it seems this happens:

It seems it's necessary to be able to "get the user from a profile picture". This is not easy, since the user seems not mentioned in any obvious way from the file object, as already stated in T15407.

Reword inline comment to be about our hero, Mario.

Can we also destroy the root project A? Sure! Added in the unit test.

Seems nice! Feel free to update.

Partial review, will conclude (I hope asd)

Plus, making the file editable by the author is the cure of T15814, so, moving that as parent task.

If we allow authors to destroy their images, we should also avoid 404 errors on them. So, the new subtask T16074.