Page MenuHomePhabricator

Trying out removing "Phabricator" from some user-visible text
Changes PlannedPublic

Authored by cspeckmim on Jul 27 2021, 12:44 AM.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T13658: How to rebrand Phabricator
Summary

Refs T13658

Trying out a small subset of changes to discuss how phrasing changes feel.

Test Plan

None, yet

Diff Detail

Repository
rP Phabricator
Branch
phlab
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 25528
Build 35297: Run Core Tests
Build 35296: arc lint + arc unit

Event Timeline

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
cspeckmim marked 7 inline comments as done.

Preferring "installation" as a generic term, where possible

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"?

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".

I'll plan to get this on a test instance and get some screenshots