From 9f7aaa8ee4cbd8c36c5cfb6e55e5366be4ff238c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 1 Oct 2019 18:46:56 -0700 Subject: [PATCH 01/12] Fix an issue where any diff which could possibly be rendered as Jupyter decided to render as Jupyter Summary: See PHI1468. Engine selection for diffs is currently too aggressive in trying to find a shared engine and will fall back a shared engine with a very low score, causing all ".json" files to render as Jupyter files. Only pick an engine as a difference engine by default if it's the highest-scoring engine for the new file. Test Plan: Viewed ".json" files and ".ipynb" files in a revision. Before, both rendered as Jupyter. Now, the former rendered as JSON and the latter rendered as Jupyter. Differential Revision: https://secure.phabricator.com/D20850 --- .../parser/DifferentialChangesetParser.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index a485a787cb..6c4ed3d14f 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1718,7 +1718,8 @@ final class DifferentialChangesetParser extends Phobject { $viewer, $new_ref); - $shared_engines = array_intersect_key($old_engines, $new_engines); + $shared_engines = array_intersect_key($new_engines, $old_engines); + $default_engine = head_key($new_engines); foreach ($shared_engines as $key => $shared_engine) { if (!$shared_engine->canDiffDocuments($old_ref, $new_ref)) { @@ -1734,7 +1735,17 @@ final class DifferentialChangesetParser extends Phobject { $document_engine = null; } } else { - $document_engine = head($shared_engines); + // If we aren't rendering with a specific engine, only use a default + // engine if the best engine for the new file is a shared engine which + // can diff files. If we're less picky (for example, by accepting any + // shared engine) we can end up with silly behavior (like ".json" files + // rendering as Jupyter documents). + + if (isset($shared_engines[$default_engine])) { + $document_engine = $shared_engines[$default_engine]; + } else { + $document_engine = null; + } } if ($document_engine) { From 344a2e39bed7e757718e38be8ba0780fe2067f4c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Oct 2019 07:26:39 -0700 Subject: [PATCH 02/12] In Jupyter notebooks, apply intraline diffing to source code lines Summary: Ref T13425. When we render a diff between two source lines, highlight intraline changes. Test Plan: Viewed a Jupyter notebook with a code diff in it, saw the changed subsequences in the line highlighted. Maniphest Tasks: T13425 Differential Revision: https://secure.phabricator.com/D20851 --- resources/celerity/map.php | 6 +- .../PhabricatorJupyterDocumentEngine.php | 62 +++++++++++++++++-- .../rsrc/css/phui/phui-property-list-view.css | 4 +- 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 7116134d1a..682f75a16b 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '7ce5a944', + 'core.pkg.css' => '88366522', 'core.pkg.js' => '6e5c894f', 'differential.pkg.css' => '607c84be', 'differential.pkg.js' => 'a0212a0b', @@ -169,7 +169,7 @@ return array( 'rsrc/css/phui/phui-pager.css' => 'd022c7ad', 'rsrc/css/phui/phui-pinboard-view.css' => '1f08f5d8', 'rsrc/css/phui/phui-policy-section-view.css' => '139fdc64', - 'rsrc/css/phui/phui-property-list-view.css' => '807b1632', + 'rsrc/css/phui/phui-property-list-view.css' => '9c477af1', 'rsrc/css/phui/phui-remarkup-preview.css' => '91767007', 'rsrc/css/phui/phui-segment-bar-view.css' => '5166b370', 'rsrc/css/phui/phui-spacing.css' => 'b05cadc3', @@ -865,7 +865,7 @@ return array( 'phui-pager-css' => 'd022c7ad', 'phui-pinboard-view-css' => '1f08f5d8', 'phui-policy-section-view-css' => '139fdc64', - 'phui-property-list-view-css' => '807b1632', + 'phui-property-list-view-css' => '9c477af1', 'phui-remarkup-preview-css' => '91767007', 'phui-segment-bar-view-css' => '5166b370', 'phui-spacing-css' => 'b05cadc3', diff --git a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php index b5c8b04b6d..d9d4c43abb 100644 --- a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php +++ b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php @@ -93,6 +93,54 @@ final class PhabricatorJupyterDocumentEngine $u_content = $this->newCellContainer($u_content); $v_content = $this->newCellContainer($v_content); + return id(new PhabricatorDocumentEngineBlockDiff()) + ->setOldContent($u_content) + ->addOldClass('old') + ->setNewContent($v_content) + ->addNewClass('new'); + case 'code/line': + $usource = idx($ucell, 'raw'); + $vsource = idx($vcell, 'raw'); + $udisplay = idx($ucell, 'display'); + $vdisplay = idx($vcell, 'display'); + $ulabel = idx($ucell, 'label'); + $vlabel = idx($vcell, 'label'); + + $intraline_segments = ArcanistDiffUtils::generateIntralineDiff( + $usource, + $vsource); + + $u_segments = array(); + foreach ($intraline_segments[0] as $u_segment) { + $u_segments[] = $u_segment; + } + + $v_segments = array(); + foreach ($intraline_segments[1] as $v_segment) { + $v_segments[] = $v_segment; + } + + $usource = ArcanistDiffUtils::applyIntralineDiff( + $udisplay, + $u_segments); + + $vsource = ArcanistDiffUtils::applyIntralineDiff( + $vdisplay, + $v_segments); + + $u_content = $this->newCodeLineCell($ucell, $usource); + $v_content = $this->newCodeLineCell($vcell, $vsource); + + $classes = array( + 'jupyter-cell-flush', + ); + + $u_content = $this->newJupyterCell($ulabel, $u_content, $classes); + $v_content = $this->newJupyterCell($vlabel, $v_content, $classes); + + $u_content = $this->newCellContainer($u_content); + $v_content = $this->newCellContainer($v_content); + return id(new PhabricatorDocumentEngineBlockDiff()) ->setOldContent($u_content) ->addOldClass('old') @@ -441,8 +489,10 @@ final class PhabricatorJupyterDocumentEngine return $this->newCodeOutputCell($cell); } - return $this->newRawCell(id(new PhutilJSON()) - ->encodeFormatted($cell)); + $json_content = id(new PhutilJSON()) + ->encodeFormatted($cell); + + return $this->newRawCell($json_content); } private function newRawCell($content) { @@ -514,7 +564,7 @@ final class PhabricatorJupyterDocumentEngine ); } - private function newCodeLineCell(array $cell) { + private function newCodeLineCell(array $cell, $content = null) { $classes = array(); $classes[] = 'PhabricatorMonospaced'; $classes[] = 'remarkup-code'; @@ -531,6 +581,10 @@ final class PhabricatorJupyterDocumentEngine $classes = implode(' ', $classes); + if ($content === null) { + $content = $cell['display']; + } + return array( $cell['label'], array( @@ -540,7 +594,7 @@ final class PhabricatorJupyterDocumentEngine 'class' => $classes, ), array( - $cell['display'], + $content, )), ), ); diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index 98cb0db820..c858905449 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -316,12 +316,12 @@ div.phui-property-list-stacked .phui-property-list-properties } td.new .jupyter-cell-code-line { - background: {$new-background}; + background: rgba(255, 255, 255, 0.5); border-color: {$new-bright}; } td.old .jupyter-cell-code-line { - background: {$old-background}; + background: rgba(255, 255, 255, 0.5); border-color: {$old-bright}; } From 960c447aab7f3059ad6d88ca5c99efed518bfcab Mon Sep 17 00:00:00 2001 From: Arturas Moskvinas Date: Wed, 2 Oct 2019 22:26:56 +0300 Subject: [PATCH 03/12] Support more than 9 portals Summary: If portal is created with id > 9 - then those portals are not reachable and always return 404. Test Plan: Create at least 10 portals, 10th and rest of them will no longer return 404. Reviewers: epriestley, Pawka, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D20852 --- .../dashboard/application/PhabricatorDashboardApplication.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/dashboard/application/PhabricatorDashboardApplication.php b/src/applications/dashboard/application/PhabricatorDashboardApplication.php index 9beac5ae53..9a09283fe3 100644 --- a/src/applications/dashboard/application/PhabricatorDashboardApplication.php +++ b/src/applications/dashboard/application/PhabricatorDashboardApplication.php @@ -69,7 +69,7 @@ final class PhabricatorDashboardApplication extends PhabricatorApplication { 'PhabricatorDashboardPortalListController', $this->getEditRoutePattern('edit/') => 'PhabricatorDashboardPortalEditController', - 'view/(?P\d)/' => array( + 'view/(?P\d+)/' => array( '' => 'PhabricatorDashboardPortalViewController', ) + $menu_rules, From 6d74736a7ec8d5bda259234a4d2d70331a0331e5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Oct 2019 13:09:52 -0700 Subject: [PATCH 04/12] When a large number of commits need to be marked as published, issue the lookup query in chunks Summary: See PHI1474. This query can become large enough to exceed reasonable packet limits. Chunk the query so it is split up if we have too many identifiers. Test Plan: Ran `bin/repository refs ...` on a repository with no new commits and a repository with some new commits. Differential Revision: https://secure.phabricator.com/D20853 --- .../engine/PhabricatorRepositoryRefEngine.php | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php index f823ead6d9..8d8e7f45d8 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php @@ -498,7 +498,7 @@ final class PhabricatorRepositoryRefEngine private function setCloseFlagOnCommits(array $identifiers) { $repository = $this->getRepository(); $commit_table = new PhabricatorRepositoryCommit(); - $conn_w = $commit_table->establishConnection('w'); + $conn = $commit_table->establishConnection('w'); $vcs = $repository->getVersionControlSystem(); switch ($vcs) { @@ -515,13 +515,27 @@ final class PhabricatorRepositoryRefEngine throw new Exception(pht("Unknown repository type '%s'!", $vcs)); } - $all_commits = queryfx_all( - $conn_w, - 'SELECT id, phid, commitIdentifier, importStatus FROM %T - WHERE repositoryID = %d AND commitIdentifier IN (%Ls)', - $commit_table->getTableName(), - $repository->getID(), - $identifiers); + $identifier_tokens = array(); + foreach ($identifiers as $identifier) { + $identifier_tokens[] = qsprintf( + $conn, + '%s', + $identifier); + } + + $all_commits = array(); + foreach (PhabricatorLiskDAO::chunkSQL($identifier_tokens) as $chunk) { + $rows = queryfx_all( + $conn, + 'SELECT id, phid, commitIdentifier, importStatus FROM %T + WHERE repositoryID = %d AND commitIdentifier IN (%LQ)', + $commit_table->getTableName(), + $repository->getID(), + $chunk); + foreach ($rows as $row) { + $all_commits[] = $row; + } + } $closeable_flag = PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE; @@ -539,7 +553,7 @@ final class PhabricatorRepositoryRefEngine if (!($row['importStatus'] & $closeable_flag)) { queryfx( - $conn_w, + $conn, 'UPDATE %T SET importStatus = (importStatus | %d) WHERE id = %d', $commit_table->getTableName(), $closeable_flag, From 7badec23a7d0a1a144d1fc7b89f366fbe528c2a0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Oct 2019 13:26:39 -0700 Subject: [PATCH 05/12] Use "git log ... --stdin" instead of "git log ... --not ..." to avoid oversized command lines Summary: Depends on D20853. See PHI1474. If the list of "--not" refs is sufficiently long, we may exceed the maximum size of a command. Use "--stdin" instead, and swap "--not" for the slightly less readable but functionally equivalent "^hash", which has the advantage of actually working with "--stdin". Test Plan: Ran `bin/repository refs ...` with nothing to be done, and with something to be done. Differential Revision: https://secure.phabricator.com/D20854 --- .../engine/PhabricatorRepositoryRefEngine.php | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php index 8d8e7f45d8..68e2c344fc 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php @@ -469,11 +469,26 @@ final class PhabricatorRepositoryRefEngine return phutil_split_lines($stdout, $retain_newlines = false); case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: if ($all_closing_heads) { - list($stdout) = $this->getRepository()->execxLocalCommand( - 'log --format=%s %s --not %Ls', - '%H', - $new_head, - $all_closing_heads); + + // See PHI1474. This length of list may exceed the maximum size of + // a command line argument list, so pipe the list in using "--stdin" + // instead. + + $ref_list = array(); + $ref_list[] = $new_head; + foreach ($all_closing_heads as $old_head) { + $ref_list[] = '^'.$old_head; + } + $ref_list[] = '--'; + $ref_list = implode("\n", $ref_list)."\n"; + + $future = $this->getRepository()->getLocalCommandFuture( + 'log --format=%s --stdin', + '%H'); + + list($stdout) = $future + ->write($ref_list) + ->resolvex(); } else { list($stdout) = $this->getRepository()->execxLocalCommand( 'log --format=%s %s', From 5dafabd5b4d039bd06b7c773a70a79236ea7409e Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 17 Oct 2019 09:06:13 -0700 Subject: [PATCH 06/12] Fix deprecated argument order for "implode()" Summary: Fixes T13428. In modern PHP, "implode()" should take the glue parameter first. Test Plan: Used the linter introduced in D20857 to identify affected callsites. ``` $ git grep -i implode | cut -d: -f1 | sort | uniq | xargs arc lint --output summary --never-apply-patches | grep -i glue ``` Maniphest Tasks: T13428 Differential Revision: https://secure.phabricator.com/D20858 --- .../PhabricatorDifferentialRevisionTestDataGenerator.php | 2 +- src/applications/people/view/PhabricatorUserCardView.php | 2 +- src/applications/project/view/PhabricatorProjectCardView.php | 2 +- src/view/phui/PHUIPropertyListView.php | 4 ++-- src/view/phui/PHUITwoColumnView.php | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php b/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php index 26632dff24..84a4cbc519 100644 --- a/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php +++ b/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php @@ -103,7 +103,7 @@ final class PhabricatorDifferentialRevisionTestDataGenerator $newcode2[] = $altcodearr[$randomlines_new[$c++]]; } } - return implode($newcode2, "\n"); + return implode("\n", $newcode2); } } diff --git a/src/applications/people/view/PhabricatorUserCardView.php b/src/applications/people/view/PhabricatorUserCardView.php index 21cb468ba8..54d0a41204 100644 --- a/src/applications/people/view/PhabricatorUserCardView.php +++ b/src/applications/people/view/PhabricatorUserCardView.php @@ -38,7 +38,7 @@ final class PhabricatorUserCardView extends AphrontTagView { } return array( - 'class' => implode($classes, ' '), + 'class' => implode(' ', $classes), ); } diff --git a/src/applications/project/view/PhabricatorProjectCardView.php b/src/applications/project/view/PhabricatorProjectCardView.php index f82ee8c99e..a56697ba7e 100644 --- a/src/applications/project/view/PhabricatorProjectCardView.php +++ b/src/applications/project/view/PhabricatorProjectCardView.php @@ -36,7 +36,7 @@ final class PhabricatorProjectCardView extends AphrontTagView { $classes[] = 'project-card-'.$color; return array( - 'class' => implode($classes, ' '), + 'class' => implode(' ', $classes), ); } diff --git a/src/view/phui/PHUIPropertyListView.php b/src/view/phui/PHUIPropertyListView.php index 336c494a3c..62fa30bba8 100644 --- a/src/view/phui/PHUIPropertyListView.php +++ b/src/view/phui/PHUIPropertyListView.php @@ -284,7 +284,7 @@ final class PHUIPropertyListView extends AphrontView { return phutil_tag( 'div', array( - 'class' => implode($classes, ' '), + 'class' => implode(' ', $classes), ), $part['content']); } @@ -295,7 +295,7 @@ final class PHUIPropertyListView extends AphrontView { return phutil_tag( 'div', array( - 'class' => implode($classes, ' '), + 'class' => implode(' ', $classes), ), $part['content']); } diff --git a/src/view/phui/PHUITwoColumnView.php b/src/view/phui/PHUITwoColumnView.php index 9240887f4b..cf4abe3a40 100644 --- a/src/view/phui/PHUITwoColumnView.php +++ b/src/view/phui/PHUITwoColumnView.php @@ -220,7 +220,7 @@ final class PHUITwoColumnView extends AphrontTagView { return phutil_tag( 'div', array( - 'class' => implode($classes, ' '), + 'class' => implode(' ', $classes), ), array( $navigation, From d34dfa37461b3c7ef598df4309a83ac3fd8970fc Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 17 Oct 2019 09:14:21 -0700 Subject: [PATCH 07/12] Fix an error message when calling "transaction.search" with a non-transactional object PHID as an "objectIdentifier" Summary: See PHI1499. This error message doesn't provide parameters, and can be a little bit more helpful. Test Plan: {F6957550} Differential Revision: https://secure.phabricator.com/D20859 --- .../conduit/TransactionSearchConduitAPIMethod.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php index 6f7f713dab..bc0311eae4 100644 --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -103,8 +103,11 @@ EOREMARKUP if (!($object instanceof PhabricatorApplicationTransactionInterface)) { throw new Exception( pht( - 'Object "%s" does not implement "%s", so transactions can not '. - 'be loaded for it.')); + 'Object "%s" (of type "%s") does not implement "%s", so '. + 'transactions can not be loaded for it.', + $object_name, + get_class($object), + 'PhabricatorApplicationTransactionInterface')); } $xaction_query = PhabricatorApplicationTransactionQuery::newQueryForObject( From f497b93e4311b424b24599f23c272ecb912ab5a6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 17 Oct 2019 09:39:26 -0700 Subject: [PATCH 08/12] Fix a fatal in the "Projects" curtain extension when a project edge connects an object to a non-project Summary: Ref T13429. It's currently possible to write "TYPE_EDGE" relationships for the "object has project" edge to PHIDs which may not actually be projects. Today, this fatals. As a first step, unfatal it. T13429 discusses general improvements and greater context. Test Plan: Used "maniphest.edit" to write a "project" edge to a user PHID, viewed the task in the UI. Previously it fataled; now it renders unusually (the object is "tagged" with a user) but faithfully reflects database state. {F6957606} Maniphest Tasks: T13429 Differential Revision: https://secure.phabricator.com/D20860 --- src/__phutil_library_map__.php | 2 ++ .../project/engine/PhabricatorBoardLayoutEngine.php | 6 ++++++ .../project/interface/PhabricatorWorkboardInterface.php | 3 +++ src/applications/project/storage/PhabricatorProject.php | 3 ++- 4 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 src/applications/project/interface/PhabricatorWorkboardInterface.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 41befc5ce3..189ecdf0bf 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -5044,6 +5044,7 @@ phutil_register_library_map(array( 'PhabricatorWeekStartDaySetting' => 'applications/settings/setting/PhabricatorWeekStartDaySetting.php', 'PhabricatorWildConfigType' => 'applications/config/type/PhabricatorWildConfigType.php', 'PhabricatorWordPressAuthProvider' => 'applications/auth/provider/PhabricatorWordPressAuthProvider.php', + 'PhabricatorWorkboardInterface' => 'applications/project/interface/PhabricatorWorkboardInterface.php', 'PhabricatorWorkboardViewState' => 'applications/project/state/PhabricatorWorkboardViewState.php', 'PhabricatorWorker' => 'infrastructure/daemon/workers/PhabricatorWorker.php', 'PhabricatorWorkerActiveTask' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php', @@ -10748,6 +10749,7 @@ phutil_register_library_map(array( 'PhabricatorColumnProxyInterface', 'PhabricatorSpacesInterface', 'PhabricatorEditEngineSubtypeInterface', + 'PhabricatorWorkboardInterface', ), 'PhabricatorProjectActivityChartEngine' => 'PhabricatorChartEngine', 'PhabricatorProjectAddHeraldAction' => 'PhabricatorProjectHeraldAction', diff --git a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php index 22aaed2d4b..a0be7e5775 100644 --- a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php +++ b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php @@ -301,6 +301,12 @@ final class PhabricatorBoardLayoutEngine extends Phobject { ->execute(); $boards = mpull($boards, null, 'getPHID'); + foreach ($boards as $key => $board) { + if (!($board instanceof PhabricatorWorkboardInterface)) { + unset($boards[$key]); + } + } + if (!$this->fetchAllBoards) { foreach ($boards as $key => $board) { if (!$board->getHasWorkboard()) { diff --git a/src/applications/project/interface/PhabricatorWorkboardInterface.php b/src/applications/project/interface/PhabricatorWorkboardInterface.php new file mode 100644 index 0000000000..2b760b4875 --- /dev/null +++ b/src/applications/project/interface/PhabricatorWorkboardInterface.php @@ -0,0 +1,3 @@ + Date: Thu, 24 Oct 2019 16:42:25 -0700 Subject: [PATCH 09/12] Add default branch, description, and metrics (commit count, recent commit) to "diffusion.repository.search" Summary: Fixes T13430. Provide more information about repositories in "diffusion.repository.search". Test Plan: Used API console to call method (with new "metrics" attachment), reviewed output. Saw new fields returned. Maniphest Tasks: T13430 Differential Revision: https://secure.phabricator.com/D20862 --- src/__phutil_library_map__.php | 2 + ...epositoryMetricsSearchEngineAttachment.php | 41 +++++++++++++++++++ .../query/PhabricatorRepositoryQuery.php | 2 + .../storage/PhabricatorRepository.php | 19 +++++++++ 4 files changed, 64 insertions(+) create mode 100644 src/applications/diffusion/engineextension/DiffusionRepositoryMetricsSearchEngineAttachment.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 189ecdf0bf..9d51d92760 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -996,6 +996,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryManagementOtherPanelGroup' => 'applications/diffusion/management/DiffusionRepositoryManagementOtherPanelGroup.php', 'DiffusionRepositoryManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryManagementPanel.php', 'DiffusionRepositoryManagementPanelGroup' => 'applications/diffusion/management/DiffusionRepositoryManagementPanelGroup.php', + 'DiffusionRepositoryMetricsSearchEngineAttachment' => 'applications/diffusion/engineextension/DiffusionRepositoryMetricsSearchEngineAttachment.php', 'DiffusionRepositoryPath' => 'applications/diffusion/data/DiffusionRepositoryPath.php', 'DiffusionRepositoryPoliciesManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryPoliciesManagementPanel.php', 'DiffusionRepositoryProfilePictureController' => 'applications/diffusion/controller/DiffusionRepositoryProfilePictureController.php', @@ -6964,6 +6965,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryManagementOtherPanelGroup' => 'DiffusionRepositoryManagementPanelGroup', 'DiffusionRepositoryManagementPanel' => 'Phobject', 'DiffusionRepositoryManagementPanelGroup' => 'Phobject', + 'DiffusionRepositoryMetricsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'DiffusionRepositoryPath' => 'Phobject', 'DiffusionRepositoryPoliciesManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryProfilePictureController' => 'DiffusionController', diff --git a/src/applications/diffusion/engineextension/DiffusionRepositoryMetricsSearchEngineAttachment.php b/src/applications/diffusion/engineextension/DiffusionRepositoryMetricsSearchEngineAttachment.php new file mode 100644 index 0000000000..9711e72352 --- /dev/null +++ b/src/applications/diffusion/engineextension/DiffusionRepositoryMetricsSearchEngineAttachment.php @@ -0,0 +1,41 @@ +needCommitCounts(true) + ->needMostRecentCommits(true); + } + + public function getAttachmentForObject($object, $data, $spec) { + $commit = $object->getMostRecentCommit(); + if ($commit !== null) { + $recent_commit = $commit->getFieldValuesForConduit(); + } else { + $recent_commit = null; + } + + $commit_count = $object->getCommitCount(); + if ($commit_count !== null) { + $commit_count = (int)$commit_count; + } + + return array( + 'commitCount' => $commit_count, + 'recentCommit' => $recent_commit, + ); + } + +} diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index e960cf888b..21465393f7 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -215,6 +215,8 @@ final class PhabricatorRepositoryQuery $commits = id(new DiffusionCommitQuery()) ->setViewer($this->getViewer()) ->withIDs($commit_ids) + ->needCommitData(true) + ->needIdentities(true) ->execute(); } else { $commits = array(); diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index fdc9a695c4..d568c755c5 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -2757,6 +2757,14 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO ->setDescription( pht( 'The "Fetch" and "Permanent Ref" rules for this repository.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('defaultBranch') + ->setType('string?') + ->setDescription(pht('Default branch name.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('description') + ->setType('remarkup') + ->setDescription(pht('Repository description.')), ); } @@ -2769,6 +2777,11 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $track_rules = $this->getStringListForConduit($track_rules); $permanent_rules = $this->getStringListForConduit($permanent_rules); + $default_branch = $this->getDefaultBranch(); + if (!strlen($default_branch)) { + $default_branch = null; + } + return array( 'name' => $this->getName(), 'vcs' => $this->getVersionControlSystem(), @@ -2782,6 +2795,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO 'trackRules' => $track_rules, 'permanentRefRules' => $permanent_rules, ), + 'defaultBranch' => $default_branch, + 'description' => array( + 'raw' => (string)$this->getDetail('description'), + ), ); } @@ -2804,6 +2821,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return array( id(new DiffusionRepositoryURIsSearchEngineAttachment()) ->setAttachmentKey('uris'), + id(new DiffusionRepositoryMetricsSearchEngineAttachment()) + ->setAttachmentKey('metrics'), ); } From 633aa5288c58d507d8227b1007e3f1dca7cb1e4f Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Oct 2019 18:03:11 -0700 Subject: [PATCH 10/12] Persist login instructions onto flow-specific login pages (username/password and LDAP) Summary: Fixes T13433. Currently, "Login Screen Instructions" in "Auth" are shown only on the main login screen. If you enter a bad password or bad LDAP credential set and move to the flow-specific login failure screen (for example, "invalid password"), the instructions vanish. Instead, persist them. There are reasonable cases where this is highly useful and the cases which spring to mind where this is possibly misleading are fairly easy to fix by making the instructions more specific. Test Plan: - Configured login instructions in "Auth". - Viewed main login screen, saw instructions. - Entered a bad username/password and a bad LDAP credential set, got kicked to workflow sub-pages and still saw instructions (previously: no instructions). - Grepped for other callers to `buildProviderPageResponse()` to look for anything weird, came up empty. Maniphest Tasks: T13433 Differential Revision: https://secure.phabricator.com/D20863 --- resources/celerity/map.php | 6 ++--- .../controller/PhabricatorAuthController.php | 22 +++++++++++++++++++ .../PhabricatorAuthLoginController.php | 12 +++++++--- .../PhabricatorAuthStartController.php | 21 ------------------ webroot/rsrc/css/application/auth/auth.css | 2 +- 5 files changed, 35 insertions(+), 28 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 682f75a16b..0e53737aaf 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '88366522', + 'core.pkg.css' => '686ae87c', 'core.pkg.js' => '6e5c894f', 'differential.pkg.css' => '607c84be', 'differential.pkg.js' => 'a0212a0b', @@ -36,7 +36,7 @@ return array( 'rsrc/css/aphront/typeahead-browse.css' => 'b7ed02d2', 'rsrc/css/aphront/typeahead.css' => '8779483d', 'rsrc/css/application/almanac/almanac.css' => '2e050f4f', - 'rsrc/css/application/auth/auth.css' => 'add92fd8', + 'rsrc/css/application/auth/auth.css' => 'c2f23d74', 'rsrc/css/application/base/main-menu-view.css' => '17b71bbc', 'rsrc/css/application/base/notification-menu.css' => '4df1ee30', 'rsrc/css/application/base/phui-theme.css' => '35883b37', @@ -540,7 +540,7 @@ return array( 'aphront-tooltip-css' => 'e3f2412f', 'aphront-typeahead-control-css' => '8779483d', 'application-search-view-css' => '0f7c06d8', - 'auth-css' => 'add92fd8', + 'auth-css' => 'c2f23d74', 'bulk-job-css' => '73af99f5', 'conduit-api-css' => 'ce2cfc41', 'config-options-css' => '16c920ae', diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php index cda56d34b1..505620b3f3 100644 --- a/src/applications/auth/controller/PhabricatorAuthController.php +++ b/src/applications/auth/controller/PhabricatorAuthController.php @@ -286,4 +286,26 @@ abstract class PhabricatorAuthController extends PhabricatorController { ->appendChild($invite_list); } + + final protected function newCustomStartMessage() { + $viewer = $this->getViewer(); + + $text = PhabricatorAuthMessage::loadMessageText( + $viewer, + PhabricatorAuthLoginMessageType::MESSAGEKEY); + + if (!strlen($text)) { + return null; + } + + $remarkup_view = new PHUIRemarkupView($viewer, $text); + + return phutil_tag( + 'div', + array( + 'class' => 'auth-custom-message', + ), + $remarkup_view); + } + } diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php index e7dabd9340..b46cf36d49 100644 --- a/src/applications/auth/controller/PhabricatorAuthLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php @@ -238,18 +238,24 @@ final class PhabricatorAuthLoginController $content) { $crumbs = $this->buildApplicationCrumbs(); + $viewer = $this->getViewer(); - if ($this->getRequest()->getUser()->isLoggedIn()) { + if ($viewer->isLoggedIn()) { $crumbs->addTextCrumb(pht('Link Account'), $provider->getSettingsURI()); } else { - $crumbs->addTextCrumb(pht('Log In'), $this->getApplicationURI('start/')); + $crumbs->addTextCrumb(pht('Login'), $this->getApplicationURI('start/')); + + $content = array( + $this->newCustomStartMessage(), + $content, + ); } $crumbs->addTextCrumb($provider->getProviderName()); $crumbs->setBorder(true); return $this->newPage() - ->setTitle(pht('Log In')) + ->setTitle(pht('Login')) ->setCrumbs($crumbs) ->appendChild($content); } diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php index 72cbbea5a8..7e9b17feff 100644 --- a/src/applications/auth/controller/PhabricatorAuthStartController.php +++ b/src/applications/auth/controller/PhabricatorAuthStartController.php @@ -298,27 +298,6 @@ final class PhabricatorAuthStartController ->setURI($auto_uri); } - private function newCustomStartMessage() { - $viewer = $this->getViewer(); - - $text = PhabricatorAuthMessage::loadMessageText( - $viewer, - PhabricatorAuthLoginMessageType::MESSAGEKEY); - - if (!strlen($text)) { - return null; - } - - $remarkup_view = new PHUIRemarkupView($viewer, $text); - - return phutil_tag( - 'div', - array( - 'class' => 'auth-custom-message', - ), - $remarkup_view); - } - private function newEmailLoginView(array $configs) { assert_instances_of($configs, 'PhabricatorAuthProviderConfig'); diff --git a/webroot/rsrc/css/application/auth/auth.css b/webroot/rsrc/css/application/auth/auth.css index 687aaf2bb4..28b18b85c5 100644 --- a/webroot/rsrc/css/application/auth/auth.css +++ b/webroot/rsrc/css/application/auth/auth.css @@ -57,7 +57,7 @@ } .auth-custom-message { - margin: 32px auto 64px; + margin: 32px auto 48px; max-width: 548px; background: #fff; padding: 16px; From 38694578e1d0c416e992a0fa95edbc270c8af5e8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Oct 2019 18:30:55 -0700 Subject: [PATCH 11/12] Improve project member list behaviors related to disabled users Summary: Fixes T13431. Increase the "panel" version of project member lists to 10 users, hide disabled users, swap the buttons to "tail buttons". Sort disabled users to the bottom of "full list" versions of member lists. For UI consistency, render the remove "X" as disabled but visible if users don't have permission to remove members. Test Plan: - Viewed a project with disabled members. - Saw only enabled members on the main project page. - Saw disabled members sorted to the bottom on the members page. - Clicked "View All" to jump from the panel to the members page. - As a user who could not edit a project, viewed the members page and saw a disabled "X" with a policy error when clicked. - Removed a member as before, as a normal user with permission to remove members. Maniphest Tasks: T13431 Differential Revision: https://secure.phabricator.com/D20864 --- .../PhabricatorProjectProfileController.php | 4 +- .../view/PhabricatorProjectUserListView.php | 86 ++++++++++++------- 2 files changed, 56 insertions(+), 34 deletions(-) diff --git a/src/applications/project/controller/PhabricatorProjectProfileController.php b/src/applications/project/controller/PhabricatorProjectProfileController.php index 386a649238..54e1d77d55 100644 --- a/src/applications/project/controller/PhabricatorProjectProfileController.php +++ b/src/applications/project/controller/PhabricatorProjectProfileController.php @@ -63,14 +63,14 @@ final class PhabricatorProjectProfileController $member_list = id(new PhabricatorProjectMemberListView()) ->setUser($viewer) ->setProject($project) - ->setLimit(5) + ->setLimit(10) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->setUserPHIDs($project->getMemberPHIDs()); $watcher_list = id(new PhabricatorProjectWatcherListView()) ->setUser($viewer) ->setProject($project) - ->setLimit(5) + ->setLimit(10) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->setUserPHIDs($project->getWatcherPHIDs()); diff --git a/src/applications/project/view/PhabricatorProjectUserListView.php b/src/applications/project/view/PhabricatorProjectUserListView.php index 51c2ced6d1..dd3e084c6e 100644 --- a/src/applications/project/view/PhabricatorProjectUserListView.php +++ b/src/applications/project/view/PhabricatorProjectUserListView.php @@ -1,6 +1,7 @@ getUserPHIDs(); $can_edit = $this->canEditList(); + $supports_edit = $project->supportsEditMembers(); $no_data = $this->getNoDataString(); $list = id(new PHUIObjectItemListView()) ->setNoDataString($no_data); $limit = $this->getLimit(); - - // If we're showing everything, show oldest to newest. If we're showing - // only a slice, show newest to oldest. - if (!$limit) { - $user_phids = array_reverse($user_phids); - } + $is_panel = (bool)$limit; $handles = $viewer->loadHandles($user_phids); - // Always put the viewer first if they are on the list. - $user_phids = array_fuse($user_phids); - $user_phids = - array_select_keys($user_phids, array($viewer->getPHID())) + - $user_phids; + // Reorder users in display order. We're going to put the viewer first + // if they're a member, then enabled users, then disabled/invalid users. - if ($limit) { - $render_phids = array_slice($user_phids, 0, $limit); - } else { - $render_phids = $user_phids; - } - - foreach ($render_phids as $user_phid) { + $phid_map = array(); + foreach ($user_phids as $user_phid) { $handle = $handles[$user_phid]; + $is_viewer = ($user_phid === $viewer->getPHID()); + $is_enabled = ($handle->isComplete() && !$handle->isDisabled()); + + // If we're showing the main member list, show oldest to newest. If we're + // showing only a slice in a panel, show newest to oldest. + if ($limit) { + $order_scalar = 1; + } else { + $order_scalar = -1; + } + + $phid_map[$user_phid] = id(new PhutilSortVector()) + ->addInt($is_viewer ? 0 : 1) + ->addInt($is_enabled ? 0 : 1) + ->addInt($order_scalar * count($phid_map)); + } + $phid_map = msortv($phid_map, 'getSelf'); + + $handles = iterator_to_array($handles); + $handles = array_select_keys($handles, array_keys($phid_map)); + + if ($limit) { + $handles = array_slice($handles, 0, $limit); + } + + foreach ($handles as $user_phid => $handle) { $item = id(new PHUIObjectItemView()) ->setHeader($handle->getFullName()) ->setHref($handle->getURI()) ->setImageURI($handle->getImageURI()); - $icon = id(new PHUIIconView()) - ->setIcon($handle->getIcon()); + if ($handle->isDisabled()) { + if ($is_panel) { + // Don't show disabled users in the panel view at all. + continue; + } - $subtitle = $handle->getSubtitle(); + $item + ->setDisabled(true) + ->addAttribute(pht('Disabled')); + } else { + $icon = id(new PHUIIconView()) + ->setIcon($handle->getIcon()); - $item->addAttribute(array($icon, ' ', $subtitle)); + $subtitle = $handle->getSubtitle(); - if ($can_edit && !$limit) { + $item->addAttribute(array($icon, ' ', $subtitle)); + } + + if ($supports_edit && !$is_panel) { $remove_uri = $this->getRemoveURI($user_phid); $item->addAction( @@ -107,6 +133,7 @@ abstract class PhabricatorProjectUserListView extends AphrontView { ->setIcon('fa-times') ->setName(pht('Remove')) ->setHref($remove_uri) + ->setDisabled(!$can_edit) ->setWorkflow(true)); } @@ -128,14 +155,9 @@ abstract class PhabricatorProjectUserListView extends AphrontView { ->setHeader($header_text); if ($limit) { - $header->addActionLink( - id(new PHUIButtonView()) - ->setTag('a') - ->setIcon( - id(new PHUIIconView()) - ->setIcon('fa-list-ul')) - ->setText(pht('View All')) - ->setHref("/project/members/{$id}/")); + $list->newTailButton() + ->setText(pht('View All')) + ->setHref("/project/members/{$id}/"); } $box = id(new PHUIObjectBoxView()) From 8ff0e3ab351f5e806c1a9e072695e4f905604913 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 25 Oct 2019 11:36:43 -0700 Subject: [PATCH 12/12] Support rich diff rendering with DocumentEngine for added/removed files Summary: Ref T13425. When a file (like a Jupyter notebook) is added or removed, we can still render a useful line-by-line diff. Test Plan: - Viewed add/modify/remove of Jupyter, source code, and images in 2up/1up mode, everything looked okay. Maniphest Tasks: T13425 Differential Revision: https://secure.phabricator.com/D20865 --- .../parser/DifferentialChangesetParser.php | 104 ++++++++++++------ .../DifferentialChangesetOneUpRenderer.php | 10 ++ .../diff/PhabricatorDocumentEngineBlocks.php | 5 +- .../document/PhabricatorDocumentEngine.php | 4 +- .../PhabricatorImageDocumentEngine.php | 33 ++++-- .../PhabricatorJupyterDocumentEngine.php | 21 +++- 6 files changed, 124 insertions(+), 53 deletions(-) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 6c4ed3d14f..571405f64b 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1686,47 +1686,79 @@ final class DifferentialChangesetParser extends Phobject { break; } - $old_ref = id(new PhabricatorDocumentRef()) - ->setName($changeset->getOldFile()); - if ($old_file) { - $old_ref->setFile($old_file); + $type_delete = DifferentialChangeType::TYPE_DELETE; + $type_add = DifferentialChangeType::TYPE_ADD; + $change_type = $changeset->getChangeType(); + + $no_old = ($change_type == $type_add); + $no_new = ($change_type == $type_delete); + + if ($no_old) { + $old_ref = null; } else { - $old_data = $this->old; - $old_data = ipull($old_data, 'text'); - $old_data = implode('', $old_data); + $old_ref = id(new PhabricatorDocumentRef()) + ->setName($changeset->getOldFile()); + if ($old_file) { + $old_ref->setFile($old_file); + } else { + $old_data = $this->old; + $old_data = ipull($old_data, 'text'); + $old_data = implode('', $old_data); - $old_ref->setData($old_data); - } - - $new_ref = id(new PhabricatorDocumentRef()) - ->setName($changeset->getFilename()); - if ($new_file) { - $new_ref->setFile($new_file); - } else { - $new_data = $this->new; - $new_data = ipull($new_data, 'text'); - $new_data = implode('', $new_data); - - $new_ref->setData($new_data); - } - - $old_engines = PhabricatorDocumentEngine::getEnginesForRef( - $viewer, - $old_ref); - - $new_engines = PhabricatorDocumentEngine::getEnginesForRef( - $viewer, - $new_ref); - - $shared_engines = array_intersect_key($new_engines, $old_engines); - $default_engine = head_key($new_engines); - - foreach ($shared_engines as $key => $shared_engine) { - if (!$shared_engine->canDiffDocuments($old_ref, $new_ref)) { - unset($shared_engines[$key]); + $old_ref->setData($old_data); } } + if ($no_new) { + $new_ref = null; + } else { + $new_ref = id(new PhabricatorDocumentRef()) + ->setName($changeset->getFilename()); + if ($new_file) { + $new_ref->setFile($new_file); + } else { + $new_data = $this->new; + $new_data = ipull($new_data, 'text'); + $new_data = implode('', $new_data); + + $new_ref->setData($new_data); + } + } + + + $old_engines = null; + if ($old_ref) { + $old_engines = PhabricatorDocumentEngine::getEnginesForRef( + $viewer, + $old_ref); + } + + $new_engines = null; + if ($new_ref) { + $new_engines = PhabricatorDocumentEngine::getEnginesForRef( + $viewer, + $new_ref); + } + + if ($new_engines !== null && $old_engines !== null) { + $shared_engines = array_intersect_key($new_engines, $old_engines); + $default_engine = head_key($new_engines); + + foreach ($shared_engines as $key => $shared_engine) { + if (!$shared_engine->canDiffDocuments($old_ref, $new_ref)) { + unset($shared_engines[$key]); + } + } + } else if ($new_engines !== null) { + $shared_engines = $new_engines; + $default_engine = head_key($shared_engines); + } else if ($old_engines !== null) { + $shared_engines = $old_engines; + $default_engine = head_key($shared_engines); + } else { + return null; + } + $engine_key = $this->getDocumentEngineKey(); if (strlen($engine_key)) { if (isset($shared_engines[$engine_key])) { diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index fc50b0d605..19c939274d 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -371,17 +371,27 @@ final class DifferentialChangesetOneUpRenderer $cell_classes = $block_diff->getNewClasses(); } } else if ($row_type === 'old') { + if (!$old_ref) { + continue; + } + $cell_content = $engine->newBlockContentView( $old_ref, $old); + $cell_classes[] = 'old'; $cell_classes[] = 'old-full'; $new_key = null; } else if ($row_type === 'new') { + if (!$new_ref) { + continue; + } + $cell_content = $engine->newBlockContentView( $new_ref, $new); + $cell_classes[] = 'new'; $cell_classes[] = 'new-full'; diff --git a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php index f8f3a414b1..47ddf194ee 100644 --- a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php +++ b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php @@ -15,7 +15,10 @@ final class PhabricatorDocumentEngineBlocks return $this->messages; } - public function addBlockList(PhabricatorDocumentRef $ref, array $blocks) { + public function addBlockList( + PhabricatorDocumentRef $ref = null, + array $blocks = array()) { + assert_instances_of($blocks, 'PhabricatorDocumentEngineBlock'); $this->lists[] = array( diff --git a/src/applications/files/document/PhabricatorDocumentEngine.php b/src/applications/files/document/PhabricatorDocumentEngine.php index bc7960b4ad..9593e1d98d 100644 --- a/src/applications/files/document/PhabricatorDocumentEngine.php +++ b/src/applications/files/document/PhabricatorDocumentEngine.php @@ -32,8 +32,8 @@ abstract class PhabricatorDocumentEngine } public function canDiffDocuments( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { return false; } diff --git a/src/applications/files/document/PhabricatorImageDocumentEngine.php b/src/applications/files/document/PhabricatorImageDocumentEngine.php index fa678cc034..449d604370 100644 --- a/src/applications/files/document/PhabricatorImageDocumentEngine.php +++ b/src/applications/files/document/PhabricatorImageDocumentEngine.php @@ -18,21 +18,38 @@ final class PhabricatorImageDocumentEngine } public function canDiffDocuments( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { - // For now, we can only render a rich image diff if both documents have + // For now, we can only render a rich image diff if the documents have // their data stored in Files already. - return ($uref->getFile() && $vref->getFile()); + if ($uref && !$uref->getFile()) { + return false; + } + + if ($vref && !$vref->getFile()) { + return false; + } + + return true; } public function newEngineBlocks( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { - $u_blocks = $this->newDiffBlocks($uref); - $v_blocks = $this->newDiffBlocks($vref); + if ($uref) { + $u_blocks = $this->newDiffBlocks($uref); + } else { + $u_blocks = array(); + } + + if ($vref) { + $v_blocks = $this->newDiffBlocks($vref); + } else { + $v_blocks = array(); + } return id(new PhabricatorDocumentEngineBlocks()) ->addBlockList($uref, $u_blocks) diff --git a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php index d9d4c43abb..0c3e891c3c 100644 --- a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php +++ b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php @@ -36,20 +36,29 @@ final class PhabricatorJupyterDocumentEngine } public function canDiffDocuments( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { return true; } public function newEngineBlocks( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { $blocks = new PhabricatorDocumentEngineBlocks(); try { - $u_blocks = $this->newDiffBlocks($uref); - $v_blocks = $this->newDiffBlocks($vref); + if ($uref) { + $u_blocks = $this->newDiffBlocks($uref); + } else { + $u_blocks = array(); + } + + if ($vref) { + $v_blocks = $this->newDiffBlocks($vref); + } else { + $v_blocks = array(); + } $blocks->addBlockList($uref, $u_blocks); $blocks->addBlockList($vref, $v_blocks);