Page MenuHomePhorge

Remove mention of Phabricator in the Auth setup check
Needs ReviewPublic

Authored by waldyrious on Tue, Aug 29, 21:18.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Sep 21, 03:33
Unknown Object (File)
Thu, Sep 21, 01:54
Unknown Object (File)
Tue, Sep 12, 10:03
Unknown Object (File)
Mon, Sep 11, 16:04
Unknown Object (File)
Sun, Sep 10, 13:48
Unknown Object (File)
Fri, Sep 8, 06:12

Details

Reviewers
None
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15006: Re-brand Phorge
Summary

The authentication setup check, available at <PHORGE_URL>/config/issue/auth.config-unlocked/,
contained a reference to Phabricator in the prompt of the command line hint to resolve the issue.
Similar checks only showed the prompt symbol, not the directory, so this one was changed to match.

Ref T15006

Test Plan
  • Run ./bin/auth unlock
  • Visit <PHORGE_URL>/config/issue/auth.config-unlocked/
  • Notice that, with this patch, "phabricator" no longer appears in the prompt prefix for the suggested fix command at the end of the page.

Diff Detail

Repository
rP Phorge
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

ooh, check out D25349 for basically this exact use-case...

Thanks for this patch. Yeah if I understand correctly, this should work:

hsprintf(
  '<tt>%s $</tt> ./bin/auth lock',
  PlatformSymbols::getPlatformServerPath())

And maybe in the future we can have a CSS selector to be applied just to the "phorge/" pathname, in order to avoid mouse selection, and select just the command. But now let's fix just this brand issue.

maybe in the future we can have a CSS selector to be applied just to the "phorge/" pathname, in order to avoid mouse selection, and select just the command.

Yeah, I would like to see that. One way to implement it is to produce the prompt character and directory prefix via CSS, as a pseudo-element (::before { content: 'foobar'; }), but it's also possible to wrap the prompt in an addressable element and apply user-select: none; to it. I personally prefer the former approach (it seems more semantically appropriate) but the latter is also valid.

src/applications/config/check/PhabricatorAuthSetupCheck.php
75

So, maybe just this, in order to have the minimal fix for the brand issue:

->addCommand(
  hsprintf(
    '<tt>%s $</tt> ./bin/auth lock',
    PlatformSymbols::getPlatformServerPath()));

Then we can do something else to have the server path non-selectable

src/applications/config/check/PhabricatorDatabaseSetupCheck.php
37–39

Maybe we can restore this intentional indentation

Historically they liked all arguments in the same indentation :D

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

src/applications/config/check/PhabricatorAuthSetupCheck.php
75

Then we can do something else to have the server path non-selectable

With this phrase I mean, later, with another patch, we can surely try to improve the <tt> tag to be non-selectable. But maybe not now. It's still useful to see where this command should be executed (so, from Arcanist, or from Phorge, etc.) so the prefix is probably still useful.

src/applications/config/check/PhabricatorAuthSetupCheck.php
75

So, maybe just this, in order to have the minimal fix for the brand issue:

I would normally agree, but this was (AFAICT) the only case of a path being shown before the prompt symbol in these check messages. So I think this change (removing "phabricator" altogether rather than just replacing it with getPlatformServerPath()) is justified because it aligns this code with all the other ones.

That's not to say I don't see the value of having the path here — but like the non-selectability bit, I think adding the path should be done in a separate patch and affecting all similar messages consistently.

src/applications/config/check/PhabricatorDatabaseSetupCheck.php
37–39

Oh, ok, I can revert these changes. I can tell there are multiple instances of either indentation style so it's probably best to do these kinds of changes in bulk and via an automated process, rather than manually.