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(); +});