From ee4245098b9f4cb996501631d94de07840db2620 Mon Sep 17 00:00:00 2001 From: Valerio Bozzolan Date: Fri, 5 Sep 2025 10:28:25 +0200 Subject: [PATCH] Transformed Pictures: Make them non-orphan Summary: Before this change, most image transforms were just orphan as default. This change introduces a new internal file API: PhabricatorFileTransform#executeTransformExplicit(PhabricatorFile) This new API can be used to easily create the transform, plus, create a PhabricatorTransformedFile, so that the transformed file is related to the original one. If the original file is a builtin, we don't persist that relationship, just like before it was orphan. Just as additional caution to don't clog the "transforms" UX. This new API pratically brings D25475 to an higher level. So, this new API is adopted by these calls of executeTransform(): - ManiphestTaskCoverImageTransaction (D25475) - just a refactor - PhabricatorPeopleProfilePictureController - 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. After this change, you can more easily find and delete your profile image. Note that we already closed T16074 so this kind of deletions do not cause 404 errors in the UX of profile pictures. Closes T15814 Ref T15407 Ref T15768 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. We have not tested Merchant. Nobody knows what Merchant is. Reviewers: O1 Blessed Committers, aklapper Reviewed By: O1 Blessed Committers, aklapper Subscribers: aklapper, tobiaswiese, Matthew, Cigaryno Tags: #files Maniphest Tasks: T15814, T15407, T15768, T16074 Differential Revision: https://we.phorge.it/D25622 --- .../PhabricatorAuthRegisterController.php | 2 +- .../ConpherenceRoomPictureController.php | 2 +- ...sionRepositoryProfilePictureController.php | 2 +- .../transform/PhabricatorFileTransform.php | 24 +++++++++++++++++++ .../ManiphestTaskCoverImageTransaction.php | 9 +------ ...bricatorPeopleProfilePictureController.php | 2 +- .../PhameBlogProfilePictureController.php | 2 +- .../PhortuneMerchantPictureController.php | 2 +- ...habricatorProjectEditPictureController.php | 2 +- 9 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorAuthRegisterController.php b/src/applications/auth/controller/PhabricatorAuthRegisterController.php index fdd0bc81ce..9b1eafd415 100644 --- a/src/applications/auth/controller/PhabricatorAuthRegisterController.php +++ b/src/applications/auth/controller/PhabricatorAuthRegisterController.php @@ -710,7 +710,7 @@ final class PhabricatorAuthRegisterController $xform = PhabricatorFileTransform::getTransformByKey( PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE); - return $xform->executeTransform($file); + return $xform->executeTransformExplicit($file); } protected function renderError($message) { diff --git a/src/applications/conpherence/controller/ConpherenceRoomPictureController.php b/src/applications/conpherence/controller/ConpherenceRoomPictureController.php index 5be8331610..47a0576d70 100644 --- a/src/applications/conpherence/controller/ConpherenceRoomPictureController.php +++ b/src/applications/conpherence/controller/ConpherenceRoomPictureController.php @@ -66,7 +66,7 @@ final class ConpherenceRoomPictureController } else { $xform = PhabricatorFileTransform::getTransformByKey( PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE); - $xformed = $xform->executeTransform($file); + $xformed = $xform->executeTransformExplicit($file); } } diff --git a/src/applications/diffusion/controller/DiffusionRepositoryProfilePictureController.php b/src/applications/diffusion/controller/DiffusionRepositoryProfilePictureController.php index 59a49def6f..d2e7700c98 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryProfilePictureController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryProfilePictureController.php @@ -67,7 +67,7 @@ final class DiffusionRepositoryProfilePictureController } else { $xform = PhabricatorFileTransform::getTransformByKey( PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE); - $xformed = $xform->executeTransform($file); + $xformed = $xform->executeTransformExplicit($file); } } diff --git a/src/applications/files/transform/PhabricatorFileTransform.php b/src/applications/files/transform/PhabricatorFileTransform.php index a60a3d4b37..acad6d6d61 100644 --- a/src/applications/files/transform/PhabricatorFileTransform.php +++ b/src/applications/files/transform/PhabricatorFileTransform.php @@ -27,6 +27,30 @@ abstract class PhabricatorFileTransform extends Phobject { return $this->getDefaultTransform($file); } + /** + * Wrapper of executeTransform() that also persists the relationship + * between the original file and the transform, if it makes sense to do so. + */ + public function executeTransformExplicit(PhabricatorFile $file) { + // This can be NULL. + $xform = $this->executeTransform($file); + + // Connect the original file to its transform, if any. + // Skip transforms that are derived from a builtin as cautionary measure: + // - Builtins may have a lot of transforms. We don't know if the UX scales. + // Example page: /file/transforms/1/ + // - Tracking builtins gives unclear benefits. + if ($xform && !$file->isBuiltin()) { + id(new PhabricatorTransformedFile()) + ->setOriginalPHID($file->getPHID()) + ->setTransformedPHID($xform->getPHID()) + ->setTransform($this->getTransformKey()) + ->save(); + } + + return $xform; + } + public static function getAllTransforms() { return id(new PhutilClassMapQuery()) ->setAncestorClass(__CLASS__) diff --git a/src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php index 0c057f54f5..7d8d986e29 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php @@ -30,14 +30,7 @@ final class ManiphestTaskCoverImageTransaction // Generate an image transformation, usually smaller (orphan now). $xform_key = PhabricatorFileThumbnailTransform::TRANSFORM_WORKCARD; $xform = PhabricatorFileTransform::getTransformByKey($xform_key) - ->executeTransform($file); - - // Make that image transformation non-orphan. - id(new PhabricatorTransformedFile()) - ->setOriginalPHID($file_phid) - ->setTransformedPHID($xform->getPHID()) - ->setTransform($xform_key) - ->save(); + ->executeTransformExplicit($file); $object->setProperty('cover.filePHID', $file->getPHID()); $object->setProperty('cover.thumbnailPHID', $xform->getPHID()); diff --git a/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php b/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php index e5d04254e6..fb2c5c6a62 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php @@ -76,7 +76,7 @@ final class PhabricatorPeopleProfilePictureController $e_file = pht('Not Supported'); $errors[] = $supported_formats_message; } else { - $xformed = $xform->executeTransform($file); + $xformed = $xform->executeTransformExplicit($file); } } diff --git a/src/applications/phame/controller/blog/PhameBlogProfilePictureController.php b/src/applications/phame/controller/blog/PhameBlogProfilePictureController.php index 91daa784cd..1f0488cce4 100644 --- a/src/applications/phame/controller/blog/PhameBlogProfilePictureController.php +++ b/src/applications/phame/controller/blog/PhameBlogProfilePictureController.php @@ -66,7 +66,7 @@ final class PhameBlogProfilePictureController } else { $xform = PhabricatorFileTransform::getTransformByKey( PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE); - $xformed = $xform->executeTransform($file); + $xformed = $xform->executeTransformExplicit($file); } } diff --git a/src/applications/phortune/controller/merchant/PhortuneMerchantPictureController.php b/src/applications/phortune/controller/merchant/PhortuneMerchantPictureController.php index a3b9ada7af..86268280a0 100644 --- a/src/applications/phortune/controller/merchant/PhortuneMerchantPictureController.php +++ b/src/applications/phortune/controller/merchant/PhortuneMerchantPictureController.php @@ -56,7 +56,7 @@ final class PhortuneMerchantPictureController } else { $xform = PhabricatorFileTransform::getTransformByKey( PhabricatorFileThumbnailTransform::TRANSFORM_PROFILE); - $xformed = $xform->executeTransform($file); + $xformed = $xform->executeTransformExplicit($file); } } diff --git a/src/applications/project/controller/PhabricatorProjectEditPictureController.php b/src/applications/project/controller/PhabricatorProjectEditPictureController.php index b41439e8d3..886e506bae 100644 --- a/src/applications/project/controller/PhabricatorProjectEditPictureController.php +++ b/src/applications/project/controller/PhabricatorProjectEditPictureController.php @@ -75,7 +75,7 @@ final class PhabricatorProjectEditPictureController $e_file = pht('Not Supported'); $errors[] = $supported_formats_message; } else { - $xformed = $xform->executeTransform($file); + $xformed = $xform->executeTransformExplicit($file); } } -- 2.51.2