Page MenuHomePhorge

Change some instances of "phabricator" to "phorge"
ClosedPublic

Authored by waldyrious on Nov 14 2023, 12:12.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 20, 00:35
Unknown Object (File)
Sun, May 19, 11:25
Unknown Object (File)
Sun, May 19, 11:14
Unknown Object (File)
Sun, May 19, 03:04
Unknown Object (File)
Fri, May 17, 13:13
Unknown Object (File)
Thu, May 16, 08:13
Unknown Object (File)
Sat, May 11, 17:54
Unknown Object (File)
Wed, May 8, 05:56

Details

Summary

Just a small set of replacements in locations that seem innocuous (user-facing messages, documentation, etc.)

Ref T15006

Test Plan

Nothing should change in terms of behavior. The places where these changes were made should now say "phorge".

Example tests:

  • Manage a single User and click on Delete User and see the popup
  • Run a test email and check the output ./bin/mail send-test --to username
  • Visit /maniphest/, shift+click on at least 1 Task, click on Bulk Edit Selected, Continue, see the popup
  • See the mentioned documentation with your big eyes. Eyes do not explode \o/

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

conf/aphlict/README
11

✅ Nice

scripts/init/lib.php
16

✅ Nice. Here we cannot use PlatformSymbols since Arcanist is not loaded yet.

src/applications/config/option/PhabricatorConfigOption.php
80

Maybe here we can adopt the PlatformSymbols thing?

src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php
184–186

Maybe here we can adopt the PlatformSymbols thing?

src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php
228–230

Maybe here we can adopt the PlatformSymbols thing?

src/applications/people/controller/PhabricatorPeopleDeleteController.php
29–35

Maybe here we can adopt the PlatformSymbols thing? Also, this is a good place for the <tt> thing

src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
33–37

Also here maybe nice to adopt PlatformSymbols, and also try the <tt> thing

src/infrastructure/events/PhabricatorAutoEventListener.php
13

✅ Nice

src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
33–37

Sure. For the record, the "tt thing" refers to D25466. We should keep in mind, though, that the CSS-based trick to make the prompt non-selectable won't work here, because the style in that revision is scoped to setup issue pages (the edited file was specifically setup-issue.css).

  • Use dynamically generated platform path where possible

We have just a minor issue but with a potentially working fix, and it's ready to land

src/applications/config/option/PhabricatorConfigOption.php
80

✅ Tested

src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php
184–186

✅ Untested but it's the same as PhabricatorMailManagementSendTestWorkflow and that works

src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php
228–230

✅ Tested running ./bin/mail send-test --to valerio.bozzolan

src/applications/people/controller/PhabricatorPeopleDeleteController.php
29–35

✅ Tested. This may deserve your lovely un-selectable thing in the future.

src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
36–37

I don't know why but we probably need to delete the cast to string there, and move that cast above. Or, this does not render correctly.

So this is the only blocking Test that is failing:

Visit /maniphest/, shift+click on at least 1 Task, click on Bulk Edit Selected, Continue, see the popup

Then, ready to land \o/

This revision now requires changes to proceed.Nov 15 2023, 13:23

So this is the only blocking Test that is failing:

Visit /maniphest/, shift+click on at least 1 Task, click on Bulk Edit Selected, Continue, see the popup

Oh, I see, you mean the <tt> is rendered verbatim in the page:

image.png (325×604 px, 33 KB)

Actually, in this case the entire string is already wrapped in a <tt>, so it's redundant to wrap the prompt as well. I tested changing the wrapping element to a <pre>, but that doesn't improve things because the nice display of pre elements is confined to the setup-issues.css stylesheet (the one modified in D25466).

I mean, technically we could do better by using newDialog()->appendCommand() instead of newDialog()->appendParagraph() — as is done in PhabricatorPeopleDeleteController.php, resulting in the following:

image.png (295×604 px, 34 KB)

...but that would require refactoring how the bulk job dialog is built; currently there's an array $parts constructed in PhabricatorEditEngineBulkJobType::getDescriptionForConfirm(), and each element of the array is passed to newDialog()->appendParagraph() in PhabricatorDaemonBulkJobMonitorController::handleRequest(). Besides being out of scope for the changes proposed in this revision, I also think that even PhabricatorPeopleDeleteController should be refactored too, to use an actual <pre> element for the commands, as is done for the setup issues pages edited in D25466, rather than a <p class="aphront-dialog-view-paragraph aphront-dialog-view-command">.

So tl;dr: as a minimal fix, I will for now just change the "phabricator" string to be the result of PlatformSymbols::getPlatformServerPath() instead, leaving the entire command wrapped in a single <tt> tag, and later we can think about refactoring these dialogs. The result is therefore this:

image.png (321×600 px, 32 KB)

  • Remove extra <tt> in bulk job dialog
src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
36–37

I have no idea why the cast to string was here indeed. I've removed it, and the result seems to be fine, but happy to put it back if we prefer a minimal patch just in case.

Thanks for the deep inspection! Tested and I agree

sgtm

src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
36–37

✅ I totally agree with you. Thanks. Tested and now it works as expected

This revision is now accepted and ready to land.Nov 18 2023, 16:41