Page MenuHomePhabricator
Files F14021567

D13115.diff
No OneTemporary
Actions

D13115.diff
View Options

diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -8,6 +8,7 @@
private $auditReasonMap = array();
private $affectedFiles;
private $rawPatch;
+ private $auditorPHIDs = array();
private $didExpandInlineState;
@@ -342,6 +343,9 @@
$object->writeImportStatusFlag($import_status_flag);
}
+ // Collect auditor PHIDs for building mail.
+ $this->auditorPHIDs = mpull($object->getAudits(), 'getAuditorPHID');
+
return $xactions;
}
@@ -689,10 +693,7 @@
$user_phids[$committer_phid][] = pht('Committer');
}
- // we loaded this in applyFinalEffects
- $audit_requests = $object->getAudits();
- $auditor_phids = mpull($audit_requests, 'getAuditorPHID');
- foreach ($auditor_phids as $auditor_phid) {
+ foreach ($this->auditorPHIDs as $auditor_phid) {
$user_phids[$auditor_phid][] = pht('Auditor');
}
@@ -983,4 +984,19 @@
return $this->shouldPublishRepositoryActivity($object, $xactions);
}
+ protected function getCustomWorkerState() {
+ return array(
+ 'rawPatch' => $this->rawPatch,
+ 'affectedFiles' => $this->affectedFiles,
+ 'auditorPHIDs' => $this->auditorPHIDs,
+ );
+ }
+
+ protected function loadCustomWorkerState(array $state) {
+ $this->rawPatch = idx($state, 'rawPatch');
+ $this->affectedFiles = idx($state, 'affectedFiles');
+ $this->auditorPHIDs = idx($state, 'auditorPHIDs');
+ return $this;
+ }
+
}
diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -1902,4 +1902,15 @@
return $section;
}
+ protected function getCustomWorkerState() {
+ return array(
+ 'changedPriorToCommitURI' => $this->changedPriorToCommitURI,
+ );
+ }
+
+ protected function loadCustomWorkerState(array $state) {
+ $this->changedPriorToCommitURI = idx($state, 'changedPriorToCommitURI');
+ return $this;
+ }
+
}
diff --git a/src/applications/paste/editor/PhabricatorPasteEditor.php b/src/applications/paste/editor/PhabricatorPasteEditor.php
--- a/src/applications/paste/editor/PhabricatorPasteEditor.php
+++ b/src/applications/paste/editor/PhabricatorPasteEditor.php
@@ -184,8 +184,4 @@
return false;
}
- protected function supportsWorkers() {
- return true;
- }
-
}
diff --git a/src/applications/phortune/editor/PhortuneCartEditor.php b/src/applications/phortune/editor/PhortuneCartEditor.php
--- a/src/applications/phortune/editor/PhortuneCartEditor.php
+++ b/src/applications/phortune/editor/PhortuneCartEditor.php
@@ -189,7 +189,7 @@
protected function getMailTo(PhabricatorLiskDAO $object) {
$phids = array();
- // Relaod the cart to pull merchant and account information, in case we
+ // Reload the cart to pull merchant and account information, in case we
// just created the object.
$cart = id(new PhortuneCartQuery())
->setViewer($this->requireActor())
@@ -220,4 +220,24 @@
->setMailReceiver($object);
}
+ protected function willPublish(PhabricatorLiskDAO $object, array $xactions) {
+ // We need the purchases in order to build mail.
+ return id(new PhortuneCartQuery())
+ ->setViewer($this->getActor())
+ ->withIDs(array($object->getID()))
+ ->needPurchases(true)
+ ->executeOne();
+ }
+
+ protected function getCustomWorkerState() {
+ return array(
+ 'invoiceIssues' => $this->invoiceIssues,
+ );
+ }
+
+ protected function loadCustomWorkerState(array $state) {
+ $this->invoiceIssues = idx($state, 'invoiceIssues');
+ return $this;
+ }
+
}
diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php
--- a/src/applications/phriction/editor/PhrictionTransactionEditor.php
+++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php
@@ -13,6 +13,7 @@
private $skipAncestorCheck;
private $contentVersion;
private $processContentVersionError = true;
+ private $contentDiffURI;
public function setDescription($description) {
$this->description = $description;
@@ -348,6 +349,20 @@
->applyTransactions($this->moveAwayDocument, $move_away_xactions);
}
+ // Compute the content diff URI for the publishing phase.
+ foreach ($xactions as $xaction) {
+ switch ($xaction->getTransactionType()) {
+ case PhrictionTransaction::TYPE_CONTENT:
+ $uri = id(new PhutilURI('/phriction/diff/'.$object->getID().'/'))
+ ->alter('l', $this->getOldContent()->getVersion())
+ ->alter('r', $this->getNewContent()->getVersion());
+ $this->contentDiffURI = (string)$uri;
+ break 2;
+ default:
+ break;
+ }
+ }
+
return $xactions;
}
@@ -403,24 +418,10 @@
$body->addTextSection(
pht('DOCUMENT CONTENT'),
$object->getContent()->getContent());
- } else {
-
- foreach ($xactions as $xaction) {
- switch ($xaction->getTransactionType()) {
- case PhrictionTransaction::TYPE_CONTENT:
- $diff_uri = id(new PhutilURI(
- '/phriction/diff/'.$object->getID().'/'))
- ->alter('l', $this->getOldContent()->getVersion())
- ->alter('r', $this->getNewContent()->getVersion());
- $body->addLinkSection(
- pht('DOCUMENT DIFF'),
- PhabricatorEnv::getProductionURI($diff_uri));
- break 2;
- default:
- break;
- }
- }
-
+ } else if ($this->contentDiffURI) {
+ $body->addLinkSection(
+ pht('DOCUMENT DIFF'),
+ PhabricatorEnv::getProductionURI($this->contentDiffURI));
}
$body->addLinkSection(
@@ -810,4 +811,15 @@
return $new_content;
}
+ protected function getCustomWorkerState() {
+ return array(
+ 'contentDiffURI' => $this->contentDiffURI,
+ );
+ }
+
+ protected function loadCustomWorkerState(array $state) {
+ $this->contentDiffURI = idx($state, 'contentDiffURI');
+ return $this;
+ }
+
}
diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
--- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
+++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
@@ -397,14 +397,13 @@
return parent::requireCapabilities($object, $xaction);
}
- protected function loadEdges(
- PhabricatorLiskDAO $object,
- array $xactions) {
-
+ protected function willPublish(PhabricatorLiskDAO $object, array $xactions) {
$member_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
$object->getPHID(),
PhabricatorProjectProjectHasMemberEdgeType::EDGECONST);
$object->attachMemberPHIDs($member_phids);
+
+ return $object;
}
protected function shouldSendMail(
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -1,6 +1,36 @@
<?php
/**
+ *
+ * Publishing and Managing State
+ * ======
+ *
+ * After applying changes, the Editor queues a worker to publish mail, feed,
+ * and notifications, and to perform other background work like updating search
+ * indexes. This allows it to do this work without impacting performance for
+ * users.
+ *
+ * When work is moved to the daemons, the Editor state is serialized by
+ * @{method:getWorkerState}, then reloaded in a daemon process by
+ * @{method:loadWorkerState}. **This is fragile.**
+ *
+ * State is not persisted into the daemons by default, because we can not send
+ * arbitrary objects into the queue. This means the default behavior of any
+ * state properties is to reset to their defaults without warning prior to
+ * publishing.
+ *
+ * The easiest way to avoid this is to keep Editors stateless: the overwhelming
+ * majority of Editors can be written statelessly. If you need to maintain
+ * state, you can either:
+ *
+ * - not require state to exist during publishing; or
+ * - pass state to the daemons by implementing @{method:getCustomWorkerState}
+ * and @{method:loadCustomWorkerState}.
+ *
+ * This architecture isn't ideal, and we may eventually split this class into
+ * "Editor" and "Publisher" parts to make it more robust. See T6367 for some
+ * discussion and context.
+ *
* @task mail Sending Mail
* @task feed Publishing Feed Stories
* @task search Search Index
@@ -34,6 +64,10 @@
private $heraldEmailPHIDs = array();
private $heraldForcedEmailPHIDs = array();
private $heraldHeader;
+ private $mailToPHIDs = array();
+ private $mailCCPHIDs = array();
+ private $feedNotifyPHIDs = array();
+ private $feedRelatedPHIDs = array();
/**
* Get the class name for the application this editor is a part of.
@@ -907,23 +941,43 @@
}
$this->heraldHeader = $herald_header;
- if ($this->supportsWorkers()) {
- PhabricatorWorker::scheduleTask(
- 'PhabricatorApplicationTransactionPublishWorker',
- array(
- 'objectPHID' => $object->getPHID(),
- 'actorPHID' => $this->getActingAsPHID(),
- 'xactionPHIDs' => mpull($xactions, 'getPHID'),
- 'state' => $this->getWorkerState(),
- ),
- array(
- 'objectPHID' => $object->getPHID(),
- 'priority' => PhabricatorWorker::PRIORITY_ALERTS,
- ));
- } else {
- $this->publishTransactions($object, $xactions);
+ // We're going to compute some of the data we'll use to publish these
+ // transactions here, before queueing a worker.
+ //
+ // Primarily, this is more correct: we want to publish the object as it
+ // exists right now. The worker may not execute for some time, and we want
+ // to use the current To/CC list, not respect any changes which may occur
+ // between now and when the worker executes.
+ //
+ // As a secondary benefit, this tends to reduce the amount of state that
+ // Editors need to pass into workers.
+ $object = $this->willPublish($object, $xactions);
+
+ if (!$this->getDisableEmail()) {
+ if ($this->shouldSendMail($object, $xactions)) {
+ $this->mailToPHIDs = $this->getMailTo($object);
+ $this->mailCCPHIDs = $this->getMailCC($object);
+ }
}
+ if ($this->shouldPublishFeedStory($object, $xactions)) {
+ $this->feedRelatedPHIDs = $this->getFeedRelatedPHIDs($object, $xactions);
+ $this->feedNotifyPHIDs = $this->getFeedNotifyPHIDs($object, $xactions);
+ }
+
+ PhabricatorWorker::scheduleTask(
+ 'PhabricatorApplicationTransactionPublishWorker',
+ array(
+ 'objectPHID' => $object->getPHID(),
+ 'actorPHID' => $this->getActingAsPHID(),
+ 'xactionPHIDs' => mpull($xactions, 'getPHID'),
+ 'state' => $this->getWorkerState(),
+ ),
+ array(
+ 'objectPHID' => $object->getPHID(),
+ 'priority' => PhabricatorWorker::PRIORITY_ALERTS,
+ ));
+
return $xactions;
}
@@ -936,7 +990,7 @@
// in various transaction phases).
$this->loadSubscribers($object);
// Hook for other edges that may need (re-)loading
- $this->loadEdges($object, $xactions);
+ $object = $this->willPublish($object, $xactions);
$this->loadHandles($xactions);
@@ -1042,12 +1096,6 @@
}
}
- protected function loadEdges(
- PhabricatorLiskDAO $object,
- array $xactions) {
- return;
- }
-
private function validateEditParameters(
PhabricatorLiskDAO $object,
array $xactions) {
@@ -2084,8 +2132,8 @@
}
$email_force = array();
- $email_to = $this->getMailTo($object);
- $email_cc = $this->getMailCC($object);
+ $email_to = $this->mailToPHIDs;
+ $email_cc = $this->mailCCPHIDs;
$email_cc = array_merge($email_cc, $this->heraldEmailPHIDs);
$email_force = $this->heraldForcedEmailPHIDs;
@@ -2511,8 +2559,8 @@
return;
}
- $related_phids = $this->getFeedRelatedPHIDs($object, $xactions);
- $subscribed_phids = $this->getFeedNotifyPHIDs($object, $xactions);
+ $related_phids = $this->feedRelatedPHIDs;
+ $subscribed_phids = $this->feedNotifyPHIDs;
$story_type = $this->getFeedStoryType();
$story_data = $this->getFeedStoryData($object, $xactions);
@@ -2800,11 +2848,17 @@
/**
+ * Load any object state which is required to publish transactions.
+ *
+ * This hook is invoked in the main process before we compute data related
+ * to publishing transactions (like email "To" and "CC" lists), and again in
+ * the worker before publishing occurs.
+ *
+ * @return object Publishable object.
* @task workers
*/
- protected function supportsWorkers() {
- // TODO: Remove this method once everything supports workers.
- return false;
+ protected function willPublish(PhabricatorLiskDAO $object, array $xactions) {
+ return $object;
}
@@ -2825,11 +2879,10 @@
$state += array(
'excludeMailRecipientPHIDs' => $this->getExcludeMailRecipientPHIDs(),
- );
-
- return $state + array(
'custom' => $this->getCustomWorkerState(),
);
+
+ return $state;
}
@@ -2900,6 +2953,10 @@
'heraldEmailPHIDs',
'heraldForcedEmailPHIDs',
'heraldHeader',
+ 'mailToPHIDs',
+ 'mailCCPHIDs',
+ 'feedNotifyPHIDs',
+ 'feedRelatedPHIDs',
);
}

File Metadata

Mime Type
text/plain
Expires
Thu, Nov 7, 9:57 AM (1 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6725617
Default Alt Text
D13115.diff (13 KB)

Event Timeline

Phabricator · Built by Phacility