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

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, 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

• Refactored to use the setControlInstructions method on the control

How would this work if the task has more than one revision associated with it?

There is one way to sort of do this already, by using milestones for your columns, then you can use herald 'add projects' rules. This only works by sort of abusing the purpose of milestones to take advantage of the fact that milestone columns are mutually exclusive. Adding one milestone removes the other milestones from the same parent project.

I agree that the hover behavior could be improved. Showing only after a delay could help here, though that would make it slightly worse for the times you actually want to interact with the hover.

Remove the mailKey column from PhabricatorPaste

In D25900#24197, @valerio.bozzolan wrote:

You know better than me the context. Is there a possibility that an installation is already without that column?

You know better than me the context. Is there a possibility that an installation is already without that column?

Created D25900 for the deleted paste mailkey migration.

Ran ./bin/storage upgrade --namespace phabricator_quickstart_test2, and that didn't fail.

OK lol no, this has nothing to do with D25898. Need another task for the "Missing input descriptions"

In D25898#24165, @avivey wrote:

• There's already a "description" field on these, which I think was supposed to be displayed; We might have lost it at some point, which is concerning.

• There's already a "description" field on these, which I think was supposed to be displayed; We might have lost it at some point, which is concerning.
• Use pht() for all human-visible text, to allow translation.