Page MenuHomePhorge
Differential D25466

Improve command line prompts in setup issue pages
ClosedPublic
Actions

Authored by waldyrious on Nov 11 2023, 12:50.
Tags
Referenced Files
F4068667: D25466.1746807273.diff
Thu, May 8, 16:14
F4042647: D25466.1746725531.diff
Wed, May 7, 17:32
F4036765: D25466.1746718872.diff
Wed, May 7, 15:41
F4036764: D25466.1746718871.diff
Wed, May 7, 15:41
F4036763: D25466.1746718870.diff
Wed, May 7, 15:41
F4036762: D25466.1746718869.diff
Wed, May 7, 15:41
F4036761: D25466.1746718868.diff
Wed, May 7, 15:41
F4036760: D25466.1746718867.diff
Wed, May 7, 15:41
View All Files
Subscribers
Cigaryno
Matthew
Rinalusi44
speck
tobiaswiese
valerio.bozzolan

Details

Reviewers
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Commits
rPc49eeb235ea0: Improve command line prompts in setup issue pages
Summary

This is a follow-up to D25425, where these improvements to the CLI prompt markers were discussed.

Changes included in this revision:

  • Build all prompts the same way
  • Remove space after the prompt marker (add it via CSS instead)
  • Add server path prefix
  • Make the prompt unselectable
Test Plan
  • Visit any of the setup issue pages, e.g. <PHORGE_URL>/config/issue/auth.config-unlocked/ (after ensuring that the corresponding issue is present — in this case, by doing ./bin/auth unlock)
  • For example, Deactivate all PHP extensions to trigger each /config/issue/extension.gd/ etc.
  • For example, update at least up to dc10a7e69ea3 to see the database upgrade tip etc.
  • Confirm that the command line prompts now include the path prefix
  • Confirm that selecting the command via double-click (or click-and-drag) does not select the prompt

Diff Detail

Repository
rP Phorge
Branch
improve-setup-issue-prompts
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 926
Build 926: arc lint + arc unit

Event Timeline

waldyrious created this revision.Nov 11 2023, 12:50
Owners added a reviewer: O1: Blessed Committers.Nov 11 2023, 12:50
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptNov 11 2023, 12:50
Harbormaster completed remote builds in B909: Diff 1477.Nov 11 2023, 12:50
waldyrious requested review of this revision.Nov 11 2023, 12:50
waldyrious edited the test plan for this revision. (Show Details)Nov 11 2023, 12:54
waldyrious edited the summary of this revision. (Show Details)
waldyrious edited the test plan for this revision. (Show Details)Nov 11 2023, 12:56
waldyrious updated this revision to Diff 1478.Nov 11 2023, 12:57
waldyrious edited the test plan for this revision. (Show Details)
  • Update the resources map
Harbormaster completed remote builds in B910: Diff 1478.Nov 11 2023, 12:57
waldyrious added a comment.Nov 11 2023, 13:08

I am not sure about all these changes. In particular, the changes in PhabricatorSetupIssueUIExample.php and PhabricatorSetupIssueView.php probably warrant extra scrutiny.

Similarly, I'd like some feedback about the changes in PhabricatorExtraConfigSetupCheck.php, which uses a different syntax to build the command compared to the other ones.

Finally, PhabricatorBaseURISetupCheck.php, I have two questions:

  1. Is it justified to use a csprintf() within an hsprintf(), or could it be simplified to a single hsprintf()? call, like the other ones?
  2. Does it even make sense to prefix the prompt with getPlatformServerPath(), considering that the error is precisely about the base URI ? In other words, does getPlatformServerPath() return a useful/correct value at that point)?
valerio.bozzolan added inline comments.Nov 13 2023, 08:19
src/applications/uiexample/examples/PhabricatorSetupIssueUIExample.php
27

Maybe also here? ↑

valerio.bozzolan added inline comments.Nov 13 2023, 08:21
src/applications/uiexample/examples/PhabricatorSetupIssueUIExample.php
29

Probably extra $ before ls

waldyrious added a comment.Nov 13 2023, 23:23

Nice catches, @valerio.bozzolan — will fix.

waldyrious added inline comments.Nov 13 2023, 23:28
src/applications/uiexample/examples/PhabricatorSetupIssueUIExample.php
27

I'm not really sure about this one. The fact that it's immediately above the other one makes it seem like it was deliberately made this way 🤔 Specifically, why wouldn't they use <tt>s around the prompt marker as done in the line below?

waldyrious updated this revision to Diff 1503.Nov 13 2023, 23:30
  • Fix double prompt marker
Harbormaster completed remote builds in B926: Diff 1503.Nov 13 2023, 23:30
valerio.bozzolan added a comment.Nov 14 2023, 07:29

I love your change. Simple and effective and without impact on other parts.

We have just few "undone" comments.

For other people, here a screenshot:

Prompt proposal.png (121×271 px, 4 KB)

After the fixes I can immediately hard-accept. Thank you so much

src/applications/config/check/PhabricatorAuthSetupCheck.php
77–78

✅ I verified this manually breaking my Phorge to trigger the issue.

src/applications/config/check/PhabricatorBaseURISetupCheck.php
99–102

✅ I verified this manually breaking my Phorge to trigger the issue.

src/applications/config/check/PhabricatorDaemonsSetupCheck.php
53–55

✅ I verified this manually breaking my Phorge to trigger the issue.

97–99

✅ I verified this manually breaking my Phorge to trigger the issue.

(I was not aware of this configuration phd.user)

src/applications/config/check/PhabricatorDatabaseSetupCheck.php
38–44

✅ I verified this manually breaking my Phorge to trigger the issue.

140–142

✅ I verified this manually breaking my Phorge to trigger the issue.

168–170

✅ I verified this manually breaking my Phorge to trigger the issue.

src/applications/config/check/PhabricatorElasticsearchSetupCheck.php
63–66

✅ I verified this manually breaking my Phorge to trigger the issue.

83–85

✅ I verified this manually breaking my Phorge to trigger the issue.

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
79–83

This was tricky! It seems we must use hsprintf() otherwise csprintf() does not render HTML correctly.

src/applications/config/view/PhabricatorSetupIssueView.php
86–91

(Non-blocking tip)

Feel free to do the same here. You can test this here:

/uiexample/view/PhabricatorSetupIssueUIExample/
288–290

✅ I verified this manually breaking my Phorge to trigger the issue. Can be tested here:

/uiexample/view/PhabricatorSetupIssueUIExample/
src/applications/uiexample/examples/PhabricatorSetupIssueUIExample.php
29

You can test this visiting this page to see the $ to be fixed:

/uiexample/view/PhabricatorSetupIssueUIExample/
waldyrious added a comment.Nov 14 2023, 11:36

For future reference, I went digging for the difference between csprintf() and hsprintf(), and found the following:

From Arcanist's src/xsprintf/csprintf.php:

Format a shell command string. This function behaves like sprintf, except that all the normal conversions (like "%s") will be properly escaped [...]

From Phorge's src/infrastructure/markup/render.php

Format a HTML code. This function behaves like sprintf(), except that all the normal conversions (like %s) will be properly escaped.

Considering that in all cases we're only interpolating %s, and not making use of any of csprint's additional conversions (%Ls, %LR, %P, %C, %R), I'd say it's safe and appropriate to rely exclusively on hsprintf since the strings indeed include HTML markup.

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
79–83

Interesting! Happy to change this as it will make things more consistent. I suppose, then, that we could remove the csprintf in src/applications/config/check/PhabricatorBaseURISetupCheck.php as well and just use hsprintf, right?

src/applications/config/view/PhabricatorSetupIssueView.php
86–91

Absolutely, nice catch :)

src/applications/uiexample/examples/PhabricatorSetupIssueUIExample.php
27

Ok, after checking https://we.phorge.it/uiexample/view/PhabricatorSetupIssueUIExample/ I agree it makes sense to add the prompt here too, since both the comment and the command are wrapped within the same <pre> tag so they look like two entries in the same shell session. Here's what they currently look like, before the changes of this revision:

image.png (99×190 px, 5 KB)

29

How do I "break my Phorge" to make this page accessible? I get a 404 Not Found error if I follow that link in my local Phorge installation...

Oh, I noticed I can simply visit https://we.phorge.it/uiexample/view/PhabricatorSetupIssueUIExample/ :)

waldyrious attached a referenced file: F397870: image.png. (Show Details)Nov 14 2023, 11:36
waldyrious updated this revision to Diff 1507.Nov 14 2023, 11:40
  • Improve prompts: use hsprintf instead of cprintf, add missing <tt> markers, simplify syntax
Harbormaster completed remote builds in B930: Diff 1507.Nov 14 2023, 11:40
waldyrious updated this revision to Diff 1508.Nov 14 2023, 11:46
  • Fix missing arguments to hsprintf; re-add csprintf to string using the %R conversion
Harbormaster completed remote builds in B931: Diff 1508.Nov 14 2023, 11:46
waldyrious updated this revision to Diff 1509.Nov 14 2023, 11:47
  • Actually the path is not needed here.
Harbormaster completed remote builds in B932: Diff 1509.Nov 14 2023, 11:47
waldyrious edited the test plan for this revision. (Show Details)Nov 14 2023, 11:50
waldyrious added a comment.Nov 14 2023, 12:24

Btw, there are some similar commands in src/applications/config/check/PhabricatorManualActivitySetupCheck.php. Should I change them as well? How can I "break Phorge" to see this page rendered?

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
178

Note that the other instance of this command in this file (line 79) used simple %s for interpolating the $key, so perhaps we could do the same here and avoid the need to nest a csprintf() call?

waldyrious mentioned this in D25473: Change some instances of "phabricator" to "phorge".Nov 14 2023, 14:28
waldyrious added projects: UX, Config.Nov 14 2023, 14:40
waldyrious mentioned this in Z5: Phorge (flame).Dec 6 2023, 15:40
valerio.bozzolan accepted this revision.Dec 7 2023, 11:54

I love it, thanks.

I've tested this, destroying my poor GNU/Linux in a lot of creative ways to spawn all sort of nice installation tips. So much relaxing.

sgtm

This revision is now accepted and ready to land.Dec 7 2023, 11:54
valerio.bozzolan edited the test plan for this revision. (Show Details)Dec 7 2023, 11:55
valerio.bozzolan added a comment.Dec 7 2023, 11:59

We may want to expand the Test Plan to mention additional tips to help newcomers in breaking their installation.

For example

  • put Phorge in a toaster with FreeBSD under water and see missing zip extension etc.
  • kill the daemon as much hard as you can (kill signal -9) and test the page /config/issue/daemons.not-running/
  • ...
Closed by commit rPc49eeb235ea0: Improve command line prompts in setup issue pages. · Explain WhyDec 7 2023, 16:22
This revision was automatically updated to reflect the committed changes.
waldyrious added a commit: rPc49eeb235ea0: Improve command line prompts in setup issue pages.
Rinalusi44 subscribed.Dec 10 2023, 17:40
This comment was removed by speck.
speck subscribed.Dec 10 2023, 23:33

Revision Contents
Changeset List

Diff 1503

View Options

resources/celerity/map.php

Loading...
View Options

src/applications/config/check/PhabricatorAuthSetupCheck.php

Loading...
View Options

src/applications/config/check/PhabricatorBaseURISetupCheck.php

Loading...
View Options

src/applications/config/check/PhabricatorDaemonsSetupCheck.php

Loading...
View Options

src/applications/config/check/PhabricatorDatabaseSetupCheck.php

Loading...
View Options

src/applications/config/check/PhabricatorElasticsearchSetupCheck.php

Loading...
View Options

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php

Loading...
View Options

src/applications/config/view/PhabricatorSetupIssueView.php

Loading...
View Options

src/applications/uiexample/examples/PhabricatorSetupIssueUIExample.php

Loading...
View Options

webroot/rsrc/css/application/config/setup-issue.css

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0