diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 53af38a33b..2c4c2fcc73 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -1414,11 +1414,33 @@ final class PhabricatorFile extends PhabricatorFileDAO /** * Write the policy edge between this file and some object. + * This method is successful even if the file is already attached. * * @param phid Object PHID to attach to. * @return this */ public function attachToObject($phid) { + self::attachFileToObject($this->getPHID(), $phid); + return $this; + } + + /** + * Write the policy edge between a file and some object. + * This method is successful even if the file is already attached. + * NOTE: Please avoid to use this static method directly. + * Instead, use PhabricatorFile#attachToObject(phid). + * + * @param phid File PHID to attach from. + * @param phid Object PHID to attach to. + * @return void + */ + public static function attachFileToObject($file_phid, $object_phid) { + + // It can be easy to confuse the two arguments. Be strict. + if (phid_get_type($file_phid) !== PhabricatorFileFilePHIDType::TYPECONST) { + throw new Exception(pht('The first argument must be a phid of a file.')); + } + $attachment_table = new PhabricatorFileAttachment(); $attachment_conn = $attachment_table->establishConnection('w'); @@ -1432,14 +1454,12 @@ final class PhabricatorFile extends PhabricatorFileDAO attacherPHID = VALUES(attacherPHID), dateModified = VALUES(dateModified)', $attachment_table, - $phid, - $this->getPHID(), + $object_phid, + $file_phid, PhabricatorFileAttachment::MODE_ATTACH, null, PhabricatorTime::getNow(), PhabricatorTime::getNow()); - - return $this; } diff --git a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php index 21ad929c3f..fa2d08dc23 100644 --- a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php +++ b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php @@ -277,6 +277,70 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase { pht('Attached Thumbnail Visibility')); } + public function testFileVisibilityManually() { + $author = $this->generateNewTestUser(); + $viewer = $this->generateNewTestUser(); + $author_and_viewer = array($author, $viewer); + + $engine = new PhabricatorTestStorageEngine(); + $params = array( + 'name' => 'test.dat', + 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, + 'authorPHID' => $author->getPHID(), + 'storageEngines' => array( + $engine, + ), + ); + + $data = Filesystem::readRandomCharacters(64); + $file = PhabricatorFile::newFromFileData($data, $params); + + // Create an object. + $object = ManiphestTask::initializeNewTask($author) + ->setTitle(pht('Test Task')) + ->setViewPolicy(PhabricatorPolicies::getMostOpenPolicy()) + ->save(); + + // Test file's visibility before attachment. + $this->assertEqual( + array( + true, + false, + ), + $this->canViewFile($author_and_viewer, $file), + pht('File Visibility Before Being Attached')); + + // Manually attach. + $file->attachToObject($object->getPHID()); + + // Test the referenced file's visibility. + $this->assertEqual( + array( + true, + true, + ), + $this->canViewFile($author_and_viewer, $file), + pht('File Visibility After Being Attached')); + + // Try again. This should not explode. + $file->attachToObject($object->getPHID()); + + // Try again with this low-level. Again, this should not explode. + PhabricatorFile::attachFileToObject($file->getPHID(), $object->getPHID()); + + // Try again but using the wrong low-level usage. + $is_wrong_usage = false; + try { + PhabricatorFile::attachFileToObject($object->getPHID(), $file->getPHID()); + } catch (Throwable $e) { + $is_wrong_usage = true; + } + $this->assertEqual( + true, + $is_wrong_usage, + pht('Check Attach Low-Level Validation')); + } + private function canViewFile(array $users, PhabricatorFile $file) { $results = array(); foreach ($users as $user) { diff --git a/src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php index 5e6f63c5c4..9b21484a3f 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php @@ -27,14 +27,31 @@ final class ManiphestTaskCoverImageTransaction return; } + // 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(); + $object->setProperty('cover.filePHID', $file->getPHID()); $object->setProperty('cover.thumbnailPHID', $xform->getPHID()); } + public function applyExternalEffects($object, $value) { + // If the File has a Cover Image, attach that as side-effect. + // Otherwise, the Cover Image may be invisible to participants. + $file_phid = $object->getProperty('cover.filePHID'); + if ($file_phid) { + PhabricatorFile::attachFileToObject($file_phid, $object->getPHID()); + } + } + public function getTitle() { $old = $this->getOldValue(); $new = $this->getNewValue(); diff --git a/src/applications/project/controller/PhabricatorProjectCoverController.php b/src/applications/project/controller/PhabricatorProjectCoverController.php index 98f6c1c995..98bf6168a9 100644 --- a/src/applications/project/controller/PhabricatorProjectCoverController.php +++ b/src/applications/project/controller/PhabricatorProjectCoverController.php @@ -35,6 +35,7 @@ final class PhabricatorProjectCoverController $xactions = array(); + // Set the new Cover Image. $xactions[] = id(new ManiphestTransaction()) ->setTransactionType(ManiphestTaskCoverImageTransaction::TRANSACTIONTYPE) ->setNewValue($file->getPHID());