Page MenuHomePhorge

Transformed Pictures: make them non-orphan
Needs ReviewPublic

Authored by valerio.bozzolan on Mon, May 6, 16:10.
Tags
None
Referenced Files
F2196874: D25622.1716228070.diff
Sun, May 19, 18:01
Unknown Object (File)
Mon, May 13, 20:02
Unknown Object (File)
Mon, May 13, 00:05
Unknown Object (File)
Sun, May 12, 10:59
Unknown Object (File)
Sat, May 11, 00:56
Unknown Object (File)
Fri, May 10, 11:35
Unknown Object (File)
Fri, May 10, 00:17
Unknown Object (File)
Fri, May 10, 00:17

Details

Summary

Before this change, any image transform is just orphan as default.

This change introduces a new file API called:

PhabricatorFileTransform#executeTransformExplicit(PhabricatorFile)

This new API creates the transform, plus, it creates a PhabricatorTransformedFile,
so that the transformed file is related to the original one.
In this phase, if the original file is a builtin, we don't persist that relationship,
just like before. Just as additional caution.

This new API executeTransformExplicit() is adopted by well-known calls of executeTransform():

  • PhabricatorPeopleProfilePictureController
  • ManiphestTaskCoverImageTransaction (D25475) - just a refactor
  • PhabricatorProjectEditPictureController
  • PhameBlogProfilePictureController
  • PhortuneMerchantPictureController

For example, in PhabricatorPeopleProfilePictureController:

Now the original picture is mentioned from the cropped thumbnail.
So, you can now at least "more easily" find the original picture and delete that,
starting from that thumbnail.

Deletion considerations:

Before and after this change, you were able to delete the parent file.
It was just a bit tricky to find it out. For example, from the Manage Profile
you see only the mention to the thumbnail, not the original file.

After this change, the parent file is related to transforms, so the phd
daemon will delete them as well as usual for transforms.

This is just a preliminary improvement on the deletion workflow reported
from T15407. Any other improvement is welcome.

WARNING SPECIFIC TO PROFILE IMAGES DELETION:

It seems profile images are handled differently, maybe for performance reasons.
So, if you delete your profile image, you may see 404 errors in the UX, instead
of seeing a default image. This is true until the user uploads a new profile image.

Ref T15407
Ref T15814

Test Plan

Test profile images:

  • Top Menu > Manage > Edit Profile Picture > Use Picture
    • still works
  • Top Menu > Manage > Edit Profile Picture > Upload New Picture
    • still works
    • Left menu > Manage > click on first Mentioned File
    • click on View Transforms - you now see the parent file
    • delete the parent file, with phd daemon running: now the transforms is deleted as well

Do the same test plan for Diffusion.

Do the same test for Phame blob.

Do the same test for a Project tag image.

Test again the same test plan of D25475. Still works.

TODO: do the same test for Merchant. I don't know how to do it.

Diff Detail

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

Event Timeline

cover all known usages, but set as stub

valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan retitled this revision from People Profile Picture: make image transform non-orphan to Transformed Pictures: make them non-orphan.Thu, May 9, 10:45
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the summary of this revision. (Show Details)

PhabricatorFileTransform: add null check