@Cigaryno does it work also applying this D25535 ? Thanks Cigaryno

Would sending messages be pevented on archived rooms and would all participants be removed (unless Can Edit is set to Room Participants)?

In T15237#14300, @valerio.bozzolan wrote:

I think a dedicated policy for "Can send messages" would be better, to cover more cases. It would be strange that all participants must also be allowed to edit all settings.

Rip out more unused ancient code like unneeded expensive database queries

It seems that there is even more code to rip out here

Thanks. I'm personally fine with bug fixes for prototype apps.

(Premising that I'm affected by T15985 lol - unrelated)

@speck @valerio.bozzolan @bekay Would you like to review this revision?

In D25897#24262, @aklapper wrote:

Thanks, I can confirm that this works as expected. :)
I agree biggest question is where to put this. In my opinion the UX is already slightly inconsistent (given their are also concepts like "Related tasks" in Pholio or "Referenced Files" in tasks).
In Differential it feels a bit like a stretch to put this into a box called Revision Contents.
In Maniphest the same "Mentions" tab is in the Related Objects box; there is nothing similar in Differential which already has numerous boxes (also Details and Diff Detail but none of those have tabs).

• Updates after review to wording and dropped the placeholders

In D25898#24255, @avivey wrote:

Also, I think that visually, the instructions are too close to the next field and too far from their own field, but that's far out-of-scope for this one.

In D25881#24322, @aklapper wrote:

@Cigaryno: Please provide references when you quote something from somewhere so others don't have to guess where to find things. Thanks!

I think this is the upstream bug report lol

Per comments in T16006 this is rather workaround (ignore) instead of a fix (make it work).

Did some digging using the Network monitor. When using the dialog, Phorge sends this:

__csrf__: B@jqccc7335f3e341bab747f2a
__form__: 1
__dialog__: 1
tokenPHID: PHID-TOKN-emoji-7
__wflow__: true
__ajax__: true
__metablock__: 4

but using the direct URL, it sends this:

__csrf__: B@2rdakjrtaeed9a29351d9a9c
__form__: 1
__dialog__: 1

Unfortunately, all I can gleam is that the JavaScript doesn't trigger correctly when clicking on a token, and never sends the tokenPHID key-value pair as a result. It's clearly not handling the separate form correctly, because disabling JavaScript results in a proper experience.

I haven't the foggiest how this transaction is supposed to use its stub variant; attempting to create a document at /w/parent/child causes an error about /w/parent/ being required.

Replace renderHandleLink() with renderHandle() and not renderObject(), as rightfully pointed out by mainframe98

Ah, thanks, that makes more sense, indeed. (I dislike changes that have an unclear reproduction path.)

@Cigaryno: Please provide references when you quote something from somewhere so others don't have to guess where to find things. Thanks!

In D25859#24307, @mainframe98 wrote:

Shall I just land this then?

This doesn't seem correct. renderObject doesn't take any arguments. I suspect the intention was renderHandle, as that method takes a PHID, which the metadata key mentions.

In D25868#23446, @slip wrote:

I found this, which suggests that this shouldn't be used for headers. https://secure.phabricator.com/D21862

Followed test plan, confirmed findings, accepted revision.

In D25859#24228, @aklapper wrote:

Yes, I'd like to see this merged, [...]

Use !empty instead of isset to match previous type juggling behaviour

@mainframe98's patch for fixing Packages makes me think about the current contribution policy for prototype applications. It currently states:

With rare exceptions, we do not accept patches for prototype applications for the same reasons that we don't accept feature requests or bug reports.

Maybe this is one obsolete Phabricator-specific contributing policy?

Thanks

Probably no need for a warning. Info is OK. Thanks

rebase

Since using E_STRICT is deprecated, I thought the simple fact of using it from whatever Arcanist CLI (e.g. arc anoid) was triggering this new warning (with or without having an exception - since that line 18 is supposed to be always executed I think)

Thanks, I can confirm that this works as expected. :)
I agree biggest question is where to put this. In my opinion the UX is already slightly inconsistent (given their are also concepts like "Related tasks" in Pholio or "Referenced Files" in tasks).
In Differential it feels a bit like a stretch to put this into a box called Revision Contents.
In Maniphest the same "Mentions" tab is in the Related Objects box; there is nothing similar in Differential which already has numerous boxes (also Details and Diff Detail but none of those have tabs).

Superseded by D25861

Because the email address is set no noreply@secure.phorge.dev, it does not seem to be possible with standard emailers to email replies. Blessed Roots may try to add Application Emails to Maniphest and Differential.

close enough for my taste; Just add . at the end of each sentence.

Done. Thank you for your tips!

@xtex: Hi, would you like to arc land your patch? Or do you need any help? Thanks!

@xtex: Hi, would you like to arc land your patch? Or do you need any help? Thanks!

In D25886#23757, @valerio.bozzolan wrote:

Finally something that can be tested using arc anoid - I guess?

rebase? please?

./bin/celerity map for real, sigh

rebase to drop unrelated wrong celerity map change

Yes, I'd like to see this merged, as it fixes going to my local user profile currently only showing

PhutilAggregateException
Encountered a processing exception, then another exception when trying to build a response for the first exception.
- RuntimeException: Undefined array key "totalAsCurrency"
- RuntimeException: Undefined array key "totalAsCurrency"

on PHP 8.3.17 after performing the steps in the Test Plan above.

Beforehand,

• Created a publisher at http://phorge.localhost/packages/publisher/edit/form/default/ but afterwards, http://phorge.localhost/packages/package/publisher/edit/1/ is a 404.
• Created a package at http://phorge.localhost/packages/package/edit/form/default/ but afterwards, http://phorge.localhost/packages/package/package/edit/1/ is a 404.
• Created a version at http://phorge.localhost/packages/version/edit/form/default/ but afterwards, http://phorge.localhost/packages/package/version/edit/1/ is a 404.
• Going to http://phorge.localhost/packages/ is a 404, had to go to http://phorge.localhost/packages/package/ instead.

After applying this patch locally, all these four URIs work as expected.

setControlInstructions() is the right approach, after looking at other usage of it.
Tested the patch locally and works as expected.
I'd probably rephrase to "Callsign is the monogram rXXXX for this repository. It cannot contain spaces." and "Short Name is part of the URI paths for this repository. It cannot contain spaces." (we don't seem to use "repo" but only "repository" across the codebase, and use full stops), and drop the Placeholders (to have all helpful info in a single line), and rename the patch to "Show descriptions for "Callsign" and "Short Name" Repository form fields" (or such).
Apart from all that my nitpicking, good to go. Thanks for the patch!

Rename source files from Tasks to TasksAssigned for consistency; update two more exposed strings

What do you think about @mturdus?

I have also noticed this issue when submitting updates to a revision -

I found a method called setControlInstructions that adds helpful text above the field