On portals, make the "selected" / "default" logic more straightforward

Summary:
Depends on D20349. Ref T13275. Currently, a default item is selected as a side effect of generating the full list of items, for absolutely no reason.

The logic to pick the currently selected item can also be separated out pretty easily.

(And fix a bug in with a weird edge case in projects.)

This doesn't really change anything, but it will probably make T12949 a bit easier to fix.

Test Plan: Viewed Home / projects / portals, clicked various links, got same default/selection behavior as before.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13275

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

src/applications/project/controller/PhabricatorProjectViewController.php

@@ -28,7 +28,7 @@ final class PhabricatorProjectViewController
       $default_key = PhabricatorProject::ITEM_MANAGE;
     }
 
-    switch ($default->getBuiltinKey()) {
+    switch ($default_key) {
       case PhabricatorProject::ITEM_WORKBOARD:
         $controller_object = new PhabricatorProjectBoardViewController();
         break;

src/applications/search/engine/PhabricatorProfileMenuEngine.php

@@ -6,7 +6,6 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
   private $profileObject;
   private $customPHID;
   private $items;
-  private $defaultItem;
   private $controller;
   private $navigation;
   private $showNavigation = true;
@@ -79,8 +78,7 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
   }
 
   public function getDefaultItem() {
-    $this->getItems();
+    return $this->pickDefaultItem($this->getItems());
-    return $this->defaultItem;
   }
 
   public function setShowNavigation($show) {
@@ -154,30 +152,10 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
 
     $item_list = $this->getItems();
 
-    $selected_item = null;
+    $selected_item = $this->pickSelectedItem(
-    if (strlen($item_id)) {
+      $item_list,
-      $item_id_int = (int)$item_id;
+      $item_id,
-      foreach ($item_list as $item) {
+      $is_view);
-        if ($item_id_int) {
-          if ((int)$item->getID() === $item_id_int) {
-            $selected_item = $item;
-            break;
-          }
-        }
-
-        $builtin_key = $item->getBuiltinKey();
-        if ($builtin_key === (string)$item_id) {
-          $selected_item = $item;
-          break;
-        }
-      }
-    }
-
-    if (!$selected_item) {
-      if ($is_view) {
-        $selected_item = $this->getDefaultItem();
-      }
-    }
 
     switch ($item_action) {
       case 'view':
@@ -485,39 +463,7 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
       }
     }
 
-    $items = $this->arrangeItems($items, $mode);
+    return $this->arrangeItems($items, $mode);
-
-    // Make sure exactly one valid item is marked as default.
-    $default = null;
-    $first = null;
-    foreach ($items as $item) {
-      if (!$item->canMakeDefault() || $item->isDisabled()) {
-        continue;
-      }
-
-      // If this engine doesn't support pinning items, don't respect any
-      // setting which might be present in the database.
-      if ($this->isMenuEnginePinnable()) {
-        if ($item->isDefault()) {
-          $default = $item;
-          break;
-        }
-      }
-
-      if ($first === null) {
-        $first = $item;
-      }
-    }
-
-    if (!$default) {
-      $default = $first;
-    }
-
-    if ($default) {
-      $this->setDefaultItem($default);
-    }
-
-    return $items;
   }
 
   private function loadBuiltinProfileItems($mode) {
@@ -1361,4 +1307,64 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
       pht('There are no menu items.'));
   }
 
+  private function pickDefaultItem(array $items) {
+    // Remove all the items which can not be the default item.
+    foreach ($items as $key => $item) {
+      if (!$item->canMakeDefault()) {
+        unset($items[$key]);
+        continue;
+      }
+
+      if ($item->isDisabled()) {
+        unset($items[$key]);
+        continue;
+      }
+    }
+
+    // If this engine supports pinning items and a valid item is pinned,
+    // pick that item as the default.
+    if ($this->isMenuEnginePinnable()) {
+      foreach ($items as $key => $item) {
+        if ($item->isDefault()) {
+          return $item;
+        }
+      }
+    }
+
+    // If we have some other valid items, pick the first one as the default.
+    if ($items) {
+      return head($items);
+    }
+
+    return null;
+  }
+
+  private function pickSelectedItem(array $items, $item_id, $is_view) {
+    if (strlen($item_id)) {
+      $item_id_int = (int)$item_id;
+      foreach ($items as $item) {
+        if ($item_id_int) {
+          if ((int)$item->getID() === $item_id_int) {
+            return $item;
+          }
+        }
+
+        $builtin_key = $item->getBuiltinKey();
+        if ($builtin_key === (string)$item_id) {
+          return $item;
+        }
+      }
+
+      // Nothing matches the selected item ID, so we don't have a valid
+      // selection.
+      return null;
+    }
+
+    if ($is_view) {
+      return $this->pickDefaultItem($items);
+    }
+
+    return null;
+  }
+
 }