refactor as recommended

Thanks, this is way cleaner (and slightly more performant) than my D25476. I've tested this locally, also in combination with other actions (e.g. opening another action field without changing it) and behavior is as expected.

remove unuseful line - thanks

Superseded by D25550

Would you like to go to court to defend that statement? :)

OK OK. So what else? Maybe this (mention from https://secure.phabricator.com/p/epriestley/):

I could not sign it if it required brushing three times a day. Removed purely due to egoistic laziness reasons.

No brushing teeth three times a day? However it looks good to me 👍

yep!

Well, I would rewrite quite a bit, so I'll post a draft here before editing directly:

Looks good to me. @speck

I forgot to invite the original author. This modification would not exist without aklapper. Eternal glory to aklapper.

Possible further enhancements, which I am loathe to do when trying to fix an existing bug, so perhaps as a future diff:

As per @avivey, update ArcanistPhpunitTestResultParser.php to call ArcanistXUnitTestResultParser and remove it's own XUnit parsing code.

Yeah sorry. That exception also occurred to me, before this change: https://we.phorge.it/transactions/detail/PHID-XACT-TASK-tuchgj42nb2ujtc/

I could not even reproduce but get an informative error instead:

As Wikimedia uninstalled Differential I cannot further debug in downstream.
We can either decline the ticket and its patch for now (if someone runs into this again, they could reopen or file a new task), or could get the patch in (setting a default value) without a test plan to have more robust code. Shrug.
Opinions? :)

As Wikimedia uninstalled Differential I cannot further debug in downstream.
We can either decline the ticket and its patch for now (if someone runs into this again, they could reopen or file a new task), or could get the patch in (setting a default value) without a test plan to have more robust code. Shrug.
Opinions? :)

Also interesting: https://github.com/phabricator-docker/phabricator by @tsc (Thanks!)

Since 2023, Wikimedia Phabricator is really Phorge.

I'm not a fan of "if not nonempty". Can I swap?

Thanks avivey. Added this:

Look into the "is creation xaction" - we had a similar diff recently about creating Revision from raw diff that had a similar behavior.

A root problem is that highlighted line number(s) should be a # fragment really, to do not multiply pages exponentially.

I'm a little worried about this migration; Do we have garbage collector for this? it would be safer to let it run.

Also, maybe combine with the ArcanistXUnitTestResultParser.

So maybe rename the parser to JUnitTestResult, or something like that (If that's the right name for the format and if phpunit is using the same format)?

@speck , @valerio.bozzolan
Sorry, yes, it's updating the test engine to parse phpunit's junit output, rather than the json output which no longer exists.

In D25472#13979:

is ArcanistPhpunitTestResultParser now learning to parse generic "junit style xml" format?

This seems reasonable but better to find a test plan. I also tried, without success :(

I cannot reproduce this. Can you?

Thanks Roberto. If you have not time to follow-up, no problem. We can help to re-patch.

Thanks \o/ Can reproduce.

Maybe unrelated. But after we fix PhabricatorCursorPagedPolicyAwareQuery, maybe we will find a crash test also for the first two lines:

Interestingly I was able to reproduce, but only creating 101+ tasks manually and going to the next page. So after that ?after=100 is introduced.

adopt PhabricatorEnv::isSelfURI()

1. (Yes, arc liberate --clean. I'll also run it in Phorge).
2. I'll check it again, I refactored some things and maybe I broke that.

Thanks. Premising I have only knowledge of traits, but not mapbuilder.

@littleggghost I think you might just need to run celerity to update the icons.

Interesting. I see. Maybe expanding its controller that should be this one:

Yeah, in this one special place it doesn't matter; But that page already have a special body, so we can save even the extra click by putting the message right there...

OK I see. Indeed we should not put any similar button anywhere else.

But in Diffusion, that button is hidden behind a "Actions -> Manage" button, inside a screen that also has dozens of other admin actions. The clutter cost there is minimized.

I see. Maybe the root problem is that I'm in love with that Diffusion popup 🤔

In that case, we don't even need another button - just add a line to the "deleted message" that says something like "To completely remove the history from the database, contact your admin".

I change the visibility now.
I intuitional think the default permission is all users. But I'm wrong. The default permission is only myself. Maybe this is for privacy reasons?

Premising the user already have an Delete Document button.

I feel that adding such a button would clutter the UI.
Users should generally "know" that in order to really delete things, they need to go to the admin, because they don't have the permissions anyway; And adding that just for the once-in-a-while that the admin needs this...

I mean, we can maybe have a "Delete" button that just shows the related CLI command, like I've seen around (Diffusion maybe)

I don't think we actually allow "deleting" from the web ui, only hiding it (in the sense that the data is not removed from the database).

This feature request could also be expanded to:

(We cannot see screenshots like F1633808 - please set permissions to Visible To All Users - thanks)

(Marking as Resolved thanks to the mentioned patch - feel free to reopen and thanks)

Since Subversion allows to checkout just a sub-directory,

To quickly double-check the backend logic, to understand why it really likes an array:

Be more friendly against the backend logic, and with PhabricatorStandardCustomFieldLink

Uh. Yes, for example PhabricatorCustomField::ROLE_DEFAULT is maybe a good choice, just to be sure to obtain its definition whatever its "frontend role"

git rebase master again :) ready.

Tested again with git rebase master. Ready.

Thanks for the reminder!

Update comments