From 991bbb6242dd6e661a8b41adb107f94e71d41b47 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 May 2013 18:12:52 -0700 Subject: [PATCH] Provide "builtin" files and use them to fix Pholio when files are deleted Summary: Fixes T3132. Currently, if a user deletes a file which is present in a mock, that mock throws an exception when loading. If the file is also the cover photo, the mock list throws an exception as well. In other applications, we can sometimes deal with this (a sub-object vanishing) by implicitly hiding the parent object (for example, we can just vanish feed stories about objects which no longer exist). We can also sometimes deal with it by preventing sub-objects from being directly deleted. However, neither approach is reasonable in this case. If we vanish the whole mock, we'll lose all the comments and it will generally be weird. Vanishing a mock is a big deal compared to vanishing a feed story. We'll also need to load more data on the list view to prevent showing a mock on the list view and then realizing we need to vanish it on the detail view (because all of its images have been deleted). We permit total deletion of files to allow users to recover from accidentally uploading sensitive files (which has happened a few times), and I'm hesitant to remove this capability because I think it serves a real need, so we can't prevent sub-objects from being deleted. So we're left in a relatively unique situation. To solve this, I've added a "builtin" mechanism, which allows us to expose some resource we ship with as a PhabricatorFile. Then we just swap it out in place of the original file and proceed forward normally, as though nothing happened. The user sees a placeholder image instead of the original, but everything else works reasonably and this seems like a fairly acceptable outcome. I believe we can use this mechanism to simplify some other code too, like default profile pictures. Test Plan: Deleted a Pholio mock cover image's file. Implemented change, saw functional Pholio again with beautiful life-affirming "?" art replacing soul-shattering exception. Reviewers: btrahan, chad Reviewed By: chad CC: aran Maniphest Tasks: T3132 Differential Revision: https://secure.phabricator.com/D5870 --- resources/builtin/missing.png | Bin 0 -> 3781 bytes .../files/query/PhabricatorFileQuery.php | 56 ++++++++++-- .../files/storage/PhabricatorFile.php | 83 ++++++++++++++++++ .../phid/PhabricatorPHIDConstants.php | 3 + .../pholio/query/PholioMockQuery.php | 12 ++- 5 files changed, 147 insertions(+), 7 deletions(-) create mode 100644 resources/builtin/missing.png diff --git a/resources/builtin/missing.png b/resources/builtin/missing.png new file mode 100644 index 0000000000000000000000000000000000000000..f301d02626b7fac1ea1122bc5957865fbe877ffb GIT binary patch literal 3781 zcmd^Cc~DbH8h;!jB{=JhmV+|5F|azLkb~1?IV41w3O!@d!7p+()(};LL8x{IONm{^6^x>et=h_v^2J z->+Zyi}!GIS-E2U3J8K$QobeAAV^aRK^h8OP0(|*(6ta$>yRBjh$lA?5z+ZD^=im}m!klx+kN0I*?%j*4K1asA_5r}F!(GU!s2pJYD9VfHyp7;gGlF>z~;S&

sN0)$~B=sYYQ zgHtbQ(U400eN#63y|nS=cy|2xJSAlQYTE+-A|2%w>eLVGgqB%ly^XaYY z&t;?DEt<$$Iu@HH@fLiUOUJTpNnb1o2CGi(Z&JP31@xnSeJ@v|XwJ3Z$W`2;BaP=w~Uakt|atGhOMI;i3r>@zwYd(ozxEy1n_64opyLYcp zBR|uxpgGv`hbJM4g>`jx2?+_R;f<}Wt!Zg#_!Z~sUQS$*qpGS>!injZ2mAW^ZpHmU zHSJPUJ@cwQEh03V*63f-U`zPr=G@@KNOYTSPs#=mo6JFyQOk65bMwu6T2pg0U0XvZ zf24b7_Q>RnT<(~+yR?VV+}vE+J}@~ssW`+^_=f4~<%dgMvmd>B^{S=C_bNFa94+Hx zW2a3fUyMlKf~T#m?ZKTNyJW7eu77k8o32ebN|Fc0Kw931#Fv^pJw4;%;zlE*gMv0c zGPJ9Ro*wJ&?yjyTs~Gamis*#V9CsPU)KuSI7#4=V*B(;zx8uipYd4yfJZcf91qEUK$_y^V9;g%>74oxb@o~>6I1Mdt!+o+UJdunRxC+=6(DL$OZaZZf>rVx5MEHoRw8dEi-ftfV(#(ENur_3bxF!i?T+nZg-Y= znXYY*(b6_B$o4A`iN#`}aO_QE)_G((wy3qgnA_>*?*5ujaQVBV^vEg7X_KBPKL5w- z*RPk9Y(qw0)}2e;xk)p0W*Lw!VcI&a{_^0=%#2JjHz|l_^puD9o*n)YK&Q^hb3=;O zTMWFob22WU6B>#T)4$86eZA%@u@3OuL^%y&>K6i$#9_*##xb|M&PP8{JI+oRYWVHh zvu^4!_wjgdb3u=unkfB>(a3Ti)6a~`rrs)>UO$k54DIWY`~?Sca2G%uNCrDA9ZC$g zf=ZT}y6XU->@A=S>?fI?o?dJ(5c}le3a>3!u=5mO_ss*+%p4w9D5~siwo8xTbxN9) zYZ7YYHM6s`q|8;L;X^AxI0(iII^GxRUv+o?qge)s3yXhv`=5X~bIaAs)3m@6@KrbV4NOc#@WKKE!Rq}Af+S$p zVOwv{E4sX_#>-Da9MTm2Z*%5WGS7k>T=+aRv8=$aJfygF*2G1+HRZ&(RE=!E zze&lzv2RPtX%pr0ZM!etJLV69O1xO|-1R3-ZG(xtsp1!J5;~_{vKXI@ZBa`KQ|?#r zBqUlj2kJZ6&lTs7CDe4?zrWL7c2XzGY&}?JbhKfTGuS6@^7He@Dpz0Goy%skjf{-y z8yeiG)Jc*FnBh#ov!qitJ%+aiT3T98jvgOK-s}#vD-?;Elt`w>kgsLinv7hY#qG|( z!c>!5|7b~xXwWa4cBQ#@wy9AHX&LJcJvg9a8_eP4HwGi6`=gctUd*d1GdLEhiH0<= z*R#YIFVQOP{Q;Y0)9S?At68f%hn0*jcX4q!O4^t%y!r)*mn8*XM}4`Wp`pWp_UF$D z_V$>R@9d9h+uW^w`t`~WO*d-z07$=|yn{C`f+k8vvhubA`K$@e$P4~XSs gj7>~W#z9)po)tOY=$|-atp2g2>~JIJll)`;4P8LfEC2ui literal 0 HcmV?d00001 diff --git a/src/applications/files/query/PhabricatorFileQuery.php b/src/applications/files/query/PhabricatorFileQuery.php index d5731ac13b..5a110afaf9 100644 --- a/src/applications/files/query/PhabricatorFileQuery.php +++ b/src/applications/files/query/PhabricatorFileQuery.php @@ -7,6 +7,7 @@ final class PhabricatorFileQuery private $phids; private $authorPHIDs; private $explicitUploads; + private $transforms; public function withIDs(array $ids) { $this->ids = $ids; @@ -23,6 +24,21 @@ final class PhabricatorFileQuery return $this; } + public function withTransforms(array $specs) { + foreach ($specs as $spec) { + if (!is_array($spec) || + empty($spec['originalPHID']) || + empty($spec['transform'])) { + throw new Exception( + "Transform specification must be a dictionary with keys ". + "'originalPHID' and 'transform'!"); + } + } + + $this->transforms = $specs; + return $this; + } + public function showOnlyExplicitUploads($explicit_uploads) { $this->explicitUploads = $explicit_uploads; return $this; @@ -34,8 +50,9 @@ final class PhabricatorFileQuery $data = queryfx_all( $conn_r, - 'SELECT * FROM %T f %Q %Q %Q', + 'SELECT * FROM %T f %Q %Q %Q %Q', $table->getTableName(), + $this->buildJoinClause($conn_r), $this->buildWhereClause($conn_r), $this->buildOrderClause($conn_r), $this->buildLimitClause($conn_r)); @@ -43,6 +60,19 @@ final class PhabricatorFileQuery return $table->loadAllFromArray($data); } + private function buildJoinClause(AphrontDatabaseConnection $conn_r) { + $joins = array(); + + if ($this->transforms) { + $joins[] = qsprintf( + $conn_r, + 'JOIN %T t ON t.transformedPHID = f.phid', + id(new PhabricatorTransformedFile())->getTableName()); + } + + return implode(' ', $joins); + } + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); @@ -51,31 +81,47 @@ final class PhabricatorFileQuery if ($this->ids) { $where[] = qsprintf( $conn_r, - 'id IN (%Ld)', + 'f.id IN (%Ld)', $this->ids); } if ($this->phids) { $where[] = qsprintf( $conn_r, - 'phid IN (%Ls)', + 'f.phid IN (%Ls)', $this->phids); } if ($this->authorPHIDs) { $where[] = qsprintf( $conn_r, - 'authorPHID IN (%Ls)', + 'f.authorPHID IN (%Ls)', $this->authorPHIDs); } if ($this->explicitUploads) { $where[] = qsprintf( $conn_r, - 'isExplicitUpload = true'); + 'f.isExplicitUpload = true'); + } + + if ($this->transforms) { + $clauses = array(); + foreach ($this->transforms as $transform) { + $clauses[] = qsprintf( + $conn_r, + '(t.originalPHID = %s AND t.transform = %s)', + $transform['originalPHID'], + $transform['transform']); + } + $where[] = qsprintf($conn_r, '(%Q)', implode(') OR (', $clauses)); } return $this->formatWhereClause($where); } + protected function getPagingColumn() { + return 'f.id'; + } + } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 888d49234c..732ede3de2 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -682,6 +682,89 @@ final class PhabricatorFile extends PhabricatorFileDAO } + /** + * Load (or build) the {@class:PhabricatorFile} objects for builtin file + * resources. The builtin mechanism allows files shipped with Phabricator + * to be treated like normal files so that APIs do not need to special case + * things like default images or deleted files. + * + * Builtins are located in `resources/builtin/` and identified by their + * name. + * + * @param PhabricatorUser Viewing user. + * @param list List of builtin file names. + * @return dict Dictionary of named builtins. + */ + public static function loadBuiltins(PhabricatorUser $user, array $names) { + $specs = array(); + foreach ($names as $name) { + $specs[] = array( + 'originalPHID' => PhabricatorPHIDConstants::PHID_VOID, + 'transform' => 'builtin:'.$name, + ); + } + + $files = id(new PhabricatorFileQuery()) + ->setViewer($user) + ->withTransforms($specs) + ->execute(); + + $files = mpull($files, null, 'getName'); + + $root = dirname(phutil_get_library_root('phabricator')); + $root = $root.'/resources/builtin/'; + + $build = array(); + foreach ($names as $name) { + if (isset($files[$name])) { + continue; + } + + // This is just a sanity check to prevent loading arbitrary files. + if (basename($name) != $name) { + throw new Exception("Invalid builtin name '{$name}'!"); + } + + $path = $root.$name; + + if (!Filesystem::pathExists($path)) { + throw new Exception("Builtin '{$path}' does not exist!"); + } + + $data = Filesystem::readFile($path); + $params = array( + 'name' => $name, + 'ttl' => time() + (60 * 60 * 24 * 7), + ); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file = PhabricatorFile::newFromFileData($data, $params); + $xform = id(new PhabricatorTransformedFile()) + ->setOriginalPHID(PhabricatorPHIDConstants::PHID_VOID) + ->setTransform('builtin:'.$name) + ->setTransformedPHID($file->getPHID()) + ->save(); + unset($unguarded); + + $files[$name] = $file; + } + + return $files; + } + + + /** + * Convenience wrapper for @{method:loadBuiltins}. + * + * @param PhabricatorUser Viewing user. + * @param string Single builtin name to load. + * @return PhabricatorFile Corresponding builtin file. + */ + public static function loadBuiltin(PhabricatorUser $user, $name) { + return idx(self::loadBuiltins($user, array($name)), $name); + } + + /* -( PhabricatorPolicyInterface Implementation )-------------------------- */ diff --git a/src/applications/phid/PhabricatorPHIDConstants.php b/src/applications/phid/PhabricatorPHIDConstants.php index 6ed089f91f..5860ca1c5b 100644 --- a/src/applications/phid/PhabricatorPHIDConstants.php +++ b/src/applications/phid/PhabricatorPHIDConstants.php @@ -44,4 +44,7 @@ final class PhabricatorPHIDConstants { const PHID_TYPE_XCMT = 'XCMT'; const PHID_TYPE_XUSR = 'XUSR'; + const PHID_TYPE_VOID = 'VOID'; + const PHID_VOID = 'PHID-VOID-00000000000000000000'; + } diff --git a/src/applications/pholio/query/PholioMockQuery.php b/src/applications/pholio/query/PholioMockQuery.php index d222fed29b..d5ce886eba 100644 --- a/src/applications/pholio/query/PholioMockQuery.php +++ b/src/applications/pholio/query/PholioMockQuery.php @@ -129,7 +129,11 @@ final class PholioMockQuery } foreach ($all_images as $image) { - $image->attachFile($all_files[$image->getFilePHID()]); + $file = idx($all_files, $image->getFilePHID()); + if (!$file) { + $file = PhabricatorFile::loadBuiltin($this->getViewer(), 'missing.png'); + } + $image->attachFile($file); if ($this->needInlineComments) { $inlines = idx($all_images, $image->getID(), array()); $image->attachInlineComments($inlines); @@ -151,7 +155,11 @@ final class PholioMockQuery $cover_file_phids), null, 'getPHID'); foreach ($mocks as $mock) { - $mock->attachCoverFile($cover_files[$mock->getCoverPHID()]); + $file = idx($cover_files, $mock->getCoverPHID()); + if (!$file) { + $file = PhabricatorFile::loadBuiltin($this->getViewer(), 'missing.png'); + } + $mock->attachCoverFile($file); } }