Page MenuHomePhorge

Profile picture destroy: implement Before-Destruction engine to restore the builtin image
Needs ReviewPublic

Authored by valerio.bozzolan on Mon, May 19, 22:35.

Details

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:

profile-picture-404-error.jpg (461×419 px, 26 KB)

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 AttachedDetach 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.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D26027
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 2000
Build 2000: arc lint + arc unit

Event Timeline

I need to test this still, but this looks good. Two nits, and one issue I spotted:

src/applications/files/query/PhabricatorFileAttachmentQuery.php
91
src/applications/people/engineextension/PeopleProfilePictureBeforeDestructionEngineExtension.php
71
84
valerio.bozzolan marked 3 inline comments as done.
  • implement review tips from @mainframe98 - thanks
  • cover the edge case of a profile picture without attachments
src/applications/files/query/PhabricatorFileAttachmentQuery.php
91

lol... good catch

aklapper added inline comments.
src/applications/files/query/PhabricatorFileAttachmentQuery.php
15–16

PHPDoc needs to be @param array<string> $object_phids

29–30

PHPDoc needs to be @param string $object_phid_type

41

PHPDoc needs to be @param string $object_phid_prefix

62

PHPDoc needs to be @param array<string> $attachment_modes