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.

Make PhabricatorDifferentialRevisionTestDataGenerator::generateDescription() have more than one sentence

Seems nice! Feel free to update.

In D25872#27180, @valerio.bozzolan wrote:

P.S. Uh, I noticed that arc work also has a --start parameter! I documented a bit the expected behavior in the task T15993.

So that parameter is currently not supported for tasks, maybe relevant for the patch author:

$ arc work --start stable T15993
 NEW BRANCH  Creating new branch "T15993-support-for-arc-work-t12345-(workontask-workflow)" from "master".
 BRANCH  Checking out branch "T15993-support-for-arc-work-t12345-(workontask-workflow)".

So you see that from "master" that means we are not supporting --start, I guess.

If we cannot add support to that, we can add another cute TODO-exception for this corner case I guess (and create another task I guess)

Partial review, will conclude (I hope asd)

git rebase master

Sigh, I swear I grep'ed for isArray before but I was in a parallel CamelCase world in that moment

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.

Thaanks

(I just sorted the description to have the same order of the preview so it was easier for me to review)

Love this, thanks

In D25872#27180, @valerio.bozzolan wrote:

P.S. Uh, I noticed that arc work also has a --start parameter! I documented a bit the expected behavior in the task T15993.

So that parameter is currently not supported for tasks, maybe relevant for the patch author:

$ arc work --start stable T15993
 NEW BRANCH  Creating new branch "T15993-support-for-arc-work-t12345-(workontask-workflow)" from "master".
 BRANCH  Checking out branch "T15993-support-for-arc-work-t12345-(workontask-workflow)".

So you see that from "master" that means we are not supporting --start, I guess.

If we cannot add support to that, we can add another cute TODO-exception for this corner case I guess (and create another task I guess)

In D25872#27178, @valerio.bozzolan wrote:

Tested for T15100 and T15993 and I'm a bit surprised to obtain, this very long branch with commas and square brackets (and just the closing bracket? uhm)

T15100-feature-request]-option-to-measure-wip-limits-based-on-card-count-instead-of-points,-to-more-closely-adhere-to-kanban-standards

and this with round brackets (that is not Bash friendly for the git autocomplete):

T15993-support-for-arc-work-t12345-(workontask-workflow)

Probably here we should normalize a bit more these branch names. To do it, I see that Phorge already has a "normalize" method that does great things, but unfortunately it's not available in Arcanist (we cannot call it, or we create a circular dependency)...

https://we.phorge.it/source/phorge/browse/master/src/infrastructure/util/PhabricatorSlug.php

What do you think about? Should we import a very minimal "ArcanistSlug" class? And now/then having PhabricatorSlug extending that? (to give 100% backward compatibility but uniform code)

I'm a bit unsure. Thanks for opinions.

remove some unneeded brackets in regex

looool double-slam-accept

Right...hmm, now I also wonder. :)
Should probably be iPhone|iPod|Android.*(Chrome/[.0-9]* Mobile|Mobile.*Firefox/[.0-9]*) (it's still that Firefox on Android has that Android string first, thus the brackets).

or with slash and version:

We shall keep version. There"s a good number of user-agent strings out there concatenating random browser names while not being these browsers.

git rebase master

git rebase master