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_2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 2055
Build 2055: 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
106
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
106

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

71

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

This isn't important, it just seems like this is a nice in-context place to leave this information :)

src/applications/files/query/PhabricatorFileAttachmentQuery.php
17

It doesn't matter for this case, because this is a final class, but this should probably be $this instead of self: https://docs.phpdoc.org/guide/references/phpdoc/types.html - scrolling down to the item for self, it looks like if the type can be extended, $this should be used instead of self because otherwise the inferred type will be the parent class instead of the child class, unless the method is going to be overridden.

valerio.bozzolan marked an inline comment as done.

improve PHPDoc thanks to amybones

and add more PHPDoc

src/applications/files/query/PhabricatorFileAttachmentQuery.php
17

Used $this - thanksss