From 731b45d8184773c563f48dc5006841cc179d1983 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Jun 2019 08:00:07 -0700 Subject: [PATCH 1/7] In "bin/repository reparse", continue on "PhabricatorWorkerPermanentFailureException" Summary: Fixes T13315. See that task for discussion. Without `--background`, we currently treat this as a catastrophic failure, but it's relatively routine for some repository states. We can safely continue reparsing other steps. Test Plan: Ran `bin/repository reparse --all X --message` with commits faked to all be unreachable. Got warnings instead of a hard failure on first problem. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13315 Differential Revision: https://secure.phabricator.com/D20588 --- ...torRepositoryManagementReparseWorkflow.php | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php index e0aa8b4c5b..ebd6edff97 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php @@ -252,12 +252,24 @@ final class PhabricatorRepositoryManagementReparseWorkflow ); foreach ($classes as $class) { - PhabricatorWorker::scheduleTask( - $class, - $spec, - array( - 'priority' => PhabricatorWorker::PRIORITY_IMPORT, - )); + try { + PhabricatorWorker::scheduleTask( + $class, + $spec, + array( + 'priority' => PhabricatorWorker::PRIORITY_IMPORT, + )); + } catch (PhabricatorWorkerPermanentFailureException $ex) { + // See T13315. We expect some reparse steps to occasionally raise + // permanent failures: for example, because they are no longer + // reachable. This is a routine condition, not a catastrophic + // failure, so let the user know something happened but continue + // reparsing any remaining commits. + echo tsprintf( + "** %s ** %s\n", + pht('WARN'), + $ex->getMessage()); + } } $progress->update(1); From 1dd62f79cedb61066264bc57eb3d3b36f639656f Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Jun 2019 07:27:57 -0700 Subject: [PATCH 2/7] Fix more "msort()" vs "msortv()" callsites Summary: See . The recent changes to turn `msort()` on a vector an error have smoked out a few more of these mistakes. These cases do not meaningfully rely on sort stability so there's no real bug being fixed, but we'd still prefer `msortv()`. Test Plan: Viewed MFA and External Account settings panels. Did a `git grep 'msort(' | grep -i vector` for any more obvious callsites, but none turned up. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20587 --- src/applications/auth/engine/PhabricatorAuthSessionEngine.php | 2 +- .../settings/panel/PhabricatorExternalAccountsSettingsPanel.php | 2 +- .../settings/panel/PhabricatorMultiFactorSettingsPanel.php | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index e4956335a0..7d73cb194d 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -485,7 +485,7 @@ final class PhabricatorAuthSessionEngine extends Phobject { // change the order of prompts for users, but the alternative is that the // Settings panel order disagrees with the prompt order, which seems more // disruptive. - $factors = msort($factors, 'newSortVector'); + $factors = msortv($factors, 'newSortVector'); // If the account has no associated multi-factor auth, just issue a token // without putting the session into high security mode. This is generally diff --git a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php index 60389e1590..d20782d2b5 100644 --- a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php @@ -94,7 +94,7 @@ final class PhabricatorExternalAccountsSettingsPanel ->setViewer($viewer) ->withIsEnabled(true) ->execute(); - $configs = msort($configs, 'getSortVector'); + $configs = msortv($configs, 'getSortVector'); $account_map = mgroup($accounts, 'getProviderConfigPHID'); diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php index 09193f3c96..abbb88c0a5 100644 --- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php @@ -54,7 +54,7 @@ final class PhabricatorMultiFactorSettingsPanel ->setViewer($viewer) ->withUserPHIDs(array($user->getPHID())) ->execute(); - $factors = msort($factors, 'newSortVector'); + $factors = msortv($factors, 'newSortVector'); $rows = array(); $rowc = array(); From 6219d30f6b0b75e1ce36acd7dbeba1b379841e8c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Jun 2019 14:56:10 -0700 Subject: [PATCH 3/7] Recommend dumping database backups with "--compress --output X" instead of "| gzip > X" Summary: Fixes T13304. Shell pipes and redirects do not have robust behavior when errors occur. We provide "--compress" and "--output" flags as robust alternatives, but do not currently recommend their use. - Recommend their use, since their error handling behavior is more robust in the face of issues like full disks. - If "--compress" is provided but won't work because the "zlib" extension is missing, raise an explicit error. I believe this extension is very common and this error should be rare. If that turns out to be untrue, we could take another look at this. - Also, verify some flag usage sooner so we can exit with an error faster if you mistype a "bin/storage dump" command. Test Plan: Read documentation, hit affected error cases, did a dump and spot-checked that it came out sane looking. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13304 Differential Revision: https://secure.phabricator.com/D20572 --- .../configuration/configuring_backups.diviner | 2 +- ...abricatorStorageManagementDumpWorkflow.php | 87 ++++++++++--------- 2 files changed, 46 insertions(+), 43 deletions(-) diff --git a/src/docs/user/configuration/configuring_backups.diviner b/src/docs/user/configuration/configuring_backups.diviner index 9c283c5722..0e851d01ac 100644 --- a/src/docs/user/configuration/configuring_backups.diviner +++ b/src/docs/user/configuration/configuring_backups.diviner @@ -42,7 +42,7 @@ but will only dump databases Phabricator owns. Since most of this data is compressible, it may be helpful to run it through gzip prior to storage. For example: - phabricator/ $ ./bin/storage dump | gzip > backup.sql.gz + phabricator/ $ ./bin/storage dump --compress --output backup.sql.gz Then store the backup somewhere safe, like in a box buried under an old tree stump. No one will ever think to look for it there. diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php index d5f1da9ecf..c3b9a32327 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php @@ -51,24 +51,59 @@ final class PhabricatorStorageManagementDumpWorkflow } public function didExecute(PhutilArgumentParser $args) { + $output_file = $args->getArg('output'); + $is_compress = $args->getArg('compress'); + $is_overwrite = $args->getArg('overwrite'); + + if ($is_compress) { + if ($output_file === null) { + throw new PhutilArgumentUsageException( + pht( + 'The "--compress" flag can only be used alongside "--output".')); + } + + if (!function_exists('gzopen')) { + throw new PhutilArgumentUsageException( + pht( + 'The "--compress" flag requires the PHP "zlib" extension, but '. + 'that extension is not available. Install the extension or '. + 'omit the "--compress" option.')); + } + } + + if ($is_overwrite) { + if ($output_file === null) { + throw new PhutilArgumentUsageException( + pht( + 'The "--overwrite" flag can only be used alongside "--output".')); + } + } + + if ($output_file !== null) { + if (Filesystem::pathExists($output_file)) { + if (!$is_overwrite) { + throw new PhutilArgumentUsageException( + pht( + 'Output file "%s" already exists. Use "--overwrite" '. + 'to overwrite.', + $output_file)); + } + } + } + $api = $this->getSingleAPI(); $patches = $this->getPatches(); - $console = PhutilConsole::getConsole(); - $with_indexes = !$args->getArg('no-indexes'); $applied = $api->getAppliedPatches(); if ($applied === null) { - $namespace = $api->getNamespace(); - $console->writeErr( + throw new PhutilArgumentUsageException( pht( - '**Storage Not Initialized**: There is no database storage '. - 'initialized in this storage namespace ("%s"). Use '. - '**%s** to initialize storage.', - $namespace, - './bin/storage upgrade')); - return 1; + 'There is no database storage initialized in the current storage '. + 'namespace ("%s"). Use "bin/storage upgrade" to initialize '. + 'storage or use "--namespace" to choose a different namespace.', + $api->getNamespace())); } $ref = $api->getRef(); @@ -141,38 +176,6 @@ final class PhabricatorStorageManagementDumpWorkflow } } - $output_file = $args->getArg('output'); - $is_compress = $args->getArg('compress'); - $is_overwrite = $args->getArg('overwrite'); - - if ($is_compress) { - if ($output_file === null) { - throw new PhutilArgumentUsageException( - pht( - 'The "--compress" flag can only be used alongside "--output".')); - } - } - - if ($is_overwrite) { - if ($output_file === null) { - throw new PhutilArgumentUsageException( - pht( - 'The "--overwrite" flag can only be used alongside "--output".')); - } - } - - if ($output_file !== null) { - if (Filesystem::pathExists($output_file)) { - if (!$is_overwrite) { - throw new PhutilArgumentUsageException( - pht( - 'Output file "%s" already exists. Use "--overwrite" '. - 'to overwrite.', - $output_file)); - } - } - } - $argv = array(); $argv[] = '--hex-blob'; $argv[] = '--single-transaction'; From 75382864995609b2471815ab8ced9632cc18f482 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Jun 2019 07:22:15 -0700 Subject: [PATCH 4/7] Fix missing link targets for "View Object" header buttons in HTML email Summary: See . I added "View Task" / "View Commit" buttons recently but the logic for generating URIs isn't quite right. Fix it up. Test Plan: - Commented on a task. - Used `bin/mail show-outbound --id ... --dump-html > out.html` to dump the HTML. - Previewed the HTML in a browser. - This time, actually clicked the button to go to the task. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20586 --- .../audit/editor/PhabricatorAuditEditor.php | 3 +- .../editor/DifferentialTransactionEditor.php | 6 ++-- .../editor/ManiphestTransactionEditor.php | 3 +- ...habricatorApplicationTransactionEditor.php | 32 ++++++++++++++++--- 4 files changed, 34 insertions(+), 10 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 3a6859549d..7fdb586f56 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -485,7 +485,8 @@ final class PhabricatorAuditEditor return $phids; } - protected function getObjectLinkButtonLabelForMail() { + protected function getObjectLinkButtonLabelForMail( + PhabricatorLiskDAO $object) { return pht('View Commit'); } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 090592046b..88f13ff828 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -601,7 +601,8 @@ final class DifferentialTransactionEditor return $xactions; } - protected function getObjectLinkButtonLabelForMail() { + protected function getObjectLinkButtonLabelForMail( + PhabricatorLiskDAO $object) { return pht('View Revision'); } @@ -614,8 +615,7 @@ final class DifferentialTransactionEditor $body = id(new PhabricatorMetaMTAMailBody()) ->setViewer($viewer); - $revision_uri = $object->getURI(); - $revision_uri = PhabricatorEnv::getProductionURI($revision_uri); + $revision_uri = $this->getObjectLinkButtonURIForMail($object); $new_uri = $revision_uri.'/new/'; $this->addHeadersAndCommentsToMailBody( diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index f7e5e25a40..6255903eff 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -206,7 +206,8 @@ final class ManiphestTransactionEditor ->setSubject("T{$id}: {$title}"); } - protected function getObjectLinkButtonLabelForMail() { + protected function getObjectLinkButtonLabelForMail( + PhabricatorLiskDAO $object) { return pht('View Task'); } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index bcce014669..da5e4d3634 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -3423,17 +3423,39 @@ abstract class PhabricatorApplicationTransactionEditor ->setContextObject($object); $button_label = $this->getObjectLinkButtonLabelForMail($object); + $button_uri = $this->getObjectLinkButtonURIForMail($object); + + $this->addHeadersAndCommentsToMailBody( + $body, + $xactions, + $button_label, + $button_uri); - $this->addHeadersAndCommentsToMailBody($body, $xactions, $button_label); $this->addCustomFieldsToMailBody($body, $object, $xactions); return $body; } - protected function getObjectLinkButtonLabelForMail() { + protected function getObjectLinkButtonLabelForMail( + PhabricatorLiskDAO $object) { return null; } + protected function getObjectLinkButtonURIForMail( + PhabricatorLiskDAO $object) { + + // Most objects define a "getURI()" method which does what we want, but + // this isn't formally part of an interface at time of writing. Try to + // call the method, expecting an exception if it does not exist. + + try { + $uri = $object->getURI(); + return PhabricatorEnv::getProductionURI($uri); + } catch (Exception $ex) { + return null; + } + } + /** * @task mail */ @@ -3455,7 +3477,7 @@ abstract class PhabricatorApplicationTransactionEditor PhabricatorMetaMTAMailBody $body, array $xactions, $object_label = null, - $object_href = null) { + $object_uri = null) { // First, remove transactions which shouldn't be rendered in mail. foreach ($xactions as $key => $xaction) { @@ -3521,7 +3543,7 @@ abstract class PhabricatorApplicationTransactionEditor $headers_html = phutil_implode_html(phutil_tag('br'), $headers_html); $header_button = null; - if ($object_label !== null) { + if ($object_label !== null && $object_uri !== null) { $button_style = array( 'text-decoration: none;', 'padding: 4px 8px;', @@ -3540,7 +3562,7 @@ abstract class PhabricatorApplicationTransactionEditor 'a', array( 'style' => implode(' ', $button_style), - 'href' => $object_href, + 'href' => $object_uri, ), $object_label); } From dda5c13ef549252e67592e046bcafaa966342ad7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 13 Jun 2019 16:28:30 -0700 Subject: [PATCH 5/7] Parse "shallow" frames in the Git "upload-pack" wire protocol parser Summary: Fixes T13309. If you void the warranty on a repository on disk and turn it into a shallow clone, Phabricator currently can't serve it. We don't support hosting shallow working copies, but we should still parse and proxy the protocol rather than breaking in a mysterious way. Test Plan: - Created a shallow working copy with `mv X X.full; git clone --depth Y file://.../X.full X` in the storage directory on disk. - Cloned it with `git clone `. - Deleted all the refs inside it so the wire only has "shallow" frames; cloned it. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13309 Differential Revision: https://secure.phabricator.com/D20577 --- .../DiffusionGitUploadPackWireProtocol.php | 106 +++++++++++------- .../protocol/DiffusionGitWireProtocolRef.php | 13 ++- 2 files changed, 80 insertions(+), 39 deletions(-) diff --git a/src/applications/diffusion/protocol/DiffusionGitUploadPackWireProtocol.php b/src/applications/diffusion/protocol/DiffusionGitUploadPackWireProtocol.php index 451ad74c9e..724c1c83b2 100644 --- a/src/applications/diffusion/protocol/DiffusionGitUploadPackWireProtocol.php +++ b/src/applications/diffusion/protocol/DiffusionGitUploadPackWireProtocol.php @@ -187,61 +187,76 @@ final class DiffusionGitUploadPackWireProtocol // \0 // // ... + // + // See T13309. The end of this list (which may be empty if a repository + // does not have any refs) has a list of zero or more of these: + // + // shallow + // + // These entries are present if the repository is a shallow clone + // which was made with the "--depth" flag. + // + // Note that "shallow" frames do not advertise capabilities, and if + // a repository has only "shallow" frames, capabilities are never + // advertised. $bytes = $frame['bytes']; $matches = array(); if ($is_first) { - $ok = preg_match( - '('. - '^'. - '(?P[0-9a-f]{40})'. - ' '. - '(?P[^\0\n]+)'. - '\0'. - '(?P[^\n]+)'. - '\n'. - '\z'. - ')', - $bytes, - $matches); - if (!$ok) { + $capabilities_pattern = '\0(?P[^\n]+)'; + } else { + $capabilities_pattern = ''; + } + + $ok = preg_match( + '('. + '^'. + '(?:'. + '(?P[0-9a-f]{40}) (?P[^\0\n]+)'.$capabilities_pattern. + '|'. + 'shallow (?P[0-9a-f]{40})'. + ')'. + '\n'. + '\z'. + ')', + $bytes, + $matches); + + if (!$ok) { + if ($is_first) { throw new Exception( pht( 'Unexpected "git upload-pack" initial protocol frame: expected '. - '" \0\n", got "%s".', + '" \0\n", or '. + '"shallow \n", got "%s".', $bytes)); - } - } else { - $ok = preg_match( - '('. - '^'. - '(?P[0-9a-f]{40})'. - ' '. - '(?P[^\0\n]+)'. - '\n'. - '\z'. - ')', - $bytes, - $matches); - if (!$ok) { + } else { throw new Exception( pht( 'Unexpected "git upload-pack" protocol frame: expected '. - '" \n", got "%s".', + '" \n", or "shallow \n", got "%s".', $bytes)); } } - $hash = $matches['hash']; - $name = $matches['name']; + if (isset($matches['shallow'])) { + $name = null; + $hash = $matches['shallow']; + $is_shallow = true; + } else { + $name = $matches['name']; + $hash = $matches['hash']; + $is_shallow = false; + } - if ($is_first) { + if (isset($matches['capabilities'])) { $capabilities = $matches['capabilities']; } $refs[] = array( 'hash' => $hash, 'name' => $name, + 'shallow' => $is_shallow, ); } @@ -252,10 +267,16 @@ final class DiffusionGitUploadPackWireProtocol ->setCapabilities($capabilities); foreach ($refs as $ref) { - $ref_list->addRef( - id(new DiffusionGitWireProtocolRef()) - ->setName($ref['name']) - ->setHash($ref['hash'])); + $wire_ref = id(new DiffusionGitWireProtocolRef()) + ->setHash($ref['hash']); + + if ($ref['shallow']) { + $wire_ref->setIsShallow(true); + } else { + $wire_ref->setName($ref['name']); + } + + $ref_list->addRef($wire_ref); } // TODO: Here, we have a structured list of refs. In a future change, @@ -275,10 +296,19 @@ final class DiffusionGitUploadPackWireProtocol // a little surprising, but is consistent with the native behavior of the // protocol. + // Likewise, we don't send back any capabilities if we're sending only + // "shallow" frames. + $output = array(); $is_first = true; foreach ($refs as $ref) { - if ($is_first) { + $is_shallow = $ref->getIsShallow(); + + if ($is_shallow) { + $result = sprintf( + "shallow %s\n", + $ref->getHash()); + } else if ($is_first) { $result = sprintf( "%s %s\0%s\n", $ref->getHash(), diff --git a/src/applications/diffusion/protocol/DiffusionGitWireProtocolRef.php b/src/applications/diffusion/protocol/DiffusionGitWireProtocolRef.php index bf5238c219..bd1672317a 100644 --- a/src/applications/diffusion/protocol/DiffusionGitWireProtocolRef.php +++ b/src/applications/diffusion/protocol/DiffusionGitWireProtocolRef.php @@ -5,6 +5,7 @@ final class DiffusionGitWireProtocolRef private $name; private $hash; + private $isShallow; public function setName($name) { $this->name = $name; @@ -24,9 +25,19 @@ final class DiffusionGitWireProtocolRef return $this->hash; } + public function setIsShallow($is_shallow) { + $this->isShallow = $is_shallow; + return $this; + } + + public function getIsShallow() { + return $this->isShallow; + } + public function newSortVector() { return id(new PhutilSortVector()) - ->addString($this->getName()); + ->addInt((int)$this->getIsShallow()) + ->addString((string)$this->getName()); } } From dcf3ca8e04508a1f8e290428cecebaecab8502f5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Jun 2019 18:42:42 -0700 Subject: [PATCH 6/7] When a user clicks a navigation link in a dialog, close the dialog Summary: Ref T13302. Currently, if you enable Quicksand (by clicking "Persistent Chat"), open a dialog with links in it (like "Create Subtask" with multiple available subtypes), and then follow a navigation link, the page content reloads behind the dialog but the dialog stays in the foreground. Fix this by closing dialogs when users click navigation links inside them. Test Plan: With Quicksand enabled and disabled, clicked a subtask type in the "Create Subtask" dialog. - Before, Quicksand Disabled: Dialog stays on screen, then navigation occurs. - After, Quicksand Disabled: Dialog vanishes, then navigation occurs. - Before, Quicksand Enabled: Dialog stays on screen, navigation occurs behind it. - After, Quicksand Enabled: Dialog vanishes, then navigation occurs. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13302 Differential Revision: https://secure.phabricator.com/D20573 --- resources/celerity/map.php | 28 +++++------ .../rsrc/externals/javelin/lib/Workflow.js | 48 +++++++++++++------ 2 files changed, 48 insertions(+), 28 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 49d8f0df0b..02d4a67281 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => 'af983028', - 'core.pkg.js' => 'ee320ca2', + 'core.pkg.js' => 'f39ebda8', 'differential.pkg.css' => '8d8360fb', 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', @@ -253,7 +253,7 @@ return array( 'rsrc/externals/javelin/lib/URI.js' => '2e255291', 'rsrc/externals/javelin/lib/Vector.js' => 'e9c80beb', 'rsrc/externals/javelin/lib/WebSocket.js' => 'fdc13e4e', - 'rsrc/externals/javelin/lib/Workflow.js' => '958e9045', + 'rsrc/externals/javelin/lib/Workflow.js' => 'e9c6d3c7', 'rsrc/externals/javelin/lib/__tests__/Cookie.js' => 'ca686f71', 'rsrc/externals/javelin/lib/__tests__/DOM.js' => '4566e249', 'rsrc/externals/javelin/lib/__tests__/JSON.js' => '710377ae', @@ -752,7 +752,7 @@ return array( 'javelin-workboard-header' => '111bfd2d', 'javelin-workboard-header-template' => 'ebe83a6b', 'javelin-workboard-order-template' => '03e8891f', - 'javelin-workflow' => '958e9045', + 'javelin-workflow' => 'e9c6d3c7', 'maniphest-report-css' => '3d53188b', 'maniphest-task-edit-css' => '272daa84', 'maniphest-task-summary-css' => '61d1667e', @@ -1712,17 +1712,6 @@ return array( 'javelin-stratcom', 'javelin-vector', ), - '958e9045' => array( - 'javelin-stratcom', - 'javelin-request', - 'javelin-dom', - 'javelin-vector', - 'javelin-install', - 'javelin-util', - 'javelin-mask', - 'javelin-uri', - 'javelin-routable', - ), '9623adc1' => array( 'javelin-behavior', 'javelin-stratcom', @@ -2107,6 +2096,17 @@ return array( 'phabricator-title', 'phabricator-favicon', ), + 'e9c6d3c7' => array( + 'javelin-stratcom', + 'javelin-request', + 'javelin-dom', + 'javelin-vector', + 'javelin-install', + 'javelin-util', + 'javelin-mask', + 'javelin-uri', + 'javelin-routable', + ), 'e9c80beb' => array( 'javelin-install', 'javelin-event', diff --git a/webroot/rsrc/externals/javelin/lib/Workflow.js b/webroot/rsrc/externals/javelin/lib/Workflow.js index d767a58780..5c8d74a54d 100644 --- a/webroot/rsrc/externals/javelin/lib/Workflow.js +++ b/webroot/rsrc/externals/javelin/lib/Workflow.js @@ -75,6 +75,7 @@ JX.install('Workflow', { var workflow = new JX.Workflow(link.href); return workflow; }, + _push : function(workflow) { JX.Mask.show(); JX.Workflow._stack.push(workflow); @@ -85,8 +86,36 @@ JX.install('Workflow', { dialog._destroy(); JX.Mask.hide(); }, - disable : function() { - JX.Workflow._disabled = true; + _onlink: function(event) { + // See T13302. When a user clicks a link in a dialog and that link + // triggers a navigation event, we want to close the dialog as though + // they had pressed a button. + + // When Quicksand is enabled, this is particularly relevant because + // the dialog will stay in the foreground while the page content changes + // in the background if we do not dismiss the dialog. + + // If this is a Command-Click, the link will open in a new window. + var is_command = !!event.getRawEvent().metaKey; + if (is_command) { + return; + } + + var link = event.getNode('tag:a'); + + // If the link is an anchor, or does not go anywhere, ignore the event. + var href = '' + link.href; + if (!href.length || href[0] === '#') { + return; + } + + // This link will open in a new window. + if (link.target === '_blank') { + return; + } + + // Close the dialog. + JX.Workflow._pop(); }, _onbutton : function(event) { @@ -94,10 +123,6 @@ JX.install('Workflow', { return; } - if (JX.Workflow._disabled) { - return; - } - // Get the button (which is sometimes actually another tag, like an ) // which triggered the event. In particular, this makes sure we get the // right node if there is a