@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Reduce surface area of DifferentialComment API
Summary:
Ref T2222. Shrink the API to make it easier to move this object's storage to ApplicationTransactions.
Fixes T3415. This moves the "Summary" and "Test Plan" into the property list, and thereby fixes all the attribution problems associated with commandeering, creating a revision from another user's diff, etc.
Test Plan: Browsed several revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3415, T2222
Differential Revision: https://secure.phabricator.com/D7375
+56
-75
+2
-51
src/applications/differential/controller/DifferentialRevisionViewController.php
+2
-51
src/applications/differential/controller/DifferentialRevisionViewController.php
···
91
91
$aux_fields = $this->loadAuxiliaryFields($revision);
92
92
93
93
$comments = $revision->loadComments();
94
-
$comments = array_merge(
95
-
$this->getImplicitComments($revision, reset($diffs)),
96
-
$comments);
97
94
98
95
$all_changesets = $changesets;
99
96
$inlines = $this->loadInlineComments(
···
111
108
mpull($comments, 'getAuthorPHID'));
112
109
113
110
foreach ($comments as $comment) {
114
-
$metadata = $comment->getMetadata();
115
-
$added_reviewers = idx(
116
-
$metadata,
117
-
DifferentialComment::METADATA_ADDED_REVIEWERS);
118
-
if ($added_reviewers) {
119
-
foreach ($added_reviewers as $phid) {
120
-
$object_phids[] = $phid;
121
-
}
122
-
}
123
-
$added_ccs = idx(
124
-
$metadata,
125
-
DifferentialComment::METADATA_ADDED_CCS);
126
-
if ($added_ccs) {
127
-
foreach ($added_ccs as $phid) {
128
-
$object_phids[] = $phid;
129
-
}
111
+
foreach ($comment->getRequiredHandlePHIDs() as $phid) {
112
+
$object_phids[] = $phid;
130
113
}
131
114
}
132
115
···
477
460
'title' => $object_id.' '.$revision->getTitle(),
478
461
'pageObjects' => array($revision->getPHID()),
479
462
));
480
-
}
481
-
482
-
private function getImplicitComments(
483
-
DifferentialRevision $revision,
484
-
DifferentialDiff $diff) {
485
-
486
-
$author_phid = nonempty(
487
-
$diff->getAuthorPHID(),
488
-
$revision->getAuthorPHID());
489
-
490
-
$template = new DifferentialComment();
491
-
$template->setAuthorPHID($author_phid);
492
-
$template->setRevisionID($revision->getID());
493
-
$template->setDateCreated($revision->getDateCreated());
494
-
495
-
$comments = array();
496
-
497
-
if (strlen($revision->getSummary())) {
498
-
$summary_comment = clone $template;
499
-
$summary_comment->setContent($revision->getSummary());
500
-
$summary_comment->setAction(DifferentialAction::ACTION_SUMMARIZE);
501
-
$comments[] = $summary_comment;
502
-
}
503
-
504
-
if (strlen($revision->getTestPlan())) {
505
-
$testplan_comment = clone $template;
506
-
$testplan_comment->setContent($revision->getTestPlan());
507
-
$testplan_comment->setAction(DifferentialAction::ACTION_TESTPLAN);
508
-
$comments[] = $testplan_comment;
509
-
}
510
-
511
-
return $comments;
512
463
}
513
464
514
465
private function getRevisionActions(DifferentialRevision $revision) {
+1
-1
src/applications/differential/editor/DifferentialCommentEditor.php
+1
-1
src/applications/differential/editor/DifferentialCommentEditor.php
+1
-2
src/applications/differential/editor/DifferentialRevisionEditor.php
+1
-2
src/applications/differential/editor/DifferentialRevisionEditor.php
···
783
783
784
784
785
785
private function createComment() {
786
-
$revision_id = $this->revision->getID();
787
786
$comment = id(new DifferentialComment())
788
787
->setAuthorPHID($this->getActorPHID())
789
-
->setRevisionID($revision_id)
788
+
->setRevision($this->revision)
790
789
->setContent($this->getComments())
791
790
->setAction(DifferentialAction::ACTION_UPDATE)
792
791
->setMetadata(
+30
-21
src/applications/differential/storage/DifferentialComment.php
+30
-21
src/applications/differential/storage/DifferentialComment.php
···
20
20
21
21
private $arbitraryDiffForFacebook;
22
22
23
+
public function setRevision(DifferentialRevision $revision) {
24
+
return $this->setRevisionID($revision->getID());
25
+
}
26
+
23
27
public function giveFacebookSomeArbitraryDiff(DifferentialDiff $diff) {
24
28
$this->arbitraryDiffForFacebook = $diff;
25
29
return $this;
26
30
}
27
31
32
+
public function getRequiredHandlePHIDs() {
33
+
$phids = array();
34
+
35
+
$metadata = $this->getMetadata();
36
+
$added_reviewers = idx(
37
+
$metadata,
38
+
self::METADATA_ADDED_REVIEWERS);
39
+
if ($added_reviewers) {
40
+
foreach ($added_reviewers as $phid) {
41
+
$phids[] = $phid;
42
+
}
43
+
}
44
+
$added_ccs = idx(
45
+
$metadata,
46
+
self::METADATA_ADDED_CCS);
47
+
if ($added_ccs) {
48
+
foreach ($added_ccs as $phid) {
49
+
$phids[] = $phid;
50
+
}
51
+
}
52
+
53
+
return $phids;
54
+
}
55
+
28
56
public function getConfiguration() {
29
57
return array(
30
58
self::CONFIG_SERIALIZATION => array(
···
42
70
return PhabricatorContentSource::newFromSerialized($this->contentSource);
43
71
}
44
72
45
-
46
73
public function getMarkupFieldKey($field) {
47
-
if ($this->getID()) {
48
-
return 'DC:'.$this->getID();
49
-
}
50
-
51
-
// The summary and test plan render as comments, but do not have IDs.
52
-
// They are also mutable. Build keys using content hashes.
53
-
$hash = PhabricatorHash::digest($this->getMarkupText($field));
54
-
return 'DC:'.$hash;
74
+
return 'DC:'.$this->getID();
55
75
}
56
76
57
77
public function newMarkupEngine($field) {
···
70
90
}
71
91
72
92
public function shouldUseMarkupCache($field) {
73
-
if ($this->getID()) {
74
-
return true;
75
-
}
76
-
77
-
$action = $this->getAction();
78
-
switch ($action) {
79
-
case DifferentialAction::ACTION_SUMMARIZE:
80
-
case DifferentialAction::ACTION_TESTPLAN:
81
-
return true;
82
-
}
83
-
84
-
return false;
93
+
return (bool)$this->getID();
85
94
}
86
95
87
96
}
+22
src/applications/differential/view/DifferentialRevisionDetailView.php
+22
src/applications/differential/view/DifferentialRevisionDetailView.php
···
112
112
$properties->setHasKeyboardShortcuts(true);
113
113
$properties->setActionList($actions);
114
114
115
+
$properties->invokeWillRenderEvent();
116
+
117
+
if (strlen($revision->getSummary())) {
118
+
$properties->addSectionHeader(pht('Summary'));
119
+
$properties->addTextContent(
120
+
PhabricatorMarkupEngine::renderOneObject(
121
+
id(new PhabricatorMarkupOneOff())->setContent(
122
+
$revision->getSummary()),
123
+
'default',
124
+
$user));
125
+
}
126
+
127
+
if (strlen($revision->getTestPlan())) {
128
+
$properties->addSectionHeader(pht('Test Plan'));
129
+
$properties->addTextContent(
130
+
PhabricatorMarkupEngine::renderOneObject(
131
+
id(new PhabricatorMarkupOneOff())->setContent(
132
+
$revision->getTestPlan()),
133
+
'default',
134
+
$user));
135
+
}
136
+
115
137
$object_box = id(new PHUIObjectBoxView())
116
138
->setHeader($header)
117
139
->addPropertyList($properties);