Page MenuHomePhorge

T15006: Replacing external-facing trademarks/assets
Changes PlannedPublic

Authored by speck on Jun 18 2021, 03:49.

Details

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

Refs T15006

  • Move the logo filename from light-eye.png to a more-generic project-logo.png
  • Update main-menu-view.css to reference project-logo.png
  • Add static methods to PhabricatorPlatformSite to get the project name and organization name, hardcode these to "Phabricator" and "Phacility"
  • Fix all occurrences of "Phabricator" and "Phacility" appearing in user-visible text, when passed through pht() and replace with the newly added methods
Test Plan

TBD

Diff Detail

Repository
rP Phorge
Branch
brand
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 5
Build 5: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
speck requested review of this revision.Jun 18 2021, 03:49

Renaming getDefaultProjectName() to getDefaultWordmark()

This revision is now accepted and ready to land.Jun 18 2021, 05:25
speck planned changes to this revision.Jun 18 2021, 12:54

I think the plan for this is going to be

  1. Try to address all external-facing "Phabricator"s
  2. Submit this patch upstream on secure.phabricator.com
  3. Phorge pulls in this change from upstream

Given #2 I think we'll probably keep the Phabricator logo/icon/wordmark, but try to extract it and all other instances of "Phabricator" to a single place to be modified

  • Revert the logo and favicon (but keep logo rename)
  • Added PhabricatorApplication::PROJECT_APPLICATION_NAME to default to Phabricator
  • Updated instances of 'Phabricator' to use the new constant instead
This revision is now accepted and ready to land.Jun 19 2021, 04:22
speck planned changes to this revision.Jun 19 2021, 04:25

Still need to search for instances of Phabricator appearing anywhere within quotes. Testing all this will be fun.

src/applications/base/PhabricatorApplication.php
17 ↗(On Diff #13)

Before I get too much further on this I think this may actually need to be defined in Arcanist and not here. I’m sure Arcanist has loads of hardcoded “Phabricator” as well and it would be better to have it defined in a single place.

deadalnix added inline comments.
src/applications/base/PhabricatorApplication.php
17 ↗(On Diff #13)

Then maybe it is best to make this a function? This way where it points can be changed later on.

src/applications/config/custom/PhabricatorCustomLogoConfigType.php
17

I know this is done in various places, but I don't really think it make sense to translate the project name.

In addition, most translation tool have a pass to extract strings from the codebase, and so passing variables (or constant for that matter) in them breaks.

src/applications/config/custom/PhabricatorCustomLogoConfigType.php
17

Yeah, a non-literal will not work in pht.

src/applications/config/custom/PhabricatorCustomLogoConfigType.php
17

I was afraid of that. Is the only route to review all externalized text and re-word it to not mention the product name? That might not be possible in all cases

src/applications/config/custom/PhabricatorCustomLogoConfigType.php
17

Not at all! The proper way is to inject the product name as a parameter. Something like:

pht('Your %s account has been successfully created!', getProductName());

You want to translate sentences, but you don't really want to translate the product name itself. Phorge is going to be called phorge in English, French, Spanish or Greek.

As an asside, you might want to split up the logo rename part of that diff in another diff, this should be able to get in eight away, and there is no reason to wait on a resolution for the translation business to move on with it.

src/applications/config/custom/PhabricatorCustomLogoConfigType.php
17

I think the idiom is something like

pht("%s", Foo::BAR);

Pretty much untranslatable, but there's qqq and also it's probably not translated anyway.

As an asside, you might want to split up the logo rename part of that diff in another diff, this should be able to get in eight away, and there is no reason to wait on a resolution for the translation business to move on with it.

I'd like to keep this in the same diff that we can submit for upstream to consider. The wordmark and logo are already configurable so I don't think there's a need to have that upstream before the rest of the rebranding.

src/applications/base/PhabricatorApplication.php
17 ↗(On Diff #13)

Yea I think turning this into a function will be better

src/applications/config/custom/PhabricatorCustomLogoConfigType.php
17

I think the idiom is something like

pht("%s", Foo::BAR);

Okay that's the approach I've taken in other parts (e.g. PhabricatorPhabricatorAuthProvider and PhabricatorMailEmailEngine. Here I was just trying to simplify that in both places this method is called it's equivalent to what it was before (see line 111 below).

I'll probably remove this method and swap out the two callers with pht('%s', PhabricatorApplication::PROJECT_APPLICATION_NAME)

Test and Lint coverage?

I guess I am not sure on the overall community philosophy here related to coverage, However I am in the "almost all code should be covered" camp...

Thoughts?

It says "Lint/Unit were skipped" for this diff - did you actually "skip" them, or is this some setup/usage artifact?

I did skip lint and unit tests for now as I could not get them working properly on my system (something blew up trying to connect to MySQL). I will be setting up a proper development environment at some point.

To be clear about the state of this diff, I estimate it to be ~10-15% done based on the changes I think still need to be made. I initially wanted to get some feedback about the approach here but I do anticipate further large changes to be needed here. If wanted I could abandon this and recreate using --draft to avoid sending out notifications.

How can I help?
Are we down to just grepping, or is there still some design left to do?

In D25002#144, @speck wrote:

Still need to search for instances of Phabricator appearing anywhere within quotes. Testing all this will be fun.

Ahh, we have scripts for that 😁
There's a bunch of scripts that collect all text in pht() - I know there's one relating to the Wikitranslate effort...

edit: It's included in the Phabricator code. The command is ./bin/i18n extract . and the output goes into ./src/.cache.

This is roughly what I was planning on

  1. Move PhabricatorApplication::PROJECT_APPLICATION_NAME into PhabricatorSite::getName() (I have a local change for this)
  2. Update all existing PhabricatorApplication::PROJECT_APPLICATION_NAME uses to PhabricatorSite::getName() (I haven't yet done this, still figuring out best tooling/environment for PHP development)
  3. Update PhabricatorCustomLogoConfigType::getDefaultWordmark() to not use pht() but instead have the callers wrap the result in pht().
  4. Grep for Phabricator and Phacility appearing anywhere within single or double quotes. Likely need to exclude celerity-generated files. Replace with PhabricatorSite::getName()
  5. Create PhabricatorSite::getProjectOrg() to return Phacility if it's needed from #4

Then following this

  1. Update rARC to define both getName() and getProjectOrg(), have PhabricatorSite::getName() and ::getProjectOrg()` pull the values from rARC classes -- this sets a hard dependency that Phabricator will require the same up-to-date version of Arcanist to run properly but I think that's already a requirement.
  2. Update rARC using the same #4 process above

And then

  1. Test this change
  2. Submit the change to Phabricator
  3. Assuming it's accepted upstream (probably with some modifications), update our local copy of rP to fork from this latest version

Running ./bin/i18n extract . and then this scripts, shows 455 strings with the words Phabricator or Phacility (1 for Phacility), plus 19 in arcanist.

#! /usr/bin/env python3

import json

with open("src/.cache/i18n_strings.json") as input:
    data = json.load(input)


def filter(key):
    return "Phabricator" in key or "Phacility" in key


data = {k: v for k, v in data.items() if filter(k)}
print(len(data))

with open("strings-with-trademark.json", "w") as output:
    json.dump(data, output, indent=2)

It's bordering at writing some script to change all instances, but also might be simple to handle manually.

This is assuming all user-facing text is wrapped in pht(), which is supposed to be.

Cool script @avivey! - I ran it (see P2) and see that we have a large task at hand...

One concern here will be the massive number of files that will be updated, and it will not be a simple update

Consider this one example

$body = sprintf(
      "%s\n\n  %s\n  %s\n",
      pht(
        '%s (%s) has changed your Phabricator username.',
        $sender_username,
        $sender_realname),
      pht(
        'Old Username: %s',
        $old_username),
      pht(
        'New Username: %s',
        $new_username));

    return id(new PhabricatorMetaMTAMail())
      ->setSubject(pht('[Phabricator] Username Changed'))
      ->setBody($body);

becomes

$body = sprintf(
      "%s\n\n  %s\n  %s\n",
      pht(
        '%s (%s) has changed your %s username.',
        $sender_username,
        $sender_realname,
        PhabricatorCustomLogoConfigType::getDefaultWordmark()
        ),
      pht(
        'Old Username: %s',
        $old_username),
      pht(
        'New Username: %s',
        $new_username));

    return id(new PhabricatorMetaMTAMail())
      ->setSubject(pht('[%s] Username Changed',PhabricatorCustomLogoConfigType::getDefaultWordmark()))
      ->setBody($body);

Now - based on that change, and assuming no test coverage - Can you tell me if I made a mistake??

I wonder if a better path forward, is to create specialized token like {Phabricator} that pht(...) knows how to replace

With tokenized Phabricator

$body = sprintf(
      "%s\n\n  %s\n  %s\n",
      pht(
        '%s (%s) has changed your {Phabricator} username.',
        $sender_username,
        $sender_realname),
      pht(
        'Old Username: %s',
        $old_username),
      pht(
        'New Username: %s',
        $new_username));

    return id(new PhabricatorMetaMTAMail())
      ->setSubject(pht('[{Phabricator}] Username Changed'))
      ->setBody($body);

Then we update pht to always check for {Phabricator} and replace with PhabricatorCustomLogoConfigType::getDefaultWordmark()

Thoughts?

How many of them are there? Eyeballing P2, it's of the order of 500. There are 5 people in this task, we can do 15/day and be done in a week. That doesn't sound intractable if we are all willing to chip in.

@speck Can we get the initial function rolling? this is what's needed for this piece of work to fan out.

Also, it doesn't seems likely anymore that this will go through all at once. The diff is going to be monstrous. We'd be trying to push an elephant through a pinhole. Let's get the initial setup going, use it in a couple of call sites, try to upstream that, do the changes upstream wants for it to be accepted, and power through translating all the callsites.

Discussion like do we add a {Phabricator} token in the translator are worth having, but really, without involving upstream, there isn't really a point. Whatever way they are happy with is going to be the way we'll do it, unless it is horrendous.

The best way to know what upstream wants to to submit something. As per Cunningham's Law, if you want the answer to something, don't ask, post the wrong answer.

  • Move the project and org definitions to PhabricatorPlatformSite methods
  • Update existing references to moved place
  • Fix the default wordmark so it's not immediately translated via pht()
This revision is now accepted and ready to land.Jun 24 2021, 03:38
This revision now requires review to proceed.Jun 24 2021, 03:38
speck planned changes to this revision.Jun 24 2021, 03:38

Still need to nuke the 500 Phabricators

Okay I was able to run lints properly but running unit tests still failed because I'm not running a proper dev environment yet. I'll definitely get this done before considering this change for landing/submission though.

In D25002#394, @speck wrote:

Still need to nuke the 500 Phabricators

Another way of looking at this is invalidating 500 * (6 supported languages) = 3000 translations

Screen Shot 2021-06-23 at 11.44.06 PM.png (408×440 px, 172 KB)

externals/pear-figlet/Text/Figlet.php
372–374 ↗(On Diff #35)

The raiseError() here doesn't use pht() so this uses string concatenation

I like the {Phabricator} idea! For technical compatibility with the rest of pht, it should probably be something like %PH.

Let's loop in Evan about this?

@speck What you have here seems reasonable to me. Can you propose this upstream and see how it goes?

@avivey has anyone been in contact with Evan that can reach out? I don’t really know of a way to reach him for this type of topic/project.

I didn't talk with him in a while. Maybe opening a ticket in secure.phabricator.com is the quickest way?

What can I do to Help? -

And, I think we have to essentially land our change, regardless of upstream - From the standpoint of : "We will become the upstream" - Maybe I am thinking wrong here, but we can propose the change and hope Phacility accepts it, but we have to do it anyway

@avivey - Good thinking on the token format!

I had a thought, that adding a %xx argument to pht without matching argument is probably not a really good idea, so I went exploring.

My next idea was to add a custom %xx format to pht, but require the argument, and require the type of the matching argument to be a specific class; This can be checked statically using ArcanistFormattedStringXHPASTLinterRule.

The bad news: pht() does not seem to go via xsprintf(), but rather calls native sprintf() directly after replacing some arguments, but it leaves the format string unchanged (or actually, replacing it with a translation, but not modifying it) ; so it doesn't support custom %xx arguments like the rest of the formatters

This means that in order to implement any solution, we'll need to change PhutilTranslator to allow it to modify the format string.

The second issue is that the linter - ArcanistFormattedStringXHPASTLinterRule - doesn't actually consider the arguments - effectively, it only makes sure that the number of arguments matches the number required by the format string.

There was a discussion in secure-dot about having pht return some proper object type (rather then a string), which I think relates to this; I can't find the discussion now.

Could we...

  1. Create a new Translation which specifically translates the token : e.g. %PHAB or {{Phabricator}} or {{PhabricatorWordmark}} or whatever
  1. Add this new translation into some property like $this->globalTranslations
  1. Always add this translation by merging it with the passed ones?? (not sure about this)
From PhutilTranslator
public function setTranslations(array $translations) {
    $this->translations = array_merge($translations,$this->globalTranslations);
    return $this;
  }

Also, just so we don't forget, if we have this global translation, we need a way to escape the token, so we have the option to not replace some escaped version of the token

Could we...

  1. Create a new Translation which specifically translates the token : e.g. %PHAB or {{Phabricator}} or {{PhabricatorWordmark}} or whatever

There are 3 separate elements going on in the i18n layer: PhutilLocale, PhutilTranslation, and the translation array.

  • A translation array is a tree, encoded in arrays, where it ends up as a static mapping from format-string + argument values (number and gender) to a new format string.
  • A Translation class has a local code ("en_US") and produces a translation array.
  • A Locale has a local code, information about navigating nodes in the translation array, and a didTranslateString function.

Of the 3, only PhutilLocale has code that can be executed each time pht() is executed (and can modify the result). This is used only once in the code, for the ALLCaps locale.

We can use didTranslateString to replace the token with the value, at a cost:

  • the performance cost of running another string replacement for each string, and
  • being post-process, it will also replace the token if it appears in values; so if we create a task titled replace %PHAB mentions with %s, it will catch that too in some contexts.
  • it won't exist in custom locales (do these exist?)

It might be worth it, if we consider it a temporary hack and start replacing the token for %s in the code with high-priority.

Oh, and it can't have % unless it uses %% - to escape the sprintf itself.

In D25002#410, @avivey wrote:

We can use didTranslateString to replace the token with the value, at a cost:

  • the performance cost of running another string replacement for each string, and
  • being post-process, it will also replace the token if it appears in values; so if we create a task titled replace %PHAB mentions with %s, it will catch that too in some contexts.
  • it won't exist in custom locales (do these exist?)

This is good information - The best solution is like you said

It might be worth it, if we consider it a temporary hack and start replacing the token for %s in the code with high-priority.

It's just a much bigger change - I am worried about all the code that will be updated and whether or not we can prove that each code change we made was executed with success and the expected result -

I also don't like the temporary hack of the token! - So, both of the potential solutions are troublesome...

For this diff I'm going to continue the current approach of swapping Phabricator with %s and adding an argument, with the understanding that this might not be the final approach we take.

In D25002#402, @avivey wrote:

I didn't talk with him in a while. Maybe opening a ticket in secure.phabricator.com is the quickest way?

That's probably a good idea, maybe just a general task with questions regarding creating a fork.

Searched for pht('Phabricator to find a few more instances to update.
Added a few notes to T15006 about non-translations which we will need to update.

Did a search for pht\('.*Phabricator.*' to find places where "Phabricator" appears on the same line as pht(.
Updated T15006 with additional notes about other things that will need updated.

As a note, when I'm searching for instances of "Phabricator" to swap out I review the entire file and not just the search result instances so all current changed files should be fully updated.

Attacking a few more instances while searching '.*\s+Phabricator\s+.*'

If you can break it to one diff with the new method/css stuff and one or two string changes, and several with only string change, we can just review it and land them one at a time.

I can get a script to split a commit into smaller commits, so if you split it to "new code" and "everything else", the other breakdown would be easy.

In D25002#451, @avivey wrote:

If you can break it to one diff with the new method/css stuff and one or two string changes, and several with only string change, we can just review it and land them one at a time.

I can get a script to split a commit into smaller commits, so if you split it to "new code" and "everything else", the other breakdown would be easy.

Yes please.

speck planned changes to this revision.Jun 26 2021, 12:45

There are still a hundred or so more locations that need updated

In D25002#451, @avivey wrote:

If you can break it to one diff with the new method/css stuff and one or two string changes, and several with only string change, we can just review it and land them one at a time.

I can get a script to split a commit into smaller commits, so if you split it to "new code" and "everything else", the other breakdown would be easy.

I'll try to look at doing that based on your script. It's something I regularly do with mercurial but I'm still fairly new to git.

src/applications/cache/spec/PhabricatorDataCacheSpec.php
93 ↗(On Diff #39)

Any idea what this lint is about? I'm pretty sure I ran arc lint --lintall at one point and this was not one of the things that had come up.

src/applications/cache/spec/PhabricatorDataCacheSpec.php
93 ↗(On Diff #39)

That's a false-positive - there's an explicit check for this function 2 lines before.

from arc-help, --lintall is "Show all lint warnings, not just those on changed lines"; lint --everything is "Lint all tracked files" - probably that's why it didn't show for you.

src/applications/cache/spec/PhabricatorDataCacheSpec.php
93 ↗(On Diff #39)

Ah that makes sense, thanks

@speck A possible path forward here - We will end up with new revisions, but that is good!

Break off the code that implements the new Name and Org methods

src/aphront/site/PhabricatorPlatformSite.php

In git, here is how this could be done assuming {existingBranch} contains all of the code in D25002 and {newBranch} is a new thing just for this

git checkout master
git checkout -b {newBranch}
git checkout {existingBranch} src/aphront/site/PhabricatorPlatformSite.php

git commit -a -m "your messsage"

arc diff

Then - once that change is approved by Blessed Committers land it, and then rebase {existingBranch} with git rebase master

Basically, then you can sort of repeat this process using something like @avivey script - OR, maybe one option could be Break these changes up by application

One revision could just contain all of the changes in src/applications/config/ - This would be a bit easier to review

I am here to help! - I think the first step is - Let's land the code that contains the methods to get the wordmark - Then we can all work on making and reviewing all of the updates

In case you're not cc'd on the task, refer to T15006#765

The upstream has provided some guidance/suggestions on how to move forward. This approach is fine, however in many cases we should consider removing "Phabricator" instead of trying to swap out with another name.

In my comment I indicate there are probably 2 options we can take at this point. I'd like to try a few of epriestley's suggestions and also attempt to submit changes to the upstream rather than landing here.

Looks good, hopefully this can be landed some time soon.