From 4c09e88c95829ad629c86e4b58e1be069fd0d018 Mon Sep 17 00:00:00 2001 From: Arturas Moskvinas Date: Wed, 1 Aug 2018 14:42:52 +0300 Subject: [PATCH 001/125] Add parsing for ssh options (-o) which are passed when using GIT v2 wire protocol by git command (SSH transport) Summary: Makes `ssh-connect` compatible with Git v2 wire protocol over SSH More details about git V2 wire: https://opensource.googleblog.com/2018/05/introducing-git-protocol-version-2.html `git` command (2.18+) passes extra options (`-o "SendEnv GIT_PROTOCOL"`) to underlying `ssh` command to enable v2 wire protocol (environment variable enabling new protocol). Phabricator `ssh-connect` command doesn't understand `-o` options and interprets it as host parts hence when you enable git v2 all clones/ls-remotes crash with: ``` #0 ExecFuture::resolvex() called at [/src/applications/repository/storage/PhabricatorRepository.php:525] #1 PhabricatorRepository::execxRemoteCommand(string, PhutilOpaqueEnvelope) called at [/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:400] #2 PhabricatorRepositoryPullEngine::loadGitRemoteRefs(PhabricatorRepository) called at [/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:343] #3 PhabricatorRepositoryPullEngine::executeGitUpdate() called at [/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:126] #4 PhabricatorRepositoryPullEngine::pullRepositoryWithLock() called at [/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:40] #5 PhabricatorRepositoryPullEngine::pullRepository() called at [/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php:59] #6 PhabricatorRepositoryManagementUpdateWorkflow::execute(PhutilArgumentParser) called at [/src/parser/argument/PhutilArgumentParser.php:441] #7 PhutilArgumentParser::parseWorkflowsFull(array) called at [/src/parser/argument/PhutilArgumentParser.php:333] #8 PhutilArgumentParser::parseWorkflows(array) called at [/scripts/repository/manage_repositories.php:22] COMMAND git ls-remote '********' STDOUT (empty) STDERR ssh: Could not resolve hostname -o: Name or service not known fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. at [/src/future/exec/ExecFuture.php:369] ``` Test Plan: How to reproduce: 1. add repository to Phabricator which is accessed via `ssh` 2. Use git 2.18+ 3. Enable wire protocol in `/etc/gitconfig`: ``` [protocol] version = 2 ``` 4. Try refreshing repository: `phabricator/bin/repository update somecallsing` 5. Repository update fails with `ssh: Could not resolve hostname -o: Name or service not known` after this changes - updates will succeed Reviewers: epriestley, Pawka, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19542 --- scripts/ssh/ssh-connect.php | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/scripts/ssh/ssh-connect.php b/scripts/ssh/ssh-connect.php index 9757f3cbbe..0d87f16973 100755 --- a/scripts/ssh/ssh-connect.php +++ b/scripts/ssh/ssh-connect.php @@ -27,7 +27,15 @@ $args->parsePartial( 'param' => pht('port'), 'help' => pht('Port number to connect to.'), ), + array( + 'name' => 'options', + 'short' => 'o', + 'param' => pht('options'), + 'repeat' => true, + 'help' => pht('SSH options.'), + ), )); + $unconsumed_argv = $args->getUnconsumedArgumentVector(); if (function_exists('pcntl_signal')) { @@ -113,6 +121,25 @@ if ($port) { $arguments[] = $port; } +$options = $args->getArg('options'); +$allowed_ssh_options = array('SendEnv=GIT_PROTOCOL'); + +if (!empty($options)) { + foreach ($options as $option) { + if (array_search($option, $allowed_ssh_options) !== false) { + $pattern[] = '-o %s'; + $arguments[] = $option; + } else { + throw new Exception( + pht( + 'Disallowed ssh option "%s" given with "-o". '. + 'Allowed options are: %s.', + $option, + implode(', ', $allowed_ssh_options))); + } + } +} + $pattern[] = '--'; $pattern[] = '%s'; From 356b2781bcadf43aaf7e4d8086fcc8ab867a38cb Mon Sep 17 00:00:00 2001 From: Arturas Moskvinas Date: Thu, 2 Aug 2018 17:41:40 +0300 Subject: [PATCH 002/125] Gracefully fail request if non existing callsign is passed to getrecentcommitsbypath instead of crashing Summary: `diffusion.getrecentcommitsbypath` fails with 500 error when non existing callsign is passed: ``` >>> UNRECOVERABLE FATAL ERROR <<< Call to a member function getCommit() on null ``` Expected Behavior: Return more graceful error notifying caller that such callsign/repository does not exist Reproduction steps: Open conduit: https://secure.phabricator.com/conduit/method/diffusion.getrecentcommitsbypath/ Enter: callsign: "obviouslynotexisting" path: "/random" Click call method Test Plan: after applying patch - call no longer fails with 500s Reviewers: Pawka, epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19558 --- ...DiffusionGetRecentCommitsByPathConduitAPIMethod.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php index 002805ac4e..65bc5f69da 100644 --- a/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php @@ -23,6 +23,12 @@ final class DiffusionGetRecentCommitsByPathConduitAPIMethod ); } + protected function defineErrorTypes() { + return array( + 'ERR_NOT_FOUND' => pht('Repository was not found.'), + ); + } + protected function defineReturnType() { return 'nonempty list'; } @@ -36,6 +42,10 @@ final class DiffusionGetRecentCommitsByPathConduitAPIMethod 'branch' => $request->getValue('branch'), )); + if ($drequest === null) { + throw new ConduitException('ERR_NOT_FOUND'); + } + $limit = nonempty( $request->getValue('limit'), self::DEFAULT_LIMIT); From 3574a55a95bda7d852dacd3082bfa312d610f615 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 2 Aug 2018 16:33:41 -0700 Subject: [PATCH 003/125] Deprecate Conduit method "diffusion.getrecentcommitsbypath" Summary: See D19558. This method has no callers and just wraps `diffusion.historyquery`, since D5960 (2013). This was introduced in D315 (which didn't make it out of FB, I think) inside Facebook for unclear purposes in 2011. Test Plan: Grepped for callers, found none. Reviewers: amckinley Reviewed By: amckinley Subscribers: artms Differential Revision: https://secure.phabricator.com/D19559 --- .../DiffusionGetRecentCommitsByPathConduitAPIMethod.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php index 65bc5f69da..286831f82b 100644 --- a/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php @@ -9,6 +9,14 @@ final class DiffusionGetRecentCommitsByPathConduitAPIMethod return 'diffusion.getrecentcommitsbypath'; } + public function getMethodStatus() { + return self::METHOD_STATUS_DEPRECATED; + } + + public function getMethodStatusDescription() { + return pht('Obsoleted by "diffusion.historyquery".'); + } + public function getMethodDescription() { return pht( 'Get commit identifiers for recent commits affecting a given path.'); From f3fa164882eab06ef10cfdfcacb82248962d9f80 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 3 Aug 2018 12:15:29 -0700 Subject: [PATCH 004/125] Add a "Last Edited" property to Wiki pages Summary: Ref T13164. See PHI797. The last edit is available in the page header, but it's not precise (just says "180 days ago") and a little weird (it's unusual for us to put that kind of information in the header). Add a precise timestamp to the footer for now. I'd imagine re-examining this the next time Phriction gets some UI work and maybe trying to integrate timeline/transactions more cleanly (see also T1894). Test Plan: Looked at a wiki page, then edited it. Saw precise "Last Edit" timestamp adjacent to "Last Author". Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19560 --- .../phriction/controller/PhrictionDocumentController.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index 8377cf7d29..c8ffb86ce8 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -254,7 +254,8 @@ final class PhrictionDocumentController PhrictionContent $content, $slug) { - $viewer = $this->getRequest()->getUser(); + $viewer = $this->getViewer(); + $view = id(new PHUIPropertyListView()) ->setUser($viewer) ->setObject($document); @@ -263,6 +264,10 @@ final class PhrictionDocumentController pht('Last Author'), $viewer->renderHandle($content->getAuthorPHID())); + $view->addProperty( + pht('Last Edited'), + phabricator_datetime($content->getDateCreated(), $viewer)); + return $view; } From 5839a54b604d972c11192105896124da38fb702a Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 3 Aug 2018 13:02:23 -0700 Subject: [PATCH 005/125] Raise a tailored error when calling "transaction.search" with empty "phids" constraint Summary: Ref T13164. See PHI725. For real "*.search" methods, parameters get validated and you get an error if you use an empty list as a constraint. Since "transaction.search" isn't really a normal "*.search" method, it doesn't benefit from this. Just do the check manually for now. Test Plan: Made `transaction.search` calls with no constraints (got results); a valid costraint (got fewer results); and an invalid empty constraint (got an exception). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19562 --- .../conduit/TransactionSearchConduitAPIMethod.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php index 3b040ea2c3..0394432ff3 100644 --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -79,6 +79,14 @@ final class TransactionSearchConduitAPIMethod )); $with_phids = idx($constraints, 'phids'); + + if ($with_phids === array()) { + throw new Exception( + pht( + 'Constraint "phids" to "transaction.search" requires nonempty list, '. + 'empty list provided.')); + } + if ($with_phids) { $xaction_query->withPHIDs($with_phids); } From 91abc0f027209908c4d9eff824ea7445e0ca5555 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 3 Aug 2018 13:38:24 -0700 Subject: [PATCH 006/125] Stop indexing the chunk data objects for large Files stored in multiple chunks Summary: Ref T13164. See PHI766. Currently, when file data is stored in small chunks, we submit each chunk to the indexing engine. However, chunks are never surfaced directly and can never be found via any search/query, so this work is pointless. Just skip it. (It would be nice to do this a little more formally on `IndexableInterface` or similar as `isThisAnIndexableObject()`, but we'd have to add like a million empty "yes, index this always" methods to do that, and it seems unlikely that we'll end up with too many other objects like these.) Test Plan: - Ran `bin/harbormaster rebuild-log --id ... --force` before and after change, saw about 200 fewer queries after the change. - Uploaded a uniquely named file and searched for it to make sure I didn't break that. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19563 --- src/applications/files/storage/PhabricatorFile.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 1517b4435b..5109202a33 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -159,10 +159,22 @@ final class PhabricatorFile extends PhabricatorFileDAO public function saveAndIndex() { $this->save(); - PhabricatorSearchWorker::queueDocumentForIndexing($this->getPHID()); + + if ($this->isIndexableFile()) { + PhabricatorSearchWorker::queueDocumentForIndexing($this->getPHID()); + } + return $this; } + private function isIndexableFile() { + if ($this->getIsChunk()) { + return false; + } + + return true; + } + public function getMonogram() { return 'F'.$this->getID(); } From 201f29fbf4ab74a2cd41433c4ba57c77d64e4e6e Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Aug 2018 09:24:48 -0700 Subject: [PATCH 007/125] Fix truncation in "bin/storage probe" of tables larger than 100GB Summary: Ref T13164. PHI805 incidentally includes some `bin/storage probe` output for 100GB+ tables which renders wrong. We have the tools to render it properly, so stop doing this manually and let ConsoleTable figure out the alignment. Test Plan: Faked very large table sizes, ran `bin/storage probe`: {F5785946} (Then, un-faked the very large table sizes and ran it again, got sensible output.) Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19567 --- ...PhabricatorStorageManagementProbeWorkflow.php | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php index a75afe69e8..5c55dc779e 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php @@ -56,8 +56,18 @@ final class PhabricatorStorageManagementProbeWorkflow ->setShowHeader(false) ->setPadding(2) ->addColumn('name', array('title' => pht('Database / Table'))) - ->addColumn('size', array('title' => pht('Size'))) - ->addColumn('percentage', array('title' => pht('Percentage'))); + ->addColumn( + 'size', + array( + 'title' => pht('Size'), + 'align' => PhutilConsoleTable::ALIGN_RIGHT, + )) + ->addColumn( + 'percentage', + array( + 'title' => pht('Percentage'), + 'align' => PhutilConsoleTable::ALIGN_RIGHT, + )); foreach ($totals as $db => $size) { list($database_size, $database_percentage) = $this->formatSize( @@ -98,7 +108,7 @@ final class PhabricatorStorageManagementProbeWorkflow private function formatSize($n, $o) { return array( - sprintf('%8.8s MB', number_format($n / (1024 * 1024), 1)), + pht('%s MB', new PhutilNumber($n / (1024 * 1024), 1)), sprintf('%3.1f%%', 100 * ($n / $o)), ); } From 3ca3f09a1aa6c0dc407ee2ae9e5f9cf4f995bbd6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Aug 2018 09:06:22 -0700 Subject: [PATCH 008/125] Add an "--as" flag to "bin/conduit call ..." to improve flexibility and ease of profiling Summary: Ref T13164. In PHI801, an install reported a particular slow Conduit method call. Conduit calls aren't easily profilable with normal tools (for example, `arc call-conduit --xprofile ...` gives you a profile of the //client//). They can be profiled most easily with `bin/conduit call ... --xprofile`. However, `bin/conduit call` currently doesn't let you pick a user to execute the command on behalf of, so it's not terribly useful for profiling `*.edit`-style methods which do a write: these need a real acting user. Test Plan: Ran `bin/conduit call --method user.whoami --as epriestley ...` with valid, invalid, and no acting users. ``` $ echo '{}' | ./bin/conduit call --method user.whoami --as epriestley --input - Reading input from stdin... { "result": { "phid": "PHID-USER-icyixzkx3f4ttv67avbn", "userName": "epriestley", "realName": "Evan Priestley", ... ``` Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19566 --- ...abricatorConduitCallManagementWorkflow.php | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php b/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php index 6cb3bd2409..f9ba48b372 100644 --- a/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php +++ b/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php @@ -21,6 +21,13 @@ final class PhabricatorConduitCallManagementWorkflow 'File to read parameters from, or "-" to read from '. 'stdin.'), ), + array( + 'name' => 'as', + 'param' => 'username', + 'help' => pht( + 'Execute the call as the given user. (If omitted, the call will '. + 'be executed as an omnipotent user.)'), + ), )); } @@ -39,6 +46,22 @@ final class PhabricatorConduitCallManagementWorkflow pht('Specify a file to read parameters from with "--input".')); } + $as = $args->getArg('as'); + if (strlen($as)) { + $actor = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withUsernames(array($as)) + ->executeOne(); + if (!$actor) { + throw new PhutilArgumentUsageException( + pht( + 'No such user "%s" exists.', + $as)); + } + } else { + $actor = $viewer; + } + if ($input === '-') { fprintf(STDERR, tsprintf("%s\n", pht('Reading input from stdin...'))); $input_json = file_get_contents('php://stdin'); @@ -49,7 +72,7 @@ final class PhabricatorConduitCallManagementWorkflow $params = phutil_json_decode($input_json); $result = id(new ConduitCall($method, $params)) - ->setUser($viewer) + ->setUser($actor) ->execute(); $output = array( From df31405d64a44a9ae7a12a324fb292dc8357be46 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Aug 2018 11:26:34 -0700 Subject: [PATCH 009/125] Improve compatibility of "Config > Cache Status" across APCu versions Summary: Ref T13164. See PHI790. Older versions of APCu reported cache keys as "key" from `apcu_cache_info()`. APC and newer APCu report it as "info". Check both indexes for compatibility. Test Plan: - Locally, with newer APCu, saw no behavioral change. - Will double check on `admin`, which has an older APCu with the "key" behavior. - (I hunted this down by dumping `apcu_cache_info()` on `admin` to see what was going on.) Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19569 --- .../cache/spec/PhabricatorDataCacheSpec.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/applications/cache/spec/PhabricatorDataCacheSpec.php b/src/applications/cache/spec/PhabricatorDataCacheSpec.php index bf6a495f7f..103b0d8394 100644 --- a/src/applications/cache/spec/PhabricatorDataCacheSpec.php +++ b/src/applications/cache/spec/PhabricatorDataCacheSpec.php @@ -106,8 +106,21 @@ final class PhabricatorDataCacheSpec extends PhabricatorCacheSpec { $cache = $info['cache_list']; $state = array(); foreach ($cache as $item) { - $info = idx($item, 'info', ''); - $key = self::getKeyPattern($info); + // Some older versions of APCu report the cachekey as "key", while + // newer APCu and APC report it as "info". Just check both indexes + // for commpatibility. See T13164 for details. + + $info = idx($item, 'info'); + if ($info === null) { + $info = idx($item, 'key'); + } + + if ($info === null) { + $key = ''; + } else { + $key = self::getKeyPattern($info); + } + if (empty($state[$key])) { $state[$key] = array( 'max' => 0, From 6df278bea84a2c8a66752bf4b591d7f3d4432f89 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Aug 2018 10:07:00 -0700 Subject: [PATCH 010/125] In "bin/ssh-auth", cache a structure instead of a flat file because paths may change at runtime Summary: Fixes T12397. Ref T13164. See PHI801. Several installs have hit various use cases where the path on disk where Phabricator lives changes at runtime. Currently, `bin/ssh-auth` caches a flat file which includes the path to `bin/ssh-exec`, so this may fall out of date if `phabricator/` moves. These use cases have varying strengths of legitimacy, but "we're migrating to a new set of hosts and the pool is half old machines and half new machines" seems reasonably compelling and not a problem entirely of one's own making. Test Plan: - Compared output on `master` to output after change, found them byte-for-byte identical. - Moved `phabricator/` to `phabricator2/`, ran `bin/ssh-auth`, got updated output. - Added a new SSH key, saw it appear in the output. - Grepped for `AUTHFILE_CACHEKEY` (no hits). - Dropped the cache, verified that the file regenerates cleanly. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164, T12397 Differential Revision: https://secure.phabricator.com/D19568 --- scripts/ssh/ssh-auth.php | 89 +++++++++++++------ .../auth/query/PhabricatorAuthSSHKeyQuery.php | 4 +- 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/scripts/ssh/ssh-auth.php b/scripts/ssh/ssh-auth.php index b25905668b..3c4f3f2b33 100755 --- a/scripts/ssh/ssh-auth.php +++ b/scripts/ssh/ssh-auth.php @@ -2,13 +2,27 @@ getKey($authfile_key); +$authstruct_key = PhabricatorAuthSSHKeyQuery::AUTHSTRUCT_CACHEKEY; +$authstruct_raw = $cache->getKey($authstruct_key); -if ($authfile === null) { +$authstruct = null; + +if (strlen($authstruct_raw)) { + try { + $authstruct = phutil_json_decode($authstruct_raw); + } catch (Exception $ex) { + // Ignore any issues with the cached data; we'll just rebuild the + // structure below. + } +} + +if ($authstruct === null) { $keys = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withIsActive(true) @@ -19,7 +33,7 @@ if ($authfile === null) { exit(1); } - $bin = $root.'/bin/ssh-exec'; + $key_list = array(); foreach ($keys as $ssh_key) { $key_argv = array(); $object = $ssh_key->getObject(); @@ -42,18 +56,7 @@ if ($authfile === null) { $key_argv[] = '--phabricator-ssh-key'; $key_argv[] = $ssh_key->getID(); - $cmd = csprintf('%s %Ls', $bin, $key_argv); - - $instance = PhabricatorEnv::getEnvConfig('cluster.instance'); - if (strlen($instance)) { - $cmd = csprintf('PHABRICATOR_INSTANCE=%s %C', $instance, $cmd); - } - - // This is additional escaping for the SSH 'command="..."' string. - $cmd = addcslashes($cmd, '"\\'); - // Strip out newlines and other nonsense from the key type and key body. - $type = $ssh_key->getKeyType(); $type = preg_replace('@[\x00-\x20]+@', '', $type); if (!strlen($type)) { @@ -66,22 +69,54 @@ if ($authfile === null) { continue; } - $options = array( - 'command="'.$cmd.'"', - 'no-port-forwarding', - 'no-X11-forwarding', - 'no-agent-forwarding', - 'no-pty', + $key_list[] = array( + 'argv' => $key_argv, + 'type' => $type, + 'key' => $key, ); - $options = implode(',', $options); - - $lines[] = $options.' '.$type.' '.$key."\n"; } - $authfile = implode('', $lines); + $authstruct = array( + 'keys' => $key_list, + ); + + $authstruct_raw = phutil_json_encode($authstruct); $ttl = phutil_units('24 hours in seconds'); - $cache->setKey($authfile_key, $authfile, $ttl); + $cache->setKey($authstruct_key, $authstruct_raw, $ttl); } +$bin = $root.'/bin/ssh-exec'; +$instance = PhabricatorEnv::getEnvConfig('cluster.instance'); + +$lines = array(); +foreach ($authstruct['keys'] as $key_struct) { + $key_argv = $key_struct['argv']; + $key = $key_struct['key']; + $type = $key_struct['type']; + + $cmd = csprintf('%s %Ls', $bin, $key_argv); + + if (strlen($instance)) { + $cmd = csprintf('PHABRICATOR_INSTANCE=%s %C', $instance, $cmd); + } + + // This is additional escaping for the SSH 'command="..."' string. + $cmd = addcslashes($cmd, '"\\'); + + $options = array( + 'command="'.$cmd.'"', + 'no-port-forwarding', + 'no-X11-forwarding', + 'no-agent-forwarding', + 'no-pty', + ); + $options = implode(',', $options); + + $lines[] = $options.' '.$type.' '.$key."\n"; +} + +$authfile = implode('', $lines); + echo $authfile; + exit(0); diff --git a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php index 77b666ea44..d8474085b2 100644 --- a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php +++ b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php @@ -3,7 +3,7 @@ final class PhabricatorAuthSSHKeyQuery extends PhabricatorCursorPagedPolicyAwareQuery { - const AUTHFILE_CACHEKEY = 'ssh.authfile'; + const AUTHSTRUCT_CACHEKEY = 'ssh.authstruct'; private $ids; private $phids; @@ -13,7 +13,7 @@ final class PhabricatorAuthSSHKeyQuery public static function deleteSSHKeyCache() { $cache = PhabricatorCaches::getMutableCache(); - $authfile_key = self::AUTHFILE_CACHEKEY; + $authfile_key = self::AUTHSTRUCT_CACHEKEY; $cache->deleteKey($authfile_key); } From 2951694c27370b501816f9eecff76e4cb65fb081 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Thu, 9 Aug 2018 12:30:42 -0700 Subject: [PATCH 011/125] Correctly spell 'committer' Summary: It's a funny word. h/t @joshuaspence Test Plan: Inspection of correct spelling. Reviewers: epriestley, joshuaspence Reviewed By: joshuaspence Subscribers: Korvin, joshuaspence Differential Revision: https://secure.phabricator.com/D19570 --- .../repository/storage/PhabricatorRepositoryCommit.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index abcf862b40..cf88c0eb54 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -206,7 +206,7 @@ final class PhabricatorRepositoryCommit return $this->assertAttached($this->authorIdentity); } - public function getCommiterIdentity() { + public function getCommitterIdentity() { return $this->assertAttached($this->committerIdentity); } From a6951a0a5aa0510b702f32ea68c014e99e174b18 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Thu, 9 Aug 2018 12:24:36 -0700 Subject: [PATCH 012/125] Add migration to encourage rebuilding repository identities Summary: Ref T12164. Defines a new manual activity that suggests rebuilding repository identities before Phabricator begins to rely on them. Test Plan: - Ran migration, observed expected setup issue: {F5788217} - Ran `bin/config done identities` and observed setup issue get marked as done. - Ran `/bin/storage upgrade --apply phabricator:20170912.ferret.01.activity.php` to make sure I didn't break the reindex migration; observed reindex setup issue appear as expected. - Ran `./bin/config done reindex` and observed reindex issue cleared as expected. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T12164 Differential Revision: https://secure.phabricator.com/D19497 --- .../20180809.repo_identities.activity.php | 20 ++ .../PhabricatorManualActivitySetupCheck.php | 177 ++++++++++++------ .../PhabricatorConfigManualActivity.php | 2 + 3 files changed, 146 insertions(+), 53 deletions(-) create mode 100644 resources/sql/autopatches/20180809.repo_identities.activity.php diff --git a/resources/sql/autopatches/20180809.repo_identities.activity.php b/resources/sql/autopatches/20180809.repo_identities.activity.php new file mode 100644 index 0000000000..e1077d4ddb --- /dev/null +++ b/resources/sql/autopatches/20180809.repo_identities.activity.php @@ -0,0 +1,20 @@ +loadAllWhere('authorIdentityPHID IS NULL LIMIT 1'); + +if (!$commits) { + return; +} + +try { + id(new PhabricatorConfigManualActivity()) + ->setActivityType(PhabricatorConfigManualActivity::TYPE_IDENTITIES) + ->save(); +} catch (AphrontDuplicateKeyQueryException $ex) { + // If we've already noted that this activity is required, just move on. +} diff --git a/src/applications/config/check/PhabricatorManualActivitySetupCheck.php b/src/applications/config/check/PhabricatorManualActivitySetupCheck.php index 44db1b57a6..c5c756e49b 100644 --- a/src/applications/config/check/PhabricatorManualActivitySetupCheck.php +++ b/src/applications/config/check/PhabricatorManualActivitySetupCheck.php @@ -13,63 +13,134 @@ final class PhabricatorManualActivitySetupCheck foreach ($activities as $activity) { $type = $activity->getActivityType(); - // For now, there is only one type of manual activity. It's not clear - // if we're really going to have too much more of this stuff so this - // is a bit under-designed for now. + switch ($type) { + case PhabricatorConfigManualActivity::TYPE_REINDEX: + $this->raiseSearchReindexIssue(); + break; - $activity_name = pht('Rebuild Search Index'); - $activity_summary = pht( - 'The search index algorithm has been updated and the index needs '. - 'be rebuilt.'); + case PhabricatorConfigManualActivity::TYPE_IDENTITIES: + $this->raiseRebuildIdentitiesIssue(); + break; - $message = array(); - - $message[] = pht( - 'The indexing algorithm for the fulltext search index has been '. - 'updated and the index needs to be rebuilt. Until you rebuild the '. - 'index, global search (and other fulltext search) will not '. - 'function correctly.'); - - $message[] = pht( - 'You can rebuild the search index while Phabricator is running.'); - - $message[] = pht( - 'To rebuild the index, run this command:'); - - $message[] = phutil_tag( - 'pre', - array(), - (string)csprintf( - 'phabricator/ $ ./bin/search index --all --force --background')); - - $message[] = pht( - 'You can find more information about rebuilding the search '. - 'index here: %s', - phutil_tag( - 'a', - array( - 'href' => 'https://phurl.io/u/reindex', - 'target' => '_blank', - ), - 'https://phurl.io/u/reindex')); - - $message[] = pht( - 'After rebuilding the index, run this command to clear this setup '. - 'warning:'); - - $message[] = phutil_tag( - 'pre', - array(), - (string)csprintf('phabricator/ $ ./bin/config done %R', $type)); - - $activity_message = phutil_implode_html("\n\n", $message); - - $this->newIssue('manual.'.$type) - ->setName($activity_name) - ->setSummary($activity_summary) - ->setMessage($activity_message); + default: + } } + } + private function raiseSearchReindexIssue() { + $activity_name = pht('Rebuild Search Index'); + $activity_summary = pht( + 'The search index algorithm has been updated and the index needs '. + 'be rebuilt.'); + + $message = array(); + + $message[] = pht( + 'The indexing algorithm for the fulltext search index has been '. + 'updated and the index needs to be rebuilt. Until you rebuild the '. + 'index, global search (and other fulltext search) will not '. + 'function correctly.'); + + $message[] = pht( + 'You can rebuild the search index while Phabricator is running.'); + + $message[] = pht( + 'To rebuild the index, run this command:'); + + $message[] = phutil_tag( + 'pre', + array(), + (string)csprintf( + 'phabricator/ $ ./bin/search index --all --force --background')); + + $message[] = pht( + 'You can find more information about rebuilding the search '. + 'index here: %s', + phutil_tag( + 'a', + array( + 'href' => 'https://phurl.io/u/reindex', + 'target' => '_blank', + ), + 'https://phurl.io/u/reindex')); + + $message[] = pht( + 'After rebuilding the index, run this command to clear this setup '. + 'warning:'); + + $message[] = phutil_tag( + 'pre', + array(), + 'phabricator/ $ ./bin/config done reindex'); + + $activity_message = phutil_implode_html("\n\n", $message); + + $this->newIssue('manual.reindex') + ->setName($activity_name) + ->setSummary($activity_summary) + ->setMessage($activity_message); + } + + private function raiseRebuildIdentitiesIssue() { + $activity_name = pht('Rebuild Repository Identities'); + $activity_summary = pht( + 'The mapping from VCS users to Phabricator users has changed '. + 'and must be rebuilt.'); + + $message = array(); + + $message[] = pht( + 'The way Phabricator attributes VCS activity to Phabricator users '. + 'has changed. There is a new indirection layer between the strings '. + 'that appear as VCS authors and committers (such as "John Developer '. + '") and the Phabricator user that gets associated '. + 'with VCS commits. This is to support situations where users '. + 'are incorrectly associated with commits by Phabricator making bad '. + 'guesses about the identity of the corresponding Phabricator user. '. + 'This also helps with situations where existing repositories are '. + 'imported without having created accounts for all the committers to '. + 'that repository. Until you rebuild these repository identities, you '. + 'are likely to encounter problems with future Phabricator features '. + 'which will rely on the existence of these identities.'); + + $message[] = pht( + 'You can rebuild repository identities while Phabricator is running.'); + + $message[] = pht( + 'To rebuild identities, run this command:'); + + $message[] = phutil_tag( + 'pre', + array(), + (string)csprintf( + 'phabricator/ $ ./bin/repository rebuild-identities --all')); + + $message[] = pht( + 'You can find more information about this new identity mapping '. + 'here: %s', + phutil_tag( + 'a', + array( + 'href' => 'https://phurl.io/u/repoIdentities', + 'target' => '_blank', + ), + 'https://phurl.io/u/repoIdentities')); + + $message[] = pht( + 'After rebuilding repository identities, run this command to clear '. + 'this setup warning:'); + + $message[] = phutil_tag( + 'pre', + array(), + 'phabricator/ $ ./bin/config done identities'); + + $activity_message = phutil_implode_html("\n\n", $message); + + $this->newIssue('manual.identities') + ->setName($activity_name) + ->setSummary($activity_summary) + ->setMessage($activity_message); } } diff --git a/src/applications/config/storage/PhabricatorConfigManualActivity.php b/src/applications/config/storage/PhabricatorConfigManualActivity.php index 44c26f77dd..9952021575 100644 --- a/src/applications/config/storage/PhabricatorConfigManualActivity.php +++ b/src/applications/config/storage/PhabricatorConfigManualActivity.php @@ -7,6 +7,8 @@ final class PhabricatorConfigManualActivity protected $parameters = array(); const TYPE_REINDEX = 'reindex'; + const TYPE_IDENTITIES = 'identities'; + protected function getConfiguration() { return array( From e5906f4e127eb9bba62397f1228fa5d01f52ce22 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 08:21:16 -0700 Subject: [PATCH 013/125] In Differential standalone views, disable some keyboard shortcuts which don't work Summary: Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files. We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI. Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled. Test Plan: - Viewed a revision in normal and standalone views. - No changes in normal view, and all keys still work ("N", "P", etc). - In standalone view, "?" no longer shows nonfunctional key commands. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19571 --- resources/celerity/map.php | 32 +++++++++---------- .../DifferentialChangesetViewController.php | 1 + .../view/DifferentialChangesetListView.php | 11 +++++++ .../js/application/diff/DiffChangesetList.js | 29 +++++++++++------ .../differential/behavior-populate.js | 3 +- 5 files changed, 50 insertions(+), 26 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 9c0d0eda0a..12d47cd5f9 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => 'f515619b', 'core.pkg.js' => '2058ec09', 'differential.pkg.css' => '06dc617c', - 'differential.pkg.js' => 'ef19e026', + 'differential.pkg.js' => '11a08e85', 'diffusion.pkg.css' => 'a2d17c7d', 'diffusion.pkg.js' => '6134c5a1', 'maniphest.pkg.css' => '4845691a', @@ -373,12 +373,12 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', 'rsrc/js/application/diff/DiffChangeset.js' => 'b49b59d6', - 'rsrc/js/application/diff/DiffChangesetList.js' => 'f0ffe8c3', + 'rsrc/js/application/diff/DiffChangesetList.js' => '7b95a80a', 'rsrc/js/application/diff/DiffInline.js' => 'e83d28f3', 'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832', 'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', - 'rsrc/js/application/differential/behavior-populate.js' => '419998ab', + 'rsrc/js/application/differential/behavior-populate.js' => 'f0eb6708', 'rsrc/js/application/differential/behavior-user-select.js' => 'a8d8459d', 'rsrc/js/application/diffusion/DiffusionLocateFileSource.js' => '00676f00', 'rsrc/js/application/diffusion/behavior-audit-preview.js' => 'd835b03a', @@ -594,7 +594,7 @@ return array( 'javelin-behavior-diff-preview-link' => '051c7832', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-feedback-preview' => '51c5ad07', - 'javelin-behavior-differential-populate' => '419998ab', + 'javelin-behavior-differential-populate' => 'f0eb6708', 'javelin-behavior-differential-user-select' => 'a8d8459d', 'javelin-behavior-diffusion-commit-branches' => 'bdaf4d04', 'javelin-behavior-diffusion-commit-graph' => '75b83cbb', @@ -752,7 +752,7 @@ return array( 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', 'phabricator-diff-changeset' => 'b49b59d6', - 'phabricator-diff-changeset-list' => 'f0ffe8c3', + 'phabricator-diff-changeset-list' => '7b95a80a', 'phabricator-diff-inline' => 'e83d28f3', 'phabricator-drag-and-drop-file-upload' => '58dea2fa', 'phabricator-draggable-list' => 'bea6e7f4', @@ -1133,14 +1133,6 @@ return array( 'javelin-workflow', 'phabricator-draggable-list', ), - '419998ab' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'phabricator-tooltip', - 'phabricator-diff-changeset-list', - 'phabricator-diff-changeset', - ), '4250a34e' => array( 'javelin-behavior', 'javelin-dom', @@ -1508,6 +1500,10 @@ return array( 'owners-path-editor', 'javelin-behavior', ), + '7b95a80a' => array( + 'javelin-install', + 'phuix-button-view', + ), '7cbe244b' => array( 'javelin-install', 'javelin-util', @@ -2117,9 +2113,13 @@ return array( 'javelin-workflow', 'javelin-json', ), - 'f0ffe8c3' => array( - 'javelin-install', - 'phuix-button-view', + 'f0eb6708' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'phabricator-tooltip', + 'phabricator-diff-changeset-list', + 'phabricator-diff-changeset', ), 'f1ff5494' => array( 'phui-button-css', diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index 9d02202f9f..c41f951394 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -276,6 +276,7 @@ final class DifferentialChangesetViewController extends DifferentialController { ->setDiff($diff) ->setTitle(pht('Standalone View')) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setIsStandalone(true) ->setParser($parser); if ($revision_id) { diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 59f73b5465..2ed963bae3 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -10,6 +10,7 @@ final class DifferentialChangesetListView extends AphrontView { private $whitespace; private $background; private $header; + private $isStandalone; private $standaloneURI; private $leftRawFileURI; @@ -124,6 +125,15 @@ final class DifferentialChangesetListView extends AphrontView { return $this; } + public function setIsStandalone($is_standalone) { + $this->isStandalone = $is_standalone; + return $this; + } + + public function getIsStandalone() { + return $this->isStandalone; + } + public function setBackground($background) { $this->background = $background; return $this; @@ -219,6 +229,7 @@ final class DifferentialChangesetListView extends AphrontView { 'changesetViewIDs' => $ids, 'inlineURI' => $this->inlineURI, 'inlineListURI' => $this->inlineListURI, + 'isStandalone' => $this->getIsStandalone(), 'pht' => array( 'Open in Editor' => pht('Open in Editor'), 'Show All Context' => pht('Show All Context'), diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 9e87fb0f31..66c80cb9cf 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -89,7 +89,8 @@ JX.install('DiffChangesetList', { properties: { translations: null, inlineURI: null, - inlineListURI: null + inlineListURI: null, + isStandalone: false }, members: { @@ -149,6 +150,12 @@ JX.install('DiffChangesetList', { this._initialized = true; var pht = this.getTranslations(); + // We may be viewing the normal "/D123" view (with all the changesets) + // or the standalone view (with just one changeset). In the standalone + // view, some options (like jumping to next or previous file) do not + // make sense and do not function. + var standalone = this.getIsStandalone(); + var label; label = pht('Jump to next change.'); @@ -157,11 +164,13 @@ JX.install('DiffChangesetList', { label = pht('Jump to previous change.'); this._installJumpKey('k', label, -1); - label = pht('Jump to next file.'); - this._installJumpKey('J', label, 1, 'file'); + if (!standalone) { + label = pht('Jump to next file.'); + this._installJumpKey('J', label, 1, 'file'); - label = pht('Jump to previous file.'); - this._installJumpKey('K', label, -1, 'file'); + label = pht('Jump to previous file.'); + this._installJumpKey('K', label, -1, 'file'); + } label = pht('Jump to next inline comment.'); this._installJumpKey('n', label, 1, 'comment'); @@ -176,11 +185,13 @@ JX.install('DiffChangesetList', { 'Jump to previous inline comment, including collapsed comments.'); this._installJumpKey('P', label, -1, 'comment', true); - label = pht('Hide or show the current file.'); - this._installKey('h', label, this._onkeytogglefile); + if (!standalone) { + label = pht('Hide or show the current file.'); + this._installKey('h', label, this._onkeytogglefile); - label = pht('Jump to the table of contents.'); - this._installKey('t', label, this._ontoc); + label = pht('Jump to the table of contents.'); + this._installKey('t', label, this._ontoc); + } label = pht('Reply to selected inline comment or change.'); this._installKey('r', label, JX.bind(this, this._onkeyreply, false)); diff --git a/webroot/rsrc/js/application/differential/behavior-populate.js b/webroot/rsrc/js/application/differential/behavior-populate.js index 0a4a7912e4..5b415fd131 100644 --- a/webroot/rsrc/js/application/differential/behavior-populate.js +++ b/webroot/rsrc/js/application/differential/behavior-populate.js @@ -61,7 +61,8 @@ JX.behavior('differential-populate', function(config, statics) { var changeset_list = new JX.DiffChangesetList() .setTranslations(JX.phtize(config.pht)) .setInlineURI(config.inlineURI) - .setInlineListURI(config.inlineListURI); + .setInlineListURI(config.inlineListURI) + .setIsStandalone(config.isStandalone); // Install and activate the current page. var page_id = JX.Quicksand.getCurrentPageID(); From b86dae6214ffccc0fec4bd29ea9b054f9eaa3ac3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 09:25:41 -0700 Subject: [PATCH 014/125] Fix an issue with error handling when no mailers are available Summary: Ref T13164. See PHI785. See D19546. I think I didn't test the updated error messaging here entirely properly, since I have some tasks in queue which error out here ("Missing argument 1 to newMailers(...)"). This is an error condition already, but we want to get through this call so we can raise a tailored message. Test Plan: Tasks which errored out here now succeed. This condition is only reachable if you misconfigure things in the first place. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19572 --- src/applications/metamta/storage/PhabricatorMetaMTAMail.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index d757868483..e7ebf6dc83 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -621,7 +621,7 @@ final class PhabricatorMetaMTAMail public function sendWithMailers(array $mailers) { if (!$mailers) { - $any_mailers = self::newMailers(); + $any_mailers = self::newMailers(array()); // NOTE: We can end up here with some custom list of "$mailers", like // from a unit test. In that case, this message could be misleading. We From fb3ae72e367f5ec57c706593d67975c4630463c6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 09:31:13 -0700 Subject: [PATCH 015/125] When cancelling addition of an Almanac interface, return to the Device page Summary: Fixes T13184. In Almanac, interfaces are always added to devices. However, if you "Add New Interface" and then "Cancel", you go to the nonexistent `/interface/` page. Instead, return to the device page. Test Plan: From a device page, clicked "Add Interface" and then "Cancel". Ended up back where I was. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13184 Differential Revision: https://secure.phabricator.com/D19573 --- src/applications/almanac/editor/AlmanacInterfaceEditEngine.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/applications/almanac/editor/AlmanacInterfaceEditEngine.php b/src/applications/almanac/editor/AlmanacInterfaceEditEngine.php index 2c68ed0374..30c371d6f9 100644 --- a/src/applications/almanac/editor/AlmanacInterfaceEditEngine.php +++ b/src/applications/almanac/editor/AlmanacInterfaceEditEngine.php @@ -127,6 +127,9 @@ final class AlmanacInterfaceEditEngine } protected function getObjectCreateCancelURI($object) { + if ($this->getDevice()) { + return $this->getDevice()->getURI(); + } return '/almanac/interface/'; } From 92a29f72c14c3f4329c337df0706849eb75e06e5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 09:49:22 -0700 Subject: [PATCH 016/125] Make the Drydock repository operation page slightly richer Summary: Ref T13164. See PHI788. The issue requests a "created" timestamp. Also add filtering for repository, state, and author. Test Plan: Used all filters. {F5795085} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19574 --- .../query/DrydockRepositoryOperationQuery.php | 13 +++++++ ...DrydockRepositoryOperationSearchEngine.php | 34 +++++++++++++++++++ .../storage/DrydockRepositoryOperation.php | 17 ++++++---- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php index 570e68e825..fb1f6583a8 100644 --- a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php +++ b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php @@ -9,6 +9,7 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { private $operationStates; private $operationTypes; private $isDismissed; + private $authorPHIDs; public function withIDs(array $ids) { $this->ids = $ids; @@ -45,6 +46,11 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { return $this; } + public function withAuthorPHIDs(array $phids) { + $this->authorPHIDs = $phids; + return $this; + } + public function newResultObject() { return new DrydockRepositoryOperation(); } @@ -165,6 +171,13 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { (int)$this->isDismissed); } + if ($this->authorPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'authorPHID IN (%Ls)', + $this->authorPHIDs); + } + return $where; } diff --git a/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php b/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php index 1de32be28f..eeb85329bf 100644 --- a/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php +++ b/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php @@ -18,11 +18,41 @@ final class DrydockRepositoryOperationSearchEngine protected function buildQueryFromParameters(array $map) { $query = $this->newQuery(); + if ($map['repositoryPHIDs']) { + $query->withRepositoryPHIDs($map['repositoryPHIDs']); + } + + if ($map['authorPHIDs']) { + $query->withAuthorPHIDs($map['authorPHIDs']); + } + + if ($map['states']) { + $query->withOperationStates($map['states']); + } + return $query; } protected function buildCustomSearchFields() { return array( + id(new PhabricatorSearchDatasourceField()) + ->setLabel(pht('Repositories')) + ->setKey('repositoryPHIDs') + ->setAliases(array('repository', 'repositories', 'repositoryPHID')) + ->setDatasource(new DiffusionRepositoryFunctionDatasource()), + + // NOTE: Repository operations aren't necessarily created by a real + // user, but for now they normally are. Just use a user typeahead until + // more use cases arise. + id(new PhabricatorUsersSearchField()) + ->setLabel(pht('Authors')) + ->setKey('authorPHIDs') + ->setAliases(array('author', 'authors', 'authorPHID')), + id(new PhabricatorSearchCheckboxesField()) + ->setLabel(pht('States')) + ->setKey('states') + ->setAliases(array('state')) + ->setOptions(DrydockRepositoryOperation::getOperationStateNameMap()), ); } @@ -72,6 +102,10 @@ final class DrydockRepositoryOperationSearchEngine $item->setStatusIcon($icon, $name); + + $created = phabricator_datetime($operation->getDateCreated(), $viewer); + $item->addIcon(null, $created); + $item->addByline( array( pht('Via:'), diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php index 190b5b1310..4e25280c3f 100644 --- a/src/applications/drydock/storage/DrydockRepositoryOperation.php +++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php @@ -99,6 +99,15 @@ final class DrydockRepositoryOperation extends DrydockDAO return $this; } + public static function getOperationStateNameMap() { + return array( + self::STATE_WAIT => pht('Waiting'), + self::STATE_WORK => pht('Working'), + self::STATE_DONE => pht('Done'), + self::STATE_FAIL => pht('Failed'), + ); + } + public static function getOperationStateIcon($state) { $map = array( self::STATE_WAIT => 'fa-clock-o', @@ -111,13 +120,7 @@ final class DrydockRepositoryOperation extends DrydockDAO } public static function getOperationStateName($state) { - $map = array( - self::STATE_WAIT => pht('Waiting'), - self::STATE_WORK => pht('Working'), - self::STATE_DONE => pht('Done'), - self::STATE_FAIL => pht('Failed'), - ); - + $map = self::getOperationStateNameMap(); return idx($map, $state, pht('', $state)); } From 9a15129b402fd7a15e81df3b5fe6fdf4a68d1ae0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 11:35:02 -0700 Subject: [PATCH 017/125] Remove 750ms timeout on owners path validation Summary: Ref T13164. See PHI748. Path validation has a 750ms timeout which blames to rP5038ab850c, in 2011. Production path validation is sometimes taking more than 750ms, particularly on the initial page load where we may validate many paths simultaneously. I have no idea why we have this timeout, and it isn't consistent with how we perform other AJAX requests. Just remove it. Test Plan: - Reproduced issue in production, saw all validation calls failing at 750ms. Actual underlying calls succeed, they just take more than 750ms to resolve. - Loaded path validator locally, got green checkmark. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19575 --- resources/celerity/map.php | 20 +++++++++---------- .../js/application/herald/PathTypeahead.js | 2 -- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 12d47cd5f9..2b81560e43 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -393,7 +393,7 @@ return array( 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '549459b8', 'rsrc/js/application/herald/HeraldRuleEditor.js' => 'dca75c0e', - 'rsrc/js/application/herald/PathTypeahead.js' => '662e9cea', + 'rsrc/js/application/herald/PathTypeahead.js' => '6d8c7912', 'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3', 'rsrc/js/application/maniphest/behavior-batch-selector.js' => 'ad54037e', 'rsrc/js/application/maniphest/behavior-line-chart.js' => 'e4232876', @@ -739,7 +739,7 @@ return array( 'owners-path-editor' => 'c96502cf', 'owners-path-editor-css' => '9c136c29', 'paste-css' => '9fcc9773', - 'path-typeahead' => '662e9cea', + 'path-typeahead' => '6d8c7912', 'people-picture-menu-item-css' => 'a06f7f34', 'people-profile-css' => '4df76faf', 'phabricator-action-list-view-css' => '0bcd9a45', @@ -1353,14 +1353,6 @@ return array( 'javelin-workflow', 'javelin-dom', ), - '662e9cea' => array( - 'javelin-install', - 'javelin-typeahead', - 'javelin-dom', - 'javelin-request', - 'javelin-typeahead-ondemand-source', - 'javelin-util', - ), 66888767 => array( 'javelin-behavior', 'javelin-stratcom', @@ -1436,6 +1428,14 @@ return array( 'javelin-typeahead', 'javelin-uri', ), + '6d8c7912' => array( + 'javelin-install', + 'javelin-typeahead', + 'javelin-dom', + 'javelin-request', + 'javelin-typeahead-ondemand-source', + 'javelin-util', + ), '70baed2f' => array( 'javelin-install', 'javelin-dom', diff --git a/webroot/rsrc/js/application/herald/PathTypeahead.js b/webroot/rsrc/js/application/herald/PathTypeahead.js index 0ce9823d13..eed6e62b62 100644 --- a/webroot/rsrc/js/application/herald/PathTypeahead.js +++ b/webroot/rsrc/js/application/herald/PathTypeahead.js @@ -208,8 +208,6 @@ JX.install('PathTypeahead', { this._validationInflight = validation_request; JX.DOM.setContent(error_display, JX.$H(this._icons.test)); - - validation_request.setTimeout(750); validation_request.send(); } } From 3b05e920e0094af372416839f36e34fc9af2458a Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Mon, 13 Aug 2018 14:21:56 -0700 Subject: [PATCH 018/125] Start changing DiffusionCommitController to use identities Summary: Depends on D19491. Test Plan: Viewed some commits where the identity was mapped to a user and another that wasn't; saw the header render either a link to the user or the identity object. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19492 --- .../controller/DiffusionCommitController.php | 32 +++------ .../storage/PhabricatorRepositoryCommit.php | 71 +++++++++++++++++++ .../storage/PhabricatorRepositoryIdentity.php | 8 +++ 3 files changed, 87 insertions(+), 24 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 67ffb73357..a6743b6557 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -46,6 +46,7 @@ final class DiffusionCommitController extends DiffusionController { ->needCommitData(true) ->needAuditRequests(true) ->setLimit(100) + ->needIdentities(true) ->execute(); $multiple_results = count($commits) > 1; @@ -504,15 +505,13 @@ final class DiffusionCommitController extends DiffusionController { $phids = $edge_query->getDestinationPHIDs(array($commit_phid)); - if ($data->getCommitDetail('authorPHID')) { - $phids[] = $data->getCommitDetail('authorPHID'); - } + if ($data->getCommitDetail('reviewerPHID')) { $phids[] = $data->getCommitDetail('reviewerPHID'); } - if ($data->getCommitDetail('committerPHID')) { - $phids[] = $data->getCommitDetail('committerPHID'); - } + + $phids[] = $commit->getCommitterDisplayPHID(); + $phids[] = $commit->getAuthorDisplayPHID(); // NOTE: We should never normally have more than a single push log, but // it can occur naturally if a commit is pushed, then the branch it was @@ -573,24 +572,11 @@ final class DiffusionCommitController extends DiffusionController { } } - $author_phid = $data->getCommitDetail('authorPHID'); - $author_name = $data->getAuthorName(); $author_epoch = $data->getCommitDetail('authorEpoch'); $committed_info = id(new PHUIStatusItemView()) - ->setNote(phabricator_datetime($commit->getEpoch(), $viewer)); - - $committer_phid = $data->getCommitDetail('committerPHID'); - $committer_name = $data->getCommitDetail('committer'); - if ($committer_phid) { - $committed_info->setTarget($handles[$committer_phid]->renderLink()); - } else if (strlen($committer_name)) { - $committed_info->setTarget($committer_name); - } else if ($author_phid) { - $committed_info->setTarget($handles[$author_phid]->renderLink()); - } else if (strlen($author_name)) { - $committed_info->setTarget($author_name); - } + ->setNote(phabricator_datetime($commit->getEpoch(), $viewer)) + ->setTarget($commit->renderAnyCommitter($viewer, $handles)); $committed_list = new PHUIStatusListView(); $committed_list->addItem($committed_info); @@ -716,7 +702,7 @@ final class DiffusionCommitController extends DiffusionController { return null; } - $author_phid = $data->getCommitDetail('authorPHID'); + $author_phid = $commit->getAuthorDisplayPHID(); $author_name = $data->getAuthorName(); $author_epoch = $data->getCommitDetail('authorEpoch'); $date = null; @@ -748,10 +734,8 @@ final class DiffusionCommitController extends DiffusionController { ->setImage($image_uri) ->setImageHref($image_href) ->setContent($content); - } - private function buildComments(PhabricatorRepositoryCommit $commit) { $timeline = $this->buildTransactionTimeline( $commit, diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index cf88c0eb54..8ad182ee7b 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -439,6 +439,77 @@ final class PhabricatorRepositoryCommit return $repository->formatCommitName($identifier, $local = true); } + /** + * Make a strong effort to find a way to render this commit's committer. + * This currently attempts to use @{PhabricatorRepositoryIdentity}, and + * falls back to examining the commit detail information. After we force + * the migration to using identities, update this method to remove the + * fallback. See T12164 for details. + */ + public function renderAnyCommitter(PhabricatorUser $viewer, array $handles) { + $committer = $this->renderCommitter($viewer, $handles); + if ($committer) { + return $committer; + } + + return $this->renderAuthor($viewer, $handles); + } + + public function renderCommitter(PhabricatorUser $viewer, array $handles) { + $committer_phid = $this->getCommitterDisplayPHID(); + if ($committer_phid) { + return $handles[$committer_phid]->renderLink(); + } + + $data = $this->getCommitData(); + $committer_name = $data->getCommitDetail('committer'); + if (strlen($committer_name)) { + return $committer_name; + } + + return null; + } + + public function renderAuthor(PhabricatorUser $viewer, array $handles) { + $author_phid = $this->getAuthorDisplayPHID(); + if ($author_phid) { + return $handles[$author_phid]->renderLink(); + } + + $data = $this->getCommitData(); + $author_name = $data->getAuthorName(); + if (strlen($author_name)) { + return $author_name; + } + + return null; + } + + public function hasCommitterIdentity() { + return ($this->getCommitterIdentity() !== null); + } + + public function hasAuthorIdentity() { + return ($this->getAuthorIdentity() !== null); + } + + public function getCommitterDisplayPHID() { + if ($this->hasCommitterIdentity()) { + return $this->getCommitterIdentity()->getIdentityDisplayPHID(); + } + + $data = $this->getCommitData(); + return $data->getCommitDetail('committerPHID'); + } + + public function getAuthorDisplayPHID() { + if ($this->hasAuthorIdentity()) { + return $this->getAuthorIdentity()->getIdentityDisplayPHID(); + } + + $data = $this->getCommitData(); + return $data->getCommitDetail('authorPHID'); + } /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php index bf421692a0..f801e06b89 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php +++ b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php @@ -78,6 +78,14 @@ final class PhabricatorRepositoryIdentity return ($this->currentEffectiveUserPHID != null); } + public function getIdentityDisplayPHID() { + if ($this->hasEffectiveUser()) { + return $this->getCurrentEffectiveUserPHID(); + } else { + return $this->getPHID(); + } + } + public function save() { if ($this->manuallySetUserPHID) { $this->currentEffectiveUserPHID = $this->manuallySetUserPHID; From cc1def6ceaab41483103919e94bd227da01f5cdc Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Mon, 13 Aug 2018 15:37:04 -0700 Subject: [PATCH 019/125] Remove some array typehints for passing around Summary: See discussion at https://secure.phabricator.com/D19492#241996 Test Plan: Refreshed a few Diffusion tabs; nothing broke. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19578 --- .../repository/storage/PhabricatorRepositoryCommit.php | 6 +++--- .../subscriptions/view/SubscriptionListDialogBuilder.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 8ad182ee7b..bf3b796a76 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -446,7 +446,7 @@ final class PhabricatorRepositoryCommit * the migration to using identities, update this method to remove the * fallback. See T12164 for details. */ - public function renderAnyCommitter(PhabricatorUser $viewer, array $handles) { + public function renderAnyCommitter(PhabricatorUser $viewer, $handles) { $committer = $this->renderCommitter($viewer, $handles); if ($committer) { return $committer; @@ -455,7 +455,7 @@ final class PhabricatorRepositoryCommit return $this->renderAuthor($viewer, $handles); } - public function renderCommitter(PhabricatorUser $viewer, array $handles) { + public function renderCommitter(PhabricatorUser $viewer, $handles) { $committer_phid = $this->getCommitterDisplayPHID(); if ($committer_phid) { return $handles[$committer_phid]->renderLink(); @@ -470,7 +470,7 @@ final class PhabricatorRepositoryCommit return null; } - public function renderAuthor(PhabricatorUser $viewer, array $handles) { + public function renderAuthor(PhabricatorUser $viewer, $handles) { $author_phid = $this->getAuthorDisplayPHID(); if ($author_phid) { return $handles[$author_phid]->renderLink(); diff --git a/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php b/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php index 7d1b8d799c..da157848ea 100644 --- a/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php +++ b/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php @@ -58,7 +58,7 @@ final class SubscriptionListDialogBuilder extends Phobject { ->addCancelButton($object_handle->getURI(), pht('Close')); } - private function buildBody(PhabricatorUser $viewer, array $handles) { + private function buildBody(PhabricatorUser $viewer, $handles) { $list = id(new PHUIObjectItemListView()) ->setUser($viewer); From ba25586016b2b6731860ab1c682ddeba65eed0bb Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 16 Aug 2018 06:39:07 +1000 Subject: [PATCH 020/125] Improve symbol generation scripts Summary: Currently the symbol generation scripts fail if passed a list containing no files because `explode("\n", $input)` returns `array("")` rather than `array()`. This means that a generic Harbormaster Build Plan with a step which executes `find . -type f -name '*.php' | ./scripts/generate_php_symbols.php` won't work because it fails in repositories that don't contain any PHP code. Test Plan: Ran `echo | generate_php_symbols` and saw no output instead of an exception. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19588 --- scripts/symbols/generate_ctags_symbols.php | 4 ++++ scripts/symbols/generate_php_symbols.php | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/scripts/symbols/generate_ctags_symbols.php b/scripts/symbols/generate_ctags_symbols.php index 8d77bfc478..e93b0c5cbc 100755 --- a/scripts/symbols/generate_ctags_symbols.php +++ b/scripts/symbols/generate_ctags_symbols.php @@ -39,6 +39,10 @@ $data = array(); $futures = array(); foreach (explode("\n", trim($input)) as $file) { + if (!strlen($file)) { + continue; + } + $file = Filesystem::readablePath($file); $futures[$file] = ctags_get_parser_future($file); } diff --git a/scripts/symbols/generate_php_symbols.php b/scripts/symbols/generate_php_symbols.php index db8412764e..af87d580d8 100755 --- a/scripts/symbols/generate_php_symbols.php +++ b/scripts/symbols/generate_php_symbols.php @@ -27,6 +27,10 @@ $data = array(); $futures = array(); foreach (explode("\n", trim($input)) as $file) { + if (!strlen($file)) { + continue; + } + $file = Filesystem::readablePath($file); $data[$file] = Filesystem::readFile($file); $futures[$file] = PhutilXHPASTBinary::getParserFuture($data[$file]); From 39d415e90eb3111c04cadee78045048bf3c4ec03 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 14:05:47 -0700 Subject: [PATCH 021/125] Move users to modular transactions Summary: Ref T13164. See PHI642. I'd like to provide a third-generation `user.edit` API endpoint and make `user.enable` and `user.disable` obsolete before meddling with policy details, even if it isn't full-fledged yet. Users do already have a transactions table and a Transaction-based editor, but it's only used for editing title, real name, etc. All of these are custom fields, so their support comes in automatically through CustomField extension code. Realign it for modular transactions so new code will be fully modern. There are no actual standalone transaction types yet so this diff is pretty thin. Test Plan: - Grepped for `UserProfileEditor`. - Edited a user's title/real name/icon. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19576 --- src/__phutil_library_map__.php | 8 +++++--- .../controller/PhabricatorPeopleProfileEditController.php | 5 ++--- ...ileEditor.php => PhabricatorUserTransactionEditor.php} | 5 ++--- src/applications/people/storage/PhabricatorUser.php | 2 +- .../people/storage/PhabricatorUserTransaction.php | 6 +++++- .../people/xaction/PhabricatorUserTransactionType.php | 4 ++++ 6 files changed, 19 insertions(+), 11 deletions(-) rename src/applications/people/editor/{PhabricatorUserProfileEditor.php => PhabricatorUserTransactionEditor.php} (73%) create mode 100644 src/applications/people/xaction/PhabricatorUserTransactionType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index bd4cfef6c2..47ea4e8bf1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4583,7 +4583,6 @@ phutil_register_library_map(array( 'PhabricatorUserPreferencesTransaction' => 'applications/settings/storage/PhabricatorUserPreferencesTransaction.php', 'PhabricatorUserPreferencesTransactionQuery' => 'applications/settings/query/PhabricatorUserPreferencesTransactionQuery.php', 'PhabricatorUserProfile' => 'applications/people/storage/PhabricatorUserProfile.php', - 'PhabricatorUserProfileEditor' => 'applications/people/editor/PhabricatorUserProfileEditor.php', 'PhabricatorUserProfileImageCacheType' => 'applications/people/cache/PhabricatorUserProfileImageCacheType.php', 'PhabricatorUserRealNameField' => 'applications/people/customfield/PhabricatorUserRealNameField.php', 'PhabricatorUserRolesField' => 'applications/people/customfield/PhabricatorUserRolesField.php', @@ -4593,6 +4592,8 @@ phutil_register_library_map(array( 'PhabricatorUserTestCase' => 'applications/people/storage/__tests__/PhabricatorUserTestCase.php', 'PhabricatorUserTitleField' => 'applications/people/customfield/PhabricatorUserTitleField.php', 'PhabricatorUserTransaction' => 'applications/people/storage/PhabricatorUserTransaction.php', + 'PhabricatorUserTransactionEditor' => 'applications/people/editor/PhabricatorUserTransactionEditor.php', + 'PhabricatorUserTransactionType' => 'applications/people/xaction/PhabricatorUserTransactionType.php', 'PhabricatorUsersEditField' => 'applications/transactions/editfield/PhabricatorUsersEditField.php', 'PhabricatorUsersPolicyRule' => 'applications/people/policyrule/PhabricatorUsersPolicyRule.php', 'PhabricatorUsersSearchField' => 'applications/people/searchfield/PhabricatorUsersSearchField.php', @@ -10575,7 +10576,6 @@ phutil_register_library_map(array( 'PhabricatorUserPreferencesTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorUserPreferencesTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorUserProfile' => 'PhabricatorUserDAO', - 'PhabricatorUserProfileEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorUserProfileImageCacheType' => 'PhabricatorUserCacheType', 'PhabricatorUserRealNameField' => 'PhabricatorUserCustomField', 'PhabricatorUserRolesField' => 'PhabricatorUserCustomField', @@ -10584,7 +10584,9 @@ phutil_register_library_map(array( 'PhabricatorUserStatusField' => 'PhabricatorUserCustomField', 'PhabricatorUserTestCase' => 'PhabricatorTestCase', 'PhabricatorUserTitleField' => 'PhabricatorUserCustomField', - 'PhabricatorUserTransaction' => 'PhabricatorApplicationTransaction', + 'PhabricatorUserTransaction' => 'PhabricatorModularTransaction', + 'PhabricatorUserTransactionEditor' => 'PhabricatorApplicationTransactionEditor', + 'PhabricatorUserTransactionType' => 'PhabricatorModularTransactionType', 'PhabricatorUsersEditField' => 'PhabricatorTokenizerEditField', 'PhabricatorUsersPolicyRule' => 'PhabricatorPolicyRule', 'PhabricatorUsersSearchField' => 'PhabricatorSearchTokenizerField', diff --git a/src/applications/people/controller/PhabricatorPeopleProfileEditController.php b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php index 3eda8a968b..4876f4495d 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileEditController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php @@ -38,10 +38,9 @@ final class PhabricatorPeopleProfileEditController new PhabricatorUserTransaction(), $request); - $editor = id(new PhabricatorUserProfileEditor()) + $editor = id(new PhabricatorUserTransactionEditor()) ->setActor($viewer) - ->setContentSource( - PhabricatorContentSource::newFromRequest($request)) + ->setContentSourceFromRequest($request) ->setContinueOnNoEffect(true); try { diff --git a/src/applications/people/editor/PhabricatorUserProfileEditor.php b/src/applications/people/editor/PhabricatorUserTransactionEditor.php similarity index 73% rename from src/applications/people/editor/PhabricatorUserProfileEditor.php rename to src/applications/people/editor/PhabricatorUserTransactionEditor.php index 9ec02ba2af..c0dfa941af 100644 --- a/src/applications/people/editor/PhabricatorUserProfileEditor.php +++ b/src/applications/people/editor/PhabricatorUserTransactionEditor.php @@ -1,6 +1,6 @@ Date: Mon, 13 Aug 2018 14:18:37 -0700 Subject: [PATCH 022/125] Add a modern "user.edit" API method for users Summary: Depends on D19576. Ref T13164. See PHI642. This adds an EditEngine for users and a `user.edit` modern API method. For now, all it supports is editing real name, blurb, title, and icon (same as "Edit Profile" from the UI). Test Plan: - Edited my stuff via the new API method. - Tried to edit another user, got rejected by policies. - Tried to create a user, got rejected by policies. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19577 --- src/__phutil_library_map__.php | 4 ++ .../conduit/UserEditConduitAPIMethod.php | 20 ++++++ .../customfield/PhabricatorUserBlurbField.php | 21 ++++++ .../customfield/PhabricatorUserIconField.php | 21 ++++++ .../PhabricatorUserRealNameField.php | 21 ++++++ .../customfield/PhabricatorUserTitleField.php | 21 ++++++ .../editor/PhabricatorUserEditEngine.php | 70 +++++++++++++++++++ 7 files changed, 178 insertions(+) create mode 100644 src/applications/people/conduit/UserEditConduitAPIMethod.php create mode 100644 src/applications/people/editor/PhabricatorUserEditEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 47ea4e8bf1..628f4958b5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4562,6 +4562,7 @@ phutil_register_library_map(array( 'PhabricatorUserCustomFieldNumericIndex' => 'applications/people/storage/PhabricatorUserCustomFieldNumericIndex.php', 'PhabricatorUserCustomFieldStringIndex' => 'applications/people/storage/PhabricatorUserCustomFieldStringIndex.php', 'PhabricatorUserDAO' => 'applications/people/storage/PhabricatorUserDAO.php', + 'PhabricatorUserEditEngine' => 'applications/people/editor/PhabricatorUserEditEngine.php', 'PhabricatorUserEditor' => 'applications/people/editor/PhabricatorUserEditor.php', 'PhabricatorUserEditorTestCase' => 'applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php', 'PhabricatorUserEmail' => 'applications/people/storage/PhabricatorUserEmail.php', @@ -5233,6 +5234,7 @@ phutil_register_library_map(array( 'TransactionSearchConduitAPIMethod' => 'applications/transactions/conduit/TransactionSearchConduitAPIMethod.php', 'UserConduitAPIMethod' => 'applications/people/conduit/UserConduitAPIMethod.php', 'UserDisableConduitAPIMethod' => 'applications/people/conduit/UserDisableConduitAPIMethod.php', + 'UserEditConduitAPIMethod' => 'applications/people/conduit/UserEditConduitAPIMethod.php', 'UserEnableConduitAPIMethod' => 'applications/people/conduit/UserEnableConduitAPIMethod.php', 'UserFindConduitAPIMethod' => 'applications/people/conduit/UserFindConduitAPIMethod.php', 'UserQueryConduitAPIMethod' => 'applications/people/conduit/UserQueryConduitAPIMethod.php', @@ -10547,6 +10549,7 @@ phutil_register_library_map(array( 'PhabricatorUserCustomFieldNumericIndex' => 'PhabricatorCustomFieldNumericIndexStorage', 'PhabricatorUserCustomFieldStringIndex' => 'PhabricatorCustomFieldStringIndexStorage', 'PhabricatorUserDAO' => 'PhabricatorLiskDAO', + 'PhabricatorUserEditEngine' => 'PhabricatorEditEngine', 'PhabricatorUserEditor' => 'PhabricatorEditor', 'PhabricatorUserEditorTestCase' => 'PhabricatorTestCase', 'PhabricatorUserEmail' => 'PhabricatorUserDAO', @@ -11388,6 +11391,7 @@ phutil_register_library_map(array( 'TransactionSearchConduitAPIMethod' => 'ConduitAPIMethod', 'UserConduitAPIMethod' => 'ConduitAPIMethod', 'UserDisableConduitAPIMethod' => 'UserConduitAPIMethod', + 'UserEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod', 'UserEnableConduitAPIMethod' => 'UserConduitAPIMethod', 'UserFindConduitAPIMethod' => 'UserConduitAPIMethod', 'UserQueryConduitAPIMethod' => 'UserConduitAPIMethod', diff --git a/src/applications/people/conduit/UserEditConduitAPIMethod.php b/src/applications/people/conduit/UserEditConduitAPIMethod.php new file mode 100644 index 0000000000..edc4204c3b --- /dev/null +++ b/src/applications/people/conduit/UserEditConduitAPIMethod.php @@ -0,0 +1,20 @@ +getModernFieldKey(); + } + public function getFieldName() { return pht('Blurb'); } @@ -50,6 +58,11 @@ final class PhabricatorUserBlurbField $this->value = $request->getStr($this->getFieldKey()); } + public function setValueFromStorage($value) { + $this->value = $value; + return $this; + } + public function renderEditControl(array $handles) { return id(new PhabricatorRemarkupControl()) ->setUser($this->getViewer()) @@ -85,4 +98,12 @@ final class PhabricatorUserBlurbField return 'block'; } + public function shouldAppearInConduitTransactions() { + return true; + } + + protected function newConduitEditParameterType() { + return new ConduitStringParameterType(); + } + } diff --git a/src/applications/people/customfield/PhabricatorUserIconField.php b/src/applications/people/customfield/PhabricatorUserIconField.php index a3f43cb7bf..0b25a9dd81 100644 --- a/src/applications/people/customfield/PhabricatorUserIconField.php +++ b/src/applications/people/customfield/PhabricatorUserIconField.php @@ -9,6 +9,14 @@ final class PhabricatorUserIconField return 'user:icon'; } + public function getModernFieldKey() { + return 'icon'; + } + + public function getFieldKeyForConduit() { + return $this->getModernFieldKey(); + } + public function getFieldName() { return pht('Icon'); } @@ -50,6 +58,11 @@ final class PhabricatorUserIconField $this->value = $request->getStr($this->getFieldKey()); } + public function setValueFromStorage($value) { + $this->value = $value; + return $this; + } + public function renderEditControl(array $handles) { return id(new PHUIFormIconSetControl()) ->setName($this->getFieldKey()) @@ -58,4 +71,12 @@ final class PhabricatorUserIconField ->setIconSet(new PhabricatorPeopleIconSet()); } + public function shouldAppearInConduitTransactions() { + return true; + } + + protected function newConduitEditParameterType() { + return new ConduitStringParameterType(); + } + } diff --git a/src/applications/people/customfield/PhabricatorUserRealNameField.php b/src/applications/people/customfield/PhabricatorUserRealNameField.php index 90e490fc3b..e7e610fb07 100644 --- a/src/applications/people/customfield/PhabricatorUserRealNameField.php +++ b/src/applications/people/customfield/PhabricatorUserRealNameField.php @@ -9,6 +9,14 @@ final class PhabricatorUserRealNameField return 'user:realname'; } + public function getModernFieldKey() { + return 'realName'; + } + + public function getFieldKeyForConduit() { + return $this->getModernFieldKey(); + } + public function getFieldName() { return pht('Real Name'); } @@ -53,6 +61,11 @@ final class PhabricatorUserRealNameField $this->value = $request->getStr($this->getFieldKey()); } + public function setValueFromStorage($value) { + $this->value = $value; + return $this; + } + public function renderEditControl(array $handles) { return id(new AphrontFormTextControl()) ->setName($this->getFieldKey()) @@ -65,4 +78,12 @@ final class PhabricatorUserRealNameField return PhabricatorEnv::getEnvConfig('account.editable'); } + public function shouldAppearInConduitTransactions() { + return true; + } + + protected function newConduitEditParameterType() { + return new ConduitStringParameterType(); + } + } diff --git a/src/applications/people/customfield/PhabricatorUserTitleField.php b/src/applications/people/customfield/PhabricatorUserTitleField.php index af6793e76e..5ceb774fa4 100644 --- a/src/applications/people/customfield/PhabricatorUserTitleField.php +++ b/src/applications/people/customfield/PhabricatorUserTitleField.php @@ -9,6 +9,14 @@ final class PhabricatorUserTitleField return 'user:title'; } + public function getModernFieldKey() { + return 'title'; + } + + public function getFieldKeyForConduit() { + return $this->getModernFieldKey(); + } + public function getFieldName() { return pht('Title'); } @@ -50,6 +58,11 @@ final class PhabricatorUserTitleField $this->value = $request->getStr($this->getFieldKey()); } + public function setValueFromStorage($value) { + $this->value = $value; + return $this; + } + public function renderEditControl(array $handles) { return id(new AphrontFormTextControl()) ->setName($this->getFieldKey()) @@ -57,4 +70,12 @@ final class PhabricatorUserTitleField ->setLabel($this->getFieldName()); } + public function shouldAppearInConduitTransactions() { + return true; + } + + protected function newConduitEditParameterType() { + return new ConduitStringParameterType(); + } + } diff --git a/src/applications/people/editor/PhabricatorUserEditEngine.php b/src/applications/people/editor/PhabricatorUserEditEngine.php new file mode 100644 index 0000000000..b978eb2618 --- /dev/null +++ b/src/applications/people/editor/PhabricatorUserEditEngine.php @@ -0,0 +1,70 @@ +getUsername()); + } + + protected function getObjectEditShortText($object) { + return $object->getMonogram(); + } + + protected function getObjectCreateShortText() { + return pht('Create User'); + } + + protected function getObjectName() { + return pht('User'); + } + + protected function getObjectViewURI($object) { + return $object->getURI(); + } + + protected function getCreateNewObjectPolicy() { + // At least for now, forbid creating new users via EditEngine. This is + // primarily enforcing that "user.edit" can not create users via the API. + return PhabricatorPolicies::POLICY_NOONE; + } + + protected function buildCustomEditFields($object) { + return array(); + } + +} From f9673a72a87d61edd69be864491518e83d4645f7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 15:32:24 -0700 Subject: [PATCH 023/125] Allow "user.edit" to enable or disable users Summary: Depends on D19577. Ref T13164. See PHI642. This adds modern transaction-oriented enable/disable support. Currently, this also doesn't let you disable normal users even when you're an administrator. I'll refine the policy model later in this change series, since that's also the goal here (let users set "Can Disable Users" to some more broad set of users than "Administrators"). This also leaves us with two different edit pathways: the old UserEditor one and the new UserTransactionEditor one. The next couple diffs will redefine the other pathways in terms of this pathway. Test Plan: - Enabled/disabled a bot. - Tried to disable another non-bot user. This isn't allowed yet, since even as an administrator you don't have CAN_EDIT on them and currently need it: right now, there's no way for a particular set of transactions to say they can move forward with reduced permissions. - Tried to enable/disable myself. This isn't allowed since you can't enable/disable yourself. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19579 --- src/__phutil_library_map__.php | 2 + .../editor/PhabricatorUserEditEngine.php | 13 +++- .../PhabricatorUserDisableTransaction.php | 72 +++++++++++++++++++ .../PhabricatorUserTransactionType.php | 11 ++- 4 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 src/applications/people/xaction/PhabricatorUserDisableTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 628f4958b5..c8b84d19fe 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4562,6 +4562,7 @@ phutil_register_library_map(array( 'PhabricatorUserCustomFieldNumericIndex' => 'applications/people/storage/PhabricatorUserCustomFieldNumericIndex.php', 'PhabricatorUserCustomFieldStringIndex' => 'applications/people/storage/PhabricatorUserCustomFieldStringIndex.php', 'PhabricatorUserDAO' => 'applications/people/storage/PhabricatorUserDAO.php', + 'PhabricatorUserDisableTransaction' => 'applications/people/xaction/PhabricatorUserDisableTransaction.php', 'PhabricatorUserEditEngine' => 'applications/people/editor/PhabricatorUserEditEngine.php', 'PhabricatorUserEditor' => 'applications/people/editor/PhabricatorUserEditor.php', 'PhabricatorUserEditorTestCase' => 'applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php', @@ -10549,6 +10550,7 @@ phutil_register_library_map(array( 'PhabricatorUserCustomFieldNumericIndex' => 'PhabricatorCustomFieldNumericIndexStorage', 'PhabricatorUserCustomFieldStringIndex' => 'PhabricatorCustomFieldStringIndexStorage', 'PhabricatorUserDAO' => 'PhabricatorLiskDAO', + 'PhabricatorUserDisableTransaction' => 'PhabricatorUserTransactionType', 'PhabricatorUserEditEngine' => 'PhabricatorEditEngine', 'PhabricatorUserEditor' => 'PhabricatorEditor', 'PhabricatorUserEditorTestCase' => 'PhabricatorTestCase', diff --git a/src/applications/people/editor/PhabricatorUserEditEngine.php b/src/applications/people/editor/PhabricatorUserEditEngine.php index b978eb2618..c547426b12 100644 --- a/src/applications/people/editor/PhabricatorUserEditEngine.php +++ b/src/applications/people/editor/PhabricatorUserEditEngine.php @@ -64,7 +64,18 @@ final class PhabricatorUserEditEngine } protected function buildCustomEditFields($object) { - return array(); + return array( + id(new PhabricatorBoolEditField()) + ->setKey('disabled') + ->setOptions(pht('Active'), pht('Disabled')) + ->setLabel(pht('Disabled')) + ->setDescription(pht('Disable the user.')) + ->setTransactionType(PhabricatorUserDisableTransaction::TRANSACTIONTYPE) + ->setIsConduitOnly(true) + ->setConduitDescription(pht('Disable or enable the user.')) + ->setConduitTypeDescription(pht('True to disable the user.')) + ->setValue($object->getIsDisabled()), + ); } } diff --git a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php new file mode 100644 index 0000000000..0ea6ef36c1 --- /dev/null +++ b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php @@ -0,0 +1,72 @@ +getIsDisabled(); + } + + public function generateNewValue($object, $value) { + return (bool)$value; + } + + public function applyInternalEffects($object, $value) { + $object->setIsDisabled((int)$value); + + $this->newUserLog(PhabricatorUserLog::ACTION_DISABLE) + ->setOldValue((bool)$object->getIsDisabled()) + ->setNewValue((bool)$value) + ->save(); + } + + public function getTitle() { + $new = $this->getNewValue(); + if ($new) { + return pht( + '%s disabled this user.', + $this->renderAuthor()); + } else { + return pht( + '%s enabled this user.', + $this->renderAuthor()); + } + } + + public function getTitleForFeed() { + $new = $this->getNewValue(); + if ($new) { + return pht( + '%s disabled %s.', + $this->renderAuthor(), + $this->renderObject()); + } else { + return pht( + '%s enabled %s.', + $this->renderAuthor(), + $this->renderObject()); + } + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + foreach ($xactions as $xaction) { + $is_disabled = (bool)$object->getIsDisabled(); + + if ((bool)$xaction->getNewValue() === $is_disabled) { + continue; + } + + if ($this->getActingAsPHID() === $object->getPHID()) { + $errors[] = $this->newInvalidError( + pht('You can not enable or disable your own account.')); + } + } + + return $errors; + } + +} diff --git a/src/applications/people/xaction/PhabricatorUserTransactionType.php b/src/applications/people/xaction/PhabricatorUserTransactionType.php index 89392fd039..dcd45d480e 100644 --- a/src/applications/people/xaction/PhabricatorUserTransactionType.php +++ b/src/applications/people/xaction/PhabricatorUserTransactionType.php @@ -1,4 +1,13 @@ getActor(), + $this->getObject()->getPHID(), + $action); + } + +} From 296bf046a8122c451ba0bf1a93733dccccaeb390 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 10:44:52 -0700 Subject: [PATCH 024/125] Remove deprecated Maniphest "Can Edit " capabilities Summary: Depends on D19579. Fixes T10003. These have been deprecated with a setup warning about their impending removal for about two and a half years. Ref T13164. See PHI642. My overall goal here is to simplify how we handle transactions which have special policy behaviors. In particular, I'm hoping to replace `ApplicationTransactionEditor->requireCapabilities()` with a new, more clear policy check. A problem with `requireCapabilities()` is that it doesn't actually enforce any policies in almost all cases: the default is "nothing", not CAN_EDIT. So it ends up looking like it's the right place to specialize policy checks, but it usually isn't. For "Disable", I need to be able to weaken the check selectively (you can disable users if you have the permission, even if you can't edit them otherwise). We have a handful of other edits which work like this (notably, leaving and joining projects) but they're very rare. Test Plan: Grepped for all removed classes. Edited a Maniphest task. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164, T10003 Differential Revision: https://secure.phabricator.com/D19581 --- src/__phutil_library_map__.php | 10 --- .../PhabricatorExtraConfigSetupCheck.php | 65 ------------------- .../PhabricatorManiphestApplication.php | 5 -- .../ManiphestEditAssignCapability.php | 15 ----- .../ManiphestEditPoliciesCapability.php | 16 ----- .../ManiphestEditPriorityCapability.php | 16 ----- .../ManiphestEditProjectsCapability.php | 16 ----- .../ManiphestEditStatusCapability.php | 15 ----- .../editor/ManiphestTransactionEditor.php | 45 ------------- .../query/ManiphestTaskSearchEngine.php | 6 +- 10 files changed, 1 insertion(+), 208 deletions(-) delete mode 100644 src/applications/maniphest/capability/ManiphestEditAssignCapability.php delete mode 100644 src/applications/maniphest/capability/ManiphestEditPoliciesCapability.php delete mode 100644 src/applications/maniphest/capability/ManiphestEditPriorityCapability.php delete mode 100644 src/applications/maniphest/capability/ManiphestEditProjectsCapability.php delete mode 100644 src/applications/maniphest/capability/ManiphestEditStatusCapability.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c8b84d19fe..af6fa96ba3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1646,13 +1646,8 @@ phutil_register_library_map(array( 'ManiphestDAO' => 'applications/maniphest/storage/ManiphestDAO.php', 'ManiphestDefaultEditCapability' => 'applications/maniphest/capability/ManiphestDefaultEditCapability.php', 'ManiphestDefaultViewCapability' => 'applications/maniphest/capability/ManiphestDefaultViewCapability.php', - 'ManiphestEditAssignCapability' => 'applications/maniphest/capability/ManiphestEditAssignCapability.php', 'ManiphestEditConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestEditConduitAPIMethod.php', 'ManiphestEditEngine' => 'applications/maniphest/editor/ManiphestEditEngine.php', - 'ManiphestEditPoliciesCapability' => 'applications/maniphest/capability/ManiphestEditPoliciesCapability.php', - 'ManiphestEditPriorityCapability' => 'applications/maniphest/capability/ManiphestEditPriorityCapability.php', - 'ManiphestEditProjectsCapability' => 'applications/maniphest/capability/ManiphestEditProjectsCapability.php', - 'ManiphestEditStatusCapability' => 'applications/maniphest/capability/ManiphestEditStatusCapability.php', 'ManiphestEmailCommand' => 'applications/maniphest/command/ManiphestEmailCommand.php', 'ManiphestGetTaskTransactionsConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestGetTaskTransactionsConduitAPIMethod.php', 'ManiphestHovercardEngineExtension' => 'applications/maniphest/engineextension/ManiphestHovercardEngineExtension.php', @@ -7152,13 +7147,8 @@ phutil_register_library_map(array( 'ManiphestDAO' => 'PhabricatorLiskDAO', 'ManiphestDefaultEditCapability' => 'PhabricatorPolicyCapability', 'ManiphestDefaultViewCapability' => 'PhabricatorPolicyCapability', - 'ManiphestEditAssignCapability' => 'PhabricatorPolicyCapability', 'ManiphestEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod', 'ManiphestEditEngine' => 'PhabricatorEditEngine', - 'ManiphestEditPoliciesCapability' => 'PhabricatorPolicyCapability', - 'ManiphestEditPriorityCapability' => 'PhabricatorPolicyCapability', - 'ManiphestEditProjectsCapability' => 'PhabricatorPolicyCapability', - 'ManiphestEditStatusCapability' => 'PhabricatorPolicyCapability', 'ManiphestEmailCommand' => 'MetaMTAEmailTransactionCommand', 'ManiphestGetTaskTransactionsConduitAPIMethod' => 'ManiphestConduitAPIMethod', 'ManiphestHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index 84fd5bedf2..7df50a4524 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -361,69 +361,4 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { return $ancient_config; } - private function executeManiphestFieldChecks() { - $maniphest_appclass = 'PhabricatorManiphestApplication'; - if (!PhabricatorApplication::isClassInstalled($maniphest_appclass)) { - return; - } - - $capabilities = array( - ManiphestEditAssignCapability::CAPABILITY, - ManiphestEditPoliciesCapability::CAPABILITY, - ManiphestEditPriorityCapability::CAPABILITY, - ManiphestEditProjectsCapability::CAPABILITY, - ManiphestEditStatusCapability::CAPABILITY, - ); - - // Check for any of these capabilities set to anything other than - // "All Users". - - $any_set = false; - $app = new PhabricatorManiphestApplication(); - foreach ($capabilities as $capability) { - $setting = $app->getPolicy($capability); - if ($setting != PhabricatorPolicies::POLICY_USER) { - $any_set = true; - break; - } - } - - if (!$any_set) { - return; - } - - $issue_summary = pht( - 'Maniphest is currently configured with deprecated policy settings '. - 'which will be removed in a future version of Phabricator.'); - - - $message = pht( - 'Some policy settings in Maniphest are now deprecated and will be '. - 'removed in a future version of Phabricator. You are currently using '. - 'at least one of these settings.'. - "\n\n". - 'The deprecated settings are "Can Assign Tasks", '. - '"Can Edit Task Policies", "Can Prioritize Tasks", '. - '"Can Edit Task Projects", and "Can Edit Task Status". You can '. - 'find these settings in Applications, or follow the link below.'. - "\n\n". - 'You can find discussion of this change (including rationale and '. - 'recommendations on how to configure similar features) in the upstream, '. - 'at the link below.'. - "\n\n". - 'To resolve this issue, set all of these policies to "All Users" after '. - 'making any necessary form customization changes.'); - - $more_href = 'https://secure.phabricator.com/T10003'; - $edit_href = '/applications/view/PhabricatorManiphestApplication/'; - - $issue = $this->newIssue('maniphest.T10003-per-field-policies') - ->setShortName(pht('Deprecated Policies')) - ->setName(pht('Deprecated Maniphest Field Policies')) - ->setSummary($issue_summary) - ->setMessage($message) - ->addLink($more_href, pht('Learn More: Upstream Discussion')) - ->addLink($edit_href, pht('Edit These Settings')); - } - } diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php index e57b9b545e..1ed4096660 100644 --- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php +++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php @@ -85,11 +85,6 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication { 'template' => ManiphestTaskPHIDType::TYPECONST, 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), - ManiphestEditStatusCapability::CAPABILITY => array(), - ManiphestEditAssignCapability::CAPABILITY => array(), - ManiphestEditPoliciesCapability::CAPABILITY => array(), - ManiphestEditPriorityCapability::CAPABILITY => array(), - ManiphestEditProjectsCapability::CAPABILITY => array(), ManiphestBulkEditCapability::CAPABILITY => array(), ); } diff --git a/src/applications/maniphest/capability/ManiphestEditAssignCapability.php b/src/applications/maniphest/capability/ManiphestEditAssignCapability.php deleted file mode 100644 index 5da3adf55e..0000000000 --- a/src/applications/maniphest/capability/ManiphestEditAssignCapability.php +++ /dev/null @@ -1,15 +0,0 @@ -setTask($object); } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - parent::requireCapabilities($object, $xaction); - - $app_capability_map = array( - ManiphestTaskPriorityTransaction::TRANSACTIONTYPE => - ManiphestEditPriorityCapability::CAPABILITY, - ManiphestTaskStatusTransaction::TRANSACTIONTYPE => - ManiphestEditStatusCapability::CAPABILITY, - ManiphestTaskOwnerTransaction::TRANSACTIONTYPE => - ManiphestEditAssignCapability::CAPABILITY, - PhabricatorTransactions::TYPE_EDIT_POLICY => - ManiphestEditPoliciesCapability::CAPABILITY, - PhabricatorTransactions::TYPE_VIEW_POLICY => - ManiphestEditPoliciesCapability::CAPABILITY, - ); - - - $transaction_type = $xaction->getTransactionType(); - - $app_capability = null; - if ($transaction_type == PhabricatorTransactions::TYPE_EDGE) { - switch ($xaction->getMetadataValue('edge:type')) { - case PhabricatorProjectObjectHasProjectEdgeType::EDGECONST: - $app_capability = ManiphestEditProjectsCapability::CAPABILITY; - break; - } - } else { - $app_capability = idx($app_capability_map, $transaction_type); - } - - if ($app_capability) { - $app = id(new PhabricatorApplicationQuery()) - ->setViewer($this->getActor()) - ->withClasses(array('PhabricatorManiphestApplication')) - ->executeOne(); - PhabricatorPolicyFilter::requireCapability( - $this->getActor(), - $app, - $app_capability); - } - } - protected function adjustObjectForPolicyChecks( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index 68f87a6f6b..437362fa41 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -369,11 +369,7 @@ final class ManiphestTaskSearchEngine $can_edit_priority = false; $can_bulk_edit = false; } else { - $can_edit_priority = PhabricatorPolicyFilter::hasCapability( - $viewer, - $this->getApplication(), - ManiphestEditPriorityCapability::CAPABILITY); - + $can_edit_priority = true; $can_bulk_edit = PhabricatorPolicyFilter::hasCapability( $viewer, $this->getApplication(), From a39852ae1b4a131354a606753495f052ea836e74 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 13:19:53 -0700 Subject: [PATCH 025/125] Remove pointless requireCapabilities() method from PhabricatorProjectColumnTransactionEditor Summary: Depends on D19581. Ref T13164. This method has no effect: - You must always have CAN_EDIT to reach an Editor in the first place. - Per previous change, I'm going to restructure this so transactions explicitly check CAN_EDIT by default anyway. Test Plan: Tried to edit and hide a project column as a user without permission, hit global permission checks long before reaching this method. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19582 --- ...abricatorProjectColumnTransactionEditor.php | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/applications/project/editor/PhabricatorProjectColumnTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectColumnTransactionEditor.php index 19f059b8b5..d494767085 100644 --- a/src/applications/project/editor/PhabricatorProjectColumnTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectColumnTransactionEditor.php @@ -137,22 +137,4 @@ final class PhabricatorProjectColumnTransactionEditor return $errors; } - - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case PhabricatorProjectColumnTransaction::TYPE_NAME: - case PhabricatorProjectColumnTransaction::TYPE_STATUS: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - return; - } - - return parent::requireCapabilities($object, $xaction); - } - } From 24d4445845489cb3fa6c9f1e7ab3dfd8dbe40255 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 13:28:37 -0700 Subject: [PATCH 026/125] Remove pointless requireCapabilities() method from PhabricatorRepositoryEditor Summary: Depends on D19582. Ref T13164. It's not possible to reach the editor without passing through a CAN_EDIT check, and it shouldn't be necessarily to manually specify that edits require CAN_EDIT by default. Test Plan: Grepped for `RepositoryEditor`, verified that all callsites pass through a CAN_EDIT check. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19583 --- .../editor/PhabricatorRepositoryEditor.php | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index 768121de5a..882c1a11fa 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -236,40 +236,6 @@ final class PhabricatorRepositoryEditor } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case PhabricatorRepositoryTransaction::TYPE_ACTIVATE: - case PhabricatorRepositoryTransaction::TYPE_NAME: - case PhabricatorRepositoryTransaction::TYPE_DESCRIPTION: - case PhabricatorRepositoryTransaction::TYPE_ENCODING: - case PhabricatorRepositoryTransaction::TYPE_DEFAULT_BRANCH: - case PhabricatorRepositoryTransaction::TYPE_TRACK_ONLY: - case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE_ONLY: - case PhabricatorRepositoryTransaction::TYPE_UUID: - case PhabricatorRepositoryTransaction::TYPE_SVN_SUBPATH: - case PhabricatorRepositoryTransaction::TYPE_VCS: - case PhabricatorRepositoryTransaction::TYPE_NOTIFY: - case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE: - case PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY: - case PhabricatorRepositoryTransaction::TYPE_DANGEROUS: - case PhabricatorRepositoryTransaction::TYPE_ENORMOUS: - case PhabricatorRepositoryTransaction::TYPE_SLUG: - case PhabricatorRepositoryTransaction::TYPE_SERVICE: - case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES: - case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE: - case PhabricatorRepositoryTransaction::TYPE_STAGING_URI: - case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - break; - } - } - protected function validateTransaction( PhabricatorLiskDAO $object, $type, From 3b92da22f4333d5bfa050d0da4397648147cee09 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 14:29:19 -0700 Subject: [PATCH 027/125] Move the hierarchical edit policy check in Phriction from requireCapabilities() to validateTransactions() Summary: Depends on D19583. Ref T13164. This continues the work of getting rid of `requireCapabilities()`. This check is valid, but can be a `validateTransactions()` check instead. This is generally more consistent with how other applications work (e.g., creating subprojects). The UI for this isn't terribly great: you get a policy error //after// you try to create the object. But that's how it worked before, so this isn't any worse than it was. The actual policy exception is (very) slightly more clear now (raised against the right object). Test Plan: - Created a child as a user with permission to do so to make sure I didn't break that. - Set edit permission on `a/` to just me, tried to create `a/b/` as another user, got a policy exception since they can't edit the parent. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19584 --- .../editor/PhrictionTransactionEditor.php | 52 ------------------- .../PhrictionDocumentTitleTransaction.php | 23 ++++++++ 2 files changed, 23 insertions(+), 52 deletions(-) diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php index e6bf46c150..b9f2c0e2ca 100644 --- a/src/applications/phriction/editor/PhrictionTransactionEditor.php +++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php @@ -516,58 +516,6 @@ final class PhrictionTransactionEditor } return $error; } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - /* - * New objects have a special case. If a user can't see - * x/y - * then definitely don't let them make some - * x/y/z - * We need to load the direct parent to handle this case. - */ - if ($this->getIsNewObject()) { - $actor = $this->requireActor(); - $parent_doc = null; - $ancestral_slugs = PhabricatorSlug::getAncestry($object->getSlug()); - // No ancestral slugs is "/"; the first person gets to play with "/". - if ($ancestral_slugs) { - $parent = end($ancestral_slugs); - $parent_doc = id(new PhrictionDocumentQuery()) - ->setViewer($actor) - ->withSlugs(array($parent)) - ->executeOne(); - // If the $actor can't see the $parent_doc then they can't create - // the child $object; throw a policy exception. - if (!$parent_doc) { - id(new PhabricatorPolicyFilter()) - ->setViewer($actor) - ->raisePolicyExceptions(true) - ->rejectObject( - $object, - $object->getEditPolicy(), - PhabricatorPolicyCapability::CAN_EDIT); - } - - // If the $actor can't edit the $parent_doc then they can't create - // the child $object; throw a policy exception. - if (!PhabricatorPolicyFilter::hasCapability( - $actor, - $parent_doc, - PhabricatorPolicyCapability::CAN_EDIT)) { - id(new PhabricatorPolicyFilter()) - ->setViewer($actor) - ->raisePolicyExceptions(true) - ->rejectObject( - $object, - $object->getEditPolicy(), - PhabricatorPolicyCapability::CAN_EDIT); - } - } - } - return parent::requireCapabilities($object, $xaction); - } protected function supportsSearch() { return true; diff --git a/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php index 4f1ba850a7..5c97d1bec8 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php @@ -91,6 +91,29 @@ final class PhrictionDocumentTitleTransaction pht('Documents must have a title.')); } + if ($this->isNewObject()) { + // No ancestral slugs is "/". No ancestry checks apply when creating the + // root document. + $ancestral_slugs = PhabricatorSlug::getAncestry($object->getSlug()); + if ($ancestral_slugs) { + // You must be able to view and edit the parent document to create a new + // child. + $parent_document = id(new PhrictionDocumentQuery()) + ->setViewer($this->getActor()) + ->withSlugs(array(last($ancestral_slugs))) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$parent_document) { + $errors[] = $this->newInvalidError( + pht('You can not create a document which does not have a parent.')); + } + } + } + return $errors; } From 7e29ec2e2a66087528027b7e80cc548ec30f6fff Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 14:46:59 -0700 Subject: [PATCH 028/125] Move the "Can Lock Projects" check from requireCapabilities() to transaction validation Summary: Depends on D19584. Ref T13164. This check is an //extra// check: you need EDIT //and// this capability. Thus, we can do it in validation without issues. Test Plan: - This code isn't reachable today: all methods of applying this transaction do a separate check for "Can Lock" upfront. - Commented out the "Can Lock" check in the LockController, tried to lock as a user without permission. Was rejected with a policy exception. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19585 --- .../editor/PhabricatorProjectTransactionEditor.php | 6 ------ .../xaction/PhabricatorProjectLockTransaction.php | 8 ++++++++ .../storage/PhabricatorModularTransactionType.php | 10 ++++++++++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index 2a90d6ce29..ef0cc65c8e 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -120,12 +120,6 @@ final class PhabricatorProjectTransactionEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case PhabricatorProjectLockTransaction::TRANSACTIONTYPE: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - newv($this->getEditorApplicationClass(), array()), - ProjectCanLockProjectsCapability::CAPABILITY); - return; case PhabricatorTransactions::TYPE_EDGE: switch ($xaction->getMetadataValue('edge:type')) { case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST: diff --git a/src/applications/project/xaction/PhabricatorProjectLockTransaction.php b/src/applications/project/xaction/PhabricatorProjectLockTransaction.php index 42551dfb2c..c54c9284ce 100644 --- a/src/applications/project/xaction/PhabricatorProjectLockTransaction.php +++ b/src/applications/project/xaction/PhabricatorProjectLockTransaction.php @@ -53,4 +53,12 @@ final class PhabricatorProjectLockTransaction } } + public function validateTransactions($object, array $xactions) { + if ($xactions) { + $this->requireApplicationCapability( + ProjectCanLockProjectsCapability::CAPABILITY); + } + return array(); + } + } diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index eaf7d029ce..b0714aeccf 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -356,4 +356,14 @@ abstract class PhabricatorModularTransactionType return array(); } + protected function requireApplicationCapability($capability) { + $application_class = $this->getEditor()->getEditorApplicationClass(); + $application = newv($application_class, array()); + + PhabricatorPolicyFilter::requireCapability( + $this->getActor(), + $application, + $capability); + } + } From 0ccf1410e057cede3e2c2a552cb78981d8ac8179 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 16 Aug 2018 10:07:28 -0700 Subject: [PATCH 029/125] Give PhabricatorAuthPassword a formal CAN_EDIT policy Summary: Depends on D19585. Ref T13164. This is a precursor for D19586, which causes Editors to start doing more explicit CAN_EDIT checks. Passwords have an Editor, but don't actually define a CAN_EDIT capability. Define one (you can edit a password if you can edit the object the password is associated with). (Today, this object is always a User -- this table just unified VCS passwords and Account passwords so they can be handled more consistently.) Test Plan: - With D19586, ran unit tests and got a pass. - Edited my own password. - Tried to edit another user's password and wasn't permitted to. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19592 --- src/applications/auth/storage/PhabricatorAuthPassword.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/applications/auth/storage/PhabricatorAuthPassword.php b/src/applications/auth/storage/PhabricatorAuthPassword.php index 5343e622fd..3bcb95693e 100644 --- a/src/applications/auth/storage/PhabricatorAuthPassword.php +++ b/src/applications/auth/storage/PhabricatorAuthPassword.php @@ -178,6 +178,7 @@ final class PhabricatorAuthPassword public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, ); } @@ -195,7 +196,7 @@ final class PhabricatorAuthPassword public function getExtendedPolicy($capability, PhabricatorUser $viewer) { return array( - array($this->getObject(), PhabricatorPolicyCapability::CAN_VIEW), + array($this->getObject(), $capability), ); } From a48e6897a4180ec074f4581c677448b2ac400812 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 16 Aug 2018 11:07:37 -0700 Subject: [PATCH 030/125] Remove obsolete setup check call to Maniphest "Can Edit " field checks Summary: Ref T13164. Missed this in D19581. Test Plan: - Forced setup checks to re-run by visiting {nav Config > Setup Issues} explicitly. - Before patch: fatal on call to nonexistent method. - After patch: setup issues. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19593 --- .../config/check/PhabricatorExtraConfigSetupCheck.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index 7df50a4524..4bf51602ba 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -84,8 +84,6 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { $issue->addPhabricatorConfig($key); } } - - $this->executeManiphestFieldChecks(); } /** From 438edde03150984b267347b0374998149f2c45e9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 16 Aug 2018 13:48:42 -0700 Subject: [PATCH 031/125] Add some missing aural button labels for accessibility Summary: Ref T13164. See PHI823. (See that issue for some more details and discussion.) Add aural labels to various buttons which were missing reasonable aural labels. The "Search" button (magnifying glass in the global search input) had an entire menu thing inside it. I moved that one level up and it doesn't look like it broke anything (?). All the other changes are pretty straightforward. Test Plan: {F5806497} {F5806498} - Will follow up on the issue to make sure things are in better shape for the reporting user. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19594 --- ...abricatorFavoritesMainMenuBarExtension.php | 3 +- .../PeopleMainMenuBarExtension.php | 3 +- ...catorApplicationTransactionCommentView.php | 2 +- .../view/PHUIDiffInlineCommentDetailView.php | 15 ++++++--- .../menu/PhabricatorMainMenuSearchView.php | 32 +++++++++++-------- .../page/menu/PhabricatorMainMenuView.php | 3 +- src/view/phui/PHUIButtonView.php | 25 +++++++++++++-- src/view/phui/PHUIHeadThingView.php | 2 +- src/view/phui/PHUITimelineEventView.php | 2 +- 9 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/applications/favorites/engineextension/PhabricatorFavoritesMainMenuBarExtension.php b/src/applications/favorites/engineextension/PhabricatorFavoritesMainMenuBarExtension.php index 9afb836b94..7b5d6d0720 100644 --- a/src/applications/favorites/engineextension/PhabricatorFavoritesMainMenuBarExtension.php +++ b/src/applications/favorites/engineextension/PhabricatorFavoritesMainMenuBarExtension.php @@ -30,7 +30,8 @@ final class PhabricatorFavoritesMainMenuBarExtension ->addClass('phabricator-core-user-menu') ->setNoCSS(true) ->setDropdown(true) - ->setDropdownMenu($dropdown); + ->setDropdownMenu($dropdown) + ->setAuralLabel(pht('Favorites Menu')); return array( $favorites_menu, diff --git a/src/applications/people/engineextension/PeopleMainMenuBarExtension.php b/src/applications/people/engineextension/PeopleMainMenuBarExtension.php index 152fb2becf..e19cedd305 100644 --- a/src/applications/people/engineextension/PeopleMainMenuBarExtension.php +++ b/src/applications/people/engineextension/PeopleMainMenuBarExtension.php @@ -43,7 +43,8 @@ final class PeopleMainMenuBarExtension ->setIcon($profile_image) ->addClass('phabricator-core-user-menu') ->setHasCaret(true) - ->setNoCSS(true); + ->setNoCSS(true) + ->setAuralLabel(pht('Account Menu')); return array( $user_menu, diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php index a5dcfcb4ae..c0d46e21a5 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php @@ -230,7 +230,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { 'div', array( 'style' => 'background-image: url('.$image_uri.')', - 'class' => 'phui-comment-image', + 'class' => 'phui-comment-image visual-only', )); $wedge = phutil_tag( 'div', diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index 27a489a705..a32197059a 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -233,7 +233,8 @@ final class PHUIDiffInlineCommentDetailView ->setIcon('fa-reply') ->setTooltip(pht('Reply')) ->addSigil('differential-inline-reply') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Reply')); } if ($this->editable && !$this->preview) { @@ -242,14 +243,16 @@ final class PHUIDiffInlineCommentDetailView ->setIcon('fa-pencil') ->setTooltip(pht('Edit')) ->addSigil('differential-inline-edit') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Edit')); $action_buttons[] = id(new PHUIButtonView()) ->setTag('a') ->setIcon('fa-trash-o') ->setTooltip(pht('Delete')) ->addSigil('differential-inline-delete') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Delete')); } else if ($this->preview) { $links[] = javelin_tag( @@ -268,7 +271,8 @@ final class PHUIDiffInlineCommentDetailView ->setTooltip(pht('Delete')) ->setIcon('fa-trash-o') ->addSigil('differential-inline-delete') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Delete')); } if (!$this->preview && $this->canHide()) { @@ -277,7 +281,8 @@ final class PHUIDiffInlineCommentDetailView ->setTooltip(pht('Collapse')) ->setIcon('fa-times') ->addSigil('hide-inline') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Collapse')); } $done_button = null; diff --git a/src/view/page/menu/PhabricatorMainMenuSearchView.php b/src/view/page/menu/PhabricatorMainMenuSearchView.php index 0b2ca8ab10..15319a357e 100644 --- a/src/view/page/menu/PhabricatorMainMenuSearchView.php +++ b/src/view/page/menu/PhabricatorMainMenuSearchView.php @@ -94,21 +94,24 @@ final class PhabricatorMainMenuSearchView extends AphrontView { 'action' => '/search/', 'method' => 'POST', ), - phutil_tag_div('phabricator-main-menu-search-container', array( - $input, - phutil_tag( - 'button', - array( - 'id' => $button_id, - 'class' => 'phui-icon-view phui-font-fa fa-search', + phutil_tag( + 'div', + array( + 'class' => 'phabricator-main-menu-search-container', + ), + array( + $input, + phutil_tag( + 'button', + array( + 'id' => $button_id, + 'class' => 'phui-icon-view phui-font-fa fa-search', ), - array( - $selector, - $search_text, - )), - $primary_input, - $target, - ))); + $search_text), + $selector, + $primary_input, + $target, + ))); return $form; } @@ -207,6 +210,7 @@ final class PhabricatorMainMenuSearchView extends AphrontView { id(new PHUIIconView()) ->addSigil('global-search-dropdown-icon') ->setIcon($current_icon)) + ->setAuralLabel(pht('Configure Global Search')) ->setDropdown(true); $input = javelin_tag( diff --git a/src/view/page/menu/PhabricatorMainMenuView.php b/src/view/page/menu/PhabricatorMainMenuView.php index 583952b0a9..67badce911 100644 --- a/src/view/page/menu/PhabricatorMainMenuView.php +++ b/src/view/page/menu/PhabricatorMainMenuView.php @@ -232,7 +232,8 @@ final class PhabricatorMainMenuView extends AphrontView { ->addClass('phabricator-core-user-menu') ->addClass('phabricator-core-user-mobile-menu') ->setNoCSS(true) - ->setDropdownMenu($dropdown); + ->setDropdownMenu($dropdown) + ->setAuralLabel(pht('Page Menu')); } private function renderApplicationMenu() { diff --git a/src/view/phui/PHUIButtonView.php b/src/view/phui/PHUIButtonView.php index 91c336eaaf..1fb2206428 100644 --- a/src/view/phui/PHUIButtonView.php +++ b/src/view/phui/PHUIButtonView.php @@ -31,6 +31,7 @@ final class PHUIButtonView extends AphrontTagView { private $noCSS; private $hasCaret; private $buttonType = self::BUTTONTYPE_DEFAULT; + private $auralLabel; public function setName($name) { $this->name = $name; @@ -123,6 +124,15 @@ final class PHUIButtonView extends AphrontTagView { return $this->buttonType; } + public function setAuralLabel($aural_label) { + $this->auralLabel = $aural_label; + return $this; + } + + public function getAuralLabel() { + return $this->auralLabel; + } + public function setIcon($icon, $first = true) { if (!($icon instanceof PHUIIconView)) { $icon = id(new PHUIIconView()) @@ -265,10 +275,21 @@ final class PHUIButtonView extends AphrontTagView { $caret = phutil_tag('span', array('class' => 'caret'), ''); } + $aural = null; + if ($this->auralLabel !== null) { + $aural = phutil_tag( + 'span', + array( + 'class' => 'aural-only', + ), + $this->auralLabel); + } + + if ($this->iconFirst == true) { - return array($icon, $text, $caret); + return array($aural, $icon, $text, $caret); } else { - return array($text, $icon, $caret); + return array($aural, $text, $icon, $caret); } } } diff --git a/src/view/phui/PHUIHeadThingView.php b/src/view/phui/PHUIHeadThingView.php index dd788399d4..ab2feee984 100644 --- a/src/view/phui/PHUIHeadThingView.php +++ b/src/view/phui/PHUIHeadThingView.php @@ -55,7 +55,7 @@ final class PHUIHeadThingView extends AphrontTagView { $image = phutil_tag( 'a', array( - 'class' => 'phui-head-thing-image', + 'class' => 'phui-head-thing-image visual-only', 'style' => 'background-image: url('.$this->image.');', 'href' => $this->imageHref, )); diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php index e64b9b06b2..e44332c1b5 100644 --- a/src/view/phui/PHUITimelineEventView.php +++ b/src/view/phui/PHUITimelineEventView.php @@ -404,7 +404,7 @@ final class PHUITimelineEventView extends AphrontView { ($this->userHandle->getURI()) ? 'a' : 'div', array( 'style' => 'background-image: url('.$image_uri.')', - 'class' => 'phui-timeline-image', + 'class' => 'phui-timeline-image visual-only', 'href' => $this->userHandle->getURI(), ), ''); From 5c4c593af32578a406857d2433c1be2a74403128 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Fri, 17 Aug 2018 12:23:59 -0700 Subject: [PATCH 032/125] Update DiffusionLastModifiedController to use identities Summary: Ref T12164. Updates another controller to use identities. Test Plan: Pretty ad-hoc, but loaded the main pages of several different repos with and without repo identities. I'm not totally convinced the `author` from this data structure is actually being used: ``` $return = array( 'commit' => $modified, 'date' => $date, 'author' => $author, 'details' => $details, ); ``` Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T12164 Differential Revision: https://secure.phabricator.com/D19580 --- .../controller/DiffusionBlameController.php | 8 ++-- .../DiffusionLastModifiedController.php | 47 ++++++------------- .../PhabricatorRepositoryIdentityPHIDType.php | 1 + .../storage/PhabricatorRepositoryCommit.php | 4 +- 4 files changed, 21 insertions(+), 39 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBlameController.php b/src/applications/diffusion/controller/DiffusionBlameController.php index 85ab393ef6..42cabbc0d9 100644 --- a/src/applications/diffusion/controller/DiffusionBlameController.php +++ b/src/applications/diffusion/controller/DiffusionBlameController.php @@ -24,6 +24,7 @@ final class DiffusionBlameController extends DiffusionController { ->setViewer($viewer) ->withRepository($repository) ->withIdentifiers($identifiers) + ->needIdentities(true) ->execute(); $commits = mpull($commits, null, 'getCommitIdentifier'); } else { @@ -68,10 +69,7 @@ final class DiffusionBlameController extends DiffusionController { $handle_phids = array(); foreach ($commits as $commit) { - $author_phid = $commit->getAuthorPHID(); - if ($author_phid) { - $handle_phids[] = $author_phid; - } + $handle_phids[] = $commit->getAuthorDisplayPHID(); } foreach ($revisions as $revision) { @@ -117,7 +115,7 @@ final class DiffusionBlameController extends DiffusionController { $author_phid = null; if ($commit) { - $author_phid = $commit->getAuthorPHID(); + $author_phid = $commit->getAuthorDisplayPHID(); } if (!$author_phid && $revision) { diff --git a/src/applications/diffusion/controller/DiffusionLastModifiedController.php b/src/applications/diffusion/controller/DiffusionLastModifiedController.php index 945f8a58b5..1a31d3a2ba 100644 --- a/src/applications/diffusion/controller/DiffusionLastModifiedController.php +++ b/src/applications/diffusion/controller/DiffusionLastModifiedController.php @@ -35,6 +35,7 @@ final class DiffusionLastModifiedController extends DiffusionController { ->withRepository($drequest->getRepository()) ->withIdentifiers(array_values($modified_map)) ->needCommitData(true) + ->needIdentities(true) ->execute(); $commit_map = mpull($commit_map, null, 'getCommitIdentifier'); } else { @@ -54,9 +55,8 @@ final class DiffusionLastModifiedController extends DiffusionController { $phids = array(); foreach ($commits as $commit) { - $data = $commit->getCommitData(); - $phids[] = $data->getCommitDetail('authorPHID'); - $phids[] = $data->getCommitDetail('committerPHID'); + $phids[] = $commit->getCommitterDisplayPHID(); + $phids[] = $commit->getAuthorDisplayPHID(); } $phids = array_filter($phids); $handles = $this->loadViewerHandles($phids); @@ -110,38 +110,21 @@ final class DiffusionLastModifiedController extends DiffusionController { $date = ''; } - $data = $commit->getCommitData(); - if ($data) { - $author_phid = $data->getCommitDetail('authorPHID'); - if ($author_phid && isset($handles[$author_phid])) { - $author = $handles[$author_phid]->renderLink(); - } else { - $author = DiffusionView::renderName($data->getAuthorName()); - } + $author = $commit->renderAuthor($viewer, $handles); + $committer = $commit->renderCommitter($viewer, $handles); - $committer = $data->getCommitDetail('committer'); - if ($committer) { - $committer_phid = $data->getCommitDetail('committerPHID'); - if ($committer_phid && isset($handles[$committer_phid])) { - $committer = $handles[$committer_phid]->renderLink(); - } else { - $committer = DiffusionView::renderName($committer); - } - if ($author != $committer) { - $author = hsprintf('%s/%s', $author, $committer); - } - } - - $details = DiffusionView::linkDetail( - $drequest->getRepository(), - $commit->getCommitIdentifier(), - $data->getSummary()); - $details = AphrontTableView::renderSingleDisplayLine($details); - } else { - $author = ''; - $details = ''; + if ($author != $committer) { + $author = hsprintf('%s/%s', $author, $committer); } + $data = $commit->getCommitData(); + $details = DiffusionView::linkDetail( + $drequest->getRepository(), + $commit->getCommitIdentifier(), + $data->getSummary()); + $details = AphrontTableView::renderSingleDisplayLine($details); + + $return = array( 'commit' => $modified, 'date' => $date, diff --git a/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php b/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php index 5bb4d5b907..5d9b06e0a3 100644 --- a/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php +++ b/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php @@ -39,6 +39,7 @@ final class PhabricatorRepositoryIdentityPHIDType $handle->setObjectName(pht('Identity %d', $id)); $handle->setName($name); $handle->setURI($identity->getURI()); + $handle->setIcon('fa-user'); } } diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index bf3b796a76..4cb9def346 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -464,7 +464,7 @@ final class PhabricatorRepositoryCommit $data = $this->getCommitData(); $committer_name = $data->getCommitDetail('committer'); if (strlen($committer_name)) { - return $committer_name; + return DiffusionView::renderName($committer_name); } return null; @@ -479,7 +479,7 @@ final class PhabricatorRepositoryCommit $data = $this->getCommitData(); $author_name = $data->getAuthorName(); if (strlen($author_name)) { - return $author_name; + return DiffusionView::renderName($author_name); } return null; From 75a5dd8d8cc0ba8cdf274ab509284ab4f7c0577c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 17 Aug 2018 10:53:28 -0700 Subject: [PATCH 033/125] Add more accessibility labels for screen readers Summary: Depends on D19594. See PHI823. Ref T13164. - Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers". - Add a label for the floating header display options menu in Differential. - Add `role="button"` to `PHUIButtonView` objects that we render with an `` tag. Test Plan: Viewed a revision with `?__aural__=true`: - Saw "Remove Action: ..." label. - Saw "Display Options" label. - Used inspector to verify that some `` now have ``. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19595 --- resources/celerity/map.php | 48 +++++++++---------- .../view/DifferentialChangesetListView.php | 1 + ...catorApplicationTransactionCommentView.php | 6 ++- src/view/phui/PHUIButtonView.php | 8 ++++ .../js/application/diff/DiffChangesetList.js | 7 +-- .../transactions/behavior-comment-actions.js | 5 +- webroot/rsrc/js/phuix/PHUIXButtonView.js | 25 ++++++++++ 7 files changed, 71 insertions(+), 29 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 2b81560e43..c60c2f7e85 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => 'f515619b', 'core.pkg.js' => '2058ec09', 'differential.pkg.css' => '06dc617c', - 'differential.pkg.js' => '11a08e85', + 'differential.pkg.js' => 'c1cfa143', 'diffusion.pkg.css' => 'a2d17c7d', 'diffusion.pkg.js' => '6134c5a1', 'maniphest.pkg.css' => '4845691a', @@ -373,7 +373,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', 'rsrc/js/application/diff/DiffChangeset.js' => 'b49b59d6', - 'rsrc/js/application/diff/DiffChangesetList.js' => '7b95a80a', + 'rsrc/js/application/diff/DiffChangesetList.js' => 'e0b984b5', 'rsrc/js/application/diff/DiffInline.js' => 'e83d28f3', 'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832', 'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07', @@ -423,7 +423,7 @@ return array( 'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e2e0a072', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08', 'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => '887ad43f', - 'rsrc/js/application/transactions/behavior-comment-actions.js' => '9a6dd75c', + 'rsrc/js/application/transactions/behavior-comment-actions.js' => '54110499', 'rsrc/js/application/transactions/behavior-reorder-configs.js' => 'd7a74243', 'rsrc/js/application/transactions/behavior-reorder-fields.js' => 'b59e1e96', 'rsrc/js/application/transactions/behavior-show-older-transactions.js' => '8f29b364', @@ -506,7 +506,7 @@ return array( 'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8', 'rsrc/js/phuix/PHUIXActionView.js' => '8d4a8c72', 'rsrc/js/phuix/PHUIXAutocomplete.js' => 'df1bbd34', - 'rsrc/js/phuix/PHUIXButtonView.js' => '8a91e1ac', + 'rsrc/js/phuix/PHUIXButtonView.js' => '85ac9772', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => '04b2ae03', 'rsrc/js/phuix/PHUIXExample.js' => '68af71ca', 'rsrc/js/phuix/PHUIXFormControl.js' => '210a16c1', @@ -575,7 +575,7 @@ return array( 'javelin-behavior-bulk-job-reload' => 'edf8a145', 'javelin-behavior-calendar-month-view' => 'fe33e256', 'javelin-behavior-choose-control' => '327a00d1', - 'javelin-behavior-comment-actions' => '9a6dd75c', + 'javelin-behavior-comment-actions' => '54110499', 'javelin-behavior-config-reorder-fields' => 'b6993408', 'javelin-behavior-conpherence-menu' => '4047cd35', 'javelin-behavior-conpherence-participant-pane' => 'd057e45a', @@ -752,7 +752,7 @@ return array( 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', 'phabricator-diff-changeset' => 'b49b59d6', - 'phabricator-diff-changeset-list' => '7b95a80a', + 'phabricator-diff-changeset-list' => 'e0b984b5', 'phabricator-diff-inline' => 'e83d28f3', 'phabricator-drag-and-drop-file-upload' => '58dea2fa', 'phabricator-draggable-list' => 'bea6e7f4', @@ -858,7 +858,7 @@ return array( 'phuix-action-list-view' => 'b5c256b8', 'phuix-action-view' => '8d4a8c72', 'phuix-autocomplete' => 'df1bbd34', - 'phuix-button-view' => '8a91e1ac', + 'phuix-button-view' => '85ac9772', 'phuix-dropdown-menu' => '04b2ae03', 'phuix-form-control-view' => '210a16c1', 'phuix-icon-view' => 'bff6884b', @@ -1251,6 +1251,15 @@ return array( 'javelin-vector', 'javelin-typeahead-static-source', ), + 54110499 => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-workflow', + 'javelin-dom', + 'phuix-form-control-view', + 'phuix-icon-view', + 'javelin-behavior-phabricator-gesture', + ), '549459b8' => array( 'javelin-behavior', ), @@ -1500,10 +1509,6 @@ return array( 'owners-path-editor', 'javelin-behavior', ), - '7b95a80a' => array( - 'javelin-install', - 'phuix-button-view', - ), '7cbe244b' => array( 'javelin-install', 'javelin-util', @@ -1533,6 +1538,10 @@ return array( 'javelin-dom', 'javelin-stratcom', ), + '85ac9772' => array( + 'javelin-install', + 'javelin-dom', + ), '85ee8ce6' => array( 'aphront-dialog-view-css', ), @@ -1560,10 +1569,6 @@ return array( 'javelin-install', 'javelin-dom', ), - '8a91e1ac' => array( - 'javelin-install', - 'javelin-dom', - ), '8ce821c5' => array( 'phabricator-notification', 'javelin-stratcom', @@ -1640,15 +1645,6 @@ return array( 'javelin-mask', 'phabricator-drag-and-drop-file-upload', ), - '9a6dd75c' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-dom', - 'phuix-form-control-view', - 'phuix-icon-view', - 'javelin-behavior-phabricator-gesture', - ), '9a860428' => array( 'javelin-behavior', 'javelin-dom', @@ -2021,6 +2017,10 @@ return array( 'phuix-icon-view', 'phabricator-prefab', ), + 'e0b984b5' => array( + 'javelin-install', + 'phuix-button-view', + ), 'e1d25dfb' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 2ed963bae3..14de553e59 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -309,6 +309,7 @@ final class DifferentialChangesetListView extends AphrontView { 'Show All Inlines' => pht('Show All Inlines'), 'List Inline Comments' => pht('List Inline Comments'), + 'Display Options' => pht('Display Options'), 'Hide or show all inline comments.' => pht('Hide or show all inline comments.'), diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php index c0d46e21a5..7469004e1b 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php @@ -319,14 +319,18 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { foreach ($comment_actions as $key => $comment_action) { $key = $comment_action->getKey(); + $label = $comment_action->getLabel(); + + $action_map[$key] = array( 'key' => $key, - 'label' => $comment_action->getLabel(), + 'label' => $label, 'type' => $comment_action->getPHUIXControlType(), 'spec' => $comment_action->getPHUIXControlSpecification(), 'initialValue' => $comment_action->getInitialValue(), 'groupKey' => $comment_action->getGroupKey(), 'conflictKey' => $comment_action->getConflictKey(), + 'auralLabel' => pht('Remove Action: %s', $label), ); $type_map[$key] = $comment_action; diff --git a/src/view/phui/PHUIButtonView.php b/src/view/phui/PHUIButtonView.php index 1fb2206428..bad24122bc 100644 --- a/src/view/phui/PHUIButtonView.php +++ b/src/view/phui/PHUIButtonView.php @@ -233,6 +233,13 @@ final class PHUIButtonView extends AphrontTagView { $classes = array(); } + // See PHI823. If we aren't rendering a "