This is just trying out some changes that modify the language to reduce instances of "Phabricator" from user-visible text. I went through the auth provider implementations to try out using "platform", "server", "service", and in one case I think just "instance".

This is only up for review for discussion about how this feels. To me the term "platform" feels a little clunky, but I also think (especially in the auth providers or clusters interface) it might be more useful to differentiate between other services or instances.

These generally look reasonable to me, some thoughts inline. In some of these cases I think we can probably just remove extra references to "this install", "this install of Phabricator on this server", etc., and let it be implied by context.

I've also used "install" as a noun a lot ("this install") but I think that's not formally correct English (it should be "this installation"). It's probably better to use "installation" if we're touching these.

You're definitely right about the icon thing.

src/applications/auth/provider/PhabricatorAmazonAuthProvider.php
17	I think just "your install" would be reasonable here, although arguably we should stop using "install" as a noun and say "your installation" or "this installation" instead (and below, "...on this install.").
src/applications/auth/provider/PhabricatorAuthProvider.php
282	Yes, it should. This code will produce the wrong outcome if provider names are translated, although it's probably common for providers like "Amazon" or "GitHub" to localize to the same string.
src/applications/auth/provider/PhabricatorGitHubAuthProvider.php
22	Maybe just "for this installation"
src/applications/auth/provider/PhabricatorJIRAAuthProvider.php
35–45	These are Remarkup so I think we can do this most gracefully with some `{{{platform.name}}}` thing. I can send you a diff for that.
202–205	Maybe just: Choose a permanent name for this instance of JIRA. This name is used internally to keep track of this particular JIRA instance, in case the URL changes later.
src/applications/auth/provider/PhabricatorLDAPAuthProvider.php
13–14	Maybe just "...to log in.", I think it's likely obvious from context.
src/applications/auth/provider/PhabricatorPhabricatorAuthProvider.php
15–19	Maybe just: Choose a Service Name Choose a permanent name for the remote service. This name is used internally to keep track of the service, in case the URL changes later.
109	These are probably clear enough as just "Instance name is required", etc.

This revision now requires changes to proceed.Jul 27 2021, 8:36 PM

Herald added a subscriber: Korvin. · View Herald TranscriptJul 27 2021, 8:36 PM

Preferring "installation" as a generic term, where possible

Harbormaster failed remote builds in B25526: Diff 51726!Jul 28 2021, 3:55 AM

Minor corrections

cspeckmim added a parent revision: D21713: Add an "eval" rule to Remarkup.Jul 28 2021, 3:59 AM

Harbormaster failed remote builds in B25527: Diff 51727!Jul 28 2021, 3:59 AM

I'll investigate the build failures -- I suspect it's the use of the new remarkup rule. At the moment I'm working from a Windows system and haven't been able to get xhpast or unit tests running properly.

src/applications/auth/provider/PhabricatorPhabricatorAuthProvider.php
109	If this is in reference to the steps above which is now "Choose a Service Name", should this be "Service name is required"? Or should "Choose a Service Name" be changed to "Choose an Instance Name"?

cspeckmim added inline comments.Jul 28 2021, 4:07 AM

src/applications/auth/provider/PhabricatorJIRAAuthProvider.php
35	Oh I bet these need escaped

That build failure may be related to recent changes to T13072, or something else server-side -- I don't expect build failures to look like that in Harbormaster. Let me see if I can reproduce it locally.

src/applications/auth/provider/PhabricatorPhabricatorAuthProvider.php
109	I think switching everything to "service" is probably more clear. I'm not sure my use of "instance" is totally obvious to readers, and when Phabricator later added support for actual instancing (via `PHABRICATOR_INSTANCE` / `cluster.instance`) that muddied the waters a bit further. Ideally, in modern documentation/UI stuff, I think we should probably use "instance" as a noun only to refer to a particular instantiation of Phabricator using instancing via the `cluster.instance` mechanism. Phacility is likely the only service in the wild that uses this mechanism.

Ah, yeah, the build issue is that ${...} in a PHP double-quoted string is semantic, so PHP is trying to do something with ${{{...}}} and failing with a syntax exception during parsing. You can either escape ${{{strings.x.y}}} as \${{{strings.x.y}}} or suggest a different syntax for the "eval" rule --- I'm not married to ${{{...}}}.

Here's the more useful output I get locally:

$ arc unit --everything
   PASS  597ms   PhabricatorLibraryTestCase::testLibraryMap
   FAIL  PhabricatorLibraryTestCase::testMethodVisibility
EXCEPTION (Exception): ParseError: syntax error, unexpected '{' in /Users/epriestley/dev/core/lib/phabricator/src/applications/auth/provider/PhabricatorJIRAAuthProvider.php:35
#0 /Users/epriestley/dev/core/lib/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude('/Users/epriestl...')
#1 /Users/epriestley/dev/core/lib/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource('phabricator', 'applications/au...')
#2 /Users/epriestley/dev/core/lib/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol(Array)
#3 /Users/epriestley/dev/core/lib/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#4 [internal function]: __phutil_autoload('PhabricatorJIRA...')
#5 [internal function]: spl_autoload_call('PhabricatorJIRA...')
#6 /Users/epriestley/dev/core/lib/arcanist/src/__tests__/PhutilLibraryTestCase.php(102): ReflectionClass->__construct('PhabricatorJIRA...')
#7 /Users/epriestley/dev/core/lib/arcanist/src/unit/engine/phutil/PhutilTestCase.php(493): PhutilLibraryTestCase->testMethodVisibility()
#8 /Users/epriestley/dev/core/lib/arcanist/src/unit/engine/PhutilUnitTestEngine.php(69): PhutilTestCase->run()
#9 /Users/epriestley/dev/core/lib/arcanist/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php(148): PhutilUnitTestEngine->run()
#10 /Users/epriestley/dev/core/lib/arcanist/src/workflow/ArcanistUnitWorkflow.php(170): ArcanistConfigurationDrivenUnitTestEngine->run()
#11 /Users/epriestley/dev/core/lib/arcanist/scripts/arcanist.php(419): ArcanistUnitWorkflow->run()
#12 {main}
   FAIL  PhabricatorLibraryTestCase::testEverythingImplemented
EXCEPTION (PhutilMissingSymbolException): Failed to load symbol "PhabricatorJIRAAuthProvider" (of type "class or interface").

The symbol map for library 'phabricator' (at '/Users/epriestley/dev/core/lib/phabricator/src') claims this class or interface is defined in 'applications/auth/provider/PhabricatorJIRAAuthProvider.php', but loading that source file did not cause the class or interface to become defined.

If you are not a developer, this almost always means that a library is out of date. For example, you may have upgraded "phabricator/" without upgrading "arcanist/", or vice versa. It might also mean that you need to restart Apache or PHP-FPM. Make sure all libraries are up to date and all services have been restarted.

If you are a developer and this symbol was recently added or moved, your library map may need to be rebuilt. You can rebuild the map by running "arc liberate".

For more information, see: https://phurl.io/u/newclasses
#0 /Users/epriestley/dev/core/lib/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol(Array)
#1 /Users/epriestley/dev/core/lib/arcanist/src/__tests__/PhutilLibraryTestCase.php(16): PhutilSymbolLoader->selectAndLoadSymbols()
#2 /Users/epriestley/dev/core/lib/arcanist/src/unit/engine/phutil/PhutilTestCase.php(493): PhutilLibraryTestCase->testEverythingImplemented()
#3 /Users/epriestley/dev/core/lib/arcanist/src/unit/engine/PhutilUnitTestEngine.php(69): PhutilTestCase->run()
#4 /Users/epriestley/dev/core/lib/arcanist/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php(148): PhutilUnitTestEngine->run()
#5 /Users/epriestley/dev/core/lib/arcanist/src/workflow/ArcanistUnitWorkflow.php(170): ArcanistConfigurationDrivenUnitTestEngine->run()
#6 /Users/epriestley/dev/core/lib/arcanist/scripts/arcanist.php(419): ArcanistUnitWorkflow->run()
#7 {main}
...

There is some other build pipeline issue causing that to not report, but it's likely something mundane like "sufficiently severe runtime fatals don't get caught by the right layer".