From 1291cdb65a5215c995910a48e173c1b957ada45b Mon Sep 17 00:00:00 2001 From: Valerio Bozzolan Date: Mon, 25 Aug 2025 15:33:18 +0200 Subject: [PATCH] Restore built-in image after user profile image destruction Summary: When you destroy a profile picture from the command line, that picture does not cause anymore a 404 error on your user page. So, you do not see anymore this problem: {F4303304} Instead, you just see your exciting builtin image, fully operational. This will be even more useful in a future when users could be able to delete their own profile pictures (T15407), and this would happen in a safe way, without causing any website damages. This feature works thanks to the brand-new "Before-Destruction Engine", described in T16079. To find which users use a file, we first look at where the file is attached, because in normal conditions this search is very efficient, even if you have a billion users with a billion profile pictures. As a last attempt, we look at the 'user' table. We may want an index in the future here, but this query is so rare that we can avoid it (T16080). Ref T15407 Ref T16079 Ref T16080 Closes T16074 Closes T16078 Test Plan: Normal test: As indicated in T16074, set your profile picture by uploading an image, then, destroy it from the command line (./bin/remove destroy MONOGRAM). You do not see anymore a broken user image causing 404 errors, but you see your lovely builtin image. Corner case test: Upload another profile picture, visit that file directly, click on {nav Attached > Detach File} (since it was possible, so we expect somebody may click on it) and destroy that file again from command line: it still works as intended and you see your builtin image, and not a blank image and 404 errors in the browser requests. Quality assurance: Images from other users are as they were before, in all cases. Or, just execute the new unit test that covers both cases: arc unit src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php You will see a green light about 'testProfilePictureDestruction'. Reviewers: O1 Blessed Committers, mainframe98, aklapper Reviewed By: O1 Blessed Committers, mainframe98, aklapper Subscribers: amybones, aklapper, mainframe98, tobiaswiese, Matthew, Cigaryno Maniphest Tasks: T16079, T16078, T16074, T15407, T16080 Differential Revision: https://we.phorge.it/D26027 --- src/__phutil_library_map__.php | 2 + .../query/PhabricatorFileAttachmentQuery.php | 77 +++++++++++++- .../PhabricatorUserEditorTestCase.php | 92 ++++++++++++++++ ...ictureBeforeDestructionEngineExtension.php | 100 ++++++++++++++++++ 4 files changed, 270 insertions(+), 1 deletion(-) create mode 100644 src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php 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(); + } + +} -- 2.51.2