From e730f55e8857bd9a32d05d83150953f4e1557601 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 14 Mar 2021 12:59:47 -0700 Subject: [PATCH 01/29] Retitle "Recently Open Revisions" panel to "Recent Open Revisions" Summary: Ref T13639. In D17754, this: > OPEN REVISIONS > Recently updated open revisions affecting this file. ...was simplified into: > RECENTLY OPEN REVISIONS This is a bit misleading, since the panel doesn't contain "recently open" results. Use "Recent Open" instead, which is a bit more consistent with other product text. This is still slightly misleading, but probably close enough. Test Plan: Read text. Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21612 --- .../diffusion/controller/DiffusionBrowseController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 76efa41866..042c21d232 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -963,7 +963,7 @@ final class DiffusionBrowseController extends DiffusionController { } $header = id(new PHUIHeaderView()) - ->setHeader(pht('Recently Open Revisions')); + ->setHeader(pht('Recent Open Revisions')); $list = id(new DifferentialRevisionListView()) ->setViewer($viewer) From bcd592cf7e153386bcb1b489c68c0c90a6701b4a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Mar 2021 09:37:14 -0700 Subject: [PATCH 02/29] Remove support for "paths" parameter in "differential.query" Summary: See T13639. This change simplifies providing a more modern approach to querying this data via "differential.revision.search". Test Plan: Called "differential.query" with paths (got an error) and without paths (got a valid query result). Differential Revision: https://secure.phabricator.com/D21613 --- .../DifferentialQueryConduitAPIMethod.php | 49 +++---------------- 1 file changed, 6 insertions(+), 43 deletions(-) diff --git a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php index eeef36ee69..c8dd77ac8a 100644 --- a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php @@ -38,7 +38,7 @@ final class DifferentialQueryConduitAPIMethod 'authors' => 'optional list', 'ccs' => 'optional list', 'reviewers' => 'optional list', - 'paths' => 'optional list>', + 'paths' => 'unsupported', 'commitHashes' => 'optional list>', 'status' => 'optional '.$status_const, 'order' => 'optional '.$order_const, @@ -92,48 +92,11 @@ final class DifferentialQueryConduitAPIMethod } if ($path_pairs) { - $paths = array(); - foreach ($path_pairs as $pair) { - list($callsign, $path) = $pair; - $paths[] = $path; - } - - $path_map = id(new DiffusionPathIDQuery($paths))->loadPathIDs(); - if (count($path_map) != count($paths)) { - $unknown_paths = array(); - foreach ($paths as $p) { - if (!idx($path_map, $p)) { - $unknown_paths[] = $p; - } - } - throw id(new ConduitException('ERR-INVALID-PARAMETER')) - ->setErrorDescription( - pht( - 'Unknown paths: %s', - implode(', ', $unknown_paths))); - } - - $repos = array(); - foreach ($path_pairs as $pair) { - list($callsign, $path) = $pair; - if (!idx($repos, $callsign)) { - $repos[$callsign] = id(new PhabricatorRepositoryQuery()) - ->setViewer($request->getUser()) - ->withCallsigns(array($callsign)) - ->executeOne(); - - if (!$repos[$callsign]) { - throw id(new ConduitException('ERR-INVALID-PARAMETER')) - ->setErrorDescription( - pht( - 'Unknown repo callsign: %s', - $callsign)); - } - } - $repo = $repos[$callsign]; - - $query->withPath($repo->getID(), idx($path_map, $path)); - } + throw new Exception( + pht( + 'Parameter "paths" to Conduit API method "differential.query" is '. + 'no longer supported. Use the "paths" constraint to '. + '"differential.revision.search" instead. See T13639.')); } if ($commit_hashes) { From c317d16bdd565d34eaad0850aa505fffc8ef811e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Mar 2021 10:02:34 -0700 Subject: [PATCH 03/29] Lift peculiar side effect of path indexing out of indexer Summary: Ref T13639. Updating the affected path table has a peculiar side effect from D19426, which is a simplification of a peculiar side effect from earlier. Don't condition Owners behavior on path index behavior. Test Plan: Created a revision. Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21614 --- .../editor/DifferentialTransactionEditor.php | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 964cf38722..af81af73a8 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -349,6 +349,9 @@ final class DifferentialTransactionEditor case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: $diff = $this->requireDiff($xaction->getNewValue(), true); + $this->ownersDiff = $diff; + $this->ownersChangesets = $diff->getChangesets(); + // Update these denormalized index tables when we attach a new // diff to a revision. @@ -1299,13 +1302,6 @@ final class DifferentialTransactionEditor $paths[] = $path_prefix.'/'.$changeset->getFilename(); } - // If this change affected paths, save the changesets so we can apply - // Owners rules to them later. - if ($paths) { - $this->ownersDiff = $diff; - $this->ownersChangesets = $changesets; - } - // Mark this as also touching all parent paths, so you can see all pending // changes to any file within a directory. $all_paths = array(); From e919b4c35a2aa2ea92a4266ec491e56e25fee1d5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Mar 2021 13:41:37 -0700 Subject: [PATCH 04/29] Add a debug view of the "Affected Path" index to Differential Summary: Ref T13639. The "Affected Path" table is currently hard to inspect: there's no UI, and using MySQL just gives you a bunch of IDs. Add a simple UI and a debug-mode link to it. Test Plan: {F8539098} {F8539099} Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21615 --- src/__phutil_library_map__.php | 2 + .../PhabricatorDifferentialApplication.php | 2 + ...rentialRevisionAffectedPathsController.php | 127 ++++++++++++++++++ .../PhabricatorSystemDebugUIEventListener.php | 10 ++ 4 files changed, 141 insertions(+) create mode 100644 src/applications/differential/controller/DifferentialRevisionAffectedPathsController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3cae40d75e..ea1e40dd57 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -613,6 +613,7 @@ phutil_register_library_map(array( 'DifferentialRevisionAcceptTransaction' => 'applications/differential/xaction/DifferentialRevisionAcceptTransaction.php', 'DifferentialRevisionActionTransaction' => 'applications/differential/xaction/DifferentialRevisionActionTransaction.php', 'DifferentialRevisionAffectedFilesHeraldField' => 'applications/differential/herald/DifferentialRevisionAffectedFilesHeraldField.php', + 'DifferentialRevisionAffectedPathsController' => 'applications/differential/controller/DifferentialRevisionAffectedPathsController.php', 'DifferentialRevisionAuthorHeraldField' => 'applications/differential/herald/DifferentialRevisionAuthorHeraldField.php', 'DifferentialRevisionAuthorPackagesHeraldField' => 'applications/differential/herald/DifferentialRevisionAuthorPackagesHeraldField.php', 'DifferentialRevisionAuthorProjectsHeraldField' => 'applications/differential/herald/DifferentialRevisionAuthorProjectsHeraldField.php', @@ -6733,6 +6734,7 @@ phutil_register_library_map(array( 'DifferentialRevisionAcceptTransaction' => 'DifferentialRevisionReviewTransaction', 'DifferentialRevisionActionTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionAffectedFilesHeraldField' => 'DifferentialRevisionHeraldField', + 'DifferentialRevisionAffectedPathsController' => 'DifferentialController', 'DifferentialRevisionAuthorHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionAuthorPackagesHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionAuthorProjectsHeraldField' => 'DifferentialRevisionHeraldField', diff --git a/src/applications/differential/application/PhabricatorDifferentialApplication.php b/src/applications/differential/application/PhabricatorDifferentialApplication.php index b1312228d1..5980f738b0 100644 --- a/src/applications/differential/application/PhabricatorDifferentialApplication.php +++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php @@ -74,6 +74,8 @@ final class PhabricatorDifferentialApplication => 'DifferentialRevisionOperationController', 'inlines/(?P[1-9]\d*)/' => 'DifferentialRevisionInlinesController', + 'paths/(?P[1-9]\d*)/' + => 'DifferentialRevisionAffectedPathsController', ), 'comment/' => array( 'inline/' => array( diff --git a/src/applications/differential/controller/DifferentialRevisionAffectedPathsController.php b/src/applications/differential/controller/DifferentialRevisionAffectedPathsController.php new file mode 100644 index 0000000000..a485d5ecfe --- /dev/null +++ b/src/applications/differential/controller/DifferentialRevisionAffectedPathsController.php @@ -0,0 +1,127 @@ +getViewer(); + $id = $request->getURIData('id'); + + $revision = id(new DifferentialRevisionQuery()) + ->withIDs(array($id)) + ->setViewer($viewer) + ->executeOne(); + if (!$revision) { + return new Aphront404Response(); + } + + $table = new DifferentialAffectedPath(); + $conn = $table->establishConnection('r'); + + $paths = queryfx_all( + $conn, + 'SELECT * FROM %R WHERE revisionID = %d', + $table, + $revision->getID()); + + $repository_ids = array(); + $path_ids = array(); + + foreach ($paths as $path) { + $repository_id = $path['repositoryID']; + $path_id = $path['pathID']; + + $repository_ids[] = $repository_id; + $path_ids[] = $path_id; + } + + $repository_ids = array_fuse($repository_ids); + + if ($repository_ids) { + $repositories = id(new PhabricatorRepositoryQuery()) + ->setViewer($viewer) + ->withIDs($repository_ids) + ->execute(); + $repositories = mpull($repositories, null, 'getID'); + } else { + $repositories = array(); + } + + $handles = $viewer->loadHandles(mpull($repositories, 'getPHID')); + + $path_ids = array_fuse($path_ids); + if ($path_ids) { + $path_names = id(new DiffusionPathQuery()) + ->withPathIDs($path_ids) + ->execute(); + } else { + $path_names = array(); + } + + $rows = array(); + foreach ($paths as $path) { + $repository_id = $path['repositoryID']; + $path_id = $path['pathID']; + + $repository = idx($repositories, $repository_id); + if ($repository) { + $repository_phid = $repository->getPHID(); + $repository_link = $handles[$repository_phid]->renderLink(); + } else { + $repository_link = null; + } + + $path_name = idx($path_names, $path_id); + if ($path_name !== null) { + $path_view = $path_name['path']; + } else { + $path_view = null; + } + + $rows[] = array( + $repository_id, + $repository_link, + $path_id, + $path_view, + ); + } + + // Sort rows by path name. + $rows = isort($rows, 3); + + $table_view = id(new AphrontTableView($rows)) + ->setNoDataString(pht('This revision has no indexed affected paths.')) + ->setHeaders( + array( + pht('Repository ID'), + pht('Repository'), + pht('Path ID'), + pht('Path'), + )) + ->setColumnClasses( + array( + null, + null, + null, + 'wide', + )); + + $box_view = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Affected Path Index')) + ->setTable($table_view); + + $crumbs = $this->buildApplicationCrumbs() + ->addTextCrumb($revision->getMonogram(), $revision->getURI()) + ->addTextCrumb(pht('Affected Path Index')); + + return $this->newPage() + ->setCrumbs($crumbs) + ->setTitle( + array( + $revision->getMonogram(), + pht('Affected Path Index'), + )) + ->appendChild($box_view); + } + +} diff --git a/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php b/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php index 5bfb6843c4..58b390fb23 100644 --- a/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php +++ b/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php @@ -44,6 +44,16 @@ final class PhabricatorSystemDebugUIEventListener ->setName(pht('View Hovercard')) ->setHref(urisprintf('/search/hovercard/?names=%s', $phid)); + if ($object instanceof DifferentialRevision) { + $submenu[] = id(new PhabricatorActionView()) + ->setIcon('fa-database') + ->setName(pht('View Affected Path Index')) + ->setHref( + urisprintf( + '/differential/revision/paths/%s/', + $object->getID())); + } + $developer_action = id(new PhabricatorActionView()) ->setName(pht('Advanced/Developer...')) ->setIcon('fa-magic') From 38ef910da8ca4fb35738e8bb3870e58c265f7c18 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Mar 2021 09:59:40 -0700 Subject: [PATCH 05/29] Move "Affected Path" index updates to a separate class Summary: Ref T13639. Move operations related to updating the "AffectedPath" index to a dedicated class. This change has no functional effect and only moves code. Test Plan: Used an external script to rebuild every revision index; destroyed a revision with `bin/remove destroy`. Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21616 --- src/__phutil_library_map__.php | 2 + .../editor/DifferentialTransactionEditor.php | 93 +----------- .../engine/DifferentialAffectedPathEngine.php | 137 ++++++++++++++++++ .../storage/DifferentialRevision.php | 13 +- 4 files changed, 148 insertions(+), 97 deletions(-) create mode 100644 src/applications/differential/engine/DifferentialAffectedPathEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ea1e40dd57..3bdc63e7b2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -450,6 +450,7 @@ phutil_register_library_map(array( 'DifferentialActionEmailCommand' => 'applications/differential/command/DifferentialActionEmailCommand.php', 'DifferentialAdjustmentMapTestCase' => 'applications/differential/storage/__tests__/DifferentialAdjustmentMapTestCase.php', 'DifferentialAffectedPath' => 'applications/differential/storage/DifferentialAffectedPath.php', + 'DifferentialAffectedPathEngine' => 'applications/differential/engine/DifferentialAffectedPathEngine.php', 'DifferentialAsanaRepresentationField' => 'applications/differential/customfield/DifferentialAsanaRepresentationField.php', 'DifferentialAuditorsCommitMessageField' => 'applications/differential/field/DifferentialAuditorsCommitMessageField.php', 'DifferentialAuditorsField' => 'applications/differential/customfield/DifferentialAuditorsField.php', @@ -6533,6 +6534,7 @@ phutil_register_library_map(array( 'DifferentialActionEmailCommand' => 'MetaMTAEmailTransactionCommand', 'DifferentialAdjustmentMapTestCase' => 'PhabricatorTestCase', 'DifferentialAffectedPath' => 'DifferentialDAO', + 'DifferentialAffectedPathEngine' => 'Phobject', 'DifferentialAsanaRepresentationField' => 'DifferentialCustomField', 'DifferentialAuditorsCommitMessageField' => 'DifferentialCommitMessageCustomField', 'DifferentialAuditorsField' => 'DifferentialStoredCustomField', diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index af81af73a8..230558d797 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -356,7 +356,12 @@ final class DifferentialTransactionEditor // diff to a revision. $this->updateRevisionHashTable($object, $diff); - $this->updateAffectedPathTable($object, $diff); + + id(new DifferentialAffectedPathEngine()) + ->setRevision($object) + ->setDiff($diff) + ->updateAffectedPaths(); + break; } } @@ -1258,92 +1263,6 @@ final class DifferentialTransactionEditor return $adapter; } - /** - * Update the table which links Differential revisions to paths they affect, - * so Diffusion can efficiently find pending revisions for a given file. - */ - private function updateAffectedPathTable( - DifferentialRevision $revision, - DifferentialDiff $diff) { - - $repository = $revision->getRepository(); - if (!$repository) { - // The repository where the code lives is untracked. - return; - } - - $path_prefix = null; - - $local_root = $diff->getSourceControlPath(); - if ($local_root) { - // We're in a working copy which supports subdirectory checkouts (e.g., - // SVN) so we need to figure out what prefix we should add to each path - // (e.g., trunk/projects/example/) to get the absolute path from the - // root of the repository. DVCS systems like Git and Mercurial are not - // affected. - - // Normalize both paths and check if the repository root is a prefix of - // the local root. If so, throw it away. Note that this correctly handles - // the case where the remote path is "/". - $local_root = id(new PhutilURI($local_root))->getPath(); - $local_root = rtrim($local_root, '/'); - - $repo_root = id(new PhutilURI($repository->getRemoteURI()))->getPath(); - $repo_root = rtrim($repo_root, '/'); - - if (!strncmp($repo_root, $local_root, strlen($repo_root))) { - $path_prefix = substr($local_root, strlen($repo_root)); - } - } - - $changesets = $diff->getChangesets(); - $paths = array(); - foreach ($changesets as $changeset) { - $paths[] = $path_prefix.'/'.$changeset->getFilename(); - } - - // Mark this as also touching all parent paths, so you can see all pending - // changes to any file within a directory. - $all_paths = array(); - foreach ($paths as $local) { - foreach (DiffusionPathIDQuery::expandPathToRoot($local) as $path) { - $all_paths[$path] = true; - } - } - $all_paths = array_keys($all_paths); - - $path_ids = - PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths( - $all_paths); - - $table = new DifferentialAffectedPath(); - $conn_w = $table->establishConnection('w'); - - $sql = array(); - foreach ($path_ids as $path_id) { - $sql[] = qsprintf( - $conn_w, - '(%d, %d, %d, %d)', - $repository->getID(), - $path_id, - time(), - $revision->getID()); - } - - queryfx( - $conn_w, - 'DELETE FROM %T WHERE revisionID = %d', - $table->getTableName(), - $revision->getID()); - foreach (array_chunk($sql, 256) as $chunk) { - queryfx( - $conn_w, - 'INSERT INTO %T (repositoryID, pathID, epoch, revisionID) VALUES %LQ', - $table->getTableName(), - $chunk); - } - } - /** * Update the table connecting revisions to DVCS local hashes, so we can * identify revisions by commit/tree hashes. diff --git a/src/applications/differential/engine/DifferentialAffectedPathEngine.php b/src/applications/differential/engine/DifferentialAffectedPathEngine.php new file mode 100644 index 0000000000..cdf1c168b1 --- /dev/null +++ b/src/applications/differential/engine/DifferentialAffectedPathEngine.php @@ -0,0 +1,137 @@ +revision = $revision; + return $this; + } + + public function getRevision() { + return $this->revision; + } + + public function setDiff(DifferentialDiff $diff) { + $this->diff = $diff; + return $this; + } + + public function getDiff() { + return $this->diff; + } + + public function updateAffectedPaths() { + $revision = $this->getRevision(); + $diff = $this->getDiff(); + $repository = $revision->getRepository(); + + if ($repository) { + $repository_id = $repository->getID(); + } else { + return; + } + + $paths = $this->getAffectedPaths(); + + $path_ids = + PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths( + $paths); + + $table = new DifferentialAffectedPath(); + $conn = $table->establishConnection('w'); + + $sql = array(); + foreach ($path_ids as $path_id) { + $sql[] = qsprintf( + $conn, + '(%d, %d, %d, %d)', + $repository_id, + $path_id, + PhabricatorTime::getNow(), + $revision->getID()); + } + + queryfx( + $conn, + 'DELETE FROM %R WHERE revisionID = %d', + $table, + $revision->getID()); + if ($sql) { + foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { + queryfx( + $conn, + 'INSERT INTO %R (repositoryID, pathID, epoch, revisionID) VALUES %LQ', + $table, + $chunk); + } + } + } + + public function destroyAffectedPaths() { + $revision = $this->getRevision(); + + $table = new DifferentialAffectedPath(); + $conn = $table->establishConnection('w'); + + queryfx( + $conn, + 'DELETE FROM %R WHERE revisionID = %d', + $table, + $revision->getID()); + } + + public function getAffectedPaths() { + $revision = $this->getRevision(); + $diff = $this->getDiff(); + $repository = $revision->getRepository(); + + $path_prefix = null; + if ($repository) { + $local_root = $diff->getSourceControlPath(); + if ($local_root) { + // We're in a working copy which supports subdirectory checkouts (e.g., + // SVN) so we need to figure out what prefix we should add to each path + // (e.g., trunk/projects/example/) to get the absolute path from the + // root of the repository. DVCS systems like Git and Mercurial are not + // affected. + + // Normalize both paths and check if the repository root is a prefix of + // the local root. If so, throw it away. Note that this correctly + // handles the case where the remote path is "/". + $local_root = id(new PhutilURI($local_root))->getPath(); + $local_root = rtrim($local_root, '/'); + + $repo_root = id(new PhutilURI($repository->getRemoteURI()))->getPath(); + $repo_root = rtrim($repo_root, '/'); + + if (!strncmp($repo_root, $local_root, strlen($repo_root))) { + $path_prefix = substr($local_root, strlen($repo_root)); + } + } + } + + $changesets = $diff->getChangesets(); + + $paths = array(); + foreach ($changesets as $changeset) { + $paths[] = $path_prefix.'/'.$changeset->getFilename(); + } + + // Mark this as also touching all parent paths, so you can see all pending + // changes to any file within a directory. + $all_paths = array(); + foreach ($paths as $local) { + foreach (DiffusionPathIDQuery::expandPathToRoot($local) as $path) { + $all_paths[$path] = true; + } + } + $all_paths = array_keys($all_paths); + + return $all_paths; + } + +} diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 15cf219f7b..23f5db9151 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -1022,16 +1022,9 @@ final class DifferentialRevision extends DifferentialDAO $engine->destroyObject($diff); } - $conn_w = $this->establishConnection('w'); - - // we have to do paths a little differently as they do not have - // an id or phid column for delete() to act on - $dummy_path = new DifferentialAffectedPath(); - queryfx( - $conn_w, - 'DELETE FROM %T WHERE revisionID = %d', - $dummy_path->getTableName(), - $this->getID()); + id(new DifferentialAffectedPathEngine()) + ->setRevision($this) + ->destroyAffectedPaths(); $viewstate_query = id(new DifferentialViewStateQuery()) ->setViewer($viewer) From 01ea84029d88b0fb5f276aab50fd9b253b4b2117 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Mar 2021 15:32:12 -0700 Subject: [PATCH 06/29] Update table schema for "AffectedPath" table Summary: Ref T13639. Make schema changes: - Make repositoryID nullable, for revisions with no repository. - Remove "epoch", which has no readers and no clear use. - Change the ordering of the key, since "pathID" has more unique values and no queries ever issue without it. Test Plan: - Ran `bin/storage upgrade`, got a clean schema. - Reindexed all revisions with an external script. - Reviewed index via debug UI, saw appropriate index for non-repositoy revisions. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21617 --- .../sql/autopatches/20210315.affectedpath.01.epoch.sql | 2 ++ .../autopatches/20210315.affectedpath.02.repositoryid.sql | 2 ++ .../engine/DifferentialAffectedPathEngine.php | 7 +++---- .../differential/storage/DifferentialAffectedPath.php | 8 ++++---- 4 files changed, 11 insertions(+), 8 deletions(-) create mode 100644 resources/sql/autopatches/20210315.affectedpath.01.epoch.sql create mode 100644 resources/sql/autopatches/20210315.affectedpath.02.repositoryid.sql diff --git a/resources/sql/autopatches/20210315.affectedpath.01.epoch.sql b/resources/sql/autopatches/20210315.affectedpath.01.epoch.sql new file mode 100644 index 0000000000..80c337fd94 --- /dev/null +++ b/resources/sql/autopatches/20210315.affectedpath.01.epoch.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_affectedpath + DROP epoch; diff --git a/resources/sql/autopatches/20210315.affectedpath.02.repositoryid.sql b/resources/sql/autopatches/20210315.affectedpath.02.repositoryid.sql new file mode 100644 index 0000000000..1975b7c071 --- /dev/null +++ b/resources/sql/autopatches/20210315.affectedpath.02.repositoryid.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_affectedpath + CHANGE repositoryID repositoryID INT UNSIGNED; diff --git a/src/applications/differential/engine/DifferentialAffectedPathEngine.php b/src/applications/differential/engine/DifferentialAffectedPathEngine.php index cdf1c168b1..0ac85e5900 100644 --- a/src/applications/differential/engine/DifferentialAffectedPathEngine.php +++ b/src/applications/differential/engine/DifferentialAffectedPathEngine.php @@ -32,7 +32,7 @@ final class DifferentialAffectedPathEngine if ($repository) { $repository_id = $repository->getID(); } else { - return; + $repository_id = null; } $paths = $this->getAffectedPaths(); @@ -48,10 +48,9 @@ final class DifferentialAffectedPathEngine foreach ($path_ids as $path_id) { $sql[] = qsprintf( $conn, - '(%d, %d, %d, %d)', + '(%nd, %d, %d)', $repository_id, $path_id, - PhabricatorTime::getNow(), $revision->getID()); } @@ -64,7 +63,7 @@ final class DifferentialAffectedPathEngine foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { queryfx( $conn, - 'INSERT INTO %R (repositoryID, pathID, epoch, revisionID) VALUES %LQ', + 'INSERT INTO %R (repositoryID, pathID, revisionID) VALUES %LQ', $table, $chunk); } diff --git a/src/applications/differential/storage/DifferentialAffectedPath.php b/src/applications/differential/storage/DifferentialAffectedPath.php index b8de95629b..400a09fe82 100644 --- a/src/applications/differential/storage/DifferentialAffectedPath.php +++ b/src/applications/differential/storage/DifferentialAffectedPath.php @@ -8,7 +8,6 @@ final class DifferentialAffectedPath extends DifferentialDAO { protected $repositoryID; protected $pathID; - protected $epoch; protected $revisionID; protected function getConfiguration() { @@ -16,15 +15,16 @@ final class DifferentialAffectedPath extends DifferentialDAO { self::CONFIG_TIMESTAMPS => false, self::CONFIG_COLUMN_SCHEMA => array( 'id' => null, + 'repositoryID' => 'id?', ), self::CONFIG_KEY_SCHEMA => array( 'PRIMARY' => null, - 'repositoryID' => array( - 'columns' => array('repositoryID', 'pathID', 'epoch'), - ), 'revisionID' => array( 'columns' => array('revisionID'), ), + 'key_path' => array( + 'columns' => array('pathID', 'repositoryID'), + ), ), ) + parent::getConfiguration(); } From 26c68942bd00d82a86e4f9c3ccd43e41c6c3091d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Mar 2021 15:45:02 -0700 Subject: [PATCH 07/29] Update "AffectedPath" table when a revision's repository changes Summary: Ref T13639. There's currently a hard-to-hit bug where editing the "Repository" of a revision doesn't update this index. Instead: update the index on repository change, not just diff update. Test Plan: - Updated a revision, used debug view to see index update. - Changed repository on a revision, used debug view to see index update. Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21618 --- .../editor/DifferentialTransactionEditor.php | 56 ++++++++++++++----- 1 file changed, 42 insertions(+), 14 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 230558d797..345fdff72a 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -340,29 +340,57 @@ final class DifferentialTransactionEditor pht('Failed to load revision from transaction finalization.')); } + $active_diff = $new_revision->getActiveDiff(); + $new_diff_phid = $active_diff->getPHID(); + $object->attachReviewers($new_revision->getReviewers()); - $object->attachActiveDiff($new_revision->getActiveDiff()); + $object->attachActiveDiff($active_diff); $object->attachRepository($new_revision->getRepository()); + $has_new_diff = false; + $should_index_paths = false; + $should_index_hashes = false; + $need_changesets = false; + foreach ($xactions as $xaction) { switch ($xaction->getTransactionType()) { case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: - $diff = $this->requireDiff($xaction->getNewValue(), true); + $need_changesets = true; - $this->ownersDiff = $diff; - $this->ownersChangesets = $diff->getChangesets(); - - // Update these denormalized index tables when we attach a new - // diff to a revision. - - $this->updateRevisionHashTable($object, $diff); - - id(new DifferentialAffectedPathEngine()) - ->setRevision($object) - ->setDiff($diff) - ->updateAffectedPaths(); + $new_diff_phid = $xaction->getNewValue(); + $has_new_diff = true; + $should_index_paths = true; + $should_index_hashes = true; break; + case DifferentialRevisionRepositoryTransaction::TRANSACTIONTYPE: + // The "AffectedPath" table denormalizes the repository, so we + // want to update the index if the repository changes. + + $need_changesets = true; + + $should_index_paths = true; + break; + } + } + + if ($need_changesets) { + $new_diff = $this->requireDiff($new_diff_phid, true); + + if ($should_index_paths) { + id(new DifferentialAffectedPathEngine()) + ->setRevision($object) + ->setDiff($new_diff) + ->updateAffectedPaths(); + } + + if ($should_index_hashes) { + $this->updateRevisionHashTable($object, $new_diff); + } + + if ($has_new_diff) { + $this->ownersDiff = $new_diff; + $this->ownersChangesets = $new_diff->getChangesets(); } } From 925c9a71e7eac24b76a6d836ed4c35a7fb4cea33 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Mar 2021 09:53:41 -0700 Subject: [PATCH 08/29] Support a "withPaths()" API in DifferentialRevisionQuery, and use it on the revision view Summary: Ref T13639. Move away from "withPath(..., ...)" to "withPaths(...)". Test Plan: {F8539323} Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21619 --- .../DifferentialRevisionViewController.php | 24 +++---- .../query/DifferentialRevisionQuery.php | 67 +++++++++++++++++++ 2 files changed, 78 insertions(+), 13 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index e37fa2b529..13ab8a8954 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -986,6 +986,8 @@ final class DifferentialRevisionViewController PhabricatorRepository $repository) { assert_instances_of($changesets, 'DifferentialChangeset'); + $viewer = $this->getViewer(); + $paths = array(); foreach ($changesets as $changeset) { $paths[] = $changeset->getAbsoluteRepositoryPath( @@ -997,34 +999,30 @@ final class DifferentialRevisionViewController return array(); } - $path_map = id(new DiffusionPathIDQuery($paths))->loadPathIDs(); - - if (!$path_map) { - return array(); - } - $recent = (PhabricatorTime::getNow() - phutil_units('30 days in seconds')); $query = id(new DifferentialRevisionQuery()) - ->setViewer($this->getRequest()->getUser()) + ->setViewer($viewer) ->withIsOpen(true) ->withUpdatedEpochBetween($recent, null) ->setOrder(DifferentialRevisionQuery::ORDER_MODIFIED) ->setLimit(10) ->needFlags(true) ->needDrafts(true) - ->needReviewers(true); - - foreach ($path_map as $path => $path_id) { - $query->withPath($repository->getID(), $path_id); - } + ->needReviewers(true) + ->withRepositoryPHIDs( + array( + $repository->getPHID(), + )) + ->withPaths($paths); $results = $query->execute(); // Strip out *this* revision. foreach ($results as $key => $result) { if ($result->getID() == $this->revisionID) { - unset($results[$key]); + unset($results[$key]); + break; } } diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 0f6d5d944b..c77e99d8c2 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -27,6 +27,7 @@ final class DifferentialRevisionQuery private $createdEpochMin; private $createdEpochMax; private $noReviewers; + private $paths; const ORDER_MODIFIED = 'order-modified'; const ORDER_CREATED = 'order-created'; @@ -62,6 +63,18 @@ final class DifferentialRevisionQuery return $this; } + /** + * Find revisions affecting one or more items in a list of paths. + * + * @param list List of file paths. + * @return this + * @task config + */ + public function withPaths(array $paths) { + $this->paths = $paths; + return $this; + } + /** * Filter results to revisions authored by one of the given PHIDs. Calling * this function will clear anything set by previous calls to @@ -576,6 +589,14 @@ final class DifferentialRevisionQuery $path_table->getTableName()); } + if ($this->paths) { + $path_table = new DifferentialAffectedPath(); + $joins[] = qsprintf( + $conn, + 'JOIN %R paths ON paths.revisionID = r.id', + $path_table); + } + if ($this->commitHashes) { $joins[] = qsprintf( $conn, @@ -635,6 +656,7 @@ final class DifferentialRevisionQuery * @task internal */ protected function buildWhereClause(AphrontDatabaseConnection $conn) { + $viewer = $this->getViewer(); $where = array(); if ($this->pathIDs) { @@ -651,6 +673,45 @@ final class DifferentialRevisionQuery $where[] = $path_clauses; } + if ($this->paths !== null) { + $paths = $this->paths; + + $path_map = id(new DiffusionPathIDQuery($paths)) + ->loadPathIDs(); + + if (!$path_map) { + // If none of the paths have entries in the PathID table, we can not + // possibly find any revisions affecting them. + throw new PhabricatorEmptyQueryException(); + } + + $where[] = qsprintf( + $conn, + 'paths.pathID IN (%Ld)', + array_fuse($path_map)); + + // If we have repository PHIDs, additionally constrain this query to + // try to help MySQL execute it efficiently. + if ($this->repositoryPHIDs !== null) { + $repositories = id(new PhabricatorRepositoryQuery()) + ->setViewer($viewer) + ->setParentQuery($this) + ->withPHIDs($this->repositoryPHIDs) + ->execute(); + + if (!$repositories) { + throw new PhabricatorEmptyQueryException(); + } + + $repository_ids = mpull($repositories, 'getID'); + + $where[] = qsprintf( + $conn, + 'paths.repositoryID IN (%Ld)', + $repository_ids); + } + } + if ($this->authors) { $where[] = qsprintf( $conn, @@ -782,6 +843,12 @@ final class DifferentialRevisionQuery return true; } + if ($this->paths) { + // (If we have exactly one repository and exactly one path, we don't + // technically need to group, but it's simpler to always group.) + return true; + } + if (count($this->ccs) > 1) { return true; } From 6d33ba7dc40152352585fb7d2c99761efa5d1724 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Mar 2021 15:56:05 -0700 Subject: [PATCH 09/29] Move Diffusion to "withPaths()" for "Recent Open Revisions", and remove "withPath()" from DifferentialRevisionQuery Summary: Ref T13639. Move Diffusion to use the new API and get rid of the old API now that it no longer has any callers. Test Plan: Grepped for remaining callers. {F8539335} Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21620 --- .../query/DifferentialRevisionQuery.php | 46 ------------------- .../controller/DiffusionBrowseController.php | 9 +--- 2 files changed, 2 insertions(+), 53 deletions(-) diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index c77e99d8c2..75d3e052e4 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -8,8 +8,6 @@ final class DifferentialRevisionQuery extends PhabricatorCursorPagedPolicyAwareQuery { - private $pathIDs = array(); - private $authors = array(); private $draftAuthors = array(); private $ccs = array(); @@ -44,25 +42,6 @@ final class DifferentialRevisionQuery /* -( Query Configuration )------------------------------------------------ */ - - /** - * Filter results to revisions which affect a Diffusion path ID in a given - * repository. You can call this multiple times to select revisions for - * several paths. - * - * @param int Diffusion repository ID. - * @param int Diffusion path ID. - * @return this - * @task config - */ - public function withPath($repository_id, $path_id) { - $this->pathIDs[] = array( - 'repositoryID' => $repository_id, - 'pathID' => $path_id, - ); - return $this; - } - /** * Find revisions affecting one or more items in a list of paths. * @@ -581,13 +560,6 @@ final class DifferentialRevisionQuery */ private function buildJoinsClause(AphrontDatabaseConnection $conn) { $joins = array(); - if ($this->pathIDs) { - $path_table = new DifferentialAffectedPath(); - $joins[] = qsprintf( - $conn, - 'JOIN %T p ON p.revisionID = r.id', - $path_table->getTableName()); - } if ($this->paths) { $path_table = new DifferentialAffectedPath(); @@ -659,20 +631,6 @@ final class DifferentialRevisionQuery $viewer = $this->getViewer(); $where = array(); - if ($this->pathIDs) { - $path_clauses = array(); - $repo_info = igroup($this->pathIDs, 'repositoryID'); - foreach ($repo_info as $repository_id => $paths) { - $path_clauses[] = qsprintf( - $conn, - '(p.repositoryID = %d AND p.pathID IN (%Ld))', - $repository_id, - ipull($paths, 'pathID')); - } - $path_clauses = qsprintf($conn, '%LO', $path_clauses); - $where[] = $path_clauses; - } - if ($this->paths !== null) { $paths = $this->paths; @@ -839,10 +797,6 @@ final class DifferentialRevisionQuery */ protected function shouldGroupQueryResultRows() { - if (count($this->pathIDs) > 1) { - return true; - } - if ($this->paths) { // (If we have exactly one repository and exactly one path, we don't // technically need to group, but it's simpler to always group.) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 042c21d232..549c0d649f 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -938,17 +938,12 @@ final class DiffusionBrowseController extends DiffusionController { $repository = $drequest->getRepository(); $path = $drequest->getPath(); - $path_map = id(new DiffusionPathIDQuery(array($path)))->loadPathIDs(); - $path_id = idx($path_map, $path); - if (!$path_id) { - return null; - } - $recent = (PhabricatorTime::getNow() - phutil_units('30 days in seconds')); $revisions = id(new DifferentialRevisionQuery()) ->setViewer($viewer) - ->withPath($repository->getID(), $path_id) + ->withPaths(array($path)) + ->withRepositoryPHIDs(array($repository->getPHID())) ->withIsOpen(true) ->withUpdatedEpochBetween($recent, null) ->setOrder(DifferentialRevisionQuery::ORDER_MODIFIED) From 30d58de1bc226358d726773b741e8ce3f23aeed3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Mar 2021 16:01:24 -0700 Subject: [PATCH 10/29] Expose "affectedPaths" to "differential.revision.search" Conduit API method Summary: Ref T13639. Support querying by "affectedPaths" in the API. Test Plan: {F8539347} Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21621 --- .../query/DifferentialRevisionSearchEngine.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/applications/differential/query/DifferentialRevisionSearchEngine.php b/src/applications/differential/query/DifferentialRevisionSearchEngine.php index 2bf196db0c..1ec5265630 100644 --- a/src/applications/differential/query/DifferentialRevisionSearchEngine.php +++ b/src/applications/differential/query/DifferentialRevisionSearchEngine.php @@ -57,6 +57,10 @@ final class DifferentialRevisionSearchEngine $map['modifiedEnd']); } + if ($map['affectedPaths']) { + $query->withPaths($map['affectedPaths']); + } + return $query; } @@ -118,6 +122,12 @@ final class DifferentialRevisionSearchEngine ->setIsHidden(true) ->setDescription( pht('Find revisions modified at or before a particular time.')), + id(new PhabricatorSearchStringListField()) + ->setKey('affectedPaths') + ->setLabel(pht('Affected Paths')) + ->setDescription( + pht('Search for revisions affecting particular paths.')) + ->setIsHidden(true), ); } From 42c0c0e3d23a7912e8d48731a6ff91f2fb6b3ad7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 09:29:48 -0700 Subject: [PATCH 11/29] Remove or correct various "phabricator/" references to "libphutil" Summary: Ref T13395. "libphutil/" was stripped for parts, but some documentation still references it. This is mostly minor corrections, but: - Removes "Javelin at Facebook", long obsolete. - Removes "php FPM warmup", which was always a prototype and is obsoleted by PHP preloading in recent PHP. Test Plan: `grep` / reading Maniphest Tasks: T13395 Differential Revision: https://secure.phabricator.com/D21624 --- scripts/fpm/warmup.php | 38 --------- scripts/install/update_phabricator.sh | 5 +- .../PhabricatorConduitController.php | 2 +- .../storage/PhabricatorRepository.php | 2 +- .../contributor/adding_new_classes.diviner | 18 ++-- src/docs/contributor/bug_reports.diviner | 7 +- src/docs/contributor/contrib_intro.diviner | 2 +- .../contributor/contributing_code.diviner | 6 +- .../general_coding_standards.diviner | 4 +- .../contributor/internationalization.diviner | 10 +-- .../contributor/php_coding_standards.diviner | 6 +- src/docs/contributor/rendering_html.diviner | 50 +++++------ src/docs/contributor/unit_tests.diviner | 10 +-- src/docs/flavor/php_pitfalls.diviner | 6 +- .../configuration/managing_daemons.diviner | 2 +- .../troubleshooting_https.diviner | 2 +- src/docs/user/field/darkconsole.diviner | 2 +- src/docs/user/userguide/arcanist.diviner | 12 +-- .../user/userguide/arcanist_coverage.diviner | 4 +- .../userguide/arcanist_quick_start.diviner | 3 - src/docs/user/userguide/conduit.diviner | 3 +- .../user/userguide/diffusion_managing.diviner | 7 +- .../user/userguide/diffusion_symbols.diviner | 4 +- src/docs/user/userguide/drydock_hosts.diviner | 2 +- src/docs/user/userguide/events.diviner | 4 +- src/docs/user/userguide/utf8.diviner | 42 ---------- .../workers/storage/PhabricatorWorkerTask.php | 3 +- src/infrastructure/storage/lisk/LiskDAO.php | 2 +- support/startup/PhabricatorStartup.php | 2 +- .../javelin/docs/concepts/behaviors.diviner | 2 +- .../externals/javelin/docs/facebook.diviner | 82 ------------------- 31 files changed, 83 insertions(+), 261 deletions(-) delete mode 100644 scripts/fpm/warmup.php delete mode 100644 webroot/rsrc/externals/javelin/docs/facebook.diviner diff --git a/scripts/fpm/warmup.php b/scripts/fpm/warmup.php deleted file mode 100644 index 956a474131..0000000000 --- a/scripts/fpm/warmup.php +++ /dev/null @@ -1,38 +0,0 @@ -selectAndLoadSymbols(); - - define('__WARMUP__', true); -} - -__warmup__(); diff --git a/scripts/install/update_phabricator.sh b/scripts/install/update_phabricator.sh index 5a3950088e..3831acd963 100755 --- a/scripts/install/update_phabricator.sh +++ b/scripts/install/update_phabricator.sh @@ -9,15 +9,12 @@ set -x # to work without modifications. # NOTE: This script assumes you are running it from a directory which contains -# arcanist/, libphutil/, and phabricator/. +# arcanist/ and phabricator/. ROOT=`pwd` # You can hard-code the path here instead. ### UPDATE WORKING COPIES ###################################################### -cd $ROOT/libphutil -git pull - cd $ROOT/arcanist git pull diff --git a/src/applications/conduit/controller/PhabricatorConduitController.php b/src/applications/conduit/controller/PhabricatorConduitController.php index 0f7b2ef54b..e66c1f8a0b 100644 --- a/src/applications/conduit/controller/PhabricatorConduitController.php +++ b/src/applications/conduit/controller/PhabricatorConduitController.php @@ -156,7 +156,7 @@ abstract class PhabricatorConduitController extends PhabricatorController { $parts = array(); - $libphutil_path = 'path/to/libphutil/src/__phutil_library_init__.php'; + $libphutil_path = 'path/to/arcanist/support/init/init-script.php'; $parts[] = '`. You should not need to explicitly use -@{function@libphutil:phutil_escape_html} anywhere. +@{function@arcanist:phutil_escape_html} anywhere. If you need to apply a string function (such as `trim()`) to safe HTML, use -@{method@libphutil:PhutilSafeHTML::applyFunction}. +@{method@arcanist:PhutilSafeHTML::applyFunction}. -If you need to extract the content of a @{class@libphutil:PhutilSafeHTML} +If you need to extract the content of a @{class@arcanist:PhutilSafeHTML} object, you should call `getHTMLContent()`, not cast it to a string. Eventually, we would like to remove the string cast entirely. -Functions @{function@libphutil:phutil_tag} and @{function@libphutil:hsprintf} +Functions @{function@arcanist:phutil_tag} and @{function@arcanist:hsprintf} are not safe if you pass the user input for the tag or attribute name. All the following examples are dangerous: diff --git a/src/docs/contributor/unit_tests.diviner b/src/docs/contributor/unit_tests.diviner index 3ac14b3e00..7977a4a876 100644 --- a/src/docs/contributor/unit_tests.diviner +++ b/src/docs/contributor/unit_tests.diviner @@ -1,13 +1,13 @@ @title Writing Unit Tests @group developer -Simple guide to libphutil, Arcanist and Phabricator unit tests. +Simple guide to Arcanist and Phabricator unit tests. = Overview = -libphutil, Arcanist and Phabricator provide and use a simple unit test -framework. This document is aimed at project contributors and describes how to -use it to add and run tests in these projects or other libphutil libraries. +Arcanist and Phabricator provide and use a simple unit test framework. This +document is aimed at project contributors and describes how to use it to add +and run tests in these projects or other libphutil libraries. In the general case, you can integrate `arc` with a custom unit test engine (like PHPUnit or any other unit testing library) to run tests in other projects. @@ -16,7 +16,7 @@ for information on customizing engines. = Adding Tests = -To add new tests to a libphutil, Arcanist or Phabricator module: +To add new tests to a Arcanist or Phabricator module: - Create a `__tests__/` directory in the module if it doesn't exist yet. - Add classes to the `__tests__/` directory which extend from diff --git a/src/docs/flavor/php_pitfalls.diviner b/src/docs/flavor/php_pitfalls.diviner index 0ffcaa42da..3f4be45dd7 100644 --- a/src/docs/flavor/php_pitfalls.diviner +++ b/src/docs/flavor/php_pitfalls.diviner @@ -18,7 +18,7 @@ If you merge a list of arrays like this: intermediate arrays and copies every element it has previously seen each time you iterate. -In a libphutil environment, you can use @{function@libphutil:array_mergev} +In a libphutil environment, you can use @{function@arcanist:array_mergev} instead. = `var_export()` Hates Baby Animals = @@ -147,7 +147,7 @@ keys that are naturally sortable with a function that uses native comparison instead, and use it to reorder the original array. In a libphutil environment, you can often do this easily with -@{function@libphutil:isort} or @{function@libphutil:msort}. +@{function@arcanist:isort} or @{function@arcanist:msort}. = `array_intersect()` and `array_diff()` are Also Slow = @@ -270,7 +270,7 @@ new $class_name($argv[0], $argv[1], ...); ...you'll probably invent a very interesting, very novel solution that is very wrong. In a libphutil environment, solve this problem with -@{function@libphutil:newv}. Elsewhere, copy `newv()`'s implementation. +@{function@arcanist:newv}. Elsewhere, copy `newv()`'s implementation. = Equality is not Transitive = diff --git a/src/docs/user/configuration/managing_daemons.diviner b/src/docs/user/configuration/managing_daemons.diviner index 0a732d5836..cf2ba85ea2 100644 --- a/src/docs/user/configuration/managing_daemons.diviner +++ b/src/docs/user/configuration/managing_daemons.diviner @@ -65,7 +65,7 @@ daemonizing it, so you can see output in your console. You can get a list of launchable daemons with **phd list**: - - **libphutil test daemons** are not generally useful unless you are + - **test daemons** are not generally useful unless you are developing daemon infrastructure or debugging a daemon problem; - **PhabricatorTaskmasterDaemon** performs work from a task queue; - **PhabricatorRepositoryPullLocalDaemon** daemons track repositories, for diff --git a/src/docs/user/configuration/troubleshooting_https.diviner b/src/docs/user/configuration/troubleshooting_https.diviner index 6b93a4f690..bdc3439d7d 100644 --- a/src/docs/user/configuration/troubleshooting_https.diviner +++ b/src/docs/user/configuration/troubleshooting_https.diviner @@ -32,7 +32,7 @@ authority and clients have a list of trusted authorities. You can self-sign a certificate by creating your own CA, but clients will not trust it by default. They need to add the CA as a trusted authority. -For instructions on adding CAs, see `libphutil/resources/ssl/README`. +For instructions on adding CAs, see `arcanist/resources/ssl/README`. If you'd prefer that `arc` not verify the identity of the server whatsoever, you can use the `https.blindly-trust-domains` setting. This will make it diff --git a/src/docs/user/field/darkconsole.diviner b/src/docs/user/field/darkconsole.diviner index cbdfb9bda5..065be2d8f1 100644 --- a/src/docs/user/field/darkconsole.diviner +++ b/src/docs/user/field/darkconsole.diviner @@ -48,7 +48,7 @@ Plugin: Error Log The "Error Log" plugin shows errors that occurred while generating the page, similar to the httpd `error.log`. You can send information to the error log -explicitly with the @{function@libphutil:phlog} function. +explicitly with the @{function@arcanist:phlog} function. If errors occurred, a red dot will appear on the plugin tab. diff --git a/src/docs/user/userguide/arcanist.diviner b/src/docs/user/userguide/arcanist.diviner index e8d6bcd5ed..0de18a9358 100644 --- a/src/docs/user/userguide/arcanist.diviner +++ b/src/docs/user/userguide/arcanist.diviner @@ -90,15 +90,8 @@ have PHP installed, you can download it from . To install Arcanist, pick an install directory and clone the code from GitHub: - some_install_path/ $ git clone https://github.com/phacility/libphutil.git some_install_path/ $ git clone https://github.com/phacility/arcanist.git -This should leave you with a directory structure like this - - some_install_path/ # Wherever you chose to install it. - arcanist/ # Arcanist-specific code and libraries. - libphutil/ # A shared library Arcanist depends upon. - Now add `some_install_path/arcanist/bin/` to your PATH environment variable. When you type "arc", you should see something like this: @@ -110,8 +103,7 @@ trouble getting this far, see these detailed guides: - On Windows: @{article:Arcanist User Guide: Windows} - On Mac OS X: @{article:Arcanist User Guide: Mac OS X} -You can later upgrade Arcanist and libphutil to the latest versions with -`arc upgrade`: +You can later upgrade Arcanist to the latest version with `arc upgrade`: $ arc upgrade @@ -122,7 +114,7 @@ installed and keep people up to date. Here are some approaches you might be able to use: - Facebook does most development on development servers, which have a standard - environment and NFS mounts. Arcanist and libphutil themselves live on an + environment and NFS mounts. Arcanist lives on an NFS mount, and the default `.bashrc` adds them to the PATH. Updating the mount source updates everyone's versions, and new employees have a working `arc` when they first log in. diff --git a/src/docs/user/userguide/arcanist_coverage.diviner b/src/docs/user/userguide/arcanist_coverage.diviner index a734e5dd80..cb25c0cc74 100644 --- a/src/docs/user/userguide/arcanist_coverage.diviner +++ b/src/docs/user/userguide/arcanist_coverage.diviner @@ -27,9 +27,9 @@ to `src/some/file.php` and give you a detailed coverage report. If the test engine enables coverage by default, it will be uploaded to Differential and displayed in the right gutter when viewing diffs. -= Enabling Coverage for libphutil, Arcanist and Phabricator = += Enabling Coverage for Arcanist and Phabricator = -If you're contributing, libphutil, Arcanist and Phabricator support coverage if +If you're contributing, Arcanist and Phabricator support coverage if you install Xdebug: http://xdebug.org/ diff --git a/src/docs/user/userguide/arcanist_quick_start.diviner b/src/docs/user/userguide/arcanist_quick_start.diviner index 743afe4a11..25847ab8a6 100644 --- a/src/docs/user/userguide/arcanist_quick_start.diviner +++ b/src/docs/user/userguide/arcanist_quick_start.diviner @@ -20,9 +20,6 @@ First, install dependencies: Then install Arcanist itself: - $ mkdir somewhere/ - $ cd somewhere/ - somewhere/ $ git clone https://github.com/phacility/libphutil.git somewhere/ $ git clone https://github.com/phacility/arcanist.git Add `arc` to your path: diff --git a/src/docs/user/userguide/conduit.diviner b/src/docs/user/userguide/conduit.diviner index 5784d8cd01..35daee505f 100644 --- a/src/docs/user/userguide/conduit.diviner +++ b/src/docs/user/userguide/conduit.diviner @@ -19,8 +19,7 @@ The primary ways to make Conduit calls are: the API and making calls. This is the best starting point for learning about the API. See the next section for details. -`ConduitClient`: This is the official client available in `libphutil`, and -the one used by `arc`. +`ConduitClient`: This is the official client available in `arcanist`. `arc call-conduit`: You can use this `arc` command to execute low-level Conduit calls by piping JSON in to stdin. This can provide a simple way diff --git a/src/docs/user/userguide/diffusion_managing.diviner b/src/docs/user/userguide/diffusion_managing.diviner index 138bc918bc..e3743526e9 100644 --- a/src/docs/user/userguide/diffusion_managing.diviner +++ b/src/docs/user/userguide/diffusion_managing.diviner @@ -55,10 +55,9 @@ If you choose to assign a callsign to a repository, it must be unique within an install but do not need to be globally unique, so you are free to use the single-letter callsigns for brevity. For example, Facebook uses "E" for the Engineering repository, "O" for the Ops repository, "Y" for a Yum package -repository, and so on, while Phabricator uses "P", "ARC", "PHU" for libphutil, -and "J" for Javelin. Keeping callsigns brief will make them easier to use, and -the use of one-character callsigns is encouraged if they are reasonably -evocative. +repository, and so on, while Phabricator uses "P" and Arcanist uses "ARC". +Keeping callsigns brief will make them easier to use, and the use of +one-character callsigns is encouraged if they are reasonably evocative. If you configure a callsign like `XYZ`, Phabricator will activate callsign URIs and activate the callsign identifier (like `rXYZ`) for the repository. These diff --git a/src/docs/user/userguide/diffusion_symbols.diviner b/src/docs/user/userguide/diffusion_symbols.diviner index f5da8aefe0..7d14ad92b2 100644 --- a/src/docs/user/userguide/diffusion_symbols.diviner +++ b/src/docs/user/userguide/diffusion_symbols.diviner @@ -83,8 +83,8 @@ You can configure some more options by going to {nav Diffusion > (Select You can leave this blank for "All languages". - **Uses Symbols From**: If this project depends on other repositories, add the other repositories which symbols should be looked for here. For example, - Phabricator lists "Arcanist" and "libphutil" because it uses classes and - functions from these repositories. + Phabricator lists "Arcanist" because it uses classes and functions defined + in `arcanist/`. == External Symbols == diff --git a/src/docs/user/userguide/drydock_hosts.diviner b/src/docs/user/userguide/drydock_hosts.diviner index 8bfed7dc60..1b8f22cce1 100644 --- a/src/docs/user/userguide/drydock_hosts.diviner +++ b/src/docs/user/userguide/drydock_hosts.diviner @@ -41,7 +41,7 @@ installing software on hosts. You'll need to make sure any hosts are configured properly with any software you need, and have tools like `git`, `hg` or `svn` that may be required to interact with working copies. -You do **not** need to install PHP, arcanist, libphutil or Phabricator on the +You do **not** need to install PHP, arcanist, or Phabricator on the hosts unless you are specifically running `arc` commands. **You must configure authentication.** Drydock also does not handle credentials diff --git a/src/docs/user/userguide/events.diviner b/src/docs/user/userguide/events.diviner index ea66448c8a..e18578288b 100644 --- a/src/docs/user/userguide/events.diviner +++ b/src/docs/user/userguide/events.diviner @@ -23,7 +23,7 @@ the most direct and powerful way to respond to events. To install event listeners in Phabricator, follow these steps: - - Write a listener class which extends @{class@libphutil:PhutilEventListener}. + - Write a listener class which extends @{class@arcanist:PhutilEventListener}. - Add it to a libphutil library, or create a new library (for instructions, see @{article@phabcontrib:Adding New Classes}. - Configure Phabricator to load the library by adding it to `load-libraries` @@ -40,7 +40,7 @@ see any events the page emitted there. For details on DarkConsole, see To install event listeners in Arcanist, follow these steps: - - Write a listener class which extends @{class@libphutil:PhutilEventListener}. + - Write a listener class which extends @{class@arcanist:PhutilEventListener}. - Add it to a libphutil library, or create a new library (for instructions, see @{article@phabcontrib:Adding New Classes}. - Configure Phabricator to load the library by adding it to `load` diff --git a/src/docs/user/userguide/utf8.diviner b/src/docs/user/userguide/utf8.diviner index a604599e9f..b6742f0c36 100644 --- a/src/docs/user/userguide/utf8.diviner +++ b/src/docs/user/userguide/utf8.diviner @@ -22,48 +22,6 @@ options: Encodings" below). This is not completely supported, and repositories with files that have multiple encodings are not supported. -= Detecting and Repairing Files = - -It is recommended that you write source files only in ASCII text, but -Phabricator fully supports UTF-8 source files. - -If you have a project which isn't valid UTF-8 because a few files have random -binary nonsense in them, there is a script in libphutil which can help you -identify and fix them: - - project/ $ libphutil/scripts/utils/utf8.php - -Generally, run this script on all source files with "-t" to find files with bad -byte ranges, and then run it without "-t" on each file to identify where there -are problems. For example: - - project/ $ find . -type f -name '*.c' -print0 | xargs -0 -n256 ./utf8 -t - ./hello_world.c - -If this script exits without output, you're in good shape and all the files that -were identified are valid UTF-8. If it found some problems, you need to repair -them. You can identify the specific problems by omitting the "-t" flag: - - project/ $ ./utf8.php hello_world.c - FAIL hello_world.c - - 3 main() - 4 { - 5 printf ("Hello World<0xE9><0xD6>!\n"); - 6 } - 7 - -This shows the offending bytes on line 5 (in the actual console display, they'll -be highlighted). Often a codebase will mostly be valid UTF-8 but have a few -scattered files that have other things in them, like curly quotes which someone -copy-pasted from Word into a comment. In these cases, you can just manually -identify and fix the problems pretty easily. - -If you have a prohibitively large number of UTF-8 issues in your source code, -Phabricator doesn't include any default tools to help you process them in a -systematic way. You could hack up `utf8.php` as a starting point, or use other -tools to batch-process your source files. - = Support for Alternate Encodings = Phabricator has some support for encodings other than UTF-8. diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php index 4faae5c83b..480a9d8614 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php @@ -66,7 +66,8 @@ abstract class PhabricatorWorkerTask extends PhabricatorWorkerDAO { $class = $this->getTaskClass(); try { - // NOTE: If the class does not exist, libphutil will throw an exception. + // NOTE: If the class does not exist, the autoloader will throw an + // exception. class_exists($class); } catch (PhutilMissingSymbolException $ex) { throw new PhabricatorWorkerPermanentFailureException( diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index 03b735d315..13b2f8d319 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/src/infrastructure/storage/lisk/LiskDAO.php @@ -116,7 +116,7 @@ * $pugs = $dog->loadAllWhere('breed = %s', 'Pug'); * $sawyer = $dog->loadOneWhere('name = %s', 'Sawyer'); * - * These methods work like @{function@libphutil:queryfx}, but only take half of + * These methods work like @{function@arcanist:queryfx}, but only take half of * a query (the part after the WHERE keyword). Lisk will handle the connection, * columns, and object construction; you are responsible for the rest of it. * @{method:loadAllWhere} returns a list of objects, while diff --git a/support/startup/PhabricatorStartup.php b/support/startup/PhabricatorStartup.php index bf66f8839b..b687f27bb0 100644 --- a/support/startup/PhabricatorStartup.php +++ b/support/startup/PhabricatorStartup.php @@ -67,7 +67,7 @@ final class PhabricatorStartup { */ public static function getMicrosecondsSinceStart() { // This is the same as "phutil_microseconds_since()", but we may not have - // loaded libphutil yet. + // loaded libraries yet. return (int)(1000000 * (microtime(true) - self::getStartTime())); } diff --git a/webroot/rsrc/externals/javelin/docs/concepts/behaviors.diviner b/webroot/rsrc/externals/javelin/docs/concepts/behaviors.diviner index 0ff27d912a..f3cea9cda6 100644 --- a/webroot/rsrc/externals/javelin/docs/concepts/behaviors.diviner +++ b/webroot/rsrc/externals/javelin/docs/concepts/behaviors.diviner @@ -124,7 +124,7 @@ This has a wide array of technical and architectural problems: the full power of arbitrary JS execution. - It's utterly hideous. -In 2007/2008, we introduced @{function@libphutil:jsprintf} and a function called +In 2007/2008, we introduced @{function@arcanist:jsprintf} and a function called onloadRegister() to solve some of the obvious problems: lang=php diff --git a/webroot/rsrc/externals/javelin/docs/facebook.diviner b/webroot/rsrc/externals/javelin/docs/facebook.diviner deleted file mode 100644 index 628ec5cfdb..0000000000 --- a/webroot/rsrc/externals/javelin/docs/facebook.diviner +++ /dev/null @@ -1,82 +0,0 @@ -@title Javelin at Facebook -@group facebook - -Information specific to Javelin at Facebook. - -= Building Support Scripts = - -Javelin now ships with the source to build several libfbjs-based binaries, which -serve to completely sever its dependencies on trunk: - - - `javelinsymbols`: used for lint - - `jsast`: used for documentation generation - - `jsxmin`: used to crush packages - -To build these, first build libfbjs: - - javelin/ $ cd externals/libfbjs - javelin/externals/libfbjs/ $ CXX=/usr/bin/g++ make - -Note that **you must specify CXX explicitly because the default CXX is broken**. - -Now you should be able to build the individual binaries: - - javelin/ $ cd support/javelinsymbols - javelin/support/javelinsymbols $ CXX=/usr/bin/g++ make - - javelin/ $ cd support/jsast - javelin/support/jsast $ CXX=/usr/bin/g++ make - - javelin/ $ cd support/jsxmin - javelin/support/jsxmin $ CXX=/usr/bin/g++ make - -= Synchronizing Javelin = - -To synchronize Javelin **from** Facebook trunk, run the synchronize script: - - javelin/ $ ./scripts/sync-from-facebook.php ~/www - -...where `~/www` is the root you want to pull Javelin files from. The script -will copy files out of `html/js/javelin` and build packages, and leave the -results in your working copy. From there you can review changes and commit, and -then push, diff, or send a pull request. - -To synchronize Javelin **to** Facebook trunk, run the, uh, reverse-synchronize -script: - - javelin/ $ ./scripts/sync-to-facebook.php ~/www - -...where `~/www` is the root you want to push Javelin files to. The script -will copy files out of the working copy into your `www` and leave you with a -dirty `www`. From there you can review changes. - -Once Facebook moves to pure git for `www` we can probably just submodule -Javelin into it and get rid of all this nonsense, but the mixed SVN/git -environment makes that difficult until then. - -= Building Documentation = - -Check out `diviner` and `libphutil` from Facebook github, and put them in a -directory with `javelin`: - - somewhere/ $ ls - diviner/ - javelin/ - libphutil/ - somewhere/ $ - -Now run `diviner` on `javelin`: - - somewhere/ $ cd javelin - somewhere/javelin/ $ ../diviner/bin/diviner . - [DivinerArticleEngine] Generating documentation for 48 files... - [JavelinDivinerEngine] Generating documentation for 74 files... - somewhere/javelin/ $ - -Documentation is now available in `javelin/docs/`. - -= Editing javelinjs.com = - -The source for javelinjs.com lives in `javelin/support/webroot/`. The site -itself is served off the phabricator.com host. You need access to that host to -push it. From c3e6db6f0b32aac80b1322da2eb068289f7da7b4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 11:09:28 -0700 Subject: [PATCH 12/29] Migrate Almanac Device "mailKey" to modern storage Summary: Ref T13065. See similar changes attached to that task. Test Plan: Ran migration, got a clean database state, saw mail keys populate in mail property table. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13065, T13641 Differential Revision: https://secure.phabricator.com/D21625 --- .../20210316.almanac.01.device-mailkey.php | 28 +++++++++++++++++++ ...20210316.almanac.02.device-dropmailkey.sql | 2 ++ .../almanac/storage/AlmanacDevice.php | 6 ---- 3 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 resources/sql/autopatches/20210316.almanac.01.device-mailkey.php create mode 100644 resources/sql/autopatches/20210316.almanac.02.device-dropmailkey.sql diff --git a/resources/sql/autopatches/20210316.almanac.01.device-mailkey.php b/resources/sql/autopatches/20210316.almanac.01.device-mailkey.php new file mode 100644 index 0000000000..7c10d1ae76 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.01.device-mailkey.php @@ -0,0 +1,28 @@ +establishConnection('w'); + +$properties_table = new PhabricatorMetaMTAMailProperties(); +$conn = $properties_table->establishConnection('w'); + +$iterator = new LiskRawMigrationIterator( + $device_conn, + $device_table->getTableName()); + +foreach ($iterator as $row) { + queryfx( + $conn, + 'INSERT IGNORE INTO %R + (objectPHID, mailProperties, dateCreated, dateModified) + VALUES + (%s, %s, %d, %d)', + $properties_table, + $row['phid'], + phutil_json_encode( + array( + 'mailKey' => $row['mailKey'], + )), + PhabricatorTime::getNow(), + PhabricatorTime::getNow()); +} diff --git a/resources/sql/autopatches/20210316.almanac.02.device-dropmailkey.sql b/resources/sql/autopatches/20210316.almanac.02.device-dropmailkey.sql new file mode 100644 index 0000000000..47079ef296 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.02.device-dropmailkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_almanac.almanac_device + DROP mailKey; diff --git a/src/applications/almanac/storage/AlmanacDevice.php b/src/applications/almanac/storage/AlmanacDevice.php index 1d1010733a..8529285dc0 100644 --- a/src/applications/almanac/storage/AlmanacDevice.php +++ b/src/applications/almanac/storage/AlmanacDevice.php @@ -15,7 +15,6 @@ final class AlmanacDevice protected $name; protected $nameIndex; - protected $mailKey; protected $viewPolicy; protected $editPolicy; protected $isBoundToClusterService; @@ -36,7 +35,6 @@ final class AlmanacDevice self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'text128', 'nameIndex' => 'bytes12', - 'mailKey' => 'bytes20', 'isBoundToClusterService' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( @@ -60,10 +58,6 @@ final class AlmanacDevice $this->nameIndex = PhabricatorHash::digestForIndex($this->getName()); - if (!$this->mailKey) { - $this->mailKey = Filesystem::readRandomCharacters(20); - } - return parent::save(); } From 9003a453691fdce5ab85a932e175c7c925e7225f Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 11:14:26 -0700 Subject: [PATCH 13/29] Make minor Almanac device modernization updates Summary: Ref T13641. Provide minor modernizations before adding a "Disabled" state. Test Plan: Browsed devices, created a new device. Maniphest Tasks: T13641 Differential Revision: https://secure.phabricator.com/D21626 --- src/applications/almanac/storage/AlmanacDevice.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/applications/almanac/storage/AlmanacDevice.php b/src/applications/almanac/storage/AlmanacDevice.php index 8529285dc0..4910112f97 100644 --- a/src/applications/almanac/storage/AlmanacDevice.php +++ b/src/applications/almanac/storage/AlmanacDevice.php @@ -49,8 +49,8 @@ final class AlmanacDevice ) + parent::getConfiguration(); } - public function generatePHID() { - return PhabricatorPHID::generateNewPHID(AlmanacDevicePHIDType::TYPECONST); + public function getPHIDType() { + return AlmanacDevicePHIDType::TYPECONST; } public function save() { @@ -62,7 +62,9 @@ final class AlmanacDevice } public function getURI() { - return '/almanac/device/view/'.$this->getName().'/'; + return urisprintf( + '/almanac/device/view/%s/', + $this->getName()); } public function rebuildClusterBindingStatus() { @@ -84,8 +86,8 @@ final class AlmanacDevice $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); queryfx( $this->establishConnection('w'), - 'UPDATE %T SET isBoundToClusterService = %d WHERE id = %d', - $this->getTableName(), + 'UPDATE %R SET isBoundToClusterService = %d WHERE id = %d', + $this, $this->getIsBoundToClusterService(), $this->getID()); unset($unguarded); From 5d64fb1815f757760ed16df3e9b2ef1bf9f79638 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 11:05:33 -0700 Subject: [PATCH 14/29] Add a "status" property to Almanac devices Summary: Ref T13641. Add a "status" property with most of the relevant support code. This currently has no impact on use of the device or bindings by Diffusion or Drydock: they ignore the status of devices bound to services. Test Plan: - Created a new device. - Changed the status of a device via web and API. - Queried for devices via API. - Searched for active and disabled devices. - Viewed UI in list view, detail view. - Used typeahead to add a new binding to an interface on a disabled device, got disabled hint in typeahead UI. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13641 Differential Revision: https://secure.phabricator.com/D21627 --- .../20210316.almanac.03.device-status.sql | 2 + ...0210316.almanac.04.device-status-value.sql | 2 + src/__phutil_library_map__.php | 4 + .../almanac/constants/AlmanacDeviceStatus.php | 89 +++++++++++++++++++ .../AlmanacDeviceViewController.php | 8 ++ .../editor/AlmanacDeviceEditEngine.php | 27 ++++++ .../almanac/phid/AlmanacInterfacePHIDType.php | 7 +- .../almanac/query/AlmanacDeviceQuery.php | 13 +++ .../query/AlmanacDeviceSearchEngine.php | 25 ++++++ .../almanac/storage/AlmanacDevice.php | 25 ++++++ .../typeahead/AlmanacInterfaceDatasource.php | 9 +- .../AlmanacDeviceStatusTransaction.php | 61 +++++++++++++ .../phid/PhabricatorObjectHandle.php | 4 + 13 files changed, 274 insertions(+), 2 deletions(-) create mode 100644 resources/sql/autopatches/20210316.almanac.03.device-status.sql create mode 100644 resources/sql/autopatches/20210316.almanac.04.device-status-value.sql create mode 100644 src/applications/almanac/constants/AlmanacDeviceStatus.php create mode 100644 src/applications/almanac/xaction/AlmanacDeviceStatusTransaction.php diff --git a/resources/sql/autopatches/20210316.almanac.03.device-status.sql b/resources/sql/autopatches/20210316.almanac.03.device-status.sql new file mode 100644 index 0000000000..08618acb8f --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.03.device-status.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_almanac.almanac_device + ADD status VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20210316.almanac.04.device-status-value.sql b/resources/sql/autopatches/20210316.almanac.04.device-status-value.sql new file mode 100644 index 0000000000..659bc7850f --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.04.device-status-value.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_almanac.almanac_device + SET status = 'active' WHERE status = ''; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3bdc63e7b2..5794fac11b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -62,6 +62,8 @@ phutil_register_library_map(array( 'AlmanacDeviceSearchConduitAPIMethod' => 'applications/almanac/conduit/AlmanacDeviceSearchConduitAPIMethod.php', 'AlmanacDeviceSearchEngine' => 'applications/almanac/query/AlmanacDeviceSearchEngine.php', 'AlmanacDeviceSetPropertyTransaction' => 'applications/almanac/xaction/AlmanacDeviceSetPropertyTransaction.php', + 'AlmanacDeviceStatus' => 'applications/almanac/constants/AlmanacDeviceStatus.php', + 'AlmanacDeviceStatusTransaction' => 'applications/almanac/xaction/AlmanacDeviceStatusTransaction.php', 'AlmanacDeviceTransaction' => 'applications/almanac/storage/AlmanacDeviceTransaction.php', 'AlmanacDeviceTransactionQuery' => 'applications/almanac/query/AlmanacDeviceTransactionQuery.php', 'AlmanacDeviceTransactionType' => 'applications/almanac/xaction/AlmanacDeviceTransactionType.php', @@ -6089,6 +6091,8 @@ phutil_register_library_map(array( 'AlmanacDeviceSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'AlmanacDeviceSearchEngine' => 'PhabricatorApplicationSearchEngine', 'AlmanacDeviceSetPropertyTransaction' => 'AlmanacDeviceTransactionType', + 'AlmanacDeviceStatus' => 'Phobject', + 'AlmanacDeviceStatusTransaction' => 'AlmanacDeviceTransactionType', 'AlmanacDeviceTransaction' => 'AlmanacModularTransaction', 'AlmanacDeviceTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'AlmanacDeviceTransactionType' => 'AlmanacTransactionType', diff --git a/src/applications/almanac/constants/AlmanacDeviceStatus.php b/src/applications/almanac/constants/AlmanacDeviceStatus.php new file mode 100644 index 0000000000..492e0d2a98 --- /dev/null +++ b/src/applications/almanac/constants/AlmanacDeviceStatus.php @@ -0,0 +1,89 @@ +value = $value; + return $status; + } + + public function getValue() { + return $this->value; + } + + public function getName() { + $name = $this->getDeviceStatusProperty('name'); + + if ($name === null) { + $name = pht('Unknown Almanac Device Status ("%s")', $this->getValue()); + } + + return $name; + } + + public function getIconIcon() { + return $this->getDeviceStatusProperty('icon.icon'); + } + + public function getIconColor() { + return $this->getDeviceStatusProperty('icon.color'); + } + + public function isDisabled() { + return ($this->getValue() === self::DISABLED); + } + + public function hasStatusTag() { + return ($this->getStatusTagIcon() !== null); + } + + public function getStatusTagIcon() { + return $this->getDeviceStatusProperty('status-tag.icon'); + } + + public function getStatusTagColor() { + return $this->getDeviceStatusProperty('status-tag.color'); + } + + public static function getStatusMap() { + $result = array(); + + foreach (self::newDeviceStatusMap() as $status_value => $ignored) { + $result[$status_value] = self::newStatusFromValue($status_value); + } + + return $result; + } + + private function getDeviceStatusProperty($key, $default = null) { + $map = self::newDeviceStatusMap(); + $properties = idx($map, $this->getValue(), array()); + return idx($properties, $key, $default); + } + + private static function newDeviceStatusMap() { + return array( + self::ACTIVE => array( + 'name' => pht('Active'), + 'icon.icon' => 'fa-server', + 'icon.color' => 'green', + ), + self::DISABLED => array( + 'name' => pht('Disabled'), + 'icon.icon' => 'fa-times', + 'icon.color' => 'grey', + 'status-tag.icon' => 'fa-times', + 'status-tag.color' => 'indigo', + ), + ); + } + + +} diff --git a/src/applications/almanac/controller/AlmanacDeviceViewController.php b/src/applications/almanac/controller/AlmanacDeviceViewController.php index 99cc9c2526..39939abfa7 100644 --- a/src/applications/almanac/controller/AlmanacDeviceViewController.php +++ b/src/applications/almanac/controller/AlmanacDeviceViewController.php @@ -31,6 +31,14 @@ final class AlmanacDeviceViewController ->setPolicyObject($device) ->setHeaderIcon('fa-server'); + $status = $device->getStatusObject(); + if ($status->hasStatusTag()) { + $header->setStatus( + $status->getStatusTagIcon(), + $status->getStatusTagColor(), + $status->getName()); + } + $issue = null; if ($device->isClusterDevice()) { $issue = $this->addClusterMessage( diff --git a/src/applications/almanac/editor/AlmanacDeviceEditEngine.php b/src/applications/almanac/editor/AlmanacDeviceEditEngine.php index fa0625e6ed..d0c2b48f7a 100644 --- a/src/applications/almanac/editor/AlmanacDeviceEditEngine.php +++ b/src/applications/almanac/editor/AlmanacDeviceEditEngine.php @@ -76,6 +76,8 @@ final class AlmanacDeviceEditEngine } protected function buildCustomEditFields($object) { + $status_map = $this->getDeviceStatusMap($object); + return array( id(new PhabricatorTextEditField()) ->setKey('name') @@ -84,7 +86,32 @@ final class AlmanacDeviceEditEngine ->setTransactionType(AlmanacDeviceNameTransaction::TRANSACTIONTYPE) ->setIsRequired(true) ->setValue($object->getName()), + id(new PhabricatorSelectEditField()) + ->setKey('status') + ->setLabel(pht('Status')) + ->setDescription(pht('Device status.')) + ->setTransactionType(AlmanacDeviceStatusTransaction::TRANSACTIONTYPE) + ->setOptions($status_map) + ->setValue($object->getStatus()), ); } + + private function getDeviceStatusMap(AlmanacDevice $device) { + $status_map = AlmanacDeviceStatus::getStatusMap(); + + // If the device currently has an unknown status, add it to the list for + // the dropdown. + $status_value = $device->getStatus(); + if (!isset($status_map[$status_value])) { + $status_map = array( + $status_value => AlmanacDeviceStatus::newStatusFromValue($status_value), + ) + $status_map; + } + + $status_map = mpull($status_map, 'getName'); + + return $status_map; + } + } diff --git a/src/applications/almanac/phid/AlmanacInterfacePHIDType.php b/src/applications/almanac/phid/AlmanacInterfacePHIDType.php index 581e86e3be..c7fe73183a 100644 --- a/src/applications/almanac/phid/AlmanacInterfacePHIDType.php +++ b/src/applications/almanac/phid/AlmanacInterfacePHIDType.php @@ -34,7 +34,8 @@ final class AlmanacInterfacePHIDType extends PhabricatorPHIDType { $id = $interface->getID(); - $device_name = $interface->getDevice()->getName(); + $device = $interface->getDevice(); + $device_name = $device->getName(); $address = $interface->getAddress(); $port = $interface->getPort(); $network = $interface->getNetwork()->getName(); @@ -48,6 +49,10 @@ final class AlmanacInterfacePHIDType extends PhabricatorPHIDType { $handle->setObjectName(pht('Interface %d', $id)); $handle->setName($name); + + if ($device->isDisabled()) { + $handle->setStatus(PhabricatorObjectHandle::STATUS_CLOSED); + } } } diff --git a/src/applications/almanac/query/AlmanacDeviceQuery.php b/src/applications/almanac/query/AlmanacDeviceQuery.php index 21f8c952ef..7fcf219571 100644 --- a/src/applications/almanac/query/AlmanacDeviceQuery.php +++ b/src/applications/almanac/query/AlmanacDeviceQuery.php @@ -9,6 +9,7 @@ final class AlmanacDeviceQuery private $namePrefix; private $nameSuffix; private $isClusterDevice; + private $statuses; public function withIDs(array $ids) { $this->ids = $ids; @@ -35,6 +36,11 @@ final class AlmanacDeviceQuery return $this; } + public function withStatuses(array $statuses) { + $this->statuses = $statuses; + return $this; + } + public function withNameNgrams($ngrams) { return $this->withNgramsConstraint( new AlmanacDeviceNameNgrams(), @@ -103,6 +109,13 @@ final class AlmanacDeviceQuery (int)$this->isClusterDevice); } + if ($this->statuses !== null) { + $where[] = qsprintf( + $conn, + 'device.status IN (%Ls)', + $this->statuses); + } + return $where; } diff --git a/src/applications/almanac/query/AlmanacDeviceSearchEngine.php b/src/applications/almanac/query/AlmanacDeviceSearchEngine.php index 1d0a1d8475..956ade5ed4 100644 --- a/src/applications/almanac/query/AlmanacDeviceSearchEngine.php +++ b/src/applications/almanac/query/AlmanacDeviceSearchEngine.php @@ -16,6 +16,9 @@ final class AlmanacDeviceSearchEngine } protected function buildCustomSearchFields() { + $status_options = AlmanacDeviceStatus::getStatusMap(); + $status_options = mpull($status_options, 'getName'); + return array( id(new PhabricatorSearchTextField()) ->setLabel(pht('Name Contains')) @@ -25,6 +28,11 @@ final class AlmanacDeviceSearchEngine ->setLabel(pht('Exact Names')) ->setKey('names') ->setDescription(pht('Search for devices with specific names.')), + id(new PhabricatorSearchCheckboxesField()) + ->setLabel(pht('Statuses')) + ->setKey('statuses') + ->setDescription(pht('Search for devices with given statuses.')) + ->setOptions($status_options), id(new PhabricatorSearchThreeStateField()) ->setLabel(pht('Cluster Device')) ->setKey('isClusterDevice') @@ -50,6 +58,10 @@ final class AlmanacDeviceSearchEngine $query->withIsClusterDevice($map['isClusterDevice']); } + if ($map['statuses']) { + $query->withStatuses($map['statuses']); + } + return $query; } @@ -99,6 +111,19 @@ final class AlmanacDeviceSearchEngine $item->addIcon('fa-sitemap', pht('Cluster Device')); } + if ($device->isDisabled()) { + $item->setDisabled(true); + } + + $status = $device->getStatusObject(); + $icon_icon = $status->getIconIcon(); + $icon_color = $status->getIconColor(); + $icon_label = $status->getName(); + + $item->setStatusIcon( + "{$icon_icon} {$icon_color}", + $icon_label); + $list->addItem($item); } diff --git a/src/applications/almanac/storage/AlmanacDevice.php b/src/applications/almanac/storage/AlmanacDevice.php index 4910112f97..dfe4c27668 100644 --- a/src/applications/almanac/storage/AlmanacDevice.php +++ b/src/applications/almanac/storage/AlmanacDevice.php @@ -17,6 +17,7 @@ final class AlmanacDevice protected $nameIndex; protected $viewPolicy; protected $editPolicy; + protected $status; protected $isBoundToClusterService; private $almanacProperties = self::ATTACHABLE; @@ -25,6 +26,7 @@ final class AlmanacDevice return id(new AlmanacDevice()) ->setViewPolicy(PhabricatorPolicies::POLICY_USER) ->setEditPolicy(PhabricatorPolicies::POLICY_ADMIN) + ->setStatus(AlmanacDeviceStatus::ACTIVE) ->attachAlmanacProperties(array()) ->setIsBoundToClusterService(0); } @@ -35,6 +37,7 @@ final class AlmanacDevice self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'text128', 'nameIndex' => 'bytes12', + 'status' => 'text32', 'isBoundToClusterService' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( @@ -100,6 +103,18 @@ final class AlmanacDevice return $this->getIsBoundToClusterService(); } + public function getStatusObject() { + return $this->newStatusObject(); + } + + private function newStatusObject() { + return AlmanacDeviceStatus::newStatusFromValue($this->getStatus()); + } + + public function isDisabled() { + return $this->getStatusObject()->isDisabled(); + } + /* -( AlmanacPropertyInterface )------------------------------------------- */ @@ -263,12 +278,22 @@ final class AlmanacDevice ->setKey('name') ->setType('string') ->setDescription(pht('The name of the device.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('status') + ->setType('map') + ->setDescription(pht('Device status information.')), ); } public function getFieldValuesForConduit() { + $status = $this->getStatusObject(); + return array( 'name' => $this->getName(), + 'status' => array( + 'value' => $status->getValue(), + 'name' => $status->getName(), + ), ); } diff --git a/src/applications/almanac/typeahead/AlmanacInterfaceDatasource.php b/src/applications/almanac/typeahead/AlmanacInterfaceDatasource.php index 15e50f5993..dca5dd986b 100644 --- a/src/applications/almanac/typeahead/AlmanacInterfaceDatasource.php +++ b/src/applications/almanac/typeahead/AlmanacInterfaceDatasource.php @@ -46,9 +46,16 @@ final class AlmanacInterfaceDatasource $results = array(); foreach ($handles as $handle) { + if ($handle->isClosed()) { + $closed = pht('Disabled'); + } else { + $closed = null; + } + $results[] = id(new PhabricatorTypeaheadResult()) ->setName($handle->getName()) - ->setPHID($handle->getPHID()); + ->setPHID($handle->getPHID()) + ->setClosed($closed); } return $results; diff --git a/src/applications/almanac/xaction/AlmanacDeviceStatusTransaction.php b/src/applications/almanac/xaction/AlmanacDeviceStatusTransaction.php new file mode 100644 index 0000000000..57c7c05507 --- /dev/null +++ b/src/applications/almanac/xaction/AlmanacDeviceStatusTransaction.php @@ -0,0 +1,61 @@ +getStatus(); + } + + public function applyInternalEffects($object, $value) { + $object->setStatus($value); + } + + public function getTitle() { + $old_value = $this->getOldValue(); + $new_value = $this->getNewValue(); + + $old_status = AlmanacDeviceStatus::newStatusFromValue($old_value); + $new_status = AlmanacDeviceStatus::newStatusFromValue($new_value); + + $old_name = $old_status->getName(); + $new_name = $new_status->getName(); + + return pht( + '%s changed the status of this device from %s to %s.', + $this->renderAuthor(), + $this->renderValue($old_name), + $this->renderValue($new_name)); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + $status_map = AlmanacDeviceStatus::getStatusMap(); + + $old_value = $this->generateOldValue($object); + foreach ($xactions as $xaction) { + $new_value = $xaction->getNewValue(); + + if ($new_value === $old_value) { + continue; + } + + if (!isset($status_map[$new_value])) { + $errors[] = $this->newInvalidError( + pht( + 'Almanac device status "%s" is unrecognized. Valid status '. + 'values are: %s.', + $new_value, + implode(', ', array_keys($status_map))), + $xaction); + continue; + } + } + + return $errors; + } + +} diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php index 966c1a7db9..95c40ffa2b 100644 --- a/src/applications/phid/PhabricatorObjectHandle.php +++ b/src/applications/phid/PhabricatorObjectHandle.php @@ -197,6 +197,10 @@ final class PhabricatorObjectHandle return $this->status; } + public function isClosed() { + return ($this->status === self::STATUS_CLOSED); + } + public function setFullName($full_name) { $this->fullName = $full_name; return $this; From 15c0c895a5062b0b5115eec2538650ca5615c684 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 13:54:11 -0700 Subject: [PATCH 15/29] Make upstream callers respect "active bindings" when querying Almanac Summary: Ref T13641. Make "active bindings" a real query and make callers that only care about active bindings only query for active bindings. Test Plan: - Queried for "bindings" and "activeBindings" via Conduit. - Disabled/enabled devices, saw binding status update in UI. - Loaded Diffusion cluster layout. - Grepped for `needBindings()`, `getActiveBindings()`, etc. Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13641 Differential Revision: https://secure.phabricator.com/D21628 --- .../almanac/constants/AlmanacDeviceStatus.php | 21 +++++++ .../AlmanacBindingsSearchEngineAttachment.php | 27 ++++++++- .../AlmanacSearchEngineAttachment.php | 2 + .../almanac/query/AlmanacBindingQuery.php | 56 +++++++++++++++++-- .../almanac/query/AlmanacServiceQuery.php | 33 +++++++++-- .../almanac/storage/AlmanacDevice.php | 5 ++ .../almanac/storage/AlmanacService.php | 37 ++++++++---- .../almanac/view/AlmanacBindingTableView.php | 22 +++++++- ...fusionRepositoryStorageManagementPanel.php | 31 ++-------- ...anacServiceHostBlueprintImplementation.php | 15 ++--- ...RepositoryManagementClusterizeWorkflow.php | 2 +- .../storage/PhabricatorRepository.php | 4 +- 12 files changed, 192 insertions(+), 63 deletions(-) diff --git a/src/applications/almanac/constants/AlmanacDeviceStatus.php b/src/applications/almanac/constants/AlmanacDeviceStatus.php index 492e0d2a98..0bd27dde86 100644 --- a/src/applications/almanac/constants/AlmanacDeviceStatus.php +++ b/src/applications/almanac/constants/AlmanacDeviceStatus.php @@ -62,6 +62,26 @@ final class AlmanacDeviceStatus return $result; } + public static function getActiveStatusList() { + $results = array(); + foreach (self::newDeviceStatusMap() as $status_value => $status) { + if (empty($status['disabled'])) { + $results[] = $status_value; + } + } + return $results; + } + + public static function getDisabledStatusList() { + $results = array(); + foreach (self::newDeviceStatusMap() as $status_value => $status) { + if (!empty($status['disabled'])) { + $results[] = $status_value; + } + } + return $results; + } + private function getDeviceStatusProperty($key, $default = null) { $map = self::newDeviceStatusMap(); $properties = idx($map, $this->getValue(), array()); @@ -81,6 +101,7 @@ final class AlmanacDeviceStatus 'icon.color' => 'grey', 'status-tag.icon' => 'fa-times', 'status-tag.color' => 'indigo', + 'disabled' => true, ), ); } diff --git a/src/applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php b/src/applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php index a3afaf3251..c4f6b823ee 100644 --- a/src/applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php +++ b/src/applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php @@ -3,6 +3,17 @@ final class AlmanacBindingsSearchEngineAttachment extends AlmanacSearchEngineAttachment { + private $isActive; + + public function setIsActive($is_active) { + $this->isActive = $is_active; + return $this; + } + + public function getIsActive() { + return $this->isActive; + } + public function getAttachmentName() { return pht('Almanac Bindings'); } @@ -13,12 +24,24 @@ final class AlmanacBindingsSearchEngineAttachment public function willLoadAttachmentData($query, $spec) { $query->needProperties(true); - $query->needBindings(true); + + if ($this->getIsActive()) { + $query->needBindings(true); + } else { + $query->needActiveBindings(true); + } } public function getAttachmentForObject($object, $data, $spec) { $bindings = array(); - foreach ($object->getBindings() as $binding) { + + if ($this->getIsActive()) { + $service_bindings = $object->getActiveBindings(); + } else { + $service_bindings = $object->getBindings(); + } + + foreach ($service_bindings as $binding) { $bindings[] = $this->getAlmanacBindingDictionary($binding); } diff --git a/src/applications/almanac/engineextension/AlmanacSearchEngineAttachment.php b/src/applications/almanac/engineextension/AlmanacSearchEngineAttachment.php index 266b281990..97672a2af0 100644 --- a/src/applications/almanac/engineextension/AlmanacSearchEngineAttachment.php +++ b/src/applications/almanac/engineextension/AlmanacSearchEngineAttachment.php @@ -51,6 +51,8 @@ abstract class AlmanacSearchEngineAttachment 'phid' => $device->getPHID(), 'name' => $device->getName(), 'properties' => $this->getAlmanacPropertyList($device), + 'status' => $device->getStatus(), + 'disabled' => $device->isDisabled(), ); } diff --git a/src/applications/almanac/query/AlmanacBindingQuery.php b/src/applications/almanac/query/AlmanacBindingQuery.php index 5d51d2ba35..574967f7bb 100644 --- a/src/applications/almanac/query/AlmanacBindingQuery.php +++ b/src/applications/almanac/query/AlmanacBindingQuery.php @@ -8,6 +8,7 @@ final class AlmanacBindingQuery private $servicePHIDs; private $devicePHIDs; private $interfacePHIDs; + private $isActive; public function withIDs(array $ids) { $this->ids = $ids; @@ -34,6 +35,11 @@ final class AlmanacBindingQuery return $this; } + public function withIsActive($active) { + $this->isActive = $active; + return $this; + } + public function newResultObject() { return new AlmanacBinding(); } @@ -95,39 +101,79 @@ final class AlmanacBindingQuery if ($this->ids !== null) { $where[] = qsprintf( $conn, - 'id IN (%Ld)', + 'binding.id IN (%Ld)', $this->ids); } if ($this->phids !== null) { $where[] = qsprintf( $conn, - 'phid IN (%Ls)', + 'binding.phid IN (%Ls)', $this->phids); } if ($this->servicePHIDs !== null) { $where[] = qsprintf( $conn, - 'servicePHID IN (%Ls)', + 'binding.servicePHID IN (%Ls)', $this->servicePHIDs); } if ($this->devicePHIDs !== null) { $where[] = qsprintf( $conn, - 'devicePHID IN (%Ls)', + 'binding.devicePHID IN (%Ls)', $this->devicePHIDs); } if ($this->interfacePHIDs !== null) { $where[] = qsprintf( $conn, - 'interfacePHID IN (%Ls)', + 'binding.interfacePHID IN (%Ls)', $this->interfacePHIDs); } + if ($this->isActive !== null) { + if ($this->isActive) { + $where[] = qsprintf( + $conn, + '(binding.isDisabled = 0) AND (device.status IN (%Ls))', + AlmanacDeviceStatus::getActiveStatusList()); + } else { + $where[] = qsprintf( + $conn, + '(binding.isDisabled = 1) OR (device.status IN (%Ls))', + AlmanacDeviceStatus::getDisabledStatusList()); + } + } + return $where; } + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { + $joins = parent::buildJoinClauseParts($conn); + + if ($this->shouldJoinDeviceTable()) { + $device_table = new AlmanacDevice(); + $joins[] = qsprintf( + $conn, + 'JOIN %R device ON binding.devicePHID = device.phid', + $device_table); + } + + return $joins; + } + + private function shouldJoinDeviceTable() { + if ($this->isActive !== null) { + return true; + } + + return false; + } + + protected function getPrimaryTableAlias() { + return 'binding'; + } + } diff --git a/src/applications/almanac/query/AlmanacServiceQuery.php b/src/applications/almanac/query/AlmanacServiceQuery.php index edc55276a9..4e374ec90c 100644 --- a/src/applications/almanac/query/AlmanacServiceQuery.php +++ b/src/applications/almanac/query/AlmanacServiceQuery.php @@ -12,6 +12,7 @@ final class AlmanacServiceQuery private $nameSuffix; private $needBindings; + private $needActiveBindings; public function withIDs(array $ids) { $this->ids = $ids; @@ -59,6 +60,11 @@ final class AlmanacServiceQuery return $this; } + public function needActiveBindings($need_active) { + $this->needActiveBindings = $need_active; + return $this; + } + public function newResultObject() { return new AlmanacService(); } @@ -160,18 +166,35 @@ final class AlmanacServiceQuery } protected function didFilterPage(array $services) { - if ($this->needBindings) { + $need_all = $this->needBindings; + $need_active = $this->needActiveBindings; + + $need_any = ($need_all || $need_active); + $only_active = ($need_active && !$need_all); + + if ($need_any) { $service_phids = mpull($services, 'getPHID'); - $bindings = id(new AlmanacBindingQuery()) + + $bindings_query = id(new AlmanacBindingQuery()) ->setViewer($this->getViewer()) ->withServicePHIDs($service_phids) - ->needProperties($this->getNeedProperties()) - ->execute(); + ->needProperties($this->getNeedProperties()); + + if ($only_active) { + $bindings_query->withIsActive(true); + } + + $bindings = $bindings_query->execute(); $bindings = mgroup($bindings, 'getServicePHID'); foreach ($services as $service) { $service_bindings = idx($bindings, $service->getPHID(), array()); - $service->attachBindings($service_bindings); + + if ($only_active) { + $service->attachActiveBindings($service_bindings); + } else { + $service->attachBindings($service_bindings); + } } } diff --git a/src/applications/almanac/storage/AlmanacDevice.php b/src/applications/almanac/storage/AlmanacDevice.php index dfe4c27668..87704d89d8 100644 --- a/src/applications/almanac/storage/AlmanacDevice.php +++ b/src/applications/almanac/storage/AlmanacDevice.php @@ -282,6 +282,10 @@ final class AlmanacDevice ->setKey('status') ->setType('map') ->setDescription(pht('Device status information.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('disabled') + ->setType('bool') + ->setDescription(pht('True if device is disabled.')), ); } @@ -294,6 +298,7 @@ final class AlmanacDevice 'value' => $status->getValue(), 'name' => $status->getName(), ), + 'disabled' => $this->isDisabled(), ); } diff --git a/src/applications/almanac/storage/AlmanacService.php b/src/applications/almanac/storage/AlmanacService.php index 2979b74367..1683ba1340 100644 --- a/src/applications/almanac/storage/AlmanacService.php +++ b/src/applications/almanac/storage/AlmanacService.php @@ -21,6 +21,7 @@ final class AlmanacService private $almanacProperties = self::ATTACHABLE; private $bindings = self::ATTACHABLE; + private $activeBindings = self::ATTACHABLE; private $serviceImplementation = self::ATTACHABLE; public static function initializeNewService($type) { @@ -91,23 +92,36 @@ final class AlmanacService } public function getActiveBindings() { - $bindings = $this->getBindings(); - - // Filter out disabled bindings. - foreach ($bindings as $key => $binding) { - if ($binding->getIsDisabled()) { - unset($bindings[$key]); - } - } - - return $bindings; + return $this->assertAttached($this->activeBindings); } public function attachBindings(array $bindings) { + $active_bindings = array(); + foreach ($bindings as $key => $binding) { + // Filter out disabled bindings. + if ($binding->getIsDisabled()) { + continue; + } + + // Filter out bindings to disabled devices. + if ($binding->getDevice()->isDisabled()) { + continue; + } + + $active_bindings[$key] = $binding; + } + + $this->attachActiveBindings($active_bindings); + $this->bindings = $bindings; return $this; } + public function attachActiveBindings(array $bindings) { + $this->activeBindings = $bindings; + return $this; + } + public function getServiceImplementation() { return $this->assertAttached($this->serviceImplementation); } @@ -289,6 +303,9 @@ final class AlmanacService ->setAttachmentKey('properties'), id(new AlmanacBindingsSearchEngineAttachment()) ->setAttachmentKey('bindings'), + id(new AlmanacBindingsSearchEngineAttachment()) + ->setIsActive(true) + ->setAttachmentKey('activeBindings'), ); } diff --git a/src/applications/almanac/view/AlmanacBindingTableView.php b/src/applications/almanac/view/AlmanacBindingTableView.php index 746d3de5c2..a74cb4f9c3 100644 --- a/src/applications/almanac/view/AlmanacBindingTableView.php +++ b/src/applications/almanac/view/AlmanacBindingTableView.php @@ -56,21 +56,41 @@ final class AlmanacBindingTableView extends AphrontView { $icon_active = id(new PHUIIconView()) ->setIcon('fa-check') + ->setColor('green') ->addSigil('has-tooltip') ->setMetadata( array( 'tip' => pht('Active'), )); + $icon_device_disabled = id(new PHUIIconView()) + ->setIcon('fa-times') + ->setColor('grey') + ->addSigil('has-tooltip') + ->setMetadata( + array( + 'tip' => pht('Device Disabled'), + )); + $rows = array(); foreach ($bindings as $binding) { $addr = $binding->getInterface()->getAddress(); $port = $binding->getInterface()->getPort(); + $device = $binding->getDevice(); + if ($device->isDisabled()) { + $binding_icon = $icon_device_disabled; + } else if ($binding->getIsDisabled()) { + $binding_icon = $icon_disabled; + } else { + $binding_icon = $icon_active; + } + $rows[] = array( $binding->getID(), - ($binding->getIsDisabled() ? $icon_disabled : $icon_active), + $binding_icon, $handles->renderHandle($binding->getServicePHID()), + $handles->renderHandle($binding->getDevicePHID()), $handles->renderHandle($binding->getInterface()->getNetworkPHID()), $binding->getInterface()->renderDisplayAddress(), diff --git a/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php index 86ff93816c..f657a81641 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php @@ -89,7 +89,7 @@ final class DiffusionRepositoryStorageManagementPanel AlmanacClusterRepositoryServiceType::SERVICETYPE, )) ->withPHIDs(array($service_phid)) - ->needBindings(true) + ->needActiveBindings(true) ->executeOne(); if (!$service) { // TODO: Viewer may not have permission to see the service, or it may @@ -104,7 +104,7 @@ final class DiffusionRepositoryStorageManagementPanel $rows = array(); if ($service) { - $bindings = $service->getBindings(); + $bindings = $service->getActiveBindings(); $bindings = mgroup($bindings, 'getDevicePHID'); // This is an unusual read which always comes from the master. @@ -117,29 +117,19 @@ final class DiffusionRepositoryStorageManagementPanel $versions = mpull($versions, null, 'getDevicePHID'); - // List enabled devices first, then sort devices in each group by name. $sort = array(); foreach ($bindings as $key => $binding_group) { - $all_disabled = $this->isDisabledGroup($binding_group); - $sort[$key] = id(new PhutilSortVector()) - ->addInt($all_disabled ? 1 : 0) ->addString(head($binding_group)->getDevice()->getName()); } $sort = msortv($sort, 'getSelf'); $bindings = array_select_keys($bindings, array_keys($sort)) + $bindings; foreach ($bindings as $binding_group) { - $all_disabled = $this->isDisabledGroup($binding_group); $any_binding = head($binding_group); - if ($all_disabled) { - $binding_icon = 'fa-times grey'; - $binding_tip = pht('Disabled'); - } else { - $binding_icon = 'fa-folder-open green'; - $binding_tip = pht('Active'); - } + $binding_icon = 'fa-folder-open green'; + $binding_tip = pht('Active'); $binding_icon = id(new PHUIIconView()) ->setIcon($binding_icon) @@ -376,17 +366,4 @@ final class DiffusionRepositoryStorageManagementPanel return $box_view; } - - private function isDisabledGroup(array $binding_group) { - assert_instances_of($binding_group, 'AlmanacBinding'); - - foreach ($binding_group as $binding) { - if (!$binding->getIsDisabled()) { - return false; - } - } - - return true; - } - } diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php index 12ccf8a343..6c8e75696f 100644 --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php @@ -34,7 +34,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation DrydockBlueprint $blueprint, DrydockLease $lease) { $services = $this->loadServices($blueprint); - $bindings = $this->loadAllBindings($services); + $bindings = $this->getActiveBindings($services); if (!$bindings) { // If there are no devices bound to the services for this blueprint, @@ -222,7 +222,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation ->setViewer($viewer) ->withPHIDs($service_phids) ->withServiceTypes($this->getAlmanacServiceTypes()) - ->needBindings(true) + ->needActiveBindings(true) ->execute(); $services = mpull($services, null, 'getPHID'); @@ -242,9 +242,9 @@ final class DrydockAlmanacServiceHostBlueprintImplementation return $this->services; } - private function loadAllBindings(array $services) { + private function getActive(array $services) { assert_instances_of($services, 'AlmanacService'); - $bindings = array_mergev(mpull($services, 'getBindings')); + $bindings = array_mergev(mpull($services, 'getActiveBindings')); return mpull($bindings, null, 'getPHID'); } @@ -271,15 +271,10 @@ final class DrydockAlmanacServiceHostBlueprintImplementation $allocated_phids = array_fuse($allocated_phids); $services = $this->loadServices($blueprint); - $bindings = $this->loadAllBindings($services); + $bindings = $this->getActiveBindings($services); $free = array(); foreach ($bindings as $binding) { - // Don't consider disabled bindings to be available. - if ($binding->getIsDisabled()) { - continue; - } - if (empty($allocated_phids[$binding->getPHID()])) { $free[] = $binding; } diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementClusterizeWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementClusterizeWorkflow.php index 2e2c3fcc92..c3de833a1f 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementClusterizeWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementClusterizeWorkflow.php @@ -61,7 +61,7 @@ final class PhabricatorRepositoryManagementClusterizeWorkflow array( AlmanacClusterRepositoryServiceType::SERVICETYPE, )) - ->needBindings(true) + ->needActiveBindings(true) ->executeOne(); if (!$service) { throw new PhutilArgumentUsageException( diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 508a886185..8c98dd8756 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -2109,7 +2109,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO throw new Exception( pht( 'The Almanac service for this repository is not bound to any '. - 'interfaces.')); + 'active interfaces.')); } $uris = array(); @@ -2531,7 +2531,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $service = id(new AlmanacServiceQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withPHIDs(array($service_phid)) - ->needBindings(true) + ->needActiveBindings(true) ->needProperties(true) ->executeOne(); if (!$service) { From fadb2bd52b5e39de7971067e43966e70c5321545 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 14:58:36 -0700 Subject: [PATCH 16/29] Modernize "mailKey" on AlamnacService Summary: Ref T13641. Ref T13065. Migrate and drop the onboard "mailKey" column for Almanac Services. Test Plan: Ran storage migration, got clean report, saw migrated value in mail properties table. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13641, T13065 Differential Revision: https://secure.phabricator.com/D21629 --- .../20210316.almanac.05.service-mailkey.php | 28 +++++++++++++++++++ ...0210316.almanac.06.service-dropmailkey.sql | 2 ++ .../almanac/storage/AlmanacService.php | 10 ++----- 3 files changed, 32 insertions(+), 8 deletions(-) create mode 100644 resources/sql/autopatches/20210316.almanac.05.service-mailkey.php create mode 100644 resources/sql/autopatches/20210316.almanac.06.service-dropmailkey.sql diff --git a/resources/sql/autopatches/20210316.almanac.05.service-mailkey.php b/resources/sql/autopatches/20210316.almanac.05.service-mailkey.php new file mode 100644 index 0000000000..33318eb663 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.05.service-mailkey.php @@ -0,0 +1,28 @@ +establishConnection('w'); + +$properties_table = new PhabricatorMetaMTAMailProperties(); +$conn = $properties_table->establishConnection('w'); + +$iterator = new LiskRawMigrationIterator( + $service_conn, + $service_table->getTableName()); + +foreach ($iterator as $row) { + queryfx( + $conn, + 'INSERT IGNORE INTO %R + (objectPHID, mailProperties, dateCreated, dateModified) + VALUES + (%s, %s, %d, %d)', + $properties_table, + $row['phid'], + phutil_json_encode( + array( + 'mailKey' => $row['mailKey'], + )), + PhabricatorTime::getNow(), + PhabricatorTime::getNow()); +} diff --git a/resources/sql/autopatches/20210316.almanac.06.service-dropmailkey.sql b/resources/sql/autopatches/20210316.almanac.06.service-dropmailkey.sql new file mode 100644 index 0000000000..b9966eddf9 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.06.service-dropmailkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_almanac.almanac_service + DROP mailKey; diff --git a/src/applications/almanac/storage/AlmanacService.php b/src/applications/almanac/storage/AlmanacService.php index 1683ba1340..58a3f681df 100644 --- a/src/applications/almanac/storage/AlmanacService.php +++ b/src/applications/almanac/storage/AlmanacService.php @@ -14,7 +14,6 @@ final class AlmanacService protected $name; protected $nameIndex; - protected $mailKey; protected $viewPolicy; protected $editPolicy; protected $serviceType; @@ -49,7 +48,6 @@ final class AlmanacService self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'text128', 'nameIndex' => 'bytes12', - 'mailKey' => 'bytes20', 'serviceType' => 'text64', ), self::CONFIG_KEY_SCHEMA => array( @@ -67,8 +65,8 @@ final class AlmanacService ) + parent::getConfiguration(); } - public function generatePHID() { - return PhabricatorPHID::generateNewPHID(AlmanacServicePHIDType::TYPECONST); + public function getPHIDType() { + return AlmanacServicePHIDType::TYPECONST; } public function save() { @@ -76,10 +74,6 @@ final class AlmanacService $this->nameIndex = PhabricatorHash::digestForIndex($this->getName()); - if (!$this->mailKey) { - $this->mailKey = Filesystem::readRandomCharacters(20); - } - return parent::save(); } From 7c0e33c34d6f7c49f7e4c5cad86e90af09333fee Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 15:04:07 -0700 Subject: [PATCH 17/29] Modernize "mailKey" for Almanac Bindings Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column. Test Plan: Ran "bin/storage upgrade", got a clean report and saw binding mail keys in the mail properties table. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13641, T13065 Differential Revision: https://secure.phabricator.com/D21630 --- .../20210316.almanac.07.binding-mailkey.php | 28 +++++++++++++++++++ ...0210316.almanac.08.binding-dropmailkey.sql | 2 ++ .../almanac/storage/AlmanacBinding.php | 17 ++++------- 3 files changed, 35 insertions(+), 12 deletions(-) create mode 100644 resources/sql/autopatches/20210316.almanac.07.binding-mailkey.php create mode 100644 resources/sql/autopatches/20210316.almanac.08.binding-dropmailkey.sql diff --git a/resources/sql/autopatches/20210316.almanac.07.binding-mailkey.php b/resources/sql/autopatches/20210316.almanac.07.binding-mailkey.php new file mode 100644 index 0000000000..84c1725876 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.07.binding-mailkey.php @@ -0,0 +1,28 @@ +establishConnection('w'); + +$properties_table = new PhabricatorMetaMTAMailProperties(); +$conn = $properties_table->establishConnection('w'); + +$iterator = new LiskRawMigrationIterator( + $binding_conn, + $binding_table->getTableName()); + +foreach ($iterator as $row) { + queryfx( + $conn, + 'INSERT IGNORE INTO %R + (objectPHID, mailProperties, dateCreated, dateModified) + VALUES + (%s, %s, %d, %d)', + $properties_table, + $row['phid'], + phutil_json_encode( + array( + 'mailKey' => $row['mailKey'], + )), + PhabricatorTime::getNow(), + PhabricatorTime::getNow()); +} diff --git a/resources/sql/autopatches/20210316.almanac.08.binding-dropmailkey.sql b/resources/sql/autopatches/20210316.almanac.08.binding-dropmailkey.sql new file mode 100644 index 0000000000..ebcf31254d --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.08.binding-dropmailkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_almanac.almanac_binding + DROP mailKey; diff --git a/src/applications/almanac/storage/AlmanacBinding.php b/src/applications/almanac/storage/AlmanacBinding.php index a7096fc51f..011a085151 100644 --- a/src/applications/almanac/storage/AlmanacBinding.php +++ b/src/applications/almanac/storage/AlmanacBinding.php @@ -13,7 +13,6 @@ final class AlmanacBinding protected $servicePHID; protected $devicePHID; protected $interfacePHID; - protected $mailKey; protected $isDisabled; private $service = self::ATTACHABLE; @@ -33,7 +32,6 @@ final class AlmanacBinding return array( self::CONFIG_AUX_PHID => true, self::CONFIG_COLUMN_SCHEMA => array( - 'mailKey' => 'bytes20', 'isDisabled' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( @@ -51,15 +49,8 @@ final class AlmanacBinding ) + parent::getConfiguration(); } - public function generatePHID() { - return PhabricatorPHID::generateNewPHID(AlmanacBindingPHIDType::TYPECONST); - } - - public function save() { - if (!$this->mailKey) { - $this->mailKey = Filesystem::readRandomCharacters(20); - } - return parent::save(); + public function getPHIDType() { + return AlmanacBindingPHIDType::TYPECONST; } public function getName() { @@ -67,7 +58,9 @@ final class AlmanacBinding } public function getURI() { - return '/almanac/binding/'.$this->getID().'/'; + return urisprintf( + '/almanac/binding/%s/', + $this->getID()); } public function getService() { From 8226b3c88082ba74ad1e2318d13b385e2219fefa Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 15:06:22 -0700 Subject: [PATCH 18/29] Modernize "mailKey" on Almanac Namespaces Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column. Test Plan: Ran "bin/storage upgrade", got a clean report, saw mail keys migrated to mail properties table. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13641, T13065 Differential Revision: https://secure.phabricator.com/D21631 --- .../20210316.almanac.09.namespace-mailkey.php | 28 +++++++++++++++++++ ...10316.almanac.10.namespace-dropmailkey.sql | 2 ++ .../almanac/storage/AlmanacNamespace.php | 15 ++++------ 3 files changed, 35 insertions(+), 10 deletions(-) create mode 100644 resources/sql/autopatches/20210316.almanac.09.namespace-mailkey.php create mode 100644 resources/sql/autopatches/20210316.almanac.10.namespace-dropmailkey.sql diff --git a/resources/sql/autopatches/20210316.almanac.09.namespace-mailkey.php b/resources/sql/autopatches/20210316.almanac.09.namespace-mailkey.php new file mode 100644 index 0000000000..261181324e --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.09.namespace-mailkey.php @@ -0,0 +1,28 @@ +establishConnection('w'); + +$properties_table = new PhabricatorMetaMTAMailProperties(); +$conn = $properties_table->establishConnection('w'); + +$iterator = new LiskRawMigrationIterator( + $namespace_conn, + $namespace_table->getTableName()); + +foreach ($iterator as $row) { + queryfx( + $conn, + 'INSERT IGNORE INTO %R + (objectPHID, mailProperties, dateCreated, dateModified) + VALUES + (%s, %s, %d, %d)', + $properties_table, + $row['phid'], + phutil_json_encode( + array( + 'mailKey' => $row['mailKey'], + )), + PhabricatorTime::getNow(), + PhabricatorTime::getNow()); +} diff --git a/resources/sql/autopatches/20210316.almanac.10.namespace-dropmailkey.sql b/resources/sql/autopatches/20210316.almanac.10.namespace-dropmailkey.sql new file mode 100644 index 0000000000..867c42fbc2 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.10.namespace-dropmailkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_almanac.almanac_namespace + DROP mailKey; diff --git a/src/applications/almanac/storage/AlmanacNamespace.php b/src/applications/almanac/storage/AlmanacNamespace.php index f3745b1d29..f6e3719057 100644 --- a/src/applications/almanac/storage/AlmanacNamespace.php +++ b/src/applications/almanac/storage/AlmanacNamespace.php @@ -12,7 +12,6 @@ final class AlmanacNamespace protected $name; protected $nameIndex; - protected $mailKey; protected $viewPolicy; protected $editPolicy; @@ -28,7 +27,6 @@ final class AlmanacNamespace self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'text128', 'nameIndex' => 'bytes12', - 'mailKey' => 'bytes20', ), self::CONFIG_KEY_SCHEMA => array( 'key_nameindex' => array( @@ -42,9 +40,8 @@ final class AlmanacNamespace ) + parent::getConfiguration(); } - public function generatePHID() { - return PhabricatorPHID::generateNewPHID( - AlmanacNamespacePHIDType::TYPECONST); + public function getPHIDType() { + return AlmanacNamespacePHIDType::TYPECONST; } public function save() { @@ -52,15 +49,13 @@ final class AlmanacNamespace $this->nameIndex = PhabricatorHash::digestForIndex($this->getName()); - if (!$this->mailKey) { - $this->mailKey = Filesystem::readRandomCharacters(20); - } - return parent::save(); } public function getURI() { - return '/almanac/namespace/view/'.$this->getName().'/'; + return urisprintf( + '/almanac/namespace/view/%s/', + $this->getName()); } public function getNameLength() { From 86669ab54f9793e75df161f21941bb74cc097e31 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 15:09:50 -0700 Subject: [PATCH 19/29] Modernize "mailKey" for Almanac Networks Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column. Test Plan: Ran "bin/storage upgrade", got a clean report, saw keys migrate in mail properties table. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13641, T13065 Differential Revision: https://secure.phabricator.com/D21632 --- .../20210316.almanac.11.network-mailkey.php | 28 +++++++++++++++++++ ...0210316.almanac.12.network-dropmailkey.sql | 2 ++ .../almanac/storage/AlmanacNetwork.php | 19 ++++--------- 3 files changed, 35 insertions(+), 14 deletions(-) create mode 100644 resources/sql/autopatches/20210316.almanac.11.network-mailkey.php create mode 100644 resources/sql/autopatches/20210316.almanac.12.network-dropmailkey.sql diff --git a/resources/sql/autopatches/20210316.almanac.11.network-mailkey.php b/resources/sql/autopatches/20210316.almanac.11.network-mailkey.php new file mode 100644 index 0000000000..1ece4e2353 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.11.network-mailkey.php @@ -0,0 +1,28 @@ +establishConnection('w'); + +$properties_table = new PhabricatorMetaMTAMailProperties(); +$conn = $properties_table->establishConnection('w'); + +$iterator = new LiskRawMigrationIterator( + $network_conn, + $network_table->getTableName()); + +foreach ($iterator as $row) { + queryfx( + $conn, + 'INSERT IGNORE INTO %R + (objectPHID, mailProperties, dateCreated, dateModified) + VALUES + (%s, %s, %d, %d)', + $properties_table, + $row['phid'], + phutil_json_encode( + array( + 'mailKey' => $row['mailKey'], + )), + PhabricatorTime::getNow(), + PhabricatorTime::getNow()); +} diff --git a/resources/sql/autopatches/20210316.almanac.12.network-dropmailkey.sql b/resources/sql/autopatches/20210316.almanac.12.network-dropmailkey.sql new file mode 100644 index 0000000000..266f1250f0 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.12.network-dropmailkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_almanac.almanac_network + DROP mailKey; diff --git a/src/applications/almanac/storage/AlmanacNetwork.php b/src/applications/almanac/storage/AlmanacNetwork.php index 78313fad77..a3dadee69b 100644 --- a/src/applications/almanac/storage/AlmanacNetwork.php +++ b/src/applications/almanac/storage/AlmanacNetwork.php @@ -10,7 +10,6 @@ final class AlmanacNetwork PhabricatorConduitResultInterface { protected $name; - protected $mailKey; protected $viewPolicy; protected $editPolicy; @@ -25,8 +24,6 @@ final class AlmanacNetwork self::CONFIG_AUX_PHID => true, self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'sort128', - 'mailKey' => 'bytes20', - ), self::CONFIG_KEY_SCHEMA => array( 'key_name' => array( @@ -37,20 +34,14 @@ final class AlmanacNetwork ) + parent::getConfiguration(); } - public function generatePHID() { - return PhabricatorPHID::generateNewPHID(AlmanacNetworkPHIDType::TYPECONST); - } - - public function save() { - if (!$this->mailKey) { - $this->mailKey = Filesystem::readRandomCharacters(20); - } - - return parent::save(); + public function getPHIDType() { + return AlmanacNetworkPHIDType::TYPECONST; } public function getURI() { - return '/almanac/network/'.$this->getID().'/'; + return urisprintf( + '/almanac/network/%s/', + $this->getID()); } From eeb009b4fa610ecf9caa3cc41518cdbeb8b1bf07 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 15:15:11 -0700 Subject: [PATCH 20/29] Modernize "mailKey" for Calendar Event Summary: Ref T13065. Migrate "mailKey" and drop the column. Test Plan: Ran "bin/storage upgrade", got a clean report, saw migrated data in mail properties table. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13065 Differential Revision: https://secure.phabricator.com/D21633 --- .../20210316.almanac.13.event-mailkey.php | 28 +++++++++++++++++++ .../20210316.almanac.14.event-dropmailkey.sql | 2 ++ .../storage/PhabricatorCalendarEvent.php | 11 ++------ 3 files changed, 32 insertions(+), 9 deletions(-) create mode 100644 resources/sql/autopatches/20210316.almanac.13.event-mailkey.php create mode 100644 resources/sql/autopatches/20210316.almanac.14.event-dropmailkey.sql diff --git a/resources/sql/autopatches/20210316.almanac.13.event-mailkey.php b/resources/sql/autopatches/20210316.almanac.13.event-mailkey.php new file mode 100644 index 0000000000..7436ce3591 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.13.event-mailkey.php @@ -0,0 +1,28 @@ +establishConnection('w'); + +$properties_table = new PhabricatorMetaMTAMailProperties(); +$conn = $properties_table->establishConnection('w'); + +$iterator = new LiskRawMigrationIterator( + $event_conn, + $event_table->getTableName()); + +foreach ($iterator as $row) { + queryfx( + $conn, + 'INSERT IGNORE INTO %R + (objectPHID, mailProperties, dateCreated, dateModified) + VALUES + (%s, %s, %d, %d)', + $properties_table, + $row['phid'], + phutil_json_encode( + array( + 'mailKey' => $row['mailKey'], + )), + PhabricatorTime::getNow(), + PhabricatorTime::getNow()); +} diff --git a/resources/sql/autopatches/20210316.almanac.14.event-dropmailkey.sql b/resources/sql/autopatches/20210316.almanac.14.event-dropmailkey.sql new file mode 100644 index 0000000000..8a7e2de519 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.14.event-dropmailkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_calendar.calendar_event + DROP mailKey; diff --git a/src/applications/calendar/storage/PhabricatorCalendarEvent.php b/src/applications/calendar/storage/PhabricatorCalendarEvent.php index a8092aaa88..941ec6220d 100644 --- a/src/applications/calendar/storage/PhabricatorCalendarEvent.php +++ b/src/applications/calendar/storage/PhabricatorCalendarEvent.php @@ -24,7 +24,6 @@ final class PhabricatorCalendarEvent extends PhabricatorCalendarDAO protected $isCancelled; protected $isAllDay; protected $icon; - protected $mailKey; protected $isStub; protected $isRecurring = 0; @@ -360,10 +359,6 @@ final class PhabricatorCalendarEvent extends PhabricatorCalendarDAO } public function save() { - if (!$this->mailKey) { - $this->mailKey = Filesystem::readRandomCharacters(20); - } - $import_uid = $this->getImportUID(); if ($import_uid !== null) { $index = PhabricatorHash::digestForIndex($import_uid); @@ -405,7 +400,6 @@ final class PhabricatorCalendarEvent extends PhabricatorCalendarDAO 'isCancelled' => 'bool', 'isAllDay' => 'bool', 'icon' => 'text32', - 'mailKey' => 'bytes20', 'isRecurring' => 'bool', 'seriesParentPHID' => 'phid?', 'instanceOfEventPHID' => 'phid?', @@ -442,9 +436,8 @@ final class PhabricatorCalendarEvent extends PhabricatorCalendarDAO ) + parent::getConfiguration(); } - public function generatePHID() { - return PhabricatorPHID::generateNewPHID( - PhabricatorCalendarEventPHIDType::TYPECONST); + public function getPHIDType() { + return PhabricatorCalendarEventPHIDType::TYPECONST; } public function getMonogram() { From 3267859aeeb2a9c99f040ee14f23483600bb6627 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 15:21:35 -0700 Subject: [PATCH 21/29] Modernize "mailKey" on Fund initiatives Summary: Ref T13065. Migrate "mailKey" and drop the column. Test Plan: Ran "bin/storage upgrade", got a clean upgrade, saw migrated values in mail properties table. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13065 Differential Revision: https://secure.phabricator.com/D21634 --- .../20210316.almanac.15.intiative-mailkey.php | 28 +++++++++++++++++++ ...0316.almanac.16.initiative-dropmailkey.sql | 2 ++ .../fund/storage/FundInitiative.php | 13 ++------- 3 files changed, 32 insertions(+), 11 deletions(-) create mode 100644 resources/sql/autopatches/20210316.almanac.15.intiative-mailkey.php create mode 100644 resources/sql/autopatches/20210316.almanac.16.initiative-dropmailkey.sql diff --git a/resources/sql/autopatches/20210316.almanac.15.intiative-mailkey.php b/resources/sql/autopatches/20210316.almanac.15.intiative-mailkey.php new file mode 100644 index 0000000000..03215ef78f --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.15.intiative-mailkey.php @@ -0,0 +1,28 @@ +establishConnection('w'); + +$properties_table = new PhabricatorMetaMTAMailProperties(); +$conn = $properties_table->establishConnection('w'); + +$iterator = new LiskRawMigrationIterator( + $initiative_conn, + $initiative_table->getTableName()); + +foreach ($iterator as $row) { + queryfx( + $conn, + 'INSERT IGNORE INTO %R + (objectPHID, mailProperties, dateCreated, dateModified) + VALUES + (%s, %s, %d, %d)', + $properties_table, + $row['phid'], + phutil_json_encode( + array( + 'mailKey' => $row['mailKey'], + )), + PhabricatorTime::getNow(), + PhabricatorTime::getNow()); +} diff --git a/resources/sql/autopatches/20210316.almanac.16.initiative-dropmailkey.sql b/resources/sql/autopatches/20210316.almanac.16.initiative-dropmailkey.sql new file mode 100644 index 0000000000..9de5e9c224 --- /dev/null +++ b/resources/sql/autopatches/20210316.almanac.16.initiative-dropmailkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_fund.fund_initiative + DROP mailKey; diff --git a/src/applications/fund/storage/FundInitiative.php b/src/applications/fund/storage/FundInitiative.php index 1ebbb35ef1..7503083c23 100644 --- a/src/applications/fund/storage/FundInitiative.php +++ b/src/applications/fund/storage/FundInitiative.php @@ -22,7 +22,6 @@ final class FundInitiative extends FundDAO protected $editPolicy; protected $status; protected $totalAsCurrency; - protected $mailKey; private $projectPHIDs = self::ATTACHABLE; @@ -62,7 +61,6 @@ final class FundInitiative extends FundDAO 'status' => 'text32', 'merchantPHID' => 'phid?', 'totalAsCurrency' => 'text64', - 'mailKey' => 'bytes20', ), self::CONFIG_APPLICATION_SERIALIZERS => array( 'totalAsCurrency' => new PhortuneCurrencySerializer(), @@ -78,8 +76,8 @@ final class FundInitiative extends FundDAO ) + parent::getConfiguration(); } - public function generatePHID() { - return PhabricatorPHID::generateNewPHID(FundInitiativePHIDType::TYPECONST); + public function getPHIDType() { + return FundInitiativePHIDType::TYPECONST; } public function getMonogram() { @@ -103,13 +101,6 @@ final class FundInitiative extends FundDAO return ($this->getStatus() == self::STATUS_CLOSED); } - public function save() { - if (!$this->mailKey) { - $this->mailKey = Filesystem::readRandomCharacters(20); - } - return parent::save(); - } - /* -( PhabricatorPolicyInterface )----------------------------------------- */ From 12341e4bc8b4a3d9ba15e51a217b9506df1bbbd7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 15:33:32 -0700 Subject: [PATCH 22/29] Forbid disabled devices from authenticating via SSH or HTTP Summary: Ref T13641. Phabricator sometimes makes intracluster requests that authenticate as a device. Forbid these requests from authenticating as a disabled device. Test Plan: - Ran `bin/ssh-exec --phabricator-ssh-device ...` as an enabled/disabled device (worked; sensible error). - Made Conduit calls as an enable/disabled device (worked; sensible error). Maniphest Tasks: T13641 Differential Revision: https://secure.phabricator.com/D21635 --- scripts/ssh/ssh-exec.php | 8 ++++++++ .../controller/PhabricatorConduitAPIController.php | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/scripts/ssh/ssh-exec.php b/scripts/ssh/ssh-exec.php index 4a46e7562d..c3a85b399c 100755 --- a/scripts/ssh/ssh-exec.php +++ b/scripts/ssh/ssh-exec.php @@ -146,6 +146,14 @@ try { $device_name)); } + if ($device->isDisabled()) { + throw new Exception( + pht( + 'This request has authenticated as a device ("%s"), but this '. + 'device is disabled.', + $device->getName())); + } + // We're authenticated as a device, but we're going to read the user out of // the command below. $is_cluster_request = true; diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php index 9ca84fff3d..bc200e7078 100644 --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -238,6 +238,16 @@ final class PhabricatorConduitAPIController if ($object instanceof PhabricatorUser) { $user = $object; } else { + if ($object->isDisabled()) { + return array( + 'ERR-INVALID-AUTH', + pht( + 'The key which signed this request is associated with a '. + 'disabled device ("%s").', + $object->getName()), + ); + } + if (!$stored_key->getIsTrusted()) { return array( 'ERR-INVALID-AUTH', From e6a23274fee433a4b84039456aab7cf36b204190 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Mar 2021 15:49:16 -0700 Subject: [PATCH 23/29] Default the Almanac Devices query to "Active Devices" Summary: Ref T13641. Now that we can provide an "Active Devices" query, provide it and make it the default. Test Plan: Viewed Almanac devices, got a list of active devices. Clicked "All Devices" to get all devices. Maniphest Tasks: T13641 Differential Revision: https://secure.phabricator.com/D21636 --- src/applications/almanac/query/AlmanacDeviceSearchEngine.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/applications/almanac/query/AlmanacDeviceSearchEngine.php b/src/applications/almanac/query/AlmanacDeviceSearchEngine.php index 956ade5ed4..9f98abb292 100644 --- a/src/applications/almanac/query/AlmanacDeviceSearchEngine.php +++ b/src/applications/almanac/query/AlmanacDeviceSearchEngine.php @@ -71,6 +71,7 @@ final class AlmanacDeviceSearchEngine protected function getBuiltinQueryNames() { $names = array( + 'active' => pht('Active Devices'), 'all' => pht('All Devices'), ); @@ -78,11 +79,13 @@ final class AlmanacDeviceSearchEngine } public function buildSavedQueryFromBuiltin($query_key) { - $query = $this->newSavedQuery(); $query->setQueryKey($query_key); switch ($query_key) { + case 'active': + $active_statuses = AlmanacDeviceStatus::getActiveStatusList(); + return $query->setParameter('statuses', $active_statuses); case 'all': return $query; } From ff0a4a2c6fce8caeac85d3435afae36f18abf7a8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Mar 2021 11:28:31 -0700 Subject: [PATCH 24/29] Use a less-misleading example for Conduit custom constraints Summary: See PHI2032. The Conduit UI shows some "Example Custom Constraints" that are intended to be generic, but use of "statuses" is misleading since many objects have a status and most of them don't support these specific values. Make it more clear that these are generic values. Test Plan: Read new text. Differential Revision: https://secure.phabricator.com/D21637 --- .../search/engine/PhabricatorSearchEngineAPIMethod.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php index 3af4f349fa..aecc44699c 100644 --- a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php +++ b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php @@ -169,8 +169,8 @@ this: { ... "constraints": { - "authors": ["PHID-USER-1111", "PHID-USER-2222"], - "statuses": ["open", "closed"], + "authorPHIDs": ["PHID-USER-1111", "PHID-USER-2222"], + "flavors": ["cherry", "orange"], ... }, ... From 1d1003af7854058ab78ea2018a4724c341353bb3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Mar 2021 12:15:09 -0700 Subject: [PATCH 25/29] When using "Update Diff" from the web UI, prefill "Repository" properly Summary: Ref T9499. When using the manual "Update Diff" workflow on the web, the "Repository" field isn't pre-filled properly. This can lead to revisions losing their repository after a manual update. Test Plan: Did a manual update of a revision with a repository, saw it stick. Maniphest Tasks: T9499 Differential Revision: https://secure.phabricator.com/D21638 --- .../controller/DifferentialDiffCreateController.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/controller/DifferentialDiffCreateController.php b/src/applications/differential/controller/DifferentialDiffCreateController.php index 9aaf407e28..f9d1006555 100644 --- a/src/applications/differential/controller/DifferentialDiffCreateController.php +++ b/src/applications/differential/controller/DifferentialDiffCreateController.php @@ -27,7 +27,13 @@ final class DifferentialDiffCreateController extends DifferentialController { $diff = null; // This object is just for policy stuff $diff_object = DifferentialDiff::initializeNewDiff($viewer); - $repository_phid = null; + + if ($revision) { + $repository_phid = $revision->getRepositoryPHID(); + } else { + $repository_phid = null; + } + $errors = array(); $e_diff = null; $e_file = null; From 527dd3ce50b23bd41ebf7d1766886fed75ef2e29 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Mar 2021 12:52:04 -0700 Subject: [PATCH 26/29] Replace Differential "unit stars" with icons Summary: Ref T9764. These "star" icons are unclear, inconsistent, and not friendly to colorblind users. They date from a time long ago when the product didn't have icons. Modernize them and make them more consistent with the similar statuses in Harbormaster. Test Plan: {F8545690} {F8545691} {F8545692} Maniphest Tasks: T9764 Differential Revision: https://secure.phabricator.com/D21639 --- src/__phutil_library_map__.php | 4 + .../PhabricatorConfigModuleController.php | 6 +- .../constants/DifferentialConstantsModule.php | 144 ++++++++++++++++++ .../constants/DifferentialUnitStatus.php | 81 ++++++++++ .../customfield/DifferentialUnitField.php | 23 +-- .../DifferentialRevisionUpdateHistoryView.php | 54 ++----- src/view/phui/PHUIColor.php | 13 ++ 7 files changed, 267 insertions(+), 58 deletions(-) create mode 100644 src/applications/differential/constants/DifferentialConstantsModule.php create mode 100644 src/view/phui/PHUIColor.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5794fac11b..e811ddbc92 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -496,6 +496,7 @@ phutil_register_library_map(array( 'DifferentialCommitsSearchEngineAttachment' => 'applications/differential/engineextension/DifferentialCommitsSearchEngineAttachment.php', 'DifferentialConduitAPIMethod' => 'applications/differential/conduit/DifferentialConduitAPIMethod.php', 'DifferentialConflictsCommitMessageField' => 'applications/differential/field/DifferentialConflictsCommitMessageField.php', + 'DifferentialConstantsModule' => 'applications/differential/constants/DifferentialConstantsModule.php', 'DifferentialController' => 'applications/differential/controller/DifferentialController.php', 'DifferentialCoreCustomField' => 'applications/differential/customfield/DifferentialCoreCustomField.php', 'DifferentialCreateCommentConduitAPIMethod' => 'applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php', @@ -2025,6 +2026,7 @@ phutil_register_library_map(array( 'PHUICalendarMonthView' => 'view/phui/calendar/PHUICalendarMonthView.php', 'PHUICalendarWeekView' => 'view/phui/calendar/PHUICalendarWeekView.php', 'PHUICalendarWidgetView' => 'view/phui/calendar/PHUICalendarWidgetView.php', + 'PHUIColor' => 'view/phui/PHUIColor.php', 'PHUIColorPalletteExample' => 'applications/uiexample/examples/PHUIColorPalletteExample.php', 'PHUICrumbView' => 'view/phui/PHUICrumbView.php', 'PHUICrumbsView' => 'view/phui/PHUICrumbsView.php', @@ -6587,6 +6589,7 @@ phutil_register_library_map(array( 'DifferentialCommitsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'DifferentialConduitAPIMethod' => 'ConduitAPIMethod', 'DifferentialConflictsCommitMessageField' => 'DifferentialCommitMessageField', + 'DifferentialConstantsModule' => 'PhabricatorConfigModule', 'DifferentialController' => 'PhabricatorController', 'DifferentialCoreCustomField' => 'DifferentialCustomField', 'DifferentialCreateCommentConduitAPIMethod' => 'DifferentialConduitAPIMethod', @@ -8356,6 +8359,7 @@ phutil_register_library_map(array( 'PHUICalendarMonthView' => 'AphrontView', 'PHUICalendarWeekView' => 'AphrontView', 'PHUICalendarWidgetView' => 'AphrontTagView', + 'PHUIColor' => 'Phobject', 'PHUIColorPalletteExample' => 'PhabricatorUIExample', 'PHUICrumbView' => 'AphrontView', 'PHUICrumbsView' => 'AphrontView', diff --git a/src/applications/config/controller/module/PhabricatorConfigModuleController.php b/src/applications/config/controller/module/PhabricatorConfigModuleController.php index aaa873f5d2..4f3f9c57cb 100644 --- a/src/applications/config/controller/module/PhabricatorConfigModuleController.php +++ b/src/applications/config/controller/module/PhabricatorConfigModuleController.php @@ -38,7 +38,11 @@ final class PhabricatorConfigModuleController $nav->selectFilter($key); $header = $this->buildHeaderView($title); - $view = $this->buildConfigBoxView($title, $content); + if ($content instanceof AphrontTableView) { + $view = $this->buildConfigBoxView($title, $content); + } else { + $view = $content; + } $crumbs = $this->buildApplicationCrumbs() ->addTextCrumb(pht('Extensions/Modules'), $modules_uri) diff --git a/src/applications/differential/constants/DifferentialConstantsModule.php b/src/applications/differential/constants/DifferentialConstantsModule.php new file mode 100644 index 0000000000..091fa51243 --- /dev/null +++ b/src/applications/differential/constants/DifferentialConstantsModule.php @@ -0,0 +1,144 @@ +getViewer(); + + return array( + $this->renderRevisionStatuses($viewer), + $this->renderUnitStatuses($viewer), + ); + } + + private function renderRevisionStatuses(PhabricatorUser $viewer) { + $statuses = DifferentialRevisionStatus::getAll(); + + $rows = array(); + foreach ($statuses as $status) { + $icon = id(new PHUIIconView()) + ->setIcon( + $status->getIcon(), + $status->getIconColor()); + + $timeline_icon = $status->getTimelineIcon(); + if ($timeline_icon !== null) { + $timeline_view = id(new PHUIIconView()) + ->setIcon( + $status->getTimelineIcon(), + $status->getTimelineColor()); + } else { + $timeline_view = null; + } + + if ($status->isClosedStatus()) { + $is_open = pht('Closed'); + } else { + $is_open = pht('Open'); + } + + $tag_color = $status->getTagColor(); + if ($tag_color !== null) { + $tag_view = id(new PHUIIconView()) + ->seticon('fa-tag', $tag_color); + } else { + $tag_view = null; + } + + $ansi_color = $status->getAnsiColor(); + if ($ansi_color !== null) { + $web_color = PHUIColor::getWebColorFromANSIColor($ansi_color); + $ansi_view = id(new PHUIIconView()) + ->setIcon('fa-stop', $web_color); + } else { + $ansi_view = null; + } + + + $rows[] = array( + $status->getKey(), + $status->getLegacyKey(), + $icon, + $timeline_view, + $tag_view, + $ansi_view, + $is_open, + $status->getDisplayName(), + ); + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Value'), + pht('Legacy Value'), + pht('Icon'), + pht('Timeline Icon'), + pht('Tag Color'), + pht('ANSI Color'), + pht('Open/Closed'), + pht('Name'), + )) + ->setColumnClasses( + array( + null, + null, + null, + null, + null, + null, + null, + 'wide pri', + )); + + $view = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Differential Revision Statuses')) + ->setTable($table); + + return $view; + } + + private function renderUnitStatuses(PhabricatorUser $viewer) { + $statuses = DifferentialUnitStatus::getStatusMap(); + + $rows = array(); + foreach ($statuses as $status) { + $rows[] = array( + $status->getValue(), + id(new PHUIIconView()) + ->setIcon($status->getIconIcon(), $status->getIconColor()), + $status->getName(), + ); + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Value'), + pht('Icon'), + pht('Name'), + )) + ->setColumnClasses( + array( + null, + null, + 'wide pri', + )); + + $view = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Differential Unit Statuses')) + ->setTable($table); + + return $view; + } + +} diff --git a/src/applications/differential/constants/DifferentialUnitStatus.php b/src/applications/differential/constants/DifferentialUnitStatus.php index 2b69853dfb..b8265522fe 100644 --- a/src/applications/differential/constants/DifferentialUnitStatus.php +++ b/src/applications/differential/constants/DifferentialUnitStatus.php @@ -9,4 +9,85 @@ final class DifferentialUnitStatus extends Phobject { const UNIT_SKIP = 4; const UNIT_AUTO_SKIP = 6; + private $value; + + public static function newStatusFromValue($value) { + $status = new self(); + $status->value = $value; + return $status; + } + + public function getValue() { + return $this->value; + } + + public function getName() { + $name = $this->getUnitStatusProperty('name'); + + if ($name === null) { + $name = pht('Unknown Unit Status ("%s")', $this->getValue()); + } + + return $name; + } + + public function getIconIcon() { + return $this->getUnitStatusProperty('icon.icon'); + } + + public function getIconColor() { + return $this->getUnitStatusProperty('icon.color'); + } + + public static function getStatusMap() { + $results = array(); + + foreach (self::newUnitStatusMap() as $value => $ignored) { + $results[$value] = self::newStatusFromValue($value); + } + + return $results; + } + + private function getUnitStatusProperty($key, $default = null) { + $map = self::newUnitStatusMap(); + $properties = idx($map, $this->getValue(), array()); + return idx($properties, $key, $default); + } + + private static function newUnitStatusMap() { + return array( + self::UNIT_NONE => array( + 'name' => pht('No Test Coverage'), + 'icon.icon' => 'fa-ban', + 'icon.color' => 'grey', + ), + self::UNIT_OKAY => array( + 'name' => pht('Tests Passed'), + 'icon.icon' => 'fa-check', + 'icon.color' => 'green', + ), + self::UNIT_WARN => array( + 'name' => pht('Test Warnings'), + 'icon.icon' => 'fa-exclamation-triangle', + 'icon.color' => 'yellow', + ), + self::UNIT_FAIL => array( + 'name' => pht('Test Failures'), + 'icon.icon' => 'fa-times', + 'icon.color' => 'red', + ), + self::UNIT_SKIP => array( + 'name' => pht('Tests Skipped'), + 'icon.icon' => 'fa-fast-forward', + 'icon.color' => 'blue', + ), + self::UNIT_AUTO_SKIP => array( + 'name' => pht('Tests Not Applicable'), + 'icon.icon' => 'fa-upload', + 'icon.color' => 'grey', + ), + ); + } + } diff --git a/src/applications/differential/customfield/DifferentialUnitField.php b/src/applications/differential/customfield/DifferentialUnitField.php index 4c1eb72cc8..6d16b2904d 100644 --- a/src/applications/differential/customfield/DifferentialUnitField.php +++ b/src/applications/differential/customfield/DifferentialUnitField.php @@ -72,29 +72,20 @@ final class DifferentialUnitField } public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + $status_value = $diff->getUnitStatus(); + $status = DifferentialUnitStatus::newStatusFromValue($status_value); - $colors = array( - DifferentialUnitStatus::UNIT_NONE => 'grey', - DifferentialUnitStatus::UNIT_OKAY => 'green', - DifferentialUnitStatus::UNIT_WARN => 'yellow', - DifferentialUnitStatus::UNIT_FAIL => 'red', - DifferentialUnitStatus::UNIT_SKIP => 'blue', - DifferentialUnitStatus::UNIT_AUTO_SKIP => 'blue', - ); - $icon_color = idx($colors, $diff->getUnitStatus(), 'grey'); - - $message = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage( - $diff->getUnitStatus()); + $status_icon = $status->getIconIcon(); + $status_color = $status->getIconColor(); + $status_name = $status->getName(); $status = id(new PHUIStatusListView()) ->addItem( id(new PHUIStatusItemView()) - ->setIcon(PHUIStatusItemView::ICON_STAR, $icon_color) - ->setTarget($message)); + ->setIcon($status_icon, $status_color) + ->setTarget($status_name)); return $status; } - - } diff --git a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php index 07ca983bc0..6be739fac3 100644 --- a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php +++ b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php @@ -153,14 +153,19 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { ), $lint); - $unit = self::renderDiffUnitStar($unit_status); - $unit = phutil_tag( - 'div', - array( - 'class' => 'lintunit-star', - 'title' => self::getDiffUnitMessage($unit_status), - ), - $unit); + $status = DifferentialUnitStatus::newStatusFromValue($unit_status); + + $unit_icon = $status->getIconIcon(); + $unit_color = $status->getIconColor(); + $unit_name = $status->getName(); + + $unit = id(new PHUIIconView()) + ->setIcon($unit_icon, $unit_color) + ->addSigil('has-tooltip') + ->setMetadata( + array( + 'tip' => $unit_name, + )); $base = $this->renderBaseRevision($diff); } else { @@ -303,20 +308,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { return self::renderDiffStar($star); } - private static function renderDiffUnitStar($unit_status) { - static $map = array( - DifferentialUnitStatus::UNIT_NONE => self::STAR_NONE, - DifferentialUnitStatus::UNIT_OKAY => self::STAR_OKAY, - DifferentialUnitStatus::UNIT_WARN => self::STAR_WARN, - DifferentialUnitStatus::UNIT_FAIL => self::STAR_FAIL, - DifferentialUnitStatus::UNIT_SKIP => self::STAR_SKIP, - DifferentialUnitStatus::UNIT_AUTO_SKIP => self::STAR_SKIP, - ); - $star = idx($map, $unit_status, self::STAR_FAIL); - - return self::renderDiffStar($star); - } - public static function getDiffLintMessage(DifferentialDiff $diff) { switch ($diff->getLintStatus()) { case DifferentialLintStatus::LINT_NONE: @@ -335,25 +326,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { return pht('Unknown'); } - public static function getDiffUnitMessage($unit_status) { - switch ($unit_status) { - case DifferentialUnitStatus::UNIT_NONE: - return pht('No Unit Test Coverage'); - case DifferentialUnitStatus::UNIT_OKAY: - return pht('Unit Tests OK'); - case DifferentialUnitStatus::UNIT_WARN: - return pht('Unit Test Warnings'); - case DifferentialUnitStatus::UNIT_FAIL: - return pht('Unit Test Errors'); - case DifferentialUnitStatus::UNIT_SKIP: - return pht('Unit Tests Skipped'); - case DifferentialUnitStatus::UNIT_AUTO_SKIP: - return pht( - 'Automatic diff as part of commit; unit tests not applicable.'); - } - return pht('Unknown'); - } - private static function renderDiffStar($star) { $class = 'diff-star-'.$star; return phutil_tag( diff --git a/src/view/phui/PHUIColor.php b/src/view/phui/PHUIColor.php new file mode 100644 index 0000000000..dc96b10914 --- /dev/null +++ b/src/view/phui/PHUIColor.php @@ -0,0 +1,13 @@ + 'sky', + 'magenta' => 'pink', + ); + + return idx($map, $ansi_color, $ansi_color); + } +} From d6ed9392d4e06a815b95000214ea3c2491b1795b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Mar 2021 13:36:32 -0700 Subject: [PATCH 27/29] Replace Differential "lint stars" with icons Summary: Ref T9764. These stars are inconsistent, not accessible, and generally weird. They predate icons. Update them to use icons instead. Test Plan: {F8545721} {F8545722} {F8545723} Maniphest Tasks: T9764 Differential Revision: https://secure.phabricator.com/D21640 --- resources/celerity/map.php | 6 +- .../constants/DifferentialConstantsModule.php | 36 ++++++ .../constants/DifferentialLintStatus.php | 81 ++++++++++++ .../constants/DifferentialUnitStatus.php | 2 +- .../customfield/DifferentialLintField.php | 30 ++--- .../DifferentialRevisionUpdateHistoryView.php | 115 ++++++------------ .../differential/revision-history.css | 5 - 7 files changed, 168 insertions(+), 107 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index e52ef012ab..3cae51e3f9 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => '0ae696de', 'core.pkg.js' => 'ab3502fe', 'dark-console.pkg.js' => '187792c2', - 'differential.pkg.css' => '5c459f92', + 'differential.pkg.css' => 'ffb69e3d', 'differential.pkg.js' => '5080baf4', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '78c9885d', @@ -67,7 +67,7 @@ return array( 'rsrc/css/application/differential/core.css' => '7300a73e', 'rsrc/css/application/differential/phui-inline-comment.css' => '9863a85e', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', - 'rsrc/css/application/differential/revision-history.css' => '8aa3eac5', + 'rsrc/css/application/differential/revision-history.css' => '237a2979', 'rsrc/css/application/differential/revision-list.css' => '93d2df7d', 'rsrc/css/application/differential/table-of-contents.css' => 'bba788b9', 'rsrc/css/application/diffusion/diffusion-icons.css' => '23b31a1b', @@ -569,7 +569,7 @@ return array( 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', - 'differential-revision-history-css' => '8aa3eac5', + 'differential-revision-history-css' => '237a2979', 'differential-revision-list-css' => '93d2df7d', 'differential-table-of-contents-css' => 'bba788b9', 'diffusion-css' => 'e46232d6', diff --git a/src/applications/differential/constants/DifferentialConstantsModule.php b/src/applications/differential/constants/DifferentialConstantsModule.php index 091fa51243..05a5f06c55 100644 --- a/src/applications/differential/constants/DifferentialConstantsModule.php +++ b/src/applications/differential/constants/DifferentialConstantsModule.php @@ -17,6 +17,7 @@ final class DifferentialConstantsModule return array( $this->renderRevisionStatuses($viewer), $this->renderUnitStatuses($viewer), + $this->renderLintStatuses($viewer), ); } @@ -141,4 +142,39 @@ final class DifferentialConstantsModule return $view; } + private function renderLintStatuses(PhabricatorUser $viewer) { + $statuses = DifferentialLintStatus::getStatusMap(); + + $rows = array(); + foreach ($statuses as $status) { + $rows[] = array( + $status->getValue(), + id(new PHUIIconView()) + ->setIcon($status->getIconIcon(), $status->getIconColor()), + $status->getName(), + ); + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Value'), + pht('Icon'), + pht('Name'), + )) + ->setColumnClasses( + array( + null, + null, + 'wide pri', + )); + + $view = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Differential Lint Statuses')) + ->setTable($table); + + return $view; + } + + } diff --git a/src/applications/differential/constants/DifferentialLintStatus.php b/src/applications/differential/constants/DifferentialLintStatus.php index 72709b8eef..8f2dedbe84 100644 --- a/src/applications/differential/constants/DifferentialLintStatus.php +++ b/src/applications/differential/constants/DifferentialLintStatus.php @@ -9,4 +9,85 @@ final class DifferentialLintStatus extends Phobject { const LINT_SKIP = 4; const LINT_AUTO_SKIP = 6; + private $value; + + public static function newStatusFromValue($value) { + $status = new self(); + $status->value = $value; + return $status; + } + + public function getValue() { + return $this->value; + } + + public function getName() { + $name = $this->getLintStatusProperty('name'); + + if ($name === null) { + $name = pht('Unknown Lint Status ("%s")', $this->getValue()); + } + + return $name; + } + + public function getIconIcon() { + return $this->getLintStatusProperty('icon.icon'); + } + + public function getIconColor() { + return $this->getLintStatusProperty('icon.color'); + } + + public static function getStatusMap() { + $results = array(); + + foreach (self::newLintStatusMap() as $value => $ignored) { + $results[$value] = self::newStatusFromValue($value); + } + + return $results; + } + + private function getLintStatusProperty($key, $default = null) { + $map = self::newLintStatusMap(); + $properties = idx($map, $this->getValue(), array()); + return idx($properties, $key, $default); + } + + private static function newLintStatusMap() { + return array( + self::LINT_NONE => array( + 'name' => pht('No Lint Coverage'), + 'icon.icon' => 'fa-ban', + 'icon.color' => 'grey', + ), + self::LINT_OKAY => array( + 'name' => pht('Lint Passed'), + 'icon.icon' => 'fa-check', + 'icon.color' => 'green', + ), + self::LINT_WARN => array( + 'name' => pht('Lint Warnings'), + 'icon.icon' => 'fa-exclamation-triangle', + 'icon.color' => 'yellow', + ), + self::LINT_FAIL => array( + 'name' => pht('Lint Errors'), + 'icon.icon' => 'fa-times', + 'icon.color' => 'red', + ), + self::LINT_SKIP => array( + 'name' => pht('Lint Skipped'), + 'icon.icon' => 'fa-fast-forward', + 'icon.color' => 'blue', + ), + self::LINT_AUTO_SKIP => array( + 'name' => pht('Lint Not Applicable'), + 'icon.icon' => 'fa-code', + 'icon.color' => 'grey', + ), + ); + } + } diff --git a/src/applications/differential/constants/DifferentialUnitStatus.php b/src/applications/differential/constants/DifferentialUnitStatus.php index b8265522fe..186f476800 100644 --- a/src/applications/differential/constants/DifferentialUnitStatus.php +++ b/src/applications/differential/constants/DifferentialUnitStatus.php @@ -84,7 +84,7 @@ final class DifferentialUnitStatus extends Phobject { ), self::UNIT_AUTO_SKIP => array( 'name' => pht('Tests Not Applicable'), - 'icon.icon' => 'fa-upload', + 'icon.icon' => 'fa-code', 'icon.color' => 'grey', ), ); diff --git a/src/applications/differential/customfield/DifferentialLintField.php b/src/applications/differential/customfield/DifferentialLintField.php index 62b94c51eb..faf065a8d7 100644 --- a/src/applications/differential/customfield/DifferentialLintField.php +++ b/src/applications/differential/customfield/DifferentialLintField.php @@ -38,7 +38,6 @@ final class DifferentialLintField protected function getDiffPropertyKeys() { return array( 'arc:lint', - 'arc:lint-excuse', ); } @@ -84,33 +83,18 @@ final class DifferentialLintField DifferentialDiff $diff, array $messages) { - $colors = array( - DifferentialLintStatus::LINT_NONE => 'grey', - DifferentialLintStatus::LINT_OKAY => 'green', - DifferentialLintStatus::LINT_WARN => 'yellow', - DifferentialLintStatus::LINT_FAIL => 'red', - DifferentialLintStatus::LINT_SKIP => 'blue', - DifferentialLintStatus::LINT_AUTO_SKIP => 'blue', - ); - $icon_color = idx($colors, $diff->getLintStatus(), 'grey'); + $status_value = $diff->getLintStatus(); + $status = DifferentialLintStatus::newStatusFromValue($status_value); - $message = DifferentialRevisionUpdateHistoryView::getDiffLintMessage($diff); - - $excuse = $diff->getProperty('arc:lint-excuse'); - if (strlen($excuse)) { - $excuse = array( - phutil_tag('strong', array(), pht('Excuse:')), - ' ', - phutil_escape_html_newlines($excuse), - ); - } + $status_icon = $status->getIconIcon(); + $status_color = $status->getIconColor(); + $status_name = $status->getName(); $status = id(new PHUIStatusListView()) ->addItem( id(new PHUIStatusItemView()) - ->setIcon(PHUIStatusItemView::ICON_STAR, $icon_color) - ->setTarget($message) - ->setNote($excuse)); + ->setIcon($status_icon, $status_color) + ->setTarget($status_name)); return $status; } diff --git a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php index 6be739fac3..2e074a2f7e 100644 --- a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php +++ b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php @@ -139,34 +139,8 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { } if ($diff) { - $unit_status = idx( - $this->unitStatus, - $diff->getPHID(), - $diff->getUnitStatus()); - - $lint = self::renderDiffLintStar($row['obj']); - $lint = phutil_tag( - 'div', - array( - 'class' => 'lintunit-star', - 'title' => self::getDiffLintMessage($diff), - ), - $lint); - - $status = DifferentialUnitStatus::newStatusFromValue($unit_status); - - $unit_icon = $status->getIconIcon(); - $unit_color = $status->getIconColor(); - $unit_name = $status->getName(); - - $unit = id(new PHUIIconView()) - ->setIcon($unit_icon, $unit_color) - ->addSigil('has-tooltip') - ->setMetadata( - array( - 'tip' => $unit_name, - )); - + $lint = $this->newLintStatusView($diff); + $unit = $this->newUnitStatusView($diff); $base = $this->renderBaseRevision($diff); } else { $lint = null; @@ -287,53 +261,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { return $content; } - const STAR_NONE = 'none'; - const STAR_OKAY = 'okay'; - const STAR_WARN = 'warn'; - const STAR_FAIL = 'fail'; - const STAR_SKIP = 'skip'; - - private static function renderDiffLintStar(DifferentialDiff $diff) { - static $map = array( - DifferentialLintStatus::LINT_NONE => self::STAR_NONE, - DifferentialLintStatus::LINT_OKAY => self::STAR_OKAY, - DifferentialLintStatus::LINT_WARN => self::STAR_WARN, - DifferentialLintStatus::LINT_FAIL => self::STAR_FAIL, - DifferentialLintStatus::LINT_SKIP => self::STAR_SKIP, - DifferentialLintStatus::LINT_AUTO_SKIP => self::STAR_SKIP, - ); - - $star = idx($map, $diff->getLintStatus(), self::STAR_FAIL); - - return self::renderDiffStar($star); - } - - public static function getDiffLintMessage(DifferentialDiff $diff) { - switch ($diff->getLintStatus()) { - case DifferentialLintStatus::LINT_NONE: - return pht('No Linters Available'); - case DifferentialLintStatus::LINT_OKAY: - return pht('Lint OK'); - case DifferentialLintStatus::LINT_WARN: - return pht('Lint Warnings'); - case DifferentialLintStatus::LINT_FAIL: - return pht('Lint Errors'); - case DifferentialLintStatus::LINT_SKIP: - return pht('Lint Skipped'); - case DifferentialLintStatus::LINT_AUTO_SKIP: - return pht('Automatic diff as part of commit; lint not applicable.'); - } - return pht('Unknown'); - } - - private static function renderDiffStar($star) { - $class = 'diff-star-'.$star; - return phutil_tag( - 'span', - array('class' => $class), - "\xE2\x98\x85"); - } - private function renderBaseRevision(DifferentialDiff $diff) { switch ($diff->getSourceControlSystem()) { case 'git': @@ -373,4 +300,42 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { } return $link; } + + private function newLintStatusView(DifferentialDiff $diff) { + $value = $diff->getLintStatus(); + $status = DifferentialLintStatus::newStatusFromValue($value); + + $icon = $status->getIconIcon(); + $color = $status->getIconColor(); + $name = $status->getName(); + + return $this->newDiffStatusIconView($icon, $color, $name); + } + + private function newUnitStatusView(DifferentialDiff $diff) { + $value = $diff->getUnitStatus(); + + // NOTE: We may be overriding the value on the diff with a value from + // Harbormaster. + $value = idx($this->unitStatus, $diff->getPHID(), $value); + + $status = DifferentialUnitStatus::newStatusFromValue($value); + + $icon = $status->getIconIcon(); + $color = $status->getIconColor(); + $name = $status->getName(); + + return $this->newDiffStatusIconView($icon, $color, $name); + } + + private function newDiffStatusIconView($icon, $color, $name) { + return id(new PHUIIconView()) + ->setIcon($icon, $color) + ->addSigil('has-tooltip') + ->setMetadata( + array( + 'tip' => $name, + )); + } + } diff --git a/webroot/rsrc/css/application/differential/revision-history.css b/webroot/rsrc/css/application/differential/revision-history.css index eca1a95ceb..0a2cc3b381 100644 --- a/webroot/rsrc/css/application/differential/revision-history.css +++ b/webroot/rsrc/css/application/differential/revision-history.css @@ -54,8 +54,3 @@ td.differential-update-history-new { background: #aaffaa; } - -.lintunit-star { - text-align: center; - padding: 0 16px; -} From db9191f9a8d5356e5c056f9f22e5bed2fd8eb791 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Mar 2021 15:27:58 -0700 Subject: [PATCH 28/29] Correct minor "jump to symbol" behavior in Differential Summary: Ref T13644. Ref T13638. - Double-encode the symbol that is used as a path component, similar to Diffusion. - Fix an outdated reference to ".path", which provided context for symbol lookup. - Prevent command-clicking headers from looking up the path as a symbol. Test Plan: - Command-clicked headers, no longer got a symbol. - Command-clicked stuff with "/", saw it double-encoded and decoded properly. - Command-clicked normal symbols, saw "path" populate correctly. Maniphest Tasks: T13644, T13638 Differential Revision: https://secure.phabricator.com/D21641 --- resources/celerity/map.php | 18 ++++---- .../view/DifferentialChangesetDetailView.php | 1 + .../controller/DiffusionSymbolController.php | 3 ++ .../DiffusionDatasourceEngineExtension.php | 4 +- .../repository/repository-crossreference.js | 46 +++++++++++++------ 5 files changed, 46 insertions(+), 26 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 3cae51e3f9..c7ac86ab3c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => 'ab3502fe', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => 'ffb69e3d', - 'differential.pkg.js' => '5080baf4', + 'differential.pkg.js' => '5986f349', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '78c9885d', 'maniphest.pkg.css' => '35995d6d', @@ -437,7 +437,7 @@ return array( 'rsrc/js/application/releeph/releeph-preview-branch.js' => '75184d68', 'rsrc/js/application/releeph/releeph-request-state-change.js' => '9f081f05', 'rsrc/js/application/releeph/releeph-request-typeahead.js' => 'aa3a100c', - 'rsrc/js/application/repository/repository-crossreference.js' => '6337cf26', + 'rsrc/js/application/repository/repository-crossreference.js' => '44d48cd1', 'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e5bdb730', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'b86f297f', 'rsrc/js/application/transactions/behavior-comment-actions.js' => '4dffaeb2', @@ -693,7 +693,7 @@ return array( 'javelin-behavior-reorder-applications' => 'aa371860', 'javelin-behavior-reorder-columns' => '8ac32fd9', 'javelin-behavior-reorder-profile-menu-items' => 'e5bdb730', - 'javelin-behavior-repository-crossreference' => '6337cf26', + 'javelin-behavior-repository-crossreference' => '44d48cd1', 'javelin-behavior-scrollbar' => '92388bae', 'javelin-behavior-search-reorder-queries' => 'b86f297f', 'javelin-behavior-select-content' => 'e8240b50', @@ -1309,6 +1309,12 @@ return array( '43bc9360' => array( 'javelin-install', ), + '44d48cd1' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-uri', + ), '457f4d16' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1536,12 +1542,6 @@ return array( 'javelin-request', 'javelin-uri', ), - '6337cf26' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-uri', - ), '65bb0011' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/differential/view/DifferentialChangesetDetailView.php b/src/applications/differential/view/DifferentialChangesetDetailView.php index d23ddc62a4..9d04d081ac 100644 --- a/src/applications/differential/view/DifferentialChangesetDetailView.php +++ b/src/applications/differential/view/DifferentialChangesetDetailView.php @@ -246,6 +246,7 @@ final class DifferentialChangesetDetailView extends AphrontView { 'displayPath' => hsprintf('%s', $display_parts), 'icon' => $display_icon, 'pathParts' => $path_parts, + 'symbolPath' => $display_filename, 'pathIconIcon' => $changeset->getPathIconIcon(), 'pathIconColor' => $changeset->getPathIconColor(), diff --git a/src/applications/diffusion/controller/DiffusionSymbolController.php b/src/applications/diffusion/controller/DiffusionSymbolController.php index b855f967db..c4ce99fa87 100644 --- a/src/applications/diffusion/controller/DiffusionSymbolController.php +++ b/src/applications/diffusion/controller/DiffusionSymbolController.php @@ -4,7 +4,10 @@ final class DiffusionSymbolController extends DiffusionController { public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); + + // See T13638 for discussion of escaping. $name = $request->getURIData('name'); + $name = phutil_unescape_uri_path_component($name); $query = id(new DiffusionSymbolQuery()) ->setViewer($viewer) diff --git a/src/applications/diffusion/engineextension/DiffusionDatasourceEngineExtension.php b/src/applications/diffusion/engineextension/DiffusionDatasourceEngineExtension.php index 9678b627b1..70e5b35726 100644 --- a/src/applications/diffusion/engineextension/DiffusionDatasourceEngineExtension.php +++ b/src/applications/diffusion/engineextension/DiffusionDatasourceEngineExtension.php @@ -66,12 +66,12 @@ final class DiffusionDatasourceEngineExtension $parts = null; if (preg_match('/(.*)(?:\\.|::|->)(.*)/', $symbol, $parts)) { return urisprintf( - '/diffusion/symbol/%s/?jump=true&context=%s', + '/diffusion/symbol/%p/?jump=true&context=%s', $parts[2], $parts[1]); } else { return urisprintf( - '/diffusion/symbol/%s/?jump=true', + '/diffusion/symbol/%p/?jump=true', $symbol); } } diff --git a/webroot/rsrc/js/application/repository/repository-crossreference.js b/webroot/rsrc/js/application/repository/repository-crossreference.js index 8158e71485..a1b501124e 100644 --- a/webroot/rsrc/js/application/repository/repository-crossreference.js +++ b/webroot/rsrc/js/application/repository/repository-crossreference.js @@ -66,13 +66,8 @@ JX.behavior('repository-crossreference', function(config, statics) { var target = e.getTarget(); - try { - // If we're in an inline comment, don't link symbols. - if (JX.DOM.findAbove(target, 'div', 'differential-inline-comment')) { - return; - } - } catch (ex) { - // Continue if we're not inside an inline comment. + if (!canLinkNode(target)) { + return; } // If only part of the symbol was edited, the symbol name itself will @@ -97,6 +92,29 @@ JX.behavior('repository-crossreference', function(config, statics) { } }); } + + function canLinkNode(node) { + try { + // If we're in an inline comment, don't link symbols. + if (JX.DOM.findAbove(node, 'div', 'differential-inline-comment')) { + return false; + } + } catch (ex) { + // Continue if we're not inside an inline comment. + } + + // See T13644. Don't open symbols if we're inside a changeset header. + try { + if (JX.DOM.findAbove(node, 'h1')) { + return false; + } + } catch (ex) { + // Continue if not inside a header. + } + + return true; + } + function unhighlight() { highlighted && JX.DOM.alterClass(highlighted, classHighlight, false); highlighted = null; @@ -159,6 +177,9 @@ JX.behavior('repository-crossreference', function(config, statics) { uri_symbol = uri_symbol.trim(); // See T13437. Symbols like "#define" need to be encoded. + // See T13644. Symbols like "a/b" must be double-encoded to survive + // one layer of decoding by the webserver. + uri_symbol = encodeURIComponent(uri_symbol); uri_symbol = encodeURIComponent(uri_symbol); var uri = JX.$U('/diffusion/symbol/' + uri_symbol + '/'); @@ -223,7 +244,7 @@ JX.behavior('repository-crossreference', function(config, statics) { var changeset; try { changeset = JX.DOM.findAbove(target, 'div', 'differential-changeset'); - return JX.Stratcom.getData(changeset).path; + return JX.Stratcom.getData(changeset).symbolPath || null; } catch (ex) { // Ignore. } @@ -315,13 +336,8 @@ JX.behavior('repository-crossreference', function(config, statics) { var target = e.getTarget(); - try { - // If we're in an inline comment, don't link symbols. - if (JX.DOM.findAbove(target, 'div', 'differential-inline-comment')) { - return; - } - } catch (ex) { - // Continue if we're not inside an inline comment. + if (!canLinkNode(target)) { + return; } // If only part of the symbol was edited, the symbol name itself will From 61272e7ac31abed35face7486ff44999c0c5fe26 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Mar 2021 10:11:15 -0700 Subject: [PATCH 29/29] Correct "getActiveBindings()" method name Summary: This method was incorrectly renamed by D21628. See . Test Plan: Looked at it, will deploy etc. Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D21644 --- .../DrydockAlmanacServiceHostBlueprintImplementation.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php index 6c8e75696f..290fae8c63 100644 --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php @@ -242,7 +242,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation return $this->services; } - private function getActive(array $services) { + private function getActiveBindings(array $services) { assert_instances_of($services, 'AlmanacService'); $bindings = array_mergev(mpull($services, 'getActiveBindings')); return mpull($bindings, null, 'getPHID');