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 @@ -2217,6 +2217,7 @@ 'PeopleDisableUsersCapability' => 'applications/people/capability/PeopleDisableUsersCapability.php', 'PeopleHovercardEngineExtension' => 'applications/people/engineextension/PeopleHovercardEngineExtension.php', 'PeopleMainMenuBarExtension' => 'applications/people/engineextension/PeopleMainMenuBarExtension.php', + 'PeopleProfilePictureBeforeDestructionEngineExtension' => 'applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php', 'PeopleUserLogGarbageCollector' => 'applications/people/garbagecollector/PeopleUserLogGarbageCollector.php', 'Phabricator404Controller' => 'applications/base/controller/Phabricator404Controller.php', 'PhabricatorAWSConfigOptions' => 'applications/config/option/PhabricatorAWSConfigOptions.php', @@ -8501,6 +8502,7 @@ 'PeopleDisableUsersCapability' => 'PhabricatorPolicyCapability', 'PeopleHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', 'PeopleMainMenuBarExtension' => 'PhabricatorMainMenuBarExtension', + 'PeopleProfilePictureBeforeDestructionEngineExtension' => 'PhabricatorBeforeDestructionEngineExtension', 'PeopleUserLogGarbageCollector' => 'PhabricatorGarbageCollector', 'Phabricator404Controller' => 'PhabricatorController', 'PhabricatorAWSConfigOptions' => 'PhabricatorApplicationConfigOptions', diff --git a/src/applications/files/query/PhabricatorFileAttachmentQuery.php b/src/applications/files/query/PhabricatorFileAttachmentQuery.php --- a/src/applications/files/query/PhabricatorFileAttachmentQuery.php +++ b/src/applications/files/query/PhabricatorFileAttachmentQuery.php @@ -4,15 +4,48 @@ extends PhabricatorCursorPagedPolicyAwareQuery { private $objectPHIDs; + private $objectPHIDPrefix; private $filePHIDs; private $needFiles; private $visibleFiles; - + private $attachmentModes; + + /** + * Filter with these object PHIDs. + * + * @param array $object_phids Example: array('PHID-USER-123abc') + * @return self + */ public function withObjectPHIDs(array $object_phids) { $this->objectPHIDs = $object_phids; return $this; } + /** + * Filter with a PHID object type. + * + * This is just syntax sugar for the method withObjectPHIDPrefix(), + * so you can pass stuff like PhabricatorPeopleUserPHIDType::TYPECONST. + * + * @param string $object_phid_type PHID type constant. Example: 'USER'. + * @return self + */ + public function withObjectPHIDType(string $object_phid_type) { + $object_phid_prefix = 'PHID-'.$object_phid_type.'-'; + return $this->withObjectPHIDPrefix($object_phid_prefix); + } + + /** + * Filter with a object PHID prefix string. + * + * @param string $object_phid_prefix Example: 'PHID-USER-' + * @return self + */ + public function withObjectPHIDPrefix(string $object_phid_prefix) { + $this->objectPHIDPrefix = $object_phid_prefix; + return $this; + } + public function withFilePHIDs(array $file_phids) { $this->filePHIDs = $file_phids; return $this; @@ -23,6 +56,18 @@ return $this; } + /** + * Filter with some attachment modes. + * + * @param array $attachment_modes Attachment modes. + * E.g. array('attach', 'reference') + * @return self + */ + public function withAttachmentModes(array $attachment_modes) { + $this->attachmentModes = $attachment_modes; + return $this; + } + public function needFiles($need) { $this->needFiles = $need; return $this; @@ -42,6 +87,13 @@ $this->objectPHIDs); } + if ($this->objectPHIDPrefix !== null) { + $where[] = qsprintf( + $conn, + 'attachments.objectPHID LIKE %>', + $this->objectPHIDPrefix); + } + if ($this->filePHIDs !== null) { $where[] = qsprintf( $conn, @@ -49,6 +101,13 @@ $this->filePHIDs); } + if ($this->attachmentModes !== null) { + $where[] = qsprintf( + $conn, + 'attachments.attachmentMode IN (%Ls)', + $this->attachmentModes); + } + return $where; } diff --git a/src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php b/src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php new file mode 100644 --- /dev/null +++ b/src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php @@ -0,0 +1,107 @@ +getIsProfileImage(); + } + + public function beforeDestroyObject( + PhabricatorDestructionEngine $destruction_engine, + $object) { + + // File that will be destroyed soon. + // The file PHID is always non-empty at this point. + $file_phid = $object->getPHID(); + + // Note that a file that is used as profile images have + // the authorPHID = null, so it's not so obvious which + // is the affected user. + // https://we.phorge.it/T15407 + + // Note that we could find the affected users by running this + // very inefficient query that would lead to a full table scan: + // SELECT * FROM user WHERE profileImagePHID = $file_phid + // In the future it might make sense to add an index on 'profileImagePHID' + // to be able to avoid the next code. + // https://we.phorge.it/T16080 + + // We look at the file attachments to find the affected user efficiently. + // Note that file attachments are only available before destroying the file, + // and... fortunately we are inside a "Before Destruction" engine. + // This query is efficient thanks to the database index on 'filePHID' and + // the low cardinality of this result set. + $viewer = $destruction_engine->getViewer(); + $file_attachments_query = new PhabricatorFileAttachmentQuery(); + $file_attachments = + $file_attachments_query + ->setViewer($viewer) + ->withFilePHIDs(array($file_phid)) + ->withObjectPHIDType(PhabricatorPeopleUserPHIDType::TYPECONST) + ->withAttachmentModes(array('attach')) + ->execute(); + $attached_objects = mpull($file_attachments, 'getObject'); + + // Be 100% sure to only operate on users, + // and that these are really using this picture. + $affected_users = array(); + foreach ($attached_objects as $attached_object) { + if ( + ($attached_object instanceof PhabricatorUser) + && $attached_object->getProfileImagePHID() == $file_phid + ) { + $affected_users[] = $attached_object; + } + } + + $user_table = new PhabricatorUser(); + + if (!$affected_users) { + // If we are here, the above fast speculation has found no users. + // It can happen when somebody manually used the "Detach File" button + // from the file (why people can generally do that? uhm). + // Only in this desperate case, we run this inefficient query. + // As already said, in the future we may want to add an index + // over the column 'profileImagePHID' in the table 'user', + // and in that case, we could avoid the above code about attachments. + // https://we.phorge.it/T16080 + $affected_users = $user_table + ->loadAllWhere( + 'profileImagePHID = %s', + $file_phid); + } + + // Avoid opening an empty transaction. + if (!$affected_users) { + return; + } + + // Set the builtin profile image to each affected user. + // Premising that it's supposed to be just one user. + // Maybe in the future multiple users may use the same + // profile picture, so let's covers more corner cases, + // because we can. + $user_table->openTransaction(); + foreach ($affected_users as $affected_user) { + $affected_user->setProfileImagePHID(null); + $affected_user->save(); + } + $user_table->saveTransaction(); + } + +}