Page MenuHomePhabricator

Fix `pht` method calls

Authored by joshuaspence on Feb 4 2015, 9:15 PM.
Referenced Files
F13346422: D11680.diff
Fri, Jun 21, 3:24 PM
Mon, Jun 17, 11:17 AM
F13330676: D11680.diff
Mon, Jun 17, 4:22 AM
F13317813: D11680.diff
Thu, Jun 13, 9:55 AM
F13304578: D11680.id28250.diff
Sat, Jun 8, 10:32 AM
F13302210: D11680.diff
Sat, Jun 8, 4:46 AM
F13268413: D11680.diff
Wed, May 29, 5:00 AM
F13266291: D11680.diff
Tue, May 28, 12:09 PM



Ref T7046. This is mainly a proof-of-concept for D11661.

Test Plan

arc lint

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

joshuaspence retitled this revision from to Fix argument count for `pht` method calls.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence retitled this revision from Fix argument count for `pht` method calls to Fix `pht` method calls.Feb 6 2015, 7:17 AM
joshuaspence edited edge metadata.
joshuaspence updated this object.

I'm not sure about this one.


As above.


As above.


As above.

epriestley edited edge metadata.

These are great.

On the %d stuff, your changes are correct (per D11683), although some of those strings may already have translations that need to be adjusted.

It's also probably slightly better to use %s instead of %d for some of them. Currently:

  • Use %s for numbers if large numbers should be written with locale formatting (e.g., "1,234", with a comma in English, like "You have 1,234 errors.").
  • Use %d for numbers if large numbers should be written without locale formatting (e.g., "1234", without a comma in English, like "Task ID 1234").

In these cases, it is unlikely any translation would choose to put the number into the translated string, so it doesn't really matter. Translations can also change the parameter type if they want (although I think only from %d -> %s, not %s -> %d?), so even getting this wrong won't prevent things from being translated properly.

This %s vs %d rule isn't very intuitive, but the cost of getting it wrong is also close to zero and I don't see a clear way to make it more natural for the right thing to be the easiest thing.

(There's maybe an argument for always using %s, and requiring PhutilNumber, and then doing PhutilNumber->setFormatAsID(...) or something, but that would be really really cumbersome and not really give us much ground in terms of tradeoffs, I think.)


This garbage is forcing pht() to return HTML. Without it, the <strong> tags in pht() will be interpreted as text and escaped (I think? We don't do this anymore so I'm actually not 100% sure of the rule).

I think we should generally move toward preventing this; the number of cases where we use this technique is very, very small and it's virtually nonexistent in the modern codebase. Nearly all modern code which does this is putting stuff like tt around a command, and we now do %s ... phutil_tag('tt', array(), ...), which is slightly more cumbersome but far better from a translatability/security/consistency/simplicity/explicitness point of view.

I'd suggest this fix:

  • Keep your changes.
  • Remove all the <strong> tags from the pht() strings.

As written, I would expect this to now render the <strong> tags so they appear as text in the UI when presented to the viewer.

This revision is now accepted and ready to land.Feb 9 2015, 2:36 PM
joshuaspence edited edge metadata.

Remove <strong> nonsense

Use PhutilNumber where appropriate

This revision was automatically updated to reflect the committed changes.