Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T7046: Add a linter rule for detecting missing variables for `pht`
- Commits
- Restricted Diffusion Commit
rPaaf8d73ec798: Fix `pht` method calls
arc lint
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
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:
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. | |