From e5a16819031e4dd1a7914f7bc471a442ba70706a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 May 2019 08:02:39 -0700 Subject: [PATCH 01/33] Render timezone names more readably, with spaces rather than underscores ("America/Los Angeles", not "America/Los_Angeles"). Summary: See downstream . Currently, timezones are rendered with their raw internal names (like `America/Los_Angeles`) which include underscores. Replacing underscores with spaces is a more human-readable (and perhaps meaningfully better for things like screen readers, although this is pure speculation). There's some vague argument against this, like "administrators may need to set a raw internal value in `phabricator.timezone` and this could mislead them", but we already give a pretty good error message if you do this and could improve hinting if necessary. Test Plan: Viewed timezone list in {nav Settings} and the timezone "reconcile" dialog, saw a more-readable "Los Angeles". Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20559 --- .../PhabricatorSettingsTimezoneController.php | 3 ++- .../settings/setting/PhabricatorTimezoneSetting.php | 9 ++++++++- src/infrastructure/time/PhabricatorTime.php | 10 ++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php b/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php index 6a0ba19d03..8ae2704161 100644 --- a/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php +++ b/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php @@ -20,7 +20,8 @@ final class PhabricatorSettingsTimezoneController $zone = new DateTimeZone($identifier); $offset = -($zone->getOffset($now) / 60); if ($offset == $client_offset) { - $options[$identifier] = $identifier; + $name = PhabricatorTime::getTimezoneDisplayName($identifier); + $options[$identifier] = $name; } } diff --git a/src/applications/settings/setting/PhabricatorTimezoneSetting.php b/src/applications/settings/setting/PhabricatorTimezoneSetting.php index 52fce77428..207ad95f62 100644 --- a/src/applications/settings/setting/PhabricatorTimezoneSetting.php +++ b/src/applications/settings/setting/PhabricatorTimezoneSetting.php @@ -81,9 +81,16 @@ final class PhabricatorTimezoneSetting } sort($group); + + $group_map = array(); + foreach ($group as $identifier) { + $name = PhabricatorTime::getTimezoneDisplayName($identifier); + $group_map[$identifier] = $name; + } + $option_groups[] = array( 'label' => $label, - 'options' => array_fuse($group), + 'options' => $group_map, ); } diff --git a/src/infrastructure/time/PhabricatorTime.php b/src/infrastructure/time/PhabricatorTime.php index b221285d13..67378ca8df 100644 --- a/src/infrastructure/time/PhabricatorTime.php +++ b/src/infrastructure/time/PhabricatorTime.php @@ -78,4 +78,14 @@ final class PhabricatorTime extends Phobject { return $datetime; } + public static function getTimezoneDisplayName($raw_identifier) { + + // Internal identifiers have names like "America/Los_Angeles", but this is + // just an implementation detail and we can render them in a more human + // readable format with spaces. + $name = str_replace('_', ' ', $raw_identifier); + + return $name; + } + } From 87472048077550e92be66c33d7c71b14704a2b9f Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 May 2019 08:15:08 -0700 Subject: [PATCH 02/33] Clean up "phabricator.timezone" configuration instructions a little bit Summary: These instructions are fairly old and can be a little fancier and more clear in the context of modern Phabricator. Drop the reference to "HPHP", link the actual timezone list, wordsmith a little. Test Plan: d( O_o )b Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20560 --- .../option/PhabricatorCoreConfigOptions.php | 31 +++++++++++-------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/applications/config/option/PhabricatorCoreConfigOptions.php b/src/applications/config/option/PhabricatorCoreConfigOptions.php index 48f6f24491..10f3e75b62 100644 --- a/src/applications/config/option/PhabricatorCoreConfigOptions.php +++ b/src/applications/config/option/PhabricatorCoreConfigOptions.php @@ -54,6 +54,20 @@ production instance were both in operation. EOREMARKUP )); + $timezone_description = $this->deformat(pht(<<newOption('phabricator.base-uri', 'string', null) @@ -96,14 +110,7 @@ EOREMARKUP $this->newOption('phabricator.timezone', 'string', null) ->setSummary( pht('The timezone Phabricator should use.')) - ->setDescription( - pht( - "PHP requires that you set a timezone in your php.ini before ". - "using date functions, or it will emit a warning. If this isn't ". - "possible (for instance, because you are using HPHP) you can set ". - "some valid constant for %s here and Phabricator will set it on ". - "your behalf, silencing the warning.", - 'date_default_timezone_set()')) + ->setDescription($timezone_description) ->addExample('America/New_York', pht('US East (EDT)')) ->addExample('America/Chicago', pht('US Central (CDT)')) ->addExample('America/Boise', pht('US Mountain (MDT)')) @@ -302,12 +309,10 @@ EOREMARKUP if (!$ok) { throw new PhabricatorConfigValidationException( pht( - "Config option '%s' is invalid. The timezone identifier must ". - "be a valid timezone identifier recognized by PHP, like '%s'. "." - You can find a list of valid identifiers here: %s", + 'Config option "%s" is invalid. The timezone identifier must '. + 'be a valid timezone identifier recognized by PHP, like "%s".', $key, - 'America/Los_Angeles', - 'http://php.net/manual/timezones.php')); + 'America/Los_Angeles')); } } } From fb5dec4c036b57c7cb780c690c09c56568c29d50 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 May 2019 12:23:51 -0700 Subject: [PATCH 03/33] Require valid comments to contain at least one non-whitespace character Summary: See downstream . This is very marginal, but we currently allow comments consisting of //only// whitespace. These are probably always mistakes, so treat them like completely empty comments. (We intentionally do not trim leading or trailing whitespace from comments when posting them becuase leading spaces can be used to trigger codeblock formatting.) Test Plan: - Posted empty, nonempty, and whitespace-only comments. - Whitespace-only comments now have the same behavior as truly empty comments (e.g., do not actually generate a transaction). Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20562 --- .../storage/PhabricatorApplicationTransaction.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 9850d66a7f..775d59d4db 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -127,7 +127,19 @@ abstract class PhabricatorApplicationTransaction } public function hasComment() { - return $this->getComment() && strlen($this->getComment()->getContent()); + if (!$this->getComment()) { + return false; + } + + $content = $this->getComment()->getContent(); + + // If the content is empty or consists of only whitespace, don't count + // this as comment. + if (!strlen(trim($content))) { + return false; + } + + return true; } public function getComment() { From 9a32a563f0478a9e3aefd2022b974ad0867964f9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 May 2019 10:11:54 -0700 Subject: [PATCH 04/33] Add a "View Task" button to HTML mail from Maniphest Summary: See downstream . Some time ago, we added a "View Revision" button to Differential mail. This hasn't created any problems and generally seems good / desirable. It isn't trivial to just add everywhere since we need a translation string in each case, but at least add it to Maniphest for now. Going forward, we can fill in more applications as they come up. Test Plan: Used `bin/mail show-outbound --id --dump-html`: {F6470461} Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20561 --- src/applications/audit/editor/PhabricatorAuditEditor.php | 4 ++++ .../differential/editor/DifferentialTransactionEditor.php | 6 +++++- .../maniphest/editor/ManiphestTransactionEditor.php | 4 ++++ .../editor/PhabricatorApplicationTransactionEditor.php | 7 ++++++- 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 7995e2a36d..3a6859549d 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -485,6 +485,10 @@ final class PhabricatorAuditEditor return $phids; } + protected function getObjectLinkButtonLabelForMail() { + return pht('View Commit'); + } + protected function buildMailBody( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index eba641771e..090592046b 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -601,6 +601,10 @@ final class DifferentialTransactionEditor return $xactions; } + protected function getObjectLinkButtonLabelForMail() { + return pht('View Revision'); + } + protected function buildMailBody( PhabricatorLiskDAO $object, array $xactions) { @@ -617,7 +621,7 @@ final class DifferentialTransactionEditor $this->addHeadersAndCommentsToMailBody( $body, $xactions, - pht('View Revision'), + $this->getObjectLinkButtonLabelForMail($object), $revision_uri); $type_inline = DifferentialTransaction::TYPE_INLINE; diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index fd5bfe0cdc..f7e5e25a40 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -206,6 +206,10 @@ final class ManiphestTransactionEditor ->setSubject("T{$id}: {$title}"); } + protected function getObjectLinkButtonLabelForMail() { + return pht('View Task'); + } + protected function buildMailBody( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index f2ca9883ca..98a52715b7 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -3417,12 +3417,17 @@ abstract class PhabricatorApplicationTransactionEditor ->setViewer($this->requireActor()) ->setContextObject($object); - $this->addHeadersAndCommentsToMailBody($body, $xactions); + $button_label = $this->getObjectLinkButtonLabelForMail($object); + + $this->addHeadersAndCommentsToMailBody($body, $xactions, $button_label); $this->addCustomFieldsToMailBody($body, $object, $xactions); return $body; } + protected function getObjectLinkButtonLabelForMail() { + return null; + } /** * @task mail From 81134d7e7d54ed2c2d42d08413b022eefe87bf03 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 May 2019 15:34:54 -0700 Subject: [PATCH 05/33] After reloading transactions for the recipient while building transaction mail, put them in the input order Summary: Ref T13303. In D20525 I fixed an issue where transaction rendering could use cached values with the wrong viewer by reloading transactions. However, reloading transactions may also reorder them as a side effect, since `withPHIDs(...)` does not imply an order. This can make transaction rendering order in mail wrong/inconsistent. Instead, reorder the transactions before continuing so mail transaction order is consistent. Test Plan: Applied a group of transactions to a task, saw a more consistent rendering order in mail after the change. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13303 Differential Revision: https://secure.phabricator.com/D20563 --- .../editor/PhabricatorApplicationTransactionEditor.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 98a52715b7..55dedb863b 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -3084,6 +3084,11 @@ abstract class PhabricatorApplicationTransactionEditor ->withObjectPHIDs(array($object->getPHID())) ->withPHIDs($xaction_phids) ->execute(); + + // Sort the mail transactions in the input order. + $mail_xactions = mpull($mail_xactions, null, 'getPHID'); + $mail_xactions = array_select_keys($mail_xactions, $xaction_phids); + $mail_xactions = array_values($mail_xactions); } else { $mail_xactions = array(); } From 67f062b0049dcc55558acfa85c6333947d6fcaab Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 May 2019 15:58:55 -0700 Subject: [PATCH 06/33] When triggering audits, count "Accepted" revisions as successfully reviewed Summary: See PHI1118. That issue may describe more than one bug, but the recent ordering changes to the import pipeline likely make this at least part of the problem. Previously, commits would always close associated revisions before we made it to the "publish" step. This is no longer true, so we might be triggering audits on a commit before the associated revision actually closes. Accommodate this by counting a revision in either "Accepted" or "Published (Was Previously Accepted)" as "reviewed". Test Plan: - With commit C affecting paths in package P with "Audit Unreviewed Commits and Commits With No Owner Involvement", associated with revision R, with both R and C authored by the same user, and "R" in the state "Accepted", used `bin/repository reparse --publish ` to republish the commit. - Before change: audit by package P triggered. - After change: audit by package P no longer triggered. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20564 --- ...PhabricatorRepositoryCommitPublishWorker.php | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php index 17074f8cb4..3865050395 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php @@ -223,13 +223,22 @@ final class PhabricatorRepositoryCommitPublishWorker // If auditing is configured to trigger on unreviewed changes, check if // the revision was "Accepted" when it landed. If not, trigger an audit. + + // We may be running before the revision actually closes, so we'll count + // either an "Accepted" or a "Closed, Previously Accepted" revision as + // good enough. + if ($audit_unreviewed) { $commit_unreviewed = true; if ($revision) { - $was_accepted = DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED; - if ($revision->isPublished()) { - if ($revision->getProperty($was_accepted)) { - $commit_unreviewed = false; + if ($revision->isAccepted()) { + $commit_unreviewed = false; + } else { + $was_accepted = DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED; + if ($revision->isPublished()) { + if ($revision->getProperty($was_accepted)) { + $commit_unreviewed = false; + } } } } From 760406762a6fdc5963e8b63df5840d19889c9a03 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 31 May 2019 06:15:28 -0700 Subject: [PATCH 07/33] When a revision is closed, always promote it out of draft Summary: Fixes T13300. Currently, if you create a revision and then immediately land it (either using `--draft` or just beating Harbormaster to the punch) it can be stuck in "Draft" forever. Instead, count landing changes like this as a publishing action. Test Plan: - Used `arc diff --hold` to create a revision, then pushed the commit immediately. - Before change: revision closed, but was stuck in draft. - After change: revision closed and was promoted out of draft. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13300 Differential Revision: https://secure.phabricator.com/D20565 --- .../xaction/DifferentialRevisionCloseTransaction.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php index 6832108dc3..1a6017a02f 100644 --- a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php @@ -44,6 +44,11 @@ final class DifferentialRevisionCloseTransaction $object->setProperty( DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED, $was_accepted); + + // See T13300. When a revision is closed, we promote it out of "Draft" + // immediately. This usually happens when a user creates a draft revision + // and then lands the associated commit before the revision leaves draft. + $object->setShouldBroadcast(true); } protected function validateAction($object, PhabricatorUser $viewer) { From d7890d08b828ce1f5375e1a5b7481b38f4708c8b Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 31 May 2019 07:56:59 -0700 Subject: [PATCH 08/33] Add "bin/herald rule ..." to modify Herald rules from the CLI Summary: Depends on D20566. Ref T13298. See PHI1280. Currently, there's no clean way to disable problematic personal rules. This comes up occasionally and sometimes isn't really the best approach to solving a problem, but is a generally reasonable capability to provide. Allow Herald rules (including personal rules) to be disabled/enabled via `bin/herald rule ... --disable/--enable`. Test Plan: Used the CLI to disable and enable a personal rule. Reviewers: amckinley Reviewed By: amckinley Subscribers: jmeador Maniphest Tasks: T13298 Differential Revision: https://secure.phabricator.com/D20567 --- src/__phutil_library_map__.php | 2 + .../HeraldRuleManagementWorkflow.php | 106 ++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 src/applications/herald/management/HeraldRuleManagementWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d10ec7d420..424d885caa 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1560,6 +1560,7 @@ phutil_register_library_map(array( 'HeraldRuleIndexEngineExtension' => 'applications/herald/engineextension/HeraldRuleIndexEngineExtension.php', 'HeraldRuleListController' => 'applications/herald/controller/HeraldRuleListController.php', 'HeraldRuleListView' => 'applications/herald/view/HeraldRuleListView.php', + 'HeraldRuleManagementWorkflow' => 'applications/herald/management/HeraldRuleManagementWorkflow.php', 'HeraldRuleNameTransaction' => 'applications/herald/xaction/HeraldRuleNameTransaction.php', 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', 'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php', @@ -7381,6 +7382,7 @@ phutil_register_library_map(array( 'HeraldRuleIndexEngineExtension' => 'PhabricatorEdgeIndexEngineExtension', 'HeraldRuleListController' => 'HeraldController', 'HeraldRuleListView' => 'AphrontView', + 'HeraldRuleManagementWorkflow' => 'HeraldManagementWorkflow', 'HeraldRuleNameTransaction' => 'HeraldRuleTransactionType', 'HeraldRulePHIDType' => 'PhabricatorPHIDType', 'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', diff --git a/src/applications/herald/management/HeraldRuleManagementWorkflow.php b/src/applications/herald/management/HeraldRuleManagementWorkflow.php new file mode 100644 index 0000000000..3380b21da7 --- /dev/null +++ b/src/applications/herald/management/HeraldRuleManagementWorkflow.php @@ -0,0 +1,106 @@ +setName('rule') + ->setExamples('**rule** --rule __rule__ --disable') + ->setSynopsis( + pht( + 'Modify a rule, bypassing policies. This workflow can disable '. + 'problematic personal rules.')) + ->setArguments( + array( + array( + 'name' => 'rule', + 'param' => 'rule', + 'help' => pht('Apply changes to this rule.'), + ), + array( + 'name' => 'disable', + 'help' => pht('Disable the rule.'), + ), + array( + 'name' => 'enable', + 'help' => pht('Enable the rule.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $rule_name = $args->getArg('rule'); + if (!strlen($rule_name)) { + throw new PhutilArgumentUsageException( + pht('Specify a rule to edit with "--rule ".')); + } + + if (preg_match('/^H\d+/', $rule_name)) { + $rule_id = substr($rule_name, 1); + } else { + $rule_id = $rule_name; + } + + $rule = id(new HeraldRuleQuery()) + ->setViewer($viewer) + ->withIDs(array($rule_id)) + ->executeOne(); + if (!$rule) { + throw new PhutilArgumentUsageException( + pht( + 'Unable to load Herald rule with ID or monogram "%s".', + $rule_name)); + } + + $is_disable = $args->getArg('disable'); + $is_enable = $args->getArg('enable'); + + $xactions = array(); + + if ($is_disable && $is_enable) { + throw new PhutilArgumentUsageException( + pht( + 'Specify "--enable" or "--disable", but not both.')); + } else if ($is_disable || $is_enable) { + $xactions[] = $rule->getApplicationTransactionTemplate() + ->setTransactionType(HeraldRuleDisableTransaction::TRANSACTIONTYPE) + ->setNewValue($is_disable); + } + + if (!$xactions) { + throw new PhutilArgumentUsageException( + pht( + 'Use flags to specify at least one edit to apply to the '. + 'rule (for example, use "--disable" to disable a rule).')); + } + + $herald_phid = id(new PhabricatorHeraldApplication())->getPHID(); + + $editor = $rule->getApplicationTransactionEditor() + ->setActor($viewer) + ->setActingAsPHID($herald_phid) + ->setContentSource($this->newContentSource()) + ->setContinueOnMissingFields(true) + ->setContinueOnNoEffect(true); + + echo tsprintf( + "%s\n", + pht( + 'Applying changes to %s: %s...', + $rule->getMonogram(), + $rule->getName())); + + $editor->applyTransactions($rule, $xactions); + + echo tsprintf( + "%s\n", + pht('Done.')); + + + return 0; + } + +} From b7aacaa4d30b5ade6bc6c599d687eb3f9b3065d7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Jun 2019 11:58:31 -0700 Subject: [PATCH 09/33] Differentiate Remarkup header sizes more clearly Summary: Ref PHI1275. Previously, see T591. See also T7963. Headers are currently very visually similar to one another, and similar to the text size: {F6485441} I think the design intent was to make it hard to make bad-looking documents, but all the headers end up being very samey. Differentiate the sizes of the headers better so they're much more obvious (e.g., when scrolling through a document) and the different levels are more distinct. This might be a little overboard, but we can always pull it back a bit if it's too much, and I think giving users more control in Remarkup (in cases where it doesn't create some weird syntax/parsing nightmare) is generally a good thing. Test Plan: {F6485447} Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20569 --- resources/celerity/map.php | 10 ++++----- webroot/rsrc/css/core/remarkup.css | 31 +++++++++----------------- webroot/rsrc/css/phui/phui-fontkit.css | 9 -------- 3 files changed, 15 insertions(+), 35 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index d682369496..49d8f0df0b 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '3dc188c0', + 'core.pkg.css' => 'af983028', 'core.pkg.js' => 'ee320ca2', 'differential.pkg.css' => '8d8360fb', 'differential.pkg.js' => '67e02996', @@ -112,7 +112,7 @@ return array( 'rsrc/css/application/tokens/tokens.css' => 'ce5a50bd', 'rsrc/css/application/uiexample/example.css' => 'b4795059', 'rsrc/css/core/core.css' => '1b29ed61', - 'rsrc/css/core/remarkup.css' => '9e627d41', + 'rsrc/css/core/remarkup.css' => 'f06cc20e', 'rsrc/css/core/syntax.css' => '4234f572', 'rsrc/css/core/z-index.css' => '99c0f5eb', 'rsrc/css/diviner/diviner-shared.css' => '4bd263b0', @@ -151,7 +151,7 @@ return array( 'rsrc/css/phui/phui-document-summary.css' => 'b068eed1', 'rsrc/css/phui/phui-document.css' => '52b748a5', 'rsrc/css/phui/phui-feed-story.css' => 'a0c05029', - 'rsrc/css/phui/phui-fontkit.css' => '9b714a5e', + 'rsrc/css/phui/phui-fontkit.css' => '1ec937e5', 'rsrc/css/phui/phui-form-view.css' => '01b796c0', 'rsrc/css/phui/phui-form.css' => '159e2d9c', 'rsrc/css/phui/phui-head-thing.css' => 'd7f293df', @@ -793,7 +793,7 @@ return array( 'phabricator-object-selector-css' => 'ee77366f', 'phabricator-phtize' => '2f1db1ed', 'phabricator-prefab' => '5793d835', - 'phabricator-remarkup-css' => '9e627d41', + 'phabricator-remarkup-css' => 'f06cc20e', 'phabricator-search-results-css' => '9ea70ace', 'phabricator-shaped-request' => 'abf88db8', 'phabricator-slowvote-css' => '1694baed', @@ -838,7 +838,7 @@ return array( 'phui-document-view-pro-css' => 'b9613a10', 'phui-feed-story-css' => 'a0c05029', 'phui-font-icon-base-css' => 'd7994e06', - 'phui-fontkit-css' => '9b714a5e', + 'phui-fontkit-css' => '1ec937e5', 'phui-form-css' => '159e2d9c', 'phui-form-view-css' => '01b796c0', 'phui-head-thing-view-css' => 'd7f293df', diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css index aa5bb8e8c1..3354be9663 100644 --- a/webroot/rsrc/css/core/remarkup.css +++ b/webroot/rsrc/css/core/remarkup.css @@ -174,40 +174,29 @@ list-style-type: none; } -.phabricator-remarkup h1.remarkup-header { - font-size: 24px; - line-height: 1.625em; - margin: 24px 0 4px; -} - .phabricator-remarkup h2.remarkup-header { - font-size: 20px; - line-height: 1.5em; - margin: 20px 0 4px; + font-size: 28px; + margin: 1em 0 0.75em; } .phabricator-remarkup h3.remarkup-header { - font-size: 18px; - line-height: 1.375em; - margin: 20px 0 4px; + font-size: 24px; + margin: 1em 0 0.75em; } .phabricator-remarkup h4.remarkup-header { - font-size: 16px; - line-height: 1.25em; - margin: 12px 0 4px; + font-size: 22px; + margin: 1em 0 0.75em; } .phabricator-remarkup h5.remarkup-header { - font-size: 15px; - line-height: 1.125em; - margin: 8px 0 4px; + font-size: 18px; + margin: 1em 0 0.75em; } .phabricator-remarkup h6.remarkup-header { - font-size: 14px; - line-height: 1em; - margin: 4px 0; + font-size: 16px; + margin: 1em 0 0.75em; } .phabricator-remarkup blockquote { diff --git a/webroot/rsrc/css/phui/phui-fontkit.css b/webroot/rsrc/css/phui/phui-fontkit.css index 43162ea618..1bc56826d7 100644 --- a/webroot/rsrc/css/phui/phui-fontkit.css +++ b/webroot/rsrc/css/phui/phui-fontkit.css @@ -6,15 +6,6 @@ color: {$blacktext}; } -.phui-document-view .phabricator-remarkup .remarkup-header { - margin-bottom: 8px; -} - -.phui-document-view .phabricator-remarkup h2.remarkup-header { - padding: 0 24px 8px 0; - margin: 32px 0 4px; -} - .phui-document-view .phabricator-remarkup .remarkup-header strong { color: #586BE9; } From f1a588c771f648173e46302bf71543e2f31d8adf Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 31 May 2019 07:22:10 -0700 Subject: [PATCH 10/33] Add a basic profiler to Herald transcripts Summary: Ref T13298. Add a simple profiler as a starting point to catch any egregiously expensive rules or conditions. This doesn't profile rule actions, so if "Add subscriber" (or whatever) is outrageously expensive it won't show up on the profile. Right now, actions get evaluated inside the Adapter so they're hard to profile. A future change could likely dig them out without too much trouble. I generally expect actions to be less expensive than conditions. This also can't pin down a //specific// condition being expensive, but if you see that `H123` takes 20s to evaluate you can probably guess that the giant complicated regex is the expensive part. Test Plan: {F6473407} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13298 Differential Revision: https://secure.phabricator.com/D20566 --- .../herald/adapter/HeraldAdapter.php | 8 + .../controller/HeraldTranscriptController.php | 95 +++++++++++ .../herald/engine/HeraldEngine.php | 151 +++++++++++++++++- .../transcript/HeraldObjectTranscript.php | 10 ++ 4 files changed, 261 insertions(+), 3 deletions(-) diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 69f538afcc..cb8545b71d 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -130,6 +130,14 @@ abstract class HeraldAdapter extends Phobject { abstract public function getHeraldName(); + final public function willGetHeraldField($field_key) { + // This method is called during rule evaluation, before we engage the + // Herald profiler. We make sure we have a concrete implementation so time + // spent loading fields out of the classmap is not mistakenly attributed to + // whichever field happens to evaluate first. + $this->requireFieldImplementation($field_key); + } + public function getHeraldField($field_key) { return $this->requireFieldImplementation($field_key) ->getHeraldFieldValue($this->getObject()); diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php index 60dd6dc6ed..09a23408d3 100644 --- a/src/applications/herald/controller/HeraldTranscriptController.php +++ b/src/applications/herald/controller/HeraldTranscriptController.php @@ -77,6 +77,7 @@ final class HeraldTranscriptController extends HeraldController { $this->buildTransactionsTranscriptPanel( $object, $xscript), + $this->buildProfilerTranscriptPanel($xscript), ); } @@ -597,4 +598,98 @@ final class HeraldTranscriptController extends HeraldController { } + private function buildProfilerTranscriptPanel(HeraldTranscript $xscript) { + $viewer = $this->getViewer(); + + $object_xscript = $xscript->getObjectTranscript(); + + $profile = $object_xscript->getProfile(); + + // If this is an older transcript without profiler information, don't + // show anything. + if ($profile === null) { + return null; + } + + $profile = isort($profile, 'elapsed'); + $profile = array_reverse($profile); + + $phids = array(); + foreach ($profile as $frame) { + if ($frame['type'] === 'rule') { + $phids[] = $frame['key']; + } + } + $handles = $viewer->loadHandles($phids); + + $field_map = HeraldField::getAllFields(); + + $rows = array(); + foreach ($profile as $frame) { + $cost = $frame['elapsed']; + $cost = 1000000 * $cost; + $cost = pht('%sus', new PhutilNumber($cost)); + + $type = $frame['type']; + switch ($type) { + case 'rule': + $type_display = pht('Rule'); + break; + case 'field': + $type_display = pht('Field'); + break; + default: + $type_display = $type; + break; + } + + $key = $frame['key']; + switch ($type) { + case 'field': + $field_object = idx($field_map, $key); + if ($field_object) { + $key_display = $field_object->getHeraldFieldName(); + } else { + $key_display = $key; + } + break; + case 'rule': + $key_display = $handles[$key]->renderLink(); + break; + default: + $key_display = $key; + break; + } + + $rows[] = array( + $type_display, + $key_display, + $cost, + pht('%s', new PhutilNumber($frame['count'])), + ); + } + + $table_view = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Type'), + pht('What'), + pht('Cost'), + pht('Count'), + )) + ->setColumnClasses( + array( + null, + 'wide', + 'right', + 'right', + )); + + $box_view = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Profile')) + ->setTable($table_view); + + return $box_view; + } + } diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index 2d96532882..9ca72821e7 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -16,6 +16,9 @@ final class HeraldEngine extends Phobject { private $forbiddenActions = array(); private $skipEffects = array(); + private $profilerStack = array(); + private $profilerFrames = array(); + public function setDryRun($dry_run) { $this->dryRun = $dry_run; return $this; @@ -137,7 +140,8 @@ final class HeraldEngine extends Phobject { ->setName($object->getHeraldName()) ->setType($object->getAdapterContentType()) ->setFields($this->fieldCache) - ->setAppliedTransactionPHIDs($xaction_phids); + ->setAppliedTransactionPHIDs($xaction_phids) + ->setProfile($this->getProfile()); $this->transcript->setObjectTranscript($object_transcript); @@ -329,7 +333,36 @@ final class HeraldEngine extends Phobject { break; } - $match = $this->doesConditionMatch($rule, $condition, $object); + // Here, we're profiling the cost to match the condition value against + // whatever test is configured. Normally, this cost should be very + // small (<<1ms) since it amounts to a single comparison: + // + // [ Task author ][ is any of ][ alice ] + // + // However, it may be expensive in some cases, particularly if you + // write a rule with a very creative regular expression that backtracks + // explosively. + // + // At time of writing, the "Another Herald Rule" field is also + // evaluated inside the matching function. This may be arbitrarily + // expensive (it can prompt us to execute any finite number of other + // Herald rules), although we'll push the profiler stack appropriately + // so we don't count the evaluation time against this rule in the final + // profile. + + $caught = null; + + $this->pushProfilerRule($rule); + try { + $match = $this->doesConditionMatch($rule, $condition, $object); + } catch (Exception $ex) { + $caught = $ex; + } + $this->popProfilerRule($rule); + + if ($caught) { + throw $ex; + } if (!$all && $match) { $reason = pht('Any condition matched.'); @@ -421,7 +454,25 @@ final class HeraldEngine extends Phobject { public function getObjectFieldValue($field) { if (!array_key_exists($field, $this->fieldCache)) { - $this->fieldCache[$field] = $this->object->getHeraldField($field); + $adapter = $this->object; + + $adapter->willGetHeraldField($field); + + $caught = null; + + $this->pushProfilerField($field); + try { + $value = $adapter->getHeraldField($field); + } catch (Exception $ex) { + $caught = $ex; + } + $this->popProfilerField($field); + + if ($caught) { + throw $caught; + } + + $this->fieldCache[$field] = $value; } return $this->fieldCache[$field]; @@ -637,4 +688,98 @@ final class HeraldEngine extends Phobject { return $is_forbidden; } +/* -( Profiler )----------------------------------------------------------- */ + + private function pushProfilerField($field_key) { + return $this->pushProfilerStack('field', $field_key); + } + + private function popProfilerField($field_key) { + return $this->popProfilerStack('field', $field_key); + } + + private function pushProfilerRule(HeraldRule $rule) { + return $this->pushProfilerStack('rule', $rule->getPHID()); + } + + private function popProfilerRule(HeraldRule $rule) { + return $this->popProfilerStack('rule', $rule->getPHID()); + } + + private function pushProfilerStack($type, $key) { + $this->profilerStack[] = array( + 'type' => $type, + 'key' => $key, + 'start' => microtime(true), + ); + + return $this; + } + + private function popProfilerStack($type, $key) { + if (!$this->profilerStack) { + throw new Exception( + pht( + 'Unable to pop profiler stack: profiler stack is empty.')); + } + + $frame = last($this->profilerStack); + if (($frame['type'] !== $type) || ($frame['key'] !== $key)) { + throw new Exception( + pht( + 'Unable to pop profiler stack: expected frame of type "%s" with '. + 'key "%s", but found frame of type "%s" with key "%s".', + $type, + $key, + $frame['type'], + $frame['key'])); + } + + // Accumulate the new timing information into the existing profile. If this + // is the first time we've seen this particular rule or field, we'll + // create a new empty frame first. + + $elapsed = microtime(true) - $frame['start']; + $frame_key = sprintf('%s/%s', $type, $key); + + if (!isset($this->profilerFrames[$frame_key])) { + $current = array( + 'type' => $type, + 'key' => $key, + 'elapsed' => 0, + 'count' => 0, + ); + } else { + $current = $this->profilerFrames[$frame_key]; + } + + $current['elapsed'] += $elapsed; + $current['count']++; + + $this->profilerFrames[$frame_key] = $current; + + array_pop($this->profilerStack); + } + + private function getProfile() { + if ($this->profilerStack) { + $frame = last($this->profilerStack); + $frame_type = $frame['type']; + $frame_key = $frame['key']; + $frame_count = count($this->profilerStack); + + throw new Exception( + pht( + 'Unable to retrieve profile: profiler stack is not empty. The '. + 'stack has %s frame(s); the final frame has type "%s" and key '. + '"%s".', + new PhutilNumber($frame_count), + $frame_type, + $frame_key)); + } + + return array_values($this->profilerFrames); + } + + } diff --git a/src/applications/herald/storage/transcript/HeraldObjectTranscript.php b/src/applications/herald/storage/transcript/HeraldObjectTranscript.php index 9d331b271e..b5f9aa783f 100644 --- a/src/applications/herald/storage/transcript/HeraldObjectTranscript.php +++ b/src/applications/herald/storage/transcript/HeraldObjectTranscript.php @@ -7,6 +7,7 @@ final class HeraldObjectTranscript extends Phobject { protected $name; protected $fields; protected $appliedTransactionPHIDs; + protected $profile; public function setPHID($phid) { $this->phid = $phid; @@ -48,6 +49,15 @@ final class HeraldObjectTranscript extends Phobject { return $this->fields; } + public function setProfile(array $profile) { + $this->profile = $profile; + return $this; + } + + public function getProfile() { + return $this->profile; + } + public function setAppliedTransactionPHIDs($phids) { $this->appliedTransactionPHIDs = $phids; return $this; From 64a95000788ded571c74a419ec02a3c6c6d9f823 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Jun 2019 14:33:18 -0700 Subject: [PATCH 11/33] Add "bin/file migrate" options to support import of a local-disk backup for Phacility instances Summary: Ref T13306. Currently, there's no easy way to import a third-party local-disk file dump into a Phacility instance. Add some more options to `bin/files migrate` to support this. In particular, this enables: ``` $ ./bin/files --from-engine local-disk --engine amazon-s3 --local-disk-source path/to/backup ``` ...to import these files into S3 directly. These are general-purpose options and theoretically useful in other use cases, although realistically those cases are probably very rare. Test Plan: Used `bin/files` with the new options to move files in and out of local disk storage in an arbitrary backup directory. Got clean exports/imports. Reviewers: amckinley Maniphest Tasks: T13306 Differential Revision: https://secure.phabricator.com/D20571 --- ...bricatorFilesManagementMigrateWorkflow.php | 34 ++++++++++++++ .../PhabricatorFilesManagementWorkflow.php | 47 ++++++++++++++----- .../files/query/PhabricatorFileQuery.php | 13 +++++ 3 files changed, 83 insertions(+), 11 deletions(-) diff --git a/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php index 909e45c31f..58d5155aed 100644 --- a/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php +++ b/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php @@ -46,10 +46,44 @@ final class PhabricatorFilesManagementMigrateWorkflow 'name' => 'names', 'wildcard' => true, ), + array( + 'name' => 'from-engine', + 'param' => 'engine', + 'help' => pht('Migrate files from the named storage engine.'), + ), + array( + 'name' => 'local-disk-source', + 'param' => 'path', + 'help' => pht( + 'When migrating from a local disk source, use the specified '. + 'path as the root directory.'), + ), )); } public function execute(PhutilArgumentParser $args) { + + // See T13306. This flag allows you to import files from a backup of + // local disk storage into some other engine. When the caller provides + // the flag, we override the local disk engine configuration and treat + // it as though it is configured to use the specified location. + + $local_disk_source = $args->getArg('local-disk-source'); + if (strlen($local_disk_source)) { + $path = Filesystem::resolvePath($local_disk_source); + try { + Filesystem::assertIsDirectory($path); + } catch (FilesystemException $ex) { + throw new PhutilArgumentUsageException( + pht( + 'The "--local-disk-source" argument must point to a valid, '. + 'readable directory on local disk.')); + } + + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig('storage.local-disk.path', $path); + } + $target_key = $args->getArg('engine'); if (!$target_key) { throw new PhutilArgumentUsageException( diff --git a/src/applications/files/management/PhabricatorFilesManagementWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementWorkflow.php index e94fa1d96a..44d43dc66a 100644 --- a/src/applications/files/management/PhabricatorFilesManagementWorkflow.php +++ b/src/applications/files/management/PhabricatorFilesManagementWorkflow.php @@ -4,23 +4,48 @@ abstract class PhabricatorFilesManagementWorkflow extends PhabricatorManagementWorkflow { protected function buildIterator(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); $names = $args->getArg('names'); - if ($args->getArg('all')) { - if ($names) { - throw new PhutilArgumentUsageException( - pht( - 'Specify either a list of files or `%s`, but not both.', - '--all')); - } - return new LiskMigrationIterator(new PhabricatorFile()); + $is_all = $args->getArg('all'); + $from_engine = $args->getArg('from-engine'); + + $any_constraint = ($from_engine || $names); + + if (!$is_all && !$any_constraint) { + throw new PhutilArgumentUsageException( + pht( + 'Use "--all" to migrate all files, or choose files to migrate '. + 'with "--names" or "--from-engine".')); } + if ($is_all && $any_constraint) { + throw new PhutilArgumentUsageException( + pht( + 'You can not migrate all files with "--all" and also migrate only '. + 'a subset of files with "--from-engine" or "--names".')); + } + + // If we're migrating specific named files, convert the names into IDs + // first. + $ids = null; if ($names) { - return $this->loadFilesWithNames($names); + $files = $this->loadFilesWithNames($names); + $ids = mpull($files, 'getID'); } - return null; + $query = id(new PhabricatorFileQuery()) + ->setViewer($viewer); + + if ($ids) { + $query->withIDs($ids); + } + + if ($from_engine) { + $query->withStorageEngines(array($from_engine)); + } + + return new PhabricatorQueryIterator($query); } protected function loadFilesWithNames(array $names) { @@ -36,7 +61,7 @@ abstract class PhabricatorFilesManagementWorkflow if (empty($files[$name])) { throw new PhutilArgumentUsageException( pht( - "No file '%s' exists!", + 'No file "%s" exists.', $name)); } } diff --git a/src/applications/files/query/PhabricatorFileQuery.php b/src/applications/files/query/PhabricatorFileQuery.php index 4205ab5c5d..c19574acaa 100644 --- a/src/applications/files/query/PhabricatorFileQuery.php +++ b/src/applications/files/query/PhabricatorFileQuery.php @@ -19,6 +19,7 @@ final class PhabricatorFileQuery private $needTransforms; private $builtinKeys; private $isBuiltin; + private $storageEngines; public function withIDs(array $ids) { $this->ids = $ids; @@ -137,6 +138,11 @@ final class PhabricatorFileQuery $ngrams); } + public function withStorageEngines(array $engines) { + $this->storageEngines = $engines; + return $this; + } + public function showOnlyExplicitUploads($explicit_uploads) { $this->explicitUploads = $explicit_uploads; return $this; @@ -469,6 +475,13 @@ final class PhabricatorFileQuery } } + if ($this->storageEngines !== null) { + $where[] = qsprintf( + $conn, + 'storageEngine IN (%Ls)', + $this->storageEngines); + } + return $where; } From d3112392d1e856c0a769f828785e2ef2e7cf8f34 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Jun 2019 19:21:13 -0700 Subject: [PATCH 12/33] Allow "Sign with MFA" to be applied as a comment action without requiring "CAN_EDIT" Summary: Fixes T13307. We currently require "CAN_EDIT" to sign actions, but it's fine to sign a comment with only "CAN_INTERACT". Since the actions like "Accept Revision" already work like this, the fix is one line. Test Plan: {F6488135} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13307 Differential Revision: https://secure.phabricator.com/D20574 --- .../engineextension/PhabricatorAuthMFAEditEngineExtension.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php b/src/applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php index adad1f9274..f138193d69 100644 --- a/src/applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php +++ b/src/applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php @@ -38,6 +38,7 @@ final class PhabricatorAuthMFAEditEngineExtension ->setLabel(pht('MFA')) ->setIsFormField(false) ->setCommentActionLabel(pht('Sign With MFA')) + ->setCanApplyWithoutEditCapability(true) ->setCommentActionOrder(12000) ->setActionDescription( pht('You will be prompted to provide MFA when you submit.')) From 874282db7589feb801874ef17895aca00ea46c82 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Jun 2019 10:36:59 -0700 Subject: [PATCH 13/33] Correct "msort()" vs "msortv()" to more fully stabilize transaction sorts after recent changes Summary: Ref T13303. I upgraded this to a vector-based sort but forgot to type a "v", which means the sort has different stability under PHP 5.5. See D20582 for a root cause fix. Test Plan: Locally, on PHP7, not much changes. I expect this to fix the odd selection of title stories in mail and notification stories on `secure`, which is running PHP 5.5. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13303 Differential Revision: https://secure.phabricator.com/D20583 --- .../editor/PhabricatorApplicationTransactionEditor.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 55dedb863b..bcce014669 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -3267,7 +3267,7 @@ abstract class PhabricatorApplicationTransactionEditor protected function getStrongestAction( PhabricatorLiskDAO $object, array $xactions) { - return head(msort($xactions, 'newActionStrengthSortVector')); + return head(msortv($xactions, 'newActionStrengthSortVector')); } From 2bc045bab8d7e68095a20d51c9b49bab6ec041e3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Jun 2019 13:10:07 -0700 Subject: [PATCH 14/33] Fix another stray "msort()/msortv()" issue Summary: Ref T13303. See B22967. This should be "msortv()" but didn't get updated properly. Test Plan: The system works! Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13303 Differential Revision: https://secure.phabricator.com/D20585 --- .../differential/command/DifferentialActionEmailCommand.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/differential/command/DifferentialActionEmailCommand.php b/src/applications/differential/command/DifferentialActionEmailCommand.php index cbf94487e9..1b1f8d64e3 100644 --- a/src/applications/differential/command/DifferentialActionEmailCommand.php +++ b/src/applications/differential/command/DifferentialActionEmailCommand.php @@ -56,7 +56,7 @@ final class DifferentialActionEmailCommand public function getCommandObjects() { $actions = DifferentialRevisionActionTransaction::loadAllActions(); - $actions = msort($actions, 'getRevisionActionOrderVector'); + $actions = msortv($actions, 'getRevisionActionOrderVector'); $objects = array(); foreach ($actions as $action) { From 14b076578f8eb1cc806dc40384383efc69902f6b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Jun 2019 09:47:19 -0700 Subject: [PATCH 15/33] In "Download Raw Diff", engage the chunk engine to handle 8MB+ changes Summary: Fixes T13313. The "Download Raw Diff" workflow in Differential currently uses an older way of interacting with Files that doesn't engage the chunk engine and can't handle 8MB+ files. Update to `IteratorFileUploadSource` -- we're still passing in a single giant blob, but this approach can be chunked. This will still break somewhere north of 8MB (it will break at 2GB with the PHP string limit if nowhere sooner, since we're putting the entire raw diff in `$raw_diff` rather than using a rope/stream) but will likely survive diffs in the hundreds-of-megabytes range for now. Test Plan: - Added `str_repeat('x', 1024 * 1024 * 9)` to the `$raw_diff` to create a 9MB+ diff. - Configured file storage with no engine explicitly configured for >8MB chunks (i.e., "reasonably"). - Clicked "Download Raw Diff". - Before: misleading file storage engine error ("no engine can store this file"). - After: large, raw diff response. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13313 Differential Revision: https://secure.phabricator.com/D20579 --- .../DifferentialRevisionViewController.php | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 9544e70c16..b0471e7c5f 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -1033,12 +1033,6 @@ final class DifferentialRevisionViewController } - /** - * Note this code is somewhat similar to the buildPatch method in - * @{class:DifferentialReviewRequestMail}. - * - * @return @{class:AphrontRedirectResponse} - */ private function buildRawDiffResponse( DifferentialRevision $revision, array $changesets, @@ -1100,15 +1094,17 @@ final class DifferentialRevisionViewController } $file_name .= 'diff'; - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $file = PhabricatorFile::newFromFileData( - $raw_diff, - array( - 'name' => $file_name, - 'ttl.relative' => phutil_units('24 hours in seconds'), - 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, - )); + $iterator = new ArrayIterator(array($raw_diff)); + $source = id(new PhabricatorIteratorFileUploadSource()) + ->setName($file_name) + ->setMIMEType('text/plain') + ->setRelativeTTL(phutil_units('24 hours in seconds')) + ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE) + ->setIterator($iterator); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file = $source->uploadFile(); $file->attachToObject($revision->getPHID()); unset($unguarded); From 4af73a625fe11628eddfaaf3ac9cf13213098722 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 13 Jun 2019 16:55:57 -0700 Subject: [PATCH 16/33] Don't require users be logged in to access the Logout controller, so users with no Spaces can log out Summary: Fixes T13310. Use cases in the form "users with no access to any spaces can not " are generally unsupported (that is, we consider this to mean that the install is misconfigured), but "log out" is a somewhat more reasonable sort of thing to do and easy to support. Drop the requirement that users be logged in to access the Logout controller. This skips the check for access to any Spaces and allows users with no Spaces to log out. For users who are already logged out, this just redirects home with no effect. Test Plan: - As a user with access to no Spaces, logged out. (Before: error; after: worked). - As a logged-out user, logged out (was redirected). - As a normal user, logged out (normal logout). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13310 Differential Revision: https://secure.phabricator.com/D20578 --- .../PhabricatorLogoutController.php | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/applications/auth/controller/PhabricatorLogoutController.php b/src/applications/auth/controller/PhabricatorLogoutController.php index 47592e0a2d..dccf6bb45b 100644 --- a/src/applications/auth/controller/PhabricatorLogoutController.php +++ b/src/applications/auth/controller/PhabricatorLogoutController.php @@ -4,7 +4,25 @@ final class PhabricatorLogoutController extends PhabricatorAuthController { public function shouldRequireLogin() { - return true; + // See T13310. We allow access to the "Logout" controller even if you are + // not logged in: otherwise, users who do not have access to any Spaces can + // not log out. + + // When you try to access a controller which requires you be logged in, + // and you do not have access to any Spaces, an access check fires first + // and prevents access with a "No Access to Spaces" error. If this + // controller requires users be logged in, users who are trying to log out + // and also have no access to Spaces get the error instead of a logout + // workflow and are trapped. + + // By permitting access to this controller even if you are not logged in, + // we bypass the Spaces check and allow users who have no access to Spaces + // to log out. + + // This incidentally allows users who are already logged out to access the + // controller, but this is harmless: we just no-op these requests. + + return false; } public function shouldRequireEmailVerification() { From 4450c908818d8a8948867b4b72cca660fd7e78c4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Jun 2019 10:00:21 -0700 Subject: [PATCH 17/33] When triggering audits, respect committer identities when importing commits Summary: Ref T13311. We currently don't use committer identity mappings when triggering audits, so if a user is only associated with an identity via manual mapping we won't treat them as the author. Instead, use the identity and manual mapping if they're available. Test Plan: - Pushed a commit as `xyz `, an address with no corresponding user. - In the UI, manually associated that identity with user `@alice`. - Ran `bin/repository reparse --publish ` to trigger audits and publishing for the commit. - Before: observed the `$author_phid` was `null`. - After: observed the `$author_phid` is Alice. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13311 Differential Revision: https://secure.phabricator.com/D20580 --- .../storage/PhabricatorRepositoryCommit.php | 12 ++++++++++++ .../PhabricatorRepositoryCommitPublishWorker.php | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 6b8ac687b3..b5c2fde82a 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -466,6 +466,18 @@ final class PhabricatorRepositoryCommit return $data->getCommitDetail('authorPHID'); } + public function getEffectiveAuthorPHID() { + if ($this->hasAuthorIdentity()) { + $identity = $this->getAuthorIdentity(); + if ($identity->hasEffectiveUser()) { + return $identity->getCurrentEffectiveUserPHID(); + } + } + + $data = $this->getCommitData(); + return $data->getCommitDetail('authorPHID'); + } + public function getAuditStatusObject() { $status = $this->getAuditStatus(); return DiffusionCommitAuditStatus::newForStatus($status); diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php index 3865050395..0b0a194806 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php @@ -147,7 +147,7 @@ final class PhabricatorRepositoryCommitPublishWorker $data = $commit->getCommitData(); - $author_phid = $data->getCommitDetail('authorPHID'); + $author_phid = $commit->getEffectiveAuthorPHID(); $revision = DiffusionCommitRevisionQuery::loadRevisionForCommit( $viewer, From bab35f28e49d7cc03f5c190482de50cf92499459 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Jun 2019 10:08:08 -0700 Subject: [PATCH 18/33] Respect repository identities when selecting author vs auditor actions Summary: Depends on D20580. Fixes T13311. When we choose which actions to show a user, we can either show them "auditor" actions (like "raise concern") or "author" actions (like "request verification"). Currently, we don't show "author" actions if you're the author of the commit via an identity mapping, but we should. Use identity mappings where they exist. (Because I've implemented `getEffectiveAuthorPHID()` in a way that requires `$data` be attached, it's possible this will make something throw a "DataNotAttached" exception, but: probably it won't?; and that's easy to fix if it happens.) Test Plan: See D20580. As `@alice`, viewed the commit in the UI. - Before: got auditor actions presented to me. - After: got author actions. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13311 Differential Revision: https://secure.phabricator.com/D20581 --- .../diffusion/xaction/DiffusionCommitActionTransaction.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php index d2e5f17ccb..1d351ffa5d 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php @@ -63,7 +63,7 @@ abstract class DiffusionCommitActionTransaction return false; } - return ($viewer->getPHID() === $commit->getAuthorPHID()); + return ($viewer->getPHID() === $commit->getEffectiveAuthorPHID()); } public function newEditField( From aba7c98baedb67e3d8042eb7970b908101db6aa1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Jun 2019 11:41:36 -0700 Subject: [PATCH 19/33] Skip loading transaction handles in an old migration Summary: Ref T13305. See that task for discussion. This old migration may indirectly cause search index worker tasks to queue by loading handles. They'll fail since we later added `dateCreated` to the worker task table. Use `needHandles(false)` (since we don't use them) to disable loading handles and avoid the problem. Test Plan: - Ran `bin/storage upgrade -f` on an older instance (late 2016) and hit this issue. - Applied the patch, got a clean migration to modernity. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13305 Differential Revision: https://secure.phabricator.com/D20570 --- resources/sql/autopatches/20180208.maniphest.02.populate.php | 1 + 1 file changed, 1 insertion(+) diff --git a/resources/sql/autopatches/20180208.maniphest.02.populate.php b/resources/sql/autopatches/20180208.maniphest.02.populate.php index e11267496c..4b4e549574 100644 --- a/resources/sql/autopatches/20180208.maniphest.02.populate.php +++ b/resources/sql/autopatches/20180208.maniphest.02.populate.php @@ -26,6 +26,7 @@ foreach (new LiskMigrationIterator($table) as $task) { $xactions = id(new ManiphestTransactionQuery()) ->setViewer($viewer) ->withObjectPHIDs(array($task->getPHID())) + ->needHandles(false) ->withTransactionTypes( array( $type_merge, From 731b45d8184773c563f48dc5006841cc179d1983 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Jun 2019 08:00:07 -0700 Subject: [PATCH 20/33] 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 21/33] 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 22/33] 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 23/33] 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 24/33] 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 25/33] 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