Page MenuHomePhabricator

Fix `pht` method calls
ClosedPublic

Authored by joshuaspence on Feb 4 2015, 9:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jan 31, 11:48 PM
Unknown Object (File)
Tue, Jan 24, 6:21 AM
Unknown Object (File)
Tue, Jan 24, 6:21 AM
Unknown Object (File)
Tue, Jan 24, 6:20 AM
Unknown Object (File)
Tue, Jan 24, 6:20 AM
Unknown Object (File)
Tue, Jan 17, 9:47 AM
Unknown Object (File)
Mon, Jan 16, 3:26 PM
Unknown Object (File)
Jan 2 2023, 3:52 AM
Subscribers

Details

Summary

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

Test Plan

arc lint

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
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.
src/applications/differential/event/DifferentialHovercardEventListener.php
63

I'm not sure about this one.

src/applications/differential/mail/DifferentialCreateMailReceiver.php
81–82

As above.

90

As above.

107

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

src/applications/differential/render/DifferentialChangesetHTMLRenderer.php
13

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.