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.

In D25002#160, @deadalnix wrote:

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.

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

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

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

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

2. Add this new translation into some property like $this->globalTranslations

3. Always add this translation by merging it with the passed ones?? (not sure about this)

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

In D25002#406, @Ekubischta wrote:

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.

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.

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