From 9d3a722eb1fedeb448f567a0c33e2f224bd3fbbe Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 8 Mar 2018 05:29:24 -0800 Subject: [PATCH] When proxying an "{image ...}" image fails, show the user an error message Summary: Depends on D19192. Ref T4190. Ref T13101. Instead of directly including the proxy endpoint with ``, emit a placeholder and use AJAX to make the request. If the proxy fetch fails, replace the placeholder with an error message. This isn't the most polished implementation imaginable, but it's much less mysterious about errors. Test Plan: Used `{image ...}` for valid and invalid images, got images and useful error messages respectively. Maniphest Tasks: T13101, T4190 Differential Revision: https://secure.phabricator.com/D19193 --- resources/celerity/map.php | 12 ++- src/__phutil_library_map__.php | 2 + .../PhabricatorFileImageProxyController.php | 85 +++++++++++++------ .../markup/PhabricatorImageRemarkupRule.php | 13 ++- .../markup/view/PHUIRemarkupImageView.php | 67 +++++++++++++++ webroot/rsrc/css/core/remarkup.css | 7 ++ .../js/core/behavior-remarkup-load-image.js | 45 ++++++++++ 7 files changed, 193 insertions(+), 38 deletions(-) create mode 100644 src/infrastructure/markup/view/PHUIRemarkupImageView.php create mode 100644 webroot/rsrc/js/core/behavior-remarkup-load-image.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 699d3288e5..48e8362194 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => '2fa91e14', + 'core.pkg.css' => 'c218ed53', 'core.pkg.js' => '32bb68e9', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '113e692c', @@ -114,7 +114,7 @@ return array( 'rsrc/css/application/tokens/tokens.css' => '3d0f239e', 'rsrc/css/application/uiexample/example.css' => '528b19de', 'rsrc/css/core/core.css' => '62fa3ace', - 'rsrc/css/core/remarkup.css' => 'cad18339', + 'rsrc/css/core/remarkup.css' => '97dc3523', 'rsrc/css/core/syntax.css' => 'cae95e89', 'rsrc/css/core/z-index.css' => '9d8f7c4b', 'rsrc/css/diviner/diviner-shared.css' => '896f1d43', @@ -504,6 +504,7 @@ return array( 'rsrc/js/core/behavior-read-only-warning.js' => 'ba158207', 'rsrc/js/core/behavior-redirect.js' => '0213259f', 'rsrc/js/core/behavior-refresh-csrf.js' => 'ab2f381b', + 'rsrc/js/core/behavior-remarkup-load-image.js' => '040fce04', 'rsrc/js/core/behavior-remarkup-preview.js' => '4b700e9e', 'rsrc/js/core/behavior-reorder-applications.js' => '76b9fc3e', 'rsrc/js/core/behavior-reveal-content.js' => '60821bc7', @@ -692,6 +693,7 @@ return array( 'javelin-behavior-releeph-preview-branch' => 'b2b4fbaf', 'javelin-behavior-releeph-request-state-change' => 'a0b57eb8', 'javelin-behavior-releeph-request-typeahead' => 'de2e896f', + 'javelin-behavior-remarkup-load-image' => '040fce04', 'javelin-behavior-remarkup-preview' => '4b700e9e', 'javelin-behavior-reorder-applications' => '76b9fc3e', 'javelin-behavior-reorder-columns' => 'e1d25dfb', @@ -800,7 +802,7 @@ return array( 'phabricator-object-selector-css' => '85ee8ce6', 'phabricator-phtize' => 'd254d646', 'phabricator-prefab' => '77b0ae28', - 'phabricator-remarkup-css' => 'cad18339', + 'phabricator-remarkup-css' => '97dc3523', 'phabricator-search-results-css' => '505dd8cf', 'phabricator-shaped-request' => '7cbe244b', 'phabricator-slowvote-css' => 'a94b7230', @@ -940,6 +942,10 @@ return array( 'javelin-behavior', 'javelin-uri', ), + '040fce04' => array( + 'javelin-behavior', + 'javelin-request', + ), '04b2ae03' => array( 'javelin-install', 'javelin-util', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6cc7450820..69c0801f9a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1883,6 +1883,7 @@ phutil_register_library_map(array( 'PHUIPropertyGroupView' => 'view/phui/PHUIPropertyGroupView.php', 'PHUIPropertyListExample' => 'applications/uiexample/examples/PHUIPropertyListExample.php', 'PHUIPropertyListView' => 'view/phui/PHUIPropertyListView.php', + 'PHUIRemarkupImageView' => 'infrastructure/markup/view/PHUIRemarkupImageView.php', 'PHUIRemarkupPreviewPanel' => 'view/phui/PHUIRemarkupPreviewPanel.php', 'PHUIRemarkupView' => 'infrastructure/markup/view/PHUIRemarkupView.php', 'PHUISegmentBarSegmentView' => 'view/phui/PHUISegmentBarSegmentView.php', @@ -7287,6 +7288,7 @@ phutil_register_library_map(array( 'PHUIPropertyGroupView' => 'AphrontTagView', 'PHUIPropertyListExample' => 'PhabricatorUIExample', 'PHUIPropertyListView' => 'AphrontView', + 'PHUIRemarkupImageView' => 'AphrontView', 'PHUIRemarkupPreviewPanel' => 'AphrontTagView', 'PHUIRemarkupView' => 'AphrontView', 'PHUISegmentBarSegmentView' => 'AphrontTagView', diff --git a/src/applications/files/controller/PhabricatorFileImageProxyController.php b/src/applications/files/controller/PhabricatorFileImageProxyController.php index 44573aea46..b1d94c0801 100644 --- a/src/applications/files/controller/PhabricatorFileImageProxyController.php +++ b/src/applications/files/controller/PhabricatorFileImageProxyController.php @@ -44,6 +44,7 @@ final class PhabricatorFileImageProxyController ->setTTL($ttl); $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $save_request = false; // Cache missed so we'll need to validate and download the image try { // Rate limit outbound fetches to make this mechanism less useful for @@ -75,44 +76,74 @@ final class PhabricatorFileImageProxyController $file->save(); } - $external_request->setIsSuccessful(true) - ->setFilePHID($file->getPHID()) - ->save(); - unset($unguarded); - return $this->getExternalResponse($external_request); + $external_request + ->setIsSuccessful(1) + ->setFilePHID($file->getPHID()); + + $save_request = true; } catch (HTTPFutureHTTPResponseStatus $status) { - $external_request->setIsSuccessful(false) + $external_request + ->setIsSuccessful(0) ->setResponseMessage($status->getMessage()) ->save(); - return $this->getExternalResponse($external_request); + + $save_request = true; } catch (Exception $ex) { // Not actually saving the request in this case $external_request->setResponseMessage($ex->getMessage()); - return $this->getExternalResponse($external_request); } + + if ($save_request) { + try { + $external_request->save(); + } catch (AphrontDuplicateKeyQueryException $ex) { + // We may have raced against another identical request. If we did, + // just throw our result away and use the winner's result. + $external_request = $external_request->loadOneWhere( + 'uriIndex = %s', + PhabricatorHash::digestForIndex($img_uri)); + if (!$external_request) { + throw new Exception( + pht( + 'Hit duplicate key collision when saving proxied image, but '. + 'failed to load duplicate row (for URI "%s").', + $img_uri)); + } + } + } + + unset($unguarded); + + + return $this->getExternalResponse($external_request); } private function getExternalResponse( PhabricatorFileExternalRequest $request) { - if ($request->getIsSuccessful()) { - $file = id(new PhabricatorFileQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs(array($request->getFilePHID())) - ->executeOne(); - if (!$file) { - throw new Exception(pht( - 'The underlying file does not exist, but the cached request was '. - 'successful. This likely means the file record was manually deleted '. - 'by an administrator.')); - } - return id(new AphrontRedirectResponse()) - ->setIsExternal(true) - ->setURI($file->getViewURI()); - } else { - throw new Exception(pht( - "The request to get the external file from '%s' was unsuccessful:\n %s", - $request->getURI(), - $request->getResponseMessage())); + if (!$request->getIsSuccessful()) { + throw new Exception( + pht( + 'Request to "%s" failed: %s', + $request->getURI(), + $request->getResponseMessage())); } + + $file = id(new PhabricatorFileQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs(array($request->getFilePHID())) + ->executeOne(); + if (!$file) { + throw new Exception( + pht( + 'The underlying file does not exist, but the cached request was '. + 'successful. This likely means the file record was manually '. + 'deleted by an administrator.')); + } + + return id(new AphrontAjaxResponse()) + ->setContent( + array( + 'imageURI' => $file->getViewURI(), + )); } } diff --git a/src/applications/files/markup/PhabricatorImageRemarkupRule.php b/src/applications/files/markup/PhabricatorImageRemarkupRule.php index 69e9d6bcf1..c07ab9d496 100644 --- a/src/applications/files/markup/PhabricatorImageRemarkupRule.php +++ b/src/applications/files/markup/PhabricatorImageRemarkupRule.php @@ -100,14 +100,11 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule { $src_uri = id(new PhutilURI('/file/imageproxy/')) ->setQueryParam('uri', $args['uri']); - $img = $this->newTag( - 'img', - array( - 'src' => $src_uri, - 'alt' => $args['alt'], - 'width' => $args['width'], - 'height' => $args['height'], - )); + $img = id(new PHUIRemarkupImageView()) + ->setURI($src_uri) + ->setAlt($args['alt']) + ->setWidth($args['width']) + ->setHeight($args['height']); $engine->overwriteStoredText($image['token'], $img); } diff --git a/src/infrastructure/markup/view/PHUIRemarkupImageView.php b/src/infrastructure/markup/view/PHUIRemarkupImageView.php new file mode 100644 index 0000000000..0a73efb83b --- /dev/null +++ b/src/infrastructure/markup/view/PHUIRemarkupImageView.php @@ -0,0 +1,67 @@ +uri = $uri; + return $this; + } + + public function getURI() { + return $this->uri; + } + + public function setWidth($width) { + $this->width = $width; + return $this; + } + + public function getWidth() { + return $this->width; + } + + public function setHeight($height) { + $this->height = $height; + return $this; + } + + public function getHeight() { + return $this->height; + } + + public function setAlt($alt) { + $this->alt = $alt; + return $this; + } + + public function getAlt() { + return $this->alt; + } + + public function render() { + $id = celerity_generate_unique_node_id(); + + Javelin::initBehavior( + 'remarkup-load-image', + array( + 'uri' => (string)$this->uri, + 'imageID' => $id, + )); + + return phutil_tag( + 'img', + array( + 'id' => $id, + 'width' => $this->getWidth(), + 'height' => $this->getHeight(), + 'alt' => $this->getAlt(), + )); + } + +} diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css index 8750598780..a51c1ce1d9 100644 --- a/webroot/rsrc/css/core/remarkup.css +++ b/webroot/rsrc/css/core/remarkup.css @@ -472,6 +472,13 @@ video.phabricator-media { margin: .5em 1em 0; } +.phabricator-remarkup-image-error { + border: 1px solid {$redborder}; + background: {$sh-redbackground}; + padding: 8px 12px; + color: {$darkgreytext}; +} + .phabricator-remarkup-embed-image { display: inline-block; border: 3px solid white; diff --git a/webroot/rsrc/js/core/behavior-remarkup-load-image.js b/webroot/rsrc/js/core/behavior-remarkup-load-image.js new file mode 100644 index 0000000000..a13f3b0d78 --- /dev/null +++ b/webroot/rsrc/js/core/behavior-remarkup-load-image.js @@ -0,0 +1,45 @@ +/** + * @provides javelin-behavior-remarkup-load-image + * @requires javelin-behavior + * javelin-request + */ + +JX.behavior('remarkup-load-image', function(config) { + + function get_node() { + try { + return JX.$(config.imageID); + } catch (ex) { + return null; + } + } + + function onload(r) { + var node = get_node(); + if (!node) { + return; + } + + node.src = r.imageURI; + } + + function onerror(r) { + var node = get_node(); + if (!node) { + return; + } + + var error = JX.$N( + 'div', + { + className: 'phabricator-remarkup-image-error' + }, + r.info); + + JX.DOM.replace(node, error); + } + + var request = new JX.Request(config.uri, onload); + request.listen('error', onerror); + request.send(); +});