Page MenuHomePhorge

Improve command line prompts in setup issue pages
ClosedPublic

Authored by waldyrious on Nov 11 2023, 12:50.
Referenced Files
Unknown Object (File)
Thu, May 23, 00:53
Unknown Object (File)
Tue, May 21, 15:06
Unknown Object (File)
Sun, May 19, 13:26
Unknown Object (File)
Sun, May 19, 07:28
Unknown Object (File)
Sat, May 18, 15:02
Unknown Object (File)
Fri, May 17, 17:09
Unknown Object (File)
Fri, May 17, 15:06
Unknown Object (File)
Fri, May 17, 06:03

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

waldyrious edited the summary of this revision. (Show Details)
waldyrious edited the test plan for this revision. (Show Details)
  • Update the resources map

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)?
src/applications/uiexample/examples/PhabricatorSetupIssueUIExample.php
26–27

Maybe also here? ↑

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

Probably extra $ before ls

src/applications/uiexample/examples/PhabricatorSetupIssueUIExample.php
26–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?

  • Fix double prompt marker

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–79

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

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

✅ 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–45

✅ 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–171

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

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

✅ 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–291

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

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

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

/uiexample/view/PhabricatorSetupIssueUIExample/

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
26–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)

28

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/ :)

  • Improve prompts: use hsprintf instead of cprintf, add missing <tt> markers, simplify syntax
  • Fix missing arguments to hsprintf; re-add csprintf to string using the %R conversion
  • Actually the path is not needed here.

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
176

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?

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

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/
  • ...
This comment was removed by speck.