Remove mention of Phabricator in the Auth setup check
Needs ReviewPublic
Actions

Authored by waldyrious on Tue, Aug 29, 21:18.

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

waldyrious created this revision.Tue, Aug 29, 21:18

Owners added a reviewer: O1: Blessed Committers.Tue, Aug 29, 21:18

Herald added subscribers: Cigaryno, Matthew, tobiaswiese. · View Herald TranscriptTue, Aug 29, 21:18

waldyrious requested review of this revision.Tue, Aug 29, 21:18

waldyrious mentioned this in Z5: Phorge (flame).

waldyrious removed a reviewer: Blessed Committers.Tue, Aug 29, 21:33

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.

valerio.bozzolan edited the summary of this revision. (Show Details)Wed, Aug 30, 07:07

valerio.bozzolan edited the summary of this revision. (Show Details)

valerio.bozzolan added a task: T15006: Re-brand Phorge.

In D25425#12230, @valerio.bozzolan wrote:

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.

valerio.bozzolan added inline comments.Tue, Sep 19, 12:29

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/

valerio.bozzolan added inline comments.Tue, Sep 19, 12:31

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.

waldyrious added inline comments.Sat, Sep 23, 14:29

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.