Details
- Reviewers
joshuaspence btrahan chad - Maniphest Tasks
- T4896: Move Audit to ApplicationTransactions
- Commits
- Restricted Diffusion Commit
rP2082eda67bfe: Convert Audit comment rendering to standard infrastructure
Viewed, previewed and edited various types of comments in Diffusion.
Diff Detail
- Repository
- rP Phabricator
- Branch
- audit10
- Lint
Lint Passed Severity Location Code Message Advice src/applications/audit/view/PhabricatorAuditTransactionView.php:17 XHP16 TODO Comment Advice src/applications/audit/view/PhabricatorAuditTransactionView.php:81 XHP16 TODO Comment Advice src/applications/audit/view/PhabricatorAuditTransactionView.php:88 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 1902 Build 1903: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
| src/applications/transactions/storage/PhabricatorApplicationTransaction.php | ||
|---|---|---|
| 267 | This makes the error a little easier to understand/debug. | |
| src/applications/transactions/view/PhabricatorApplicationTransactionView.php | ||
| 162–163 | These parameters are no longer used by the behavior. This fixes an issue where the behavior could be initialized twice. | |
| src/view/phui/PHUITimelineEventView.php | ||
| 511 | This allows "Edit" to be shown even if "Quote" isn't hooked up. | |
| src/applications/audit/storage/PhabricatorAuditTransaction.php | ||
|---|---|---|
| 46 | I find this blank line to be inconsistent | |
| 106 | Is "added subscribers" correct here? | |
| 129 | As above | |
| src/applications/audit/view/PhabricatorAuditTransactionView.php | ||
| 29 | What if $v->getDateCreated() < $u->getDateCreated()? Should you wrap in abs()? | |
| 34–45 | I'm guessing that there is some implied meaning of $u and $v? | |
| 75 | I am never sure whether exception strings should use pht... | |
| src/applications/diffusion/controller/DiffusionCommitController.php | ||
| 103 | Did you mean to remove this? | |
| webroot/rsrc/css/application/diffusion/commit-view.css | ||
| 1 | Oh I see, you deleted the CSS | |
| src/applications/audit/storage/PhabricatorAuditTransaction.php | ||
|---|---|---|
| 106 | This happens in two cases:
In the former it's not terrible; in the latter case it's good. This was written with consideration for the preview case, although I'm not 100% sure it's actually reachable right now. All this stuff will be nuked once I move to real subscribers, I'm just trying to avoid the most severe UI regressions (completely meaningless strings and bad inline rendering). For example, these strings "should" have %d's in them and do "subscriber(s)" and such, but should be short-lived, and the Subscribers strings are already correct. | |
| src/applications/audit/view/PhabricatorAuditTransactionView.php | ||
| 34–45 | This method is a bit magical, but $u and $v are always ordered. This method is more like a fold than a sort, in that $u is the "group" transaction and $v is a single transaction which we might want to add to the group. This is hard to read/understand, and there are three nearly-identical copies of it (in Pholio, Differential and now in Audit). I plan to move them up after this is over, and I can make sure the new version of this is readable. | |
| 75 | The modern policy on this is just "yes, always use pht()". This one I copy/pasted from Differential. I'll either fix or aggregate this when I deal with the sorting/grouping above. | |
For a while I was doing some kind organic rule which was roughly "pht() if users might see it, no-pht() if it's purely internal/developer-facing" but I think that rule is pretty ambiguous and hard to make judgement calls on.
There's no real cost to "unnecessary" pht(), so I think a better rule is: always use it (maybe unless there's some technical/bootstrapping reason why using it would fatal/loop the program, e.g. you're throwing from within pht() itself).