Improve implementation of ManiphestTaskQuery

Summary:
Currently, we have a single `projectPHIDs` field, and a separate flag which makes it act like AND or OR.

This is silly. Make two separate methods for setting `AND` vs `OR` projects. This also simplifies the implmentation.

This doesn't change the UI or any behavior (yet), it just makes the API more usable.

Test Plan: Loaded homepage, "All Projects" task view, verified queries made sense and returned correct results. Grepped for changed method name.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1610

Differential Revision: https://secure.phabricator.com/D3630

src/applications/directory/controller/PhabricatorDirectoryMainController.php

@@ -247,8 +247,7 @@ final class PhabricatorDirectoryMainController
       $task_query = new ManiphestTaskQuery();
       $task_query->withStatus(ManiphestTaskQuery::STATUS_OPEN);
       $task_query->withPriority(ManiphestTaskPriority::PRIORITY_TRIAGE);
-      $task_query->withProjects(mpull($projects, 'getPHID'));
-      $task_query->withAnyProject(true);
+      $task_query->withAnyProjects(mpull($projects, 'getPHID'));
       $task_query->setLimit(10);
       $tasks = $task_query->execute();
     } else {

src/applications/maniphest/ManiphestTaskQuery.php

@@ -22,7 +22,7 @@
  *
  * @group maniphest
  */
-final class ManiphestTaskQuery {
+final class ManiphestTaskQuery extends PhabricatorQuery {
 
   private $taskIDs          = array();
   private $authorPHIDs      = array();
@@ -31,7 +31,7 @@ final class ManiphestTaskQuery {
   private $projectPHIDs     = array();
   private $xprojectPHIDs    = array();
   private $subscriberPHIDs  = array();
-  private $anyProject       = false;
+  private $anyProjectPHIDs  = array();
   private $includeNoProject = null;
 
   private $fullTextSearch   = '';
@@ -179,8 +179,8 @@ final class ManiphestTaskQuery {
     return $this->groupByProjectResults;
   }
 
-  public function withAnyProject($any_project) {
-    $this->anyProject = $any_project;
+  public function withAnyProjects(array $projects) {
+    $this->anyProjectPHIDs = $projects;
     return $this;
   }
 
@@ -203,18 +203,15 @@ final class ManiphestTaskQuery {
     $where[] = $this->buildOwnerWhereClause($conn);
     $where[] = $this->buildSubscriberWhereClause($conn);
     $where[] = $this->buildProjectWhereClause($conn);
+    $where[] = $this->buildAnyProjectWhereClause($conn);
     $where[] = $this->buildXProjectWhereClause($conn);
     $where[] = $this->buildFullTextWhereClause($conn);
 
-    $where = array_filter($where);
-    if ($where) {
-      $where = 'WHERE ('.implode(') AND (', $where).')';
-    } else {
-      $where = '';
-    }
+    $where = $this->formatWhereClause($where);
 
     $join = array();
     $join[] = $this->buildProjectJoinClause($conn);
+    $join[] = $this->buildAnyProjectJoinClause($conn);
     $join[] = $this->buildXProjectJoinClause($conn);
     $join[] = $this->buildSubscriberJoinClause($conn);
 
@@ -228,25 +225,25 @@ final class ManiphestTaskQuery {
     $having = '';
     $count = '';
     $group = '';
-    if (count($this->projectPHIDs) > 1) {
-
-      // If we're searching for more than one project:
-      //  - We'll get multiple rows for tasks when they join the project table
-      //    multiple times. We use GROUP BY to make them distinct again.
-      //  - We want to treat the query as an intersection query, not a union
-      //    query. We sum the project count and require it be the same as the
-      //    number of projects we're searching for. (If 'anyProject' is set,
-      //    we do union instead.)
 
+    if (count($this->projectPHIDs) > 1 || count($this->anyProjectPHIDs) > 1) {
+      // If we're joining multiple rows, we need to group the results by the
+      // task IDs.
       $group = 'GROUP BY task.id';
+    } else {
+      $group = '';
+    }
 
-      if (!$this->anyProject) {
-        $count = ', COUNT(project.projectPHID) projectCount';
-        $having = qsprintf(
-          $conn,
-          'HAVING projectCount = %d',
-          count($this->projectPHIDs));
-      }
+    if (count($this->projectPHIDs) > 1) {
+      // We want to treat the query as an intersection query, not a union
+      // query. We sum the project count and require it be the same as the
+      // number of projects we're searching for.
+
+      $count = ', COUNT(project.projectPHID) projectCount';
+      $having = qsprintf(
+        $conn,
+        'HAVING projectCount = %d',
+        count($this->projectPHIDs));
     }
 
     $order = $this->buildOrderClause($conn);
@@ -456,6 +453,29 @@ final class ManiphestTaskQuery {
       $project_dao->getTableName());
   }
 
+  private function buildAnyProjectWhereClause($conn) {
+    if (!$this->anyProjectPHIDs) {
+      return null;
+    }
+
+    return qsprintf(
+      $conn,
+      'anyproject.projectPHID IN (%Ls)',
+      $this->anyProjectPHIDs);
+  }
+
+  private function buildAnyProjectJoinClause($conn) {
+    if (!$this->anyProjectPHIDs) {
+      return null;
+    }
+
+    $project_dao = new ManiphestTaskProject();
+    return qsprintf(
+      $conn,
+      'LEFT JOIN %T anyproject ON anyproject.taskPHID = task.phid',
+      $project_dao->getTableName());
+  }
+
   private function buildXProjectWhereClause($conn) {
     if (!$this->xprojectPHIDs) {
       return null;
@@ -592,7 +612,7 @@ final class ManiphestTaskQuery {
     $ii = 0;
     foreach ($tasks as $key => $task) {
       $phids = $task->getProjectPHIDs();
-      if (!$this->anyProject && $this->projectPHIDs) {
+      if ($this->projectPHIDs) {
         $phids = array_diff($phids, $this->projectPHIDs);
       }
       if ($phids) {

src/applications/maniphest/controller/ManiphestTaskListController.php

@@ -406,8 +406,11 @@ final class ManiphestTaskListController extends ManiphestController {
     $any_project = false;
     $search_text = $search_query->getParameter('fullTextSearch');
     $user_phids = $search_query->getParameter('userPHIDs', array());
-    $project_phids = $search_query->getParameter('projectPHIDs', array());
     $task_ids = $search_query->getParameter('taskIDs', array());
+    $project_phids = $search_query->getParameter('projectPHIDs', array());
+    $any_project_phids = $search_query->getParameter(
+      'anyProjectPHIDs',
+      array());
     $xproject_phids = $search_query->getParameter(
       'excludeProjectPHIDs',
       array());
@@ -467,18 +470,16 @@ final class ManiphestTaskListController extends ManiphestController {
         break;
       case 'projecttriage':
         $query->withPriority(ManiphestTaskPriority::PRIORITY_TRIAGE);
-        $any_project = true;
+        $query->withAnyProjects($any_project_phids);
         break;
       case 'projectall':
-        $any_project = true;
+        $query->withAnyProjects($any_project_phids);
         break;
       case 'custom':
         $query->withPrioritiesBetween($low_priority, $high_priority);
         break;
     }
 
-    $query->withAnyProject($any_project);
-
     $query->withFullTextSearch($search_text);
 
     $order_map = array(
@@ -689,15 +690,16 @@ final class ManiphestTaskListController extends ManiphestController {
       array($user->getPHID()));
 
     if ($this->view == 'projecttriage' || $this->view == 'projectall') {
-      $project_query = new PhabricatorProjectQuery();
-      $project_query->setViewer($user);
-      $project_query->withMemberPHIDs($user_phids);
-      $projects = $project_query->execute();
-      $project_phids = mpull($projects, 'getPHID');
+      $projects = id(new PhabricatorProjectQuery())
+        ->setViewer($user)
+        ->withMemberPHIDs($user_phids)
+        ->execute();
+      $any_project_phids = mpull($projects, 'getPHID');
     } else {
-      $project_phids = $request->getStrList('projects');
+      $any_project_phids = $request->getStrList('aprojects');
     }
 
+    $project_phids = $request->getStrList('projects');
     $exclude_project_phids = $request->getStrList('xprojects');
     $task_ids = $request->getStrList('tasks');
 
@@ -739,6 +741,7 @@ final class ManiphestTaskListController extends ManiphestController {
         'view'                => $this->view,
         'userPHIDs'           => $user_phids,
         'projectPHIDs'        => $project_phids,
+        'anyProjectPHIDs'     => $any_project_phids,
         'excludeProjectPHIDs' => $exclude_project_phids,
         'ownerPHIDs'          => $owner_phids,
         'authorPHIDs'         => $author_phids,

src/applications/project/controller/PhabricatorProjectListController.php

@@ -84,8 +84,7 @@ final class PhabricatorProjectListController
     $groups = array();
     if ($project_phids) {
       $query = id(new ManiphestTaskQuery())
-        ->withProjects($project_phids)
-        ->withAnyProject(true)
+        ->withAnyProjects($project_phids)
         ->withStatus(ManiphestTaskQuery::STATUS_OPEN)
         ->setLimit(PHP_INT_MAX);