When enabled, this will show the full history of review comments in an
email-compatible threading-view.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- Restricted Diffusion Commit
rPeb3ed9bbc994: Add an option to put comment context into emails.
Sending emails with the option on and off.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- comment-thread
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 2041 Build 2042: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
This is basically fine, technically. Some inlines, but only one (draft selection) is an actual correctness issue.
I'm pretty hesitant to upstream this because I think your install is the only install using the existing option and I haven't seen more interest in this, but I've had to port the existing stuff forward once already (in connection with T2222) and am likely to need to generalize it for Audit (in connection with T4896), and make it interact correctly with planned changes to inline behavior (T1460). Making the feature more complicated is increasing the upstream cost for a feature that only your install uses.
In T1460, we also plan to introduce explicit "done" and "reply" states to inline comments, but have a different definition of "reply".
If you:
- fix the correctness issue around drafts;
- remove the configuration option (so this behavior is always enabled if you have inlines enabled);
- are comfortable that we will change what "reply" means in the future when T1460 happens; and
- do what you reasonably can about the query stuff (the hard query should get easier after T1460, so it's not hugely important that it get corrected)
...I guess we can upstream this. The config option is the biggest issue, but I think that's not contentious to remove.
| src/applications/differential/config/PhabricatorDifferentialConfigOptions.php | ||
|---|---|---|
| 297–311 | Do we need this option? Do you think it's useful to enable one the other option but not this one? At a minimum, this should mention that the option has no effect unless inlines are already enabled. | |
| src/applications/differential/editor/DifferentialTransactionEditor.php | ||
| 1280 | Since this must be an array, prefer an array typehint. | |
| 1281 | By convention, avoid foreach-by-ref, or explicitly unset the reference variable after exiting the loop. This pattern (binding references and not unsetting them) tends to lead to subtle bugs where reuse of an iterator later in a function has surprising, nonlocal side effects. | |
| 1287 | Prefer class typehint. | |
| 1289–1292 | This incorrectly selects draft inlines (which have not yet been submitted, and should not be visible to other users) in addition to visible inlines. Add a transactionPHID IS NOT NULL clause to avoid selecting them. | |
| 1289–1292 | This suffers from the "N+1" query problem: https://secure.phabricator.com/book/phabcontrib/article/n_plus_one/ Specifically, if there are 100 inlines, we'll issue 100 queries. Although this probably isn't a huge problem since it's rare to have 100 inlines, it would be better to issue a single query. | |
| 1298–1299 | This is another N+1 query. Instead, authors should be loaded in a single query. For objects where a real Query class is available (most objects, but not DifferentialTransactionComment), prefer it. The appropriate Query class in this case is PhabricatorPeopleQuery. | |
| 1301 | Using getRealName() is unconventional: Phabricator identifies users with getUsername(). | |
Comment Actions
Regarding your effort to maintain this, a few thoughts:
- Thank you so much for doing it; no matter whether you take this in the end, I really appreciate your feedback and time; I understand the trade-off between features and maintenance headaches, so I fully appreciate if we have to keep our own fork.
- That said, keeping our own fork often feels very very frustrating; I know that's not your problem, but I think that 99.9% of the time I've spent countless hours in syncing and resolving merge conflicts would have cost no time if the person writing the feature (-> you ;) would have changed the code with the refactoring (I love the refactorings, just not the merging ...); that would save so much of my time that you dropping me an email whenever there is a problem with this code so I can resolve this would be immediately prioritized for me, based on that I would lose so much if I have to keep our own fork
- <<</me opens the bag and pulls out a carrot; the carrot looks somewhat translucent, so you can't really be sure whether it exists>>>: we would very much like not having to maintain our own instance, and we'd like to become paying customers (the countless hours I throw at maintaining our own GCE instance with this stuff are beyond reasonable); not sure whether you still aim at providing such a service though...
- I think the feature might not be used very much until now because it's not feature complete yet; I think the comments in the diff actually make the whole email diff thingy much more useful, and if we add parsing in-line replies and adding them to comment hunks as replies, that would make it even more useful (it would mean I could just do reviews completely by email if I have enough context); but of course I would say that, so probably not a very convincing argument on its own :P
| src/applications/differential/config/PhabricatorDifferentialConfigOptions.php | ||
|---|---|---|
| 297–311 | Done. | |
| src/applications/differential/editor/DifferentialTransactionEditor.php | ||
| 1280 | Done. | |
| 1281 | Yea, that doesn't make any sense. /me shudders. Sorry for having left that in ... | |
| 1287 | Done. | |
| 1289–1292 | Draft bug: Yikes. Done. | |
| 1289–1292 | N+1. Done, yea, that was bad. /me's first real LAMP project, so thanks for teaching :) | |
| 1298–1299 | And done. | |
| 1301 | Hm, I'd like to honor the user-address-format setting, but I don't think people care too much (hopefully). I'll go with getUserName and we'll see whether people scream at me. | |
Comment Actions
Cool, this is in pretty good shape. One query construction safety thing and a couple of minor things.
| src/applications/differential/editor/DifferentialTransactionEditor.php | ||
|---|---|---|
| 1304 | Slightly better as: pht('%s wrote:', $user->getUserName())...so it can be translated. | |
| 1335–1337 | This should use qsprintf() to prevent possible SQL injection: $table = new DifferentialTransactionComment();
$conn_r = $table->establishConnection('r');
$queries = array();
foreach ($stuff as $thing) {
$queries[] = qsprintf(
$conn_r,
'(changesetID = %d AND (lineNumber IN (%Ld))',
$id,
$line_numbers);
}Then, below, use: $table->loadAllWhere(
'transactionPHID IS NOT NULL AND (%Q)',
implode(' OR ', $queries)); | |
| 1347 | Slightly simpler as mpull($all_comments, 'getAuthorPHID'). | |
| src/applications/differential/editor/DifferentialTransactionEditor.php | ||
|---|---|---|
| 1304 | Done. | |
| 1335–1337 | Done, thx again for the teaching effort :) | |
| 1347 | So that would assume that 'withPHIDs' doesn't depend on the keys. I assume in php it's basically "unless otherwise stated, if you take a list of things you shouldn't depend on the keys in the array?". This seems a little fragile at a first glance, but probably just takes some getting used to... So, done :) | |
Comment Actions
On join/implode, they're totally equivalent, we just try to pick one and stick with it in this codebase. We should write a lint rule but it doesn't come up too often.