• 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"

Thanks! Confirming; tested, also works as expected with other epoch values.

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

Yet another one strlen(null)...

I'm too stuck in Serious Business Mode to introduce another string to translate, I'm afraid :-/

In D26000#27460, @valerio.bozzolan wrote:

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

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).

indentation is arrsome

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

oh true

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)

Hmm could you share why it's a "legacy feature"? I guess I'm clueless

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.

Rephrase to not mentioned "second MFA"; Avoid empty HTML paragraph

good points <3

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

Proposed behavior: Add additional text when setting up a second MFA.

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

I need to test this still, but this looks good. Two nits, and one issue I spotted:

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:

...and now even in the correct order of parameters, sigh

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.

You are correct. (And I am disappointed by PHPStan to not realize that.)

Reword inline comment to be about our hero, Mario.

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