diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 61638335bb..bf1bfbb61e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2217,6 +2217,7 @@ phutil_register_library_map(array( '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', @@ -8503,6 +8504,7 @@ phutil_register_library_map(array( '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 index 34634018da..531cd9ffe6 100644 --- a/src/applications/files/query/PhabricatorFileAttachmentQuery.php +++ b/src/applications/files/query/PhabricatorFileAttachmentQuery.php @@ -7,25 +7,86 @@ final class PhabricatorFileAttachmentQuery 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 $this + */ 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 constants like PhabricatorPeopleUserPHIDType::TYPECONST. + * + * @param string $phid_type PHID type constant. Example: 'USER'. + * @return $this + */ + public function withObjectPHIDType(string $phid_type) { + return $this->withObjectPHIDPrefix("PHID-{$phid_type}-"); + } + + /** + * Filter with a object PHID prefix string. + * + * @param string $phid_prefix PHID prefix. Example: 'PHID-USER-' + * @return $this + */ + public function withObjectPHIDPrefix(string $phid_prefix) { + $this->objectPHIDPrefix = $phid_prefix; + return $this; + } + + /** + * @param array $file_phids Array of file PHIDs. + * @return $this + */ public function withFilePHIDs(array $file_phids) { $this->filePHIDs = $file_phids; return $this; } + /** + * If the files must be visible by the current viewer. + * + * @param bool $visible_files + * @return $this + */ public function withVisibleFiles($visible_files) { $this->visibleFiles = $visible_files; return $this; } + /** + * Filter with some attachment modes. + * + * @param array $attachment_modes Array of attachment modes defined + * in the in the PhabricatorFileAttachment class. + * Example: 'array('attach','reference')'. + * @return $this + */ + public function withAttachmentModes(array $attachment_modes) { + $this->attachmentModes = $attachment_modes; + return $this; + } + + /** + * If you also need the file objects. + * + * @param bool $need True if you also need the file objects. + * @return $this + */ public function needFiles($need) { $this->needFiles = $need; return $this; @@ -45,6 +106,13 @@ final class PhabricatorFileAttachmentQuery $this->objectPHIDs); } + if ($this->objectPHIDPrefix !== null) { + $where[] = qsprintf( + $conn, + 'attachments.objectPHID LIKE %>', + $this->objectPHIDPrefix); + } + if ($this->filePHIDs !== null) { $where[] = qsprintf( $conn, @@ -52,6 +120,13 @@ final class PhabricatorFileAttachmentQuery $this->filePHIDs); } + if ($this->attachmentModes !== null) { + $where[] = qsprintf( + $conn, + 'attachments.attachmentMode IN (%Ls)', + $this->attachmentModes); + } + return $where; } diff --git a/src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php b/src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php index ef0c58060a..92fc220fe0 100644 --- a/src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php +++ b/src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php @@ -85,4 +85,96 @@ final class PhabricatorUserEditorTestCase extends PhabricatorTestCase { ->createNewUser($user, $email); } + /** + * Test the destruction of one profile picture in use. + * This test covers the "before-destruction" engine called + * 'PeopleProfilePictureBeforeDestructionEngineExtension'. + */ + public function testProfilePictureDestruction() { + // Create some users with different profile pictures. + // Note that 'avatar.png' is Psyduck, btw. + $user1 = $this->generateNewTestUser(); + $user2 = $this->generateNewTestUser(); + $user3 = $this->generateNewTestUser(); + $pic1 = $this->attachBuiltinImage($user1, 'avatar.png'); + $pic2 = $this->attachBuiltinImage($user2, 'user2.png'); + $pic3 = $this->attachBuiltinImage($user3, 'user3.png'); + + // Base test. + $this->assertEqual($pic1->getPHID(), $user1->getProfileImagePHID()); + $this->assertEqual($pic2->getPHID(), $user2->getProfileImagePHID()); + $this->assertEqual($pic3->getPHID(), $user3->getProfileImagePHID()); + + // Destroy the profile image of user1. + // Our intention is to test this: + // ./bin/remove destroy $PIC1 + // As desired side-effect, the "before destruction engine" + // 'PeopleProfilePictureBeforeDestructionEngineExtension' + // is executed too, to orphanize the profile image of $user1. + // All other users must remain untouched. + $engine = new PhabricatorDestructionEngine(); + $engine->destroyObject($pic1); + $user1 = $user1->reload(); + $user2 = $user2->reload(); + $user3 = $user3->reload(); + $this->assertEqual(null, $user1->getProfileImagePHID()); + $this->assertEqual($pic2->getPHID(), $user2->getProfileImagePHID()); + $this->assertEqual($pic3->getPHID(), $user3->getProfileImagePHID()); + + // Test if pic2 can be destroyed, even if the user clicked 'Detach'. + // This test can be potentially removed if the related micro-optimizations + // are removed from the engine + // (PeopleProfilePictureBeforeDestructionEngineExtension). + // See code about https://we.phorge.it/T16080. + // Also, test again that pic3 is untouched. + $this->detachFileFromAllObjects($pic2); // Click 'Detach'. + $engine->destroyObject($pic2); + $user2 = $user2->reload(); + $user3 = $user3->reload(); + $this->assertEqual(null, $user2->getProfileImagePHID()); + $this->assertEqual($pic3->getPHID(), $user3->getProfileImagePHID()); + } + + /** + * Assign a profile picture to one user, starting from a builtin image. + * Get the resulting profile picture. + * @param PhabricatorUser $user User receiving the profile picture. + * @param string $image Builtin image name used as starting point. + * @return PhabricatorFile File transform used as profile picture. + */ + private function attachBuiltinImage( + PhabricatorUser $user, + string $image): PhabricatorFile { + // Code credit: PhabricatorPeopleProfilePictureController + + $file = PhabricatorFile::loadBuiltin($user, $image); + $xform = PhabricatorFileTransform::getTransformByKey( + PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE); + $xformed = $xform->executeTransform($file); + + // Assign the profile image to the user. + $xformed->attachToObject($user->getPHID()); + $user->setProfileImagePHID($xformed->getPHID()); + $user->save(); + + return $xformed; + } + + /** + * Detach a file from all objects. + * For example if the file was shown as attachment in the user profile page, + * this file will be not anymore, like you clicked on the 'Detach' button. + * @param PhabricatorFile $file + * @return void + */ + private function detachFileFromAllObjects(PhabricatorFile $file) { + $table = new PhabricatorFileAttachment(); + $attachments = $table->loadAllWhere( + 'filePHID = %s', + $file->getPHID()); + foreach ($attachments as $attachment) { + $attachment->delete(); + } + } + } diff --git a/src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php b/src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php new file mode 100644 index 0000000000..b515151f49 --- /dev/null +++ b/src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php @@ -0,0 +1,100 @@ +getIsProfileImage(); + } + + public function beforeDestroyObject( + PhabricatorDestructionEngine $destruction_engine, + $object): void { + // 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' + // if more frontend features will read that info, so we can also avoid the + // following lines of 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(PhabricatorFileAttachment::MODE_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) { + // 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. + $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(); + } + +}