@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
Move all ApplicationTransaction publishing to daemons
Summary:
Ref T6367. Do all mail, feed, notification and search stuff from the daemons, in all editors.
There are four relatively-stateful editors (Audit, Differential, Phriction, PhortuneCart) which needed special care to move state into the daemons properly.
Beyond that, I moved mailTo/mailCC/feedRelated/feedNotify to be computed before we enter the worker:
- This is simpler, since a lot of editors rely on being able to call `$object->getReviewers()` or similar to compute them.
- This is more correct, since we want to freeze the lists at this moment in time.
Finally, I renamed `loadEdges` to `willPublish` and made it a slightly more general hook.
---
This is a bit fragile and I'm not //thrilled// about it.
It would probably be cleaner to have separate Editor and Publisher classes (something like @fabe's D11329 did). However, I think that's quite a lot of work, and I'd like to see stronger motivation for it (either in this actually being more fragile than I think, or there being other things we get out of it). Overall, I'm comfortable with this change, just definitely not a big fan of the "save" + "load" pattern since I think it's really fragile, nonobvious, hard to debug/predict, etc.
Test Plan:
Directly updated editors:
- Created a new Phriction page, saw "Document Content".
- Edited a Phriction page, saw "Document Diff".
- Edited a revision, got normal looking mail.
- Faked in `changedPriorToCommitURI` and verified it survived the state boundary.
- Sent Audit mail.
- Sent invoice mail.
Indirect editors - for these, I just made a change and made sure the mail generated:
- Updated a paste.
- Updated an event.
- Updated a thread.
- Updated a task.
- Updated a mock.
- Updated a question.
- Updated a project.
- Updated a file.
- Updated an initiative.
- Updated a Legalpad document.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, fabe
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13115
+174
-63
+20
-4
src/applications/audit/editor/PhabricatorAuditEditor.php
+20
-4
src/applications/audit/editor/PhabricatorAuditEditor.php
···
8
8
private $auditReasonMap = array();
9
9
private $affectedFiles;
10
10
private $rawPatch;
11
+
private $auditorPHIDs = array();
11
12
12
13
private $didExpandInlineState;
13
14
···
341
342
if ($import_status_flag) {
342
343
$object->writeImportStatusFlag($import_status_flag);
343
344
}
345
+
346
+
// Collect auditor PHIDs for building mail.
347
+
$this->auditorPHIDs = mpull($object->getAudits(), 'getAuditorPHID');
344
348
345
349
return $xactions;
346
350
}
···
689
693
$user_phids[$committer_phid][] = pht('Committer');
690
694
}
691
695
692
-
// we loaded this in applyFinalEffects
693
-
$audit_requests = $object->getAudits();
694
-
$auditor_phids = mpull($audit_requests, 'getAuditorPHID');
695
-
foreach ($auditor_phids as $auditor_phid) {
696
+
foreach ($this->auditorPHIDs as $auditor_phid) {
696
697
$user_phids[$auditor_phid][] = pht('Auditor');
697
698
}
698
699
···
981
982
PhabricatorLiskDAO $object,
982
983
array $xactions) {
983
984
return $this->shouldPublishRepositoryActivity($object, $xactions);
985
+
}
986
+
987
+
protected function getCustomWorkerState() {
988
+
return array(
989
+
'rawPatch' => $this->rawPatch,
990
+
'affectedFiles' => $this->affectedFiles,
991
+
'auditorPHIDs' => $this->auditorPHIDs,
992
+
);
993
+
}
994
+
995
+
protected function loadCustomWorkerState(array $state) {
996
+
$this->rawPatch = idx($state, 'rawPatch');
997
+
$this->affectedFiles = idx($state, 'affectedFiles');
998
+
$this->auditorPHIDs = idx($state, 'auditorPHIDs');
999
+
return $this;
984
1000
}
985
1001
986
1002
}
+11
src/applications/differential/editor/DifferentialTransactionEditor.php
+11
src/applications/differential/editor/DifferentialTransactionEditor.php
···
1902
1902
return $section;
1903
1903
}
1904
1904
1905
+
protected function getCustomWorkerState() {
1906
+
return array(
1907
+
'changedPriorToCommitURI' => $this->changedPriorToCommitURI,
1908
+
);
1909
+
}
1910
+
1911
+
protected function loadCustomWorkerState(array $state) {
1912
+
$this->changedPriorToCommitURI = idx($state, 'changedPriorToCommitURI');
1913
+
return $this;
1914
+
}
1915
+
1905
1916
}
-4
src/applications/paste/editor/PhabricatorPasteEditor.php
-4
src/applications/paste/editor/PhabricatorPasteEditor.php
+21
-1
src/applications/phortune/editor/PhortuneCartEditor.php
+21
-1
src/applications/phortune/editor/PhortuneCartEditor.php
···
189
189
protected function getMailTo(PhabricatorLiskDAO $object) {
190
190
$phids = array();
191
191
192
-
// Relaod the cart to pull merchant and account information, in case we
192
+
// Reload the cart to pull merchant and account information, in case we
193
193
// just created the object.
194
194
$cart = id(new PhortuneCartQuery())
195
195
->setViewer($this->requireActor())
···
218
218
protected function buildReplyHandler(PhabricatorLiskDAO $object) {
219
219
return id(new PhortuneCartReplyHandler())
220
220
->setMailReceiver($object);
221
+
}
222
+
223
+
protected function willPublish(PhabricatorLiskDAO $object, array $xactions) {
224
+
// We need the purchases in order to build mail.
225
+
return id(new PhortuneCartQuery())
226
+
->setViewer($this->getActor())
227
+
->withIDs(array($object->getID()))
228
+
->needPurchases(true)
229
+
->executeOne();
230
+
}
231
+
232
+
protected function getCustomWorkerState() {
233
+
return array(
234
+
'invoiceIssues' => $this->invoiceIssues,
235
+
);
236
+
}
237
+
238
+
protected function loadCustomWorkerState(array $state) {
239
+
$this->invoiceIssues = idx($state, 'invoiceIssues');
240
+
return $this;
221
241
}
222
242
223
243
}
+30
-18
src/applications/phriction/editor/PhrictionTransactionEditor.php
+30
-18
src/applications/phriction/editor/PhrictionTransactionEditor.php
···
13
13
private $skipAncestorCheck;
14
14
private $contentVersion;
15
15
private $processContentVersionError = true;
16
+
private $contentDiffURI;
16
17
17
18
public function setDescription($description) {
18
19
$this->description = $description;
···
348
349
->applyTransactions($this->moveAwayDocument, $move_away_xactions);
349
350
}
350
351
352
+
// Compute the content diff URI for the publishing phase.
353
+
foreach ($xactions as $xaction) {
354
+
switch ($xaction->getTransactionType()) {
355
+
case PhrictionTransaction::TYPE_CONTENT:
356
+
$uri = id(new PhutilURI('/phriction/diff/'.$object->getID().'/'))
357
+
->alter('l', $this->getOldContent()->getVersion())
358
+
->alter('r', $this->getNewContent()->getVersion());
359
+
$this->contentDiffURI = (string)$uri;
360
+
break 2;
361
+
default:
362
+
break;
363
+
}
364
+
}
365
+
351
366
return $xactions;
352
367
}
353
368
···
403
418
$body->addTextSection(
404
419
pht('DOCUMENT CONTENT'),
405
420
$object->getContent()->getContent());
406
-
} else {
407
-
408
-
foreach ($xactions as $xaction) {
409
-
switch ($xaction->getTransactionType()) {
410
-
case PhrictionTransaction::TYPE_CONTENT:
411
-
$diff_uri = id(new PhutilURI(
412
-
'/phriction/diff/'.$object->getID().'/'))
413
-
->alter('l', $this->getOldContent()->getVersion())
414
-
->alter('r', $this->getNewContent()->getVersion());
415
-
$body->addLinkSection(
416
-
pht('DOCUMENT DIFF'),
417
-
PhabricatorEnv::getProductionURI($diff_uri));
418
-
break 2;
419
-
default:
420
-
break;
421
-
}
422
-
}
423
-
421
+
} else if ($this->contentDiffURI) {
422
+
$body->addLinkSection(
423
+
pht('DOCUMENT DIFF'),
424
+
PhabricatorEnv::getProductionURI($this->contentDiffURI));
424
425
}
425
426
426
427
$body->addLinkSection(
···
808
809
$new_content->setVersion($this->getOldContent()->getVersion() + 1);
809
810
810
811
return $new_content;
812
+
}
813
+
814
+
protected function getCustomWorkerState() {
815
+
return array(
816
+
'contentDiffURI' => $this->contentDiffURI,
817
+
);
818
+
}
819
+
820
+
protected function loadCustomWorkerState(array $state) {
821
+
$this->contentDiffURI = idx($state, 'contentDiffURI');
822
+
return $this;
811
823
}
812
824
813
825
}
+3
-4
src/applications/project/editor/PhabricatorProjectTransactionEditor.php
+3
-4
src/applications/project/editor/PhabricatorProjectTransactionEditor.php
···
397
397
return parent::requireCapabilities($object, $xaction);
398
398
}
399
399
400
-
protected function loadEdges(
401
-
PhabricatorLiskDAO $object,
402
-
array $xactions) {
403
-
400
+
protected function willPublish(PhabricatorLiskDAO $object, array $xactions) {
404
401
$member_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
405
402
$object->getPHID(),
406
403
PhabricatorProjectProjectHasMemberEdgeType::EDGECONST);
407
404
$object->attachMemberPHIDs($member_phids);
405
+
406
+
return $object;
408
407
}
409
408
410
409
protected function shouldSendMail(
+89
-32
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+89
-32
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
···
1
1
<?php
2
2
3
3
/**
4
+
*
5
+
* Publishing and Managing State
6
+
* ======
7
+
*
8
+
* After applying changes, the Editor queues a worker to publish mail, feed,
9
+
* and notifications, and to perform other background work like updating search
10
+
* indexes. This allows it to do this work without impacting performance for
11
+
* users.
12
+
*
13
+
* When work is moved to the daemons, the Editor state is serialized by
14
+
* @{method:getWorkerState}, then reloaded in a daemon process by
15
+
* @{method:loadWorkerState}. **This is fragile.**
16
+
*
17
+
* State is not persisted into the daemons by default, because we can not send
18
+
* arbitrary objects into the queue. This means the default behavior of any
19
+
* state properties is to reset to their defaults without warning prior to
20
+
* publishing.
21
+
*
22
+
* The easiest way to avoid this is to keep Editors stateless: the overwhelming
23
+
* majority of Editors can be written statelessly. If you need to maintain
24
+
* state, you can either:
25
+
*
26
+
* - not require state to exist during publishing; or
27
+
* - pass state to the daemons by implementing @{method:getCustomWorkerState}
28
+
* and @{method:loadCustomWorkerState}.
29
+
*
30
+
* This architecture isn't ideal, and we may eventually split this class into
31
+
* "Editor" and "Publisher" parts to make it more robust. See T6367 for some
32
+
* discussion and context.
33
+
*
4
34
* @task mail Sending Mail
5
35
* @task feed Publishing Feed Stories
6
36
* @task search Search Index
···
34
64
private $heraldEmailPHIDs = array();
35
65
private $heraldForcedEmailPHIDs = array();
36
66
private $heraldHeader;
67
+
private $mailToPHIDs = array();
68
+
private $mailCCPHIDs = array();
69
+
private $feedNotifyPHIDs = array();
70
+
private $feedRelatedPHIDs = array();
37
71
38
72
/**
39
73
* Get the class name for the application this editor is a part of.
···
907
941
}
908
942
$this->heraldHeader = $herald_header;
909
943
910
-
if ($this->supportsWorkers()) {
911
-
PhabricatorWorker::scheduleTask(
912
-
'PhabricatorApplicationTransactionPublishWorker',
913
-
array(
914
-
'objectPHID' => $object->getPHID(),
915
-
'actorPHID' => $this->getActingAsPHID(),
916
-
'xactionPHIDs' => mpull($xactions, 'getPHID'),
917
-
'state' => $this->getWorkerState(),
918
-
),
919
-
array(
920
-
'objectPHID' => $object->getPHID(),
921
-
'priority' => PhabricatorWorker::PRIORITY_ALERTS,
922
-
));
923
-
} else {
924
-
$this->publishTransactions($object, $xactions);
944
+
// We're going to compute some of the data we'll use to publish these
945
+
// transactions here, before queueing a worker.
946
+
//
947
+
// Primarily, this is more correct: we want to publish the object as it
948
+
// exists right now. The worker may not execute for some time, and we want
949
+
// to use the current To/CC list, not respect any changes which may occur
950
+
// between now and when the worker executes.
951
+
//
952
+
// As a secondary benefit, this tends to reduce the amount of state that
953
+
// Editors need to pass into workers.
954
+
$object = $this->willPublish($object, $xactions);
955
+
956
+
if (!$this->getDisableEmail()) {
957
+
if ($this->shouldSendMail($object, $xactions)) {
958
+
$this->mailToPHIDs = $this->getMailTo($object);
959
+
$this->mailCCPHIDs = $this->getMailCC($object);
960
+
}
961
+
}
962
+
963
+
if ($this->shouldPublishFeedStory($object, $xactions)) {
964
+
$this->feedRelatedPHIDs = $this->getFeedRelatedPHIDs($object, $xactions);
965
+
$this->feedNotifyPHIDs = $this->getFeedNotifyPHIDs($object, $xactions);
925
966
}
926
967
968
+
PhabricatorWorker::scheduleTask(
969
+
'PhabricatorApplicationTransactionPublishWorker',
970
+
array(
971
+
'objectPHID' => $object->getPHID(),
972
+
'actorPHID' => $this->getActingAsPHID(),
973
+
'xactionPHIDs' => mpull($xactions, 'getPHID'),
974
+
'state' => $this->getWorkerState(),
975
+
),
976
+
array(
977
+
'objectPHID' => $object->getPHID(),
978
+
'priority' => PhabricatorWorker::PRIORITY_ALERTS,
979
+
));
980
+
927
981
return $xactions;
928
982
}
929
983
···
936
990
// in various transaction phases).
937
991
$this->loadSubscribers($object);
938
992
// Hook for other edges that may need (re-)loading
939
-
$this->loadEdges($object, $xactions);
993
+
$object = $this->willPublish($object, $xactions);
940
994
941
995
$this->loadHandles($xactions);
942
996
···
1040
1094
} else {
1041
1095
$this->subscribers = array();
1042
1096
}
1043
-
}
1044
-
1045
-
protected function loadEdges(
1046
-
PhabricatorLiskDAO $object,
1047
-
array $xactions) {
1048
-
return;
1049
1097
}
1050
1098
1051
1099
private function validateEditParameters(
···
2084
2132
}
2085
2133
2086
2134
$email_force = array();
2087
-
$email_to = $this->getMailTo($object);
2088
-
$email_cc = $this->getMailCC($object);
2135
+
$email_to = $this->mailToPHIDs;
2136
+
$email_cc = $this->mailCCPHIDs;
2089
2137
2090
2138
$email_cc = array_merge($email_cc, $this->heraldEmailPHIDs);
2091
2139
$email_force = $this->heraldForcedEmailPHIDs;
···
2511
2559
return;
2512
2560
}
2513
2561
2514
-
$related_phids = $this->getFeedRelatedPHIDs($object, $xactions);
2515
-
$subscribed_phids = $this->getFeedNotifyPHIDs($object, $xactions);
2562
+
$related_phids = $this->feedRelatedPHIDs;
2563
+
$subscribed_phids = $this->feedNotifyPHIDs;
2516
2564
2517
2565
$story_type = $this->getFeedStoryType();
2518
2566
$story_data = $this->getFeedStoryData($object, $xactions);
···
2800
2848
2801
2849
2802
2850
/**
2851
+
* Load any object state which is required to publish transactions.
2852
+
*
2853
+
* This hook is invoked in the main process before we compute data related
2854
+
* to publishing transactions (like email "To" and "CC" lists), and again in
2855
+
* the worker before publishing occurs.
2856
+
*
2857
+
* @return object Publishable object.
2803
2858
* @task workers
2804
2859
*/
2805
-
protected function supportsWorkers() {
2806
-
// TODO: Remove this method once everything supports workers.
2807
-
return false;
2860
+
protected function willPublish(PhabricatorLiskDAO $object, array $xactions) {
2861
+
return $object;
2808
2862
}
2809
2863
2810
2864
···
2825
2879
2826
2880
$state += array(
2827
2881
'excludeMailRecipientPHIDs' => $this->getExcludeMailRecipientPHIDs(),
2882
+
'custom' => $this->getCustomWorkerState(),
2828
2883
);
2829
2884
2830
-
return $state + array(
2831
-
'custom' => $this->getCustomWorkerState(),
2832
-
);
2885
+
return $state;
2833
2886
}
2834
2887
2835
2888
···
2900
2953
'heraldEmailPHIDs',
2901
2954
'heraldForcedEmailPHIDs',
2902
2955
'heraldHeader',
2956
+
'mailToPHIDs',
2957
+
'mailCCPHIDs',
2958
+
'feedNotifyPHIDs',
2959
+
'feedRelatedPHIDs',
2903
2960
);
2904
2961
}
2905
2962