Changeset View
Standalone View
src/applications/diviner/query/DivinerBookQuery.php
| <?php | <?php | ||||
| final class DivinerBookQuery extends PhabricatorCursorPagedPolicyAwareQuery { | final class DivinerBookQuery extends PhabricatorCursorPagedPolicyAwareQuery { | ||||
| private $ids; | private $ids; | ||||
| private $phids; | private $phids; | ||||
| private $names; | private $names; | ||||
| private $repositoryPHIDs; | |||||
| private $needProjectPHIDs; | private $needProjectPHIDs; | ||||
| private $needRepositories; | |||||
| public function withIDs(array $ids) { | public function withIDs(array $ids) { | ||||
| $this->ids = $ids; | $this->ids = $ids; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| public function withPHIDs(array $phids) { | public function withPHIDs(array $phids) { | ||||
| $this->phids = $phids; | $this->phids = $phids; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| public function withNames(array $names) { | public function withNames(array $names) { | ||||
| $this->names = $names; | $this->names = $names; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| public function withRepositoryPHIDs(array $repository_phids) { | |||||
| $this->repositoryPHIDs = $repository_phids; | |||||
| return $this; | |||||
| } | |||||
| public function needProjectPHIDs($need_phids) { | public function needProjectPHIDs($need_phids) { | ||||
| $this->needProjectPHIDs = $need_phids; | $this->needProjectPHIDs = $need_phids; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
epriestley: I think we should always load these and perform policy filtering; if you can't see a repository… | |||||
| public function needRepositories($need_repositories) { | |||||
| $this->needRepositories = $need_repositories; | |||||
| return $this; | |||||
| } | |||||
| protected function loadPage() { | protected function loadPage() { | ||||
| $table = new DivinerLiveBook(); | $table = new DivinerLiveBook(); | ||||
| $conn_r = $table->establishConnection('r'); | $conn_r = $table->establishConnection('r'); | ||||
| $data = queryfx_all( | $data = queryfx_all( | ||||
| $conn_r, | $conn_r, | ||||
| 'SELECT * FROM %T %Q %Q %Q', | 'SELECT * FROM %T %Q %Q %Q', | ||||
| $table->getTableName(), | $table->getTableName(), | ||||
| $this->buildWhereClause($conn_r), | $this->buildWhereClause($conn_r), | ||||
| $this->buildOrderClause($conn_r), | $this->buildOrderClause($conn_r), | ||||
| $this->buildLimitClause($conn_r)); | $this->buildLimitClause($conn_r)); | ||||
| return $table->loadAllFromArray($data); | return $table->loadAllFromArray($data); | ||||
| } | } | ||||
| protected function didFilterPage(array $books) { | protected function didFilterPage(array $books) { | ||||
| assert_instances_of($books, 'DivinerLiveBook'); | assert_instances_of($books, 'DivinerLiveBook'); | ||||
Done Inline ActionsFor consistency, do this in willFilterPage(). epriestley: For consistency, do this in `willFilterPage()`. | |||||
| if ($this->needRepositories) { | |||||
Not Done Inline ActionsThis will technically work fine, but mpull($books, 'getRepositoryPHID') might return something like array(null, null, null). This might be slightly clearer (although also slightly longer) with more effort to handle that case. epriestley: This will technically work fine, but `mpull($books, 'getRepositoryPHID')` might return… | |||||
| $repositories = id(new PhabricatorRepositoryQuery()) | |||||
| ->setViewer($this->getViewer()) | |||||
| ->withPHIDs(mpull($books, 'getRepositoryPHID')) | |||||
| ->execute(); | |||||
| $repositories = mpull($repositories, null, 'getPHID'); | |||||
| foreach ($books as $key => $book) { | |||||
| if ($book->getRepositoryPHID() === null) { | |||||
Done Inline ActionsThis likely needs to $book->attachRepository(null) to avoid throwing when loading books with no repository and later calling getRepository() on them. (That will imply a corresponding PhabricatorRepository $repository = null default parameter value in attachRepository().) epriestley: This likely needs to `$book->attachRepository(null)` to avoid throwing when loading books with… | |||||
| $book->attachRepository(null); | |||||
| continue; | |||||
| } | |||||
| $repository = idx($repositories, $book->getRepositoryPHID()); | |||||
| if (!$repository) { | |||||
Not Done Inline ActionsPer that recent handle stuff, should didRejectResult() here. epriestley: Per that recent handle stuff, should `didRejectResult()` here. | |||||
| $this->didRejectResult($book); | |||||
| unset($books[$key]); | |||||
| continue; | |||||
| } | |||||
| $book->attachRepository($repository); | |||||
| } | |||||
| } | |||||
| if ($this->needProjectPHIDs) { | if ($this->needProjectPHIDs) { | ||||
| $edge_query = id(new PhabricatorEdgeQuery()) | $edge_query = id(new PhabricatorEdgeQuery()) | ||||
| ->withSourcePHIDs(mpull($books, 'getPHID')) | ->withSourcePHIDs(mpull($books, 'getPHID')) | ||||
| ->withEdgeTypes( | ->withEdgeTypes( | ||||
| array( | array( | ||||
| PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, | PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, | ||||
| )); | )); | ||||
| $edge_query->execute(); | $edge_query->execute(); | ||||
| Show All 29 Lines | protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { | ||||
| if ($this->names) { | if ($this->names) { | ||||
| $where[] = qsprintf( | $where[] = qsprintf( | ||||
| $conn_r, | $conn_r, | ||||
| 'name IN (%Ls)', | 'name IN (%Ls)', | ||||
| $this->names); | $this->names); | ||||
| } | } | ||||
| if ($this->repositoryPHIDs !== null) { | |||||
Done Inline ActionsPrefer !== null, so setRepositoryPHIDs($phids) is an error when $phids is array(), instead of an accidental query for all results. epriestley: Prefer `!== null`, so `setRepositoryPHIDs($phids)` is an error when `$phids` is `array()`… | |||||
| $where[] = qsprintf( | |||||
| $conn_r, | |||||
| 'repositoryPHID IN (%Ls)', | |||||
| $this->repositoryPHIDs); | |||||
| } | |||||
| $where[] = $this->buildPagingClause($conn_r); | $where[] = $this->buildPagingClause($conn_r); | ||||
| return $this->formatWhereClause($where); | return $this->formatWhereClause($where); | ||||
| } | } | ||||
| public function getQueryApplicationClass() { | public function getQueryApplicationClass() { | ||||
| return 'PhabricatorDivinerApplication'; | return 'PhabricatorDivinerApplication'; | ||||
| } | } | ||||
| } | } | ||||
Done Inline ActionsIf the repository fails to load, I think we should filter the book out (i.e., same deal as revisions: if you can't see a repository, you can't see its stuff). epriestley: If the repository fails to load, I think we should filter the book out (i.e., same deal as… | |||||
I think we should always load these and perform policy filtering; if you can't see a repository, you should not be able to see its books (similar to how revisions work).
(This might create some issues with generating public documentation from private repositories, but I don't think that's a major use case and it shouldn't be the default behavior.)