Thanks. I would like to help in reviewing but I don't get the difference. Can you describe the inline change?

0. Welcome!

1. This diff is based of a commit that's not in master - some local changes that make it not-apply
2. the space should be on the inside of the pht(), not attached to it.
3. even better, find whatever is calling describeAutomaticCapability(), and if it accepts an array (some implementations return array, some return string), have the caller add a space between each element of the array - solving this once for all callers...

In D25419#12006, @valerio.bozzolan wrote:

It seems your "master" branch has (many?) local unexpected commits.

In order to work on this feature, in Phorge, try these:

git status
# you should be on master - please eventually manually delete any local uncommitted change

Then, create another clean branch:

git branch   pippo_feature
git switch   pippo_feature
git reset --hard origin/master

So now you are on a custom branch pippo_feature that is super-clean. From there you can add fresh features and propose whatever patch.

(To switch between branches, just git switch master or git switch pippo_feature ecc.)

Thanks a lot for the workflow, pippo example is always the best example

Nice catch

I'm not 100% sure but maybe it's the consumer of getSpecialRules() that should do that.

Maybe here:

https://we.phorge.it/source/phorge/browse/master/src/applications/policy/controller/PhabricatorPolicyExplainController.php;7868ab3754fad13714640451d664d5bc71b7a02f$224-228

What do you think about?

What do you think about the related tip? Also here I can maybe try to propose a counter-patch next week

@valerio.bozzolan I actually think the right place to look at the usage of getSpecialRules() might be here: https://we.phorge.it/source/phorge/browse/master/src/applications/policy/filter/PhabricatorPolicyFilter.php;4f845d8f8c77$693-719

I don't know exactly what to change there, though. I see the $text_details = implode(' ', $text_details); which seems to be doing the right thing, but that variable is then used in the exception's full message, whereas the $html_details is constructed as merely array($head, $more, $exceptions); and then used as setMoreInfo($html_details);.

I see also the usage of this PhabricatorPolicyException::setMoreInfo() sets the moreInfo class member which is then read by PhabricatorPolicyException::getMoreInfo(), in particular here: https://we.phorge.it/source/phorge/browse/master/src/aphront/handler/PhabricatorPolicyRequestExceptionHandler.php;9481b9eff111$58-84 — which is weird, because the AphrontDialogView::appendList() method, which is called there, appears to produce an <ul> tag, as per the code in defined in https://we.phorge.it/source/phorge/browse/master/src/view/AphrontDialogView.php;a89b4ff5b8bc$226-243. So I'm a bit lost at this point...

... whereas the $html_details is constructed as merely array($head, $more, $exceptions); ...

I suspect that was/is there to allow $exceptions to have fancy HTML content, but if there is any fancy HTML content in there, the result of $text_details = implode(' ', $text_details) would end up strange (and be text anyway. and maybe start throwing/warning in php 8?). I wish we had more static typing.

..., appears to produce an <ul> tag, as per the code...

The value of moreInfo here is a list (array) of 3 things, (https://we.phorge.it/source/phorge/browse/master/src/applications/policy/filter/PhabricatorPolicyFilter.php;4f845d8f8c77$702) - $head, $more and $exceptions, and I'm guessing from context that the appendList() creates one <ul> for the list and one <li> for each item in the list - so $exceptions at this point is a single thing.

Anyway, I think it would be ok to assume that getSpecialRules should return a "list of strings", and then either implode it in habricatorPolicyFilter, or go with the added whitespace as currently suggested here.

In D25419#12602, @avivey wrote:

The value of moreInfo here is a list (array) of 3 things, (https://we.phorge.it/source/phorge/browse/master/src/applications/policy/filter/PhabricatorPolicyFilter.php;4f845d8f8c77$702) - $head, $more and $exceptions, and I'm guessing from context that the appendList() creates one <ul> for the list and one <li> for each item in the list - so $exceptions at this point is a single thing.

Oh! Yeah, that makes sense, thanks for the clarification!

Anyway, I think it would be ok to assume that getSpecialRules should return a "list of strings", and then either implode it in habricatorPolicyFilter, or go with the added whitespace as currently suggested here.

So would the latter option mean doing $html_details = array($head, $more, implode(' ', $exceptions)); here? I kind of find this approach cleaner than appending a space to each item in the $exceptions array.

So would the latter option mean doing $html_details = array($head, $more, implode(' ', $exceptions)); here? I kind of find this approach cleaner than appending a space to each item in the $exceptions array.

Yeah, exactly.

Should I go ahead and submit a different patch? Or could I somehow contribute the edit to this patch?

In D25419#12616, @waldyrious wrote:

Should I go ahead and submit a different patch? Or could I somehow contribute the edit to this patch?

@waldyrious: I think you could ./arcanist/bin/arc patch D25419, make changes, and then ./arcanist/bin/arc diff --update D25419 to push an updated version.
Anyone could always travel back in time via arc patch --diff 1334.

Thanks for the note, Andre! I've went ahead and updated the revision as discussed above.

I can confirm that this patch fixes the string problem. However, I am super super puzzled that after applying this patch, the CSS definition line-height: 22px for .phabricator-wordmark is suddenly missing in both Chromium 120 and Firefox 120, moving the wordmark a bit higher in the upper left corner (on my two screenshots, that is "Phoriorge").

That's so weird! My instinct is of course to say that it should be completely unrelated to this patch since no CSS is touched in this one-line change... I have no idea how to debug this ☹️

I'm just unsure between implode() or phutil_implode_html()

https://we.phorge.it/book/contrib/article/rendering_html/

In D25419#14821, @valerio.bozzolan wrote:

I'm just unsure between implode() or phutil_implode_html()

In my understanding all these strings should be plain text (no hypertext involved).

On the other hand, as the variable is called $html_details let's go for phutil_implode_html() to be super-safe? ping @roberto.urbani

Thanks for the recommendation, Andre! I conferred with @valerio.bozzolan and he agrees, too. I have updated the patch.

Below are the screenshots of the result:

Before: