diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -984,6 +984,7 @@ 'DiffusionRepositoryEditUpdateController' => 'applications/diffusion/controller/DiffusionRepositoryEditUpdateController.php', 'DiffusionRepositoryFunctionDatasource' => 'applications/diffusion/typeahead/DiffusionRepositoryFunctionDatasource.php', 'DiffusionRepositoryHistoryManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryHistoryManagementPanel.php', + 'DiffusionRepositoryIdentityDestructionEngineExtension' => 'applications/diffusion/identity/DiffusionRepositoryIdentityDestructionEngineExtension.php', 'DiffusionRepositoryIdentityEditor' => 'applications/diffusion/editor/DiffusionRepositoryIdentityEditor.php', 'DiffusionRepositoryIdentityEngine' => 'applications/diffusion/identity/DiffusionRepositoryIdentityEngine.php', 'DiffusionRepositoryIdentitySearchEngine' => 'applications/diffusion/query/DiffusionRepositoryIdentitySearchEngine.php', @@ -6968,6 +6969,7 @@ 'DiffusionRepositoryEditUpdateController' => 'DiffusionRepositoryManageController', 'DiffusionRepositoryFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DiffusionRepositoryHistoryManagementPanel' => 'DiffusionRepositoryManagementPanel', + 'DiffusionRepositoryIdentityDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension', 'DiffusionRepositoryIdentityEditor' => 'PhabricatorApplicationTransactionEditor', 'DiffusionRepositoryIdentityEngine' => 'Phobject', 'DiffusionRepositoryIdentitySearchEngine' => 'PhabricatorApplicationSearchEngine', diff --git a/src/applications/diffusion/identity/DiffusionRepositoryIdentityDestructionEngineExtension.php b/src/applications/diffusion/identity/DiffusionRepositoryIdentityDestructionEngineExtension.php new file mode 100644 --- /dev/null +++ b/src/applications/diffusion/identity/DiffusionRepositoryIdentityDestructionEngineExtension.php @@ -0,0 +1,40 @@ +getPHID(); + } + + if ($object instanceof PhabricatorUserEmail) { + $email_addresses[] = $object->getAddress(); + } + + if ($related_phids || $email_addresses) { + PhabricatorWorker::scheduleTask( + 'PhabricatorRepositoryIdentityChangeWorker', + array( + 'relatedPHIDs' => $related_phids, + 'emailAddresses' => $email_addresses, + )); + } + } + +} diff --git a/src/applications/diffusion/identity/DiffusionRepositoryIdentityEngine.php b/src/applications/diffusion/identity/DiffusionRepositoryIdentityEngine.php --- a/src/applications/diffusion/identity/DiffusionRepositoryIdentityEngine.php +++ b/src/applications/diffusion/identity/DiffusionRepositoryIdentityEngine.php @@ -95,4 +95,12 @@ return $identity; } + public function didUpdateEmailAddress($raw_address) { + PhabricatorWorker::scheduleTask( + 'PhabricatorRepositoryIdentityChangeWorker', + array( + 'emailAddresses' => array($raw_address), + )); + } + } diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php --- a/src/applications/people/editor/PhabricatorUserEditor.php +++ b/src/applications/people/editor/PhabricatorUserEditor.php @@ -89,6 +89,9 @@ $this->didVerifyEmail($user, $email); } + id(new DiffusionRepositoryIdentityEngine()) + ->didUpdateEmailAddress($email->getAddress()); + return $this; } @@ -202,11 +205,8 @@ $user->endWriteLocking(); $user->saveTransaction(); - // Try and match this new address against unclaimed `RepositoryIdentity`s - PhabricatorWorker::scheduleTask( - 'PhabricatorRepositoryIdentityChangeWorker', - array('userPHID' => $user->getPHID()), - array('objectPHID' => $user->getPHID())); + id(new DiffusionRepositoryIdentityEngine()) + ->didUpdateEmailAddress($email->getAddress()); return $this; } @@ -241,7 +241,8 @@ throw new Exception(pht('Email not owned by user!')); } - id(new PhabricatorDestructionEngine()) + $destruction_engine = id(new PhabricatorDestructionEngine()) + ->setWaitToFinalizeDestruction(true) ->destroyObject($email); $log = PhabricatorUserLog::initializeNewLog( @@ -255,6 +256,7 @@ $user->saveTransaction(); $this->revokePasswordResetLinks($user); + $destruction_engine->finalizeDestruction(); return $this; } @@ -327,7 +329,6 @@ } $email->sendNewPrimaryEmail($user); - $this->revokePasswordResetLinks($user); return $this; @@ -441,6 +442,9 @@ $user->endWriteLocking(); $user->saveTransaction(); + + id(new DiffusionRepositoryIdentityEngine()) + ->didUpdateEmailAddress($email->getAddress()); } diff --git a/src/applications/repository/worker/PhabricatorRepositoryIdentityChangeWorker.php b/src/applications/repository/worker/PhabricatorRepositoryIdentityChangeWorker.php --- a/src/applications/repository/worker/PhabricatorRepositoryIdentityChangeWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryIdentityChangeWorker.php @@ -6,30 +6,54 @@ protected function doWork() { $viewer = PhabricatorUser::getOmnipotentUser(); - $task_data = $this->getTaskData(); - $user_phid = idx($task_data, 'userPHID'); + $related_phids = $this->getTaskDataValue('relatedPHIDs'); + $email_addresses = $this->getTaskDataValue('emailAddresses'); + + // Retain backward compatibility with older tasks which may still be in + // queue. Previously, this worker accepted a single "userPHID". See + // T13444. This can be removed in some future version of Phabricator once + // these tasks have likely flushed out of queue. + $legacy_phid = $this->getTaskDataValue('userPHID'); + if ($legacy_phid) { + if (!is_array($related_phids)) { + $related_phids = array(); + } + $related_phids[] = $legacy_phid; + } - $user = id(new PhabricatorPeopleQuery()) - ->withPHIDs(array($user_phid)) - ->setViewer($viewer) - ->executeOne(); + // Note that we may arrive in this worker after the associated objects + // have already been destroyed, so we can't (and shouldn't) verify that + // PHIDs correspond to real objects. If you "bin/remove destroy" a user, + // we'll end up here with a now-bogus user PHID that we should + // disassociate from identities. - $emails = id(new PhabricatorUserEmail())->loadAllWhere( - 'userPHID = %s', - $user->getPHID()); + $identity_map = array(); - $identity_engine = id(new DiffusionRepositoryIdentityEngine()) - ->setViewer($viewer); + if ($related_phids) { + $identities = id(new PhabricatorRepositoryIdentityQuery()) + ->setViewer($viewer) + ->withRelatedPHIDs($related_phids) + ->execute(); + $identity_map += mpull($identities, null, 'getPHID'); + } - foreach ($emails as $email) { + if ($email_addresses) { $identities = id(new PhabricatorRepositoryIdentityQuery()) ->setViewer($viewer) - ->withEmailAddresses(array($email->getAddress())) + ->withEmailAddresses($email_addresses) ->execute(); + $identity_map += mpull($identities, null, 'getPHID'); + } - foreach ($identities as $identity) { - $identity_engine->newUpdatedIdentity($identity); - } + // If we didn't find any related identities, we're all set. + if (!$identity_map) { + return; + } + + $identity_engine = id(new DiffusionRepositoryIdentityEngine()) + ->setViewer($viewer); + foreach ($identity_map as $identity) { + $identity_engine->newUpdatedIdentity($identity); } } diff --git a/src/applications/system/engine/PhabricatorDestructionEngine.php b/src/applications/system/engine/PhabricatorDestructionEngine.php --- a/src/applications/system/engine/PhabricatorDestructionEngine.php +++ b/src/applications/system/engine/PhabricatorDestructionEngine.php @@ -5,6 +5,9 @@ private $rootLogID; private $collectNotes; private $notes = array(); + private $depth = 0; + private $destroyedObjects = array(); + private $waitToFinalizeDestruction = false; public function setCollectNotes($collect_notes) { $this->collectNotes = $collect_notes; @@ -19,9 +22,20 @@ return PhabricatorUser::getOmnipotentUser(); } + public function setWaitToFinalizeDestruction($wait) { + $this->waitToFinalizeDestruction = $wait; + return $this; + } + + public function getWaitToFinalizeDestruction() { + return $this->waitToFinalizeDestruction; + } + public function destroyObject(PhabricatorDestructibleInterface $object) { + $this->depth++; + $log = id(new PhabricatorSystemDestructionLog()) - ->setEpoch(time()) + ->setEpoch(PhabricatorTime::getNow()) ->setObjectClass(get_class($object)); if ($this->rootLogID) { @@ -73,7 +87,42 @@ foreach ($extensions as $key => $extension) { $extension->destroyObject($this, $object); } + + $this->destroyedObjects[] = $object; + } + + $this->depth--; + + // If this is a root-level invocation of "destroyObject()", flush the + // queue of destroyed objects and fire "didDestroyObject()" hooks. This + // hook allows extensions to do things like queue cache updates which + // might race if we fire them during object destruction. + + if (!$this->depth) { + if (!$this->getWaitToFinalizeDestruction()) { + $this->finalizeDestruction(); + } + } + + return $this; + } + + public function finalizeDestruction() { + $extensions = PhabricatorDestructionEngineExtension::getAllExtensions(); + + foreach ($this->destroyedObjects as $object) { + foreach ($extensions as $extension) { + if (!$extension->canDestroyObject($this, $object)) { + continue; + } + + $extension->didDestroyObject($this, $object); + } } + + $this->destroyedObjects = array(); + + return $this; } private function getObjectPHID($object) { diff --git a/src/applications/system/engine/PhabricatorDestructionEngineExtension.php b/src/applications/system/engine/PhabricatorDestructionEngineExtension.php --- a/src/applications/system/engine/PhabricatorDestructionEngineExtension.php +++ b/src/applications/system/engine/PhabricatorDestructionEngineExtension.php @@ -14,9 +14,17 @@ return true; } - abstract public function destroyObject( + public function destroyObject( PhabricatorDestructionEngine $engine, - $object); + $object) { + return null; + } + + public function didDestroyObject( + PhabricatorDestructionEngine $engine, + $object) { + return null; + } final public static function getAllExtensions() { return id(new PhutilClassMapQuery())