@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
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
+269
+2
src/__phutil_library_map__.php
+2
src/__phutil_library_map__.php
···
2217
2217
'PeopleDisableUsersCapability' => 'applications/people/capability/PeopleDisableUsersCapability.php',
2218
2218
'PeopleHovercardEngineExtension' => 'applications/people/engineextension/PeopleHovercardEngineExtension.php',
2219
2219
'PeopleMainMenuBarExtension' => 'applications/people/engineextension/PeopleMainMenuBarExtension.php',
2220
+
'PeopleProfilePictureBeforeDestructionEngineExtension' => 'applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php',
2220
2221
'PeopleUserLogGarbageCollector' => 'applications/people/garbagecollector/PeopleUserLogGarbageCollector.php',
2221
2222
'Phabricator404Controller' => 'applications/base/controller/Phabricator404Controller.php',
2222
2223
'PhabricatorAWSConfigOptions' => 'applications/config/option/PhabricatorAWSConfigOptions.php',
···
8503
8504
'PeopleDisableUsersCapability' => 'PhabricatorPolicyCapability',
8504
8505
'PeopleHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension',
8505
8506
'PeopleMainMenuBarExtension' => 'PhabricatorMainMenuBarExtension',
8507
+
'PeopleProfilePictureBeforeDestructionEngineExtension' => 'PhabricatorBeforeDestructionEngineExtension',
8506
8508
'PeopleUserLogGarbageCollector' => 'PhabricatorGarbageCollector',
8507
8509
'Phabricator404Controller' => 'PhabricatorController',
8508
8510
'PhabricatorAWSConfigOptions' => 'PhabricatorApplicationConfigOptions',
+75
src/applications/files/query/PhabricatorFileAttachmentQuery.php
+75
src/applications/files/query/PhabricatorFileAttachmentQuery.php
···
7
7
extends PhabricatorCursorPagedPolicyAwareQuery {
8
8
9
9
private $objectPHIDs;
10
+
private $objectPHIDPrefix;
10
11
private $filePHIDs;
11
12
private $needFiles;
12
13
private $visibleFiles;
14
+
private $attachmentModes;
13
15
16
+
/**
17
+
* Filter with these object PHIDs.
18
+
*
19
+
* @param array<string> $object_phids Example: array('PHID-USER-123abc')
20
+
* @return $this
21
+
*/
14
22
public function withObjectPHIDs(array $object_phids) {
15
23
$this->objectPHIDs = $object_phids;
16
24
return $this;
17
25
}
18
26
27
+
/**
28
+
* Filter with a PHID object type.
29
+
*
30
+
* This is just syntax sugar for the method withObjectPHIDPrefix(),
31
+
* so you can pass constants like PhabricatorPeopleUserPHIDType::TYPECONST.
32
+
*
33
+
* @param string $phid_type PHID type constant. Example: 'USER'.
34
+
* @return $this
35
+
*/
36
+
public function withObjectPHIDType(string $phid_type) {
37
+
return $this->withObjectPHIDPrefix("PHID-{$phid_type}-");
38
+
}
39
+
40
+
/**
41
+
* Filter with a object PHID prefix string.
42
+
*
43
+
* @param string $phid_prefix PHID prefix. Example: 'PHID-USER-'
44
+
* @return $this
45
+
*/
46
+
public function withObjectPHIDPrefix(string $phid_prefix) {
47
+
$this->objectPHIDPrefix = $phid_prefix;
48
+
return $this;
49
+
}
50
+
51
+
/**
52
+
* @param array<string> $file_phids Array of file PHIDs.
53
+
* @return $this
54
+
*/
19
55
public function withFilePHIDs(array $file_phids) {
20
56
$this->filePHIDs = $file_phids;
21
57
return $this;
22
58
}
23
59
60
+
/**
61
+
* If the files must be visible by the current viewer.
62
+
*
63
+
* @param bool $visible_files
64
+
* @return $this
65
+
*/
24
66
public function withVisibleFiles($visible_files) {
25
67
$this->visibleFiles = $visible_files;
26
68
return $this;
27
69
}
28
70
71
+
/**
72
+
* Filter with some attachment modes.
73
+
*
74
+
* @param array<string> $attachment_modes Array of attachment modes defined
75
+
* in the in the PhabricatorFileAttachment class.
76
+
* Example: 'array('attach','reference')'.
77
+
* @return $this
78
+
*/
79
+
public function withAttachmentModes(array $attachment_modes) {
80
+
$this->attachmentModes = $attachment_modes;
81
+
return $this;
82
+
}
83
+
84
+
/**
85
+
* If you also need the file objects.
86
+
*
87
+
* @param bool $need True if you also need the file objects.
88
+
* @return $this
89
+
*/
29
90
public function needFiles($need) {
30
91
$this->needFiles = $need;
31
92
return $this;
···
45
106
$this->objectPHIDs);
46
107
}
47
108
109
+
if ($this->objectPHIDPrefix !== null) {
110
+
$where[] = qsprintf(
111
+
$conn,
112
+
'attachments.objectPHID LIKE %>',
113
+
$this->objectPHIDPrefix);
114
+
}
115
+
48
116
if ($this->filePHIDs !== null) {
49
117
$where[] = qsprintf(
50
118
$conn,
51
119
'attachments.filePHID IN (%Ls)',
52
120
$this->filePHIDs);
121
+
}
122
+
123
+
if ($this->attachmentModes !== null) {
124
+
$where[] = qsprintf(
125
+
$conn,
126
+
'attachments.attachmentMode IN (%Ls)',
127
+
$this->attachmentModes);
53
128
}
54
129
55
130
return $where;
+92
src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php
+92
src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php
···
85
85
->createNewUser($user, $email);
86
86
}
87
87
88
+
/**
89
+
* Test the destruction of one profile picture in use.
90
+
* This test covers the "before-destruction" engine called
91
+
* 'PeopleProfilePictureBeforeDestructionEngineExtension'.
92
+
*/
93
+
public function testProfilePictureDestruction() {
94
+
// Create some users with different profile pictures.
95
+
// Note that 'avatar.png' is Psyduck, btw.
96
+
$user1 = $this->generateNewTestUser();
97
+
$user2 = $this->generateNewTestUser();
98
+
$user3 = $this->generateNewTestUser();
99
+
$pic1 = $this->attachBuiltinImage($user1, 'avatar.png');
100
+
$pic2 = $this->attachBuiltinImage($user2, 'user2.png');
101
+
$pic3 = $this->attachBuiltinImage($user3, 'user3.png');
102
+
103
+
// Base test.
104
+
$this->assertEqual($pic1->getPHID(), $user1->getProfileImagePHID());
105
+
$this->assertEqual($pic2->getPHID(), $user2->getProfileImagePHID());
106
+
$this->assertEqual($pic3->getPHID(), $user3->getProfileImagePHID());
107
+
108
+
// Destroy the profile image of user1.
109
+
// Our intention is to test this:
110
+
// ./bin/remove destroy $PIC1
111
+
// As desired side-effect, the "before destruction engine"
112
+
// 'PeopleProfilePictureBeforeDestructionEngineExtension'
113
+
// is executed too, to orphanize the profile image of $user1.
114
+
// All other users must remain untouched.
115
+
$engine = new PhabricatorDestructionEngine();
116
+
$engine->destroyObject($pic1);
117
+
$user1 = $user1->reload();
118
+
$user2 = $user2->reload();
119
+
$user3 = $user3->reload();
120
+
$this->assertEqual(null, $user1->getProfileImagePHID());
121
+
$this->assertEqual($pic2->getPHID(), $user2->getProfileImagePHID());
122
+
$this->assertEqual($pic3->getPHID(), $user3->getProfileImagePHID());
123
+
124
+
// Test if pic2 can be destroyed, even if the user clicked 'Detach'.
125
+
// This test can be potentially removed if the related micro-optimizations
126
+
// are removed from the engine
127
+
// (PeopleProfilePictureBeforeDestructionEngineExtension).
128
+
// See code about https://we.phorge.it/T16080.
129
+
// Also, test again that pic3 is untouched.
130
+
$this->detachFileFromAllObjects($pic2); // Click 'Detach'.
131
+
$engine->destroyObject($pic2);
132
+
$user2 = $user2->reload();
133
+
$user3 = $user3->reload();
134
+
$this->assertEqual(null, $user2->getProfileImagePHID());
135
+
$this->assertEqual($pic3->getPHID(), $user3->getProfileImagePHID());
136
+
}
137
+
138
+
/**
139
+
* Assign a profile picture to one user, starting from a builtin image.
140
+
* Get the resulting profile picture.
141
+
* @param PhabricatorUser $user User receiving the profile picture.
142
+
* @param string $image Builtin image name used as starting point.
143
+
* @return PhabricatorFile File transform used as profile picture.
144
+
*/
145
+
private function attachBuiltinImage(
146
+
PhabricatorUser $user,
147
+
string $image): PhabricatorFile {
148
+
// Code credit: PhabricatorPeopleProfilePictureController
149
+
150
+
$file = PhabricatorFile::loadBuiltin($user, $image);
151
+
$xform = PhabricatorFileTransform::getTransformByKey(
152
+
PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE);
153
+
$xformed = $xform->executeTransform($file);
154
+
155
+
// Assign the profile image to the user.
156
+
$xformed->attachToObject($user->getPHID());
157
+
$user->setProfileImagePHID($xformed->getPHID());
158
+
$user->save();
159
+
160
+
return $xformed;
161
+
}
162
+
163
+
/**
164
+
* Detach a file from all objects.
165
+
* For example if the file was shown as attachment in the user profile page,
166
+
* this file will be not anymore, like you clicked on the 'Detach' button.
167
+
* @param PhabricatorFile $file
168
+
* @return void
169
+
*/
170
+
private function detachFileFromAllObjects(PhabricatorFile $file) {
171
+
$table = new PhabricatorFileAttachment();
172
+
$attachments = $table->loadAllWhere(
173
+
'filePHID = %s',
174
+
$file->getPHID());
175
+
foreach ($attachments as $attachment) {
176
+
$attachment->delete();
177
+
}
178
+
}
179
+
88
180
}
+100
src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php
+100
src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php
···
1
+
<?php
2
+
3
+
/**
4
+
* Before a profile picture is destroyed, restore the builtin picture.
5
+
* https://we.phorge.it/T16074
6
+
*/
7
+
final class PeopleProfilePictureBeforeDestructionEngineExtension
8
+
extends PhabricatorBeforeDestructionEngineExtension {
9
+
10
+
const EXTENSIONKEY = 'people-profiles';
11
+
12
+
public function getExtensionName(): string {
13
+
return pht('People Profile Pictures');
14
+
}
15
+
16
+
public function canBeforeDestroyObject(
17
+
PhabricatorDestructionEngine $destruction_engine,
18
+
$object): bool {
19
+
return ($object instanceof PhabricatorFile)
20
+
&& $object->getIsProfileImage();
21
+
}
22
+
23
+
public function beforeDestroyObject(
24
+
PhabricatorDestructionEngine $destruction_engine,
25
+
$object): void {
26
+
// File that will be destroyed soon.
27
+
// The file PHID is always non-empty at this point.
28
+
$file_phid = $object->getPHID();
29
+
30
+
// Note that a file that is used as profile images have
31
+
// the authorPHID = null, so it's not so obvious which
32
+
// is the affected user.
33
+
// https://we.phorge.it/T15407
34
+
35
+
// Note that we could find the affected users by running this
36
+
// very inefficient query that would lead to a full table scan:
37
+
// SELECT * FROM user WHERE profileImagePHID = $file_phid
38
+
// In the future it might make sense to add an index on 'profileImagePHID'
39
+
// if more frontend features will read that info, so we can also avoid the
40
+
// following lines of code.
41
+
// https://we.phorge.it/T16080
42
+
43
+
// We look at the file attachments to find the affected user efficiently.
44
+
// Note that file attachments are only available before destroying the file,
45
+
// and... fortunately we are inside a "Before Destruction" engine.
46
+
// This query is efficient thanks to the database index on 'filePHID' and
47
+
// the low cardinality of this result set.
48
+
$viewer = $destruction_engine->getViewer();
49
+
$file_attachments_query = new PhabricatorFileAttachmentQuery();
50
+
$file_attachments =
51
+
$file_attachments_query
52
+
->setViewer($viewer)
53
+
->withFilePHIDs(array($file_phid))
54
+
->withObjectPHIDType(PhabricatorPeopleUserPHIDType::TYPECONST)
55
+
->withAttachmentModes(array(PhabricatorFileAttachment::MODE_ATTACH))
56
+
->execute();
57
+
$attached_objects = mpull($file_attachments, 'getObject');
58
+
59
+
// Be 100% sure to only operate on users,
60
+
// and that these are really using this picture.
61
+
$affected_users = array();
62
+
foreach ($attached_objects as $attached_object) {
63
+
if (($attached_object instanceof PhabricatorUser) &&
64
+
($attached_object->getProfileImagePHID() == $file_phid)) {
65
+
$affected_users[] = $attached_object;
66
+
}
67
+
}
68
+
69
+
$user_table = new PhabricatorUser();
70
+
71
+
if (!$affected_users) {
72
+
// The above fast speculation has found no users.
73
+
// It can happen when somebody manually used the "Detach File" button
74
+
// from the file (why people can generally do that? uhm).
75
+
// Only in this desperate case, we run this inefficient query.
76
+
$affected_users = $user_table
77
+
->loadAllWhere(
78
+
'profileImagePHID = %s',
79
+
$file_phid);
80
+
}
81
+
82
+
// Avoid opening an empty transaction.
83
+
if (!$affected_users) {
84
+
return;
85
+
}
86
+
87
+
// Set the builtin profile image to each affected user.
88
+
// Premising that it's supposed to be just one user.
89
+
// Maybe in the future multiple users may use the same
90
+
// profile picture, so let's covers more corner cases,
91
+
// because we can.
92
+
$user_table->openTransaction();
93
+
foreach ($affected_users as $affected_user) {
94
+
$affected_user->setProfileImagePHID(null);
95
+
$affected_user->save();
96
+
}
97
+
$user_table->saveTransaction();
98
+
}
99
+
100
+
}