Load only valid properties in LiskDAO::loadFromArray()

Summary:
Fixes a TODO, and silences a warning introduced by D3601.

There are several cases where we load data like:

  SELECT *, ... AS extraData FROM ...

...and then pass it to `loadAllFromArray()`. Currently, this causes us to set an `extraData` property on the object.

This idiom seems fairly useful and non-dangerous, so I made `loadFromArray()` just drop extra keys.

Since we hit this loop a potentially huge number of times (10,000+ for full Maniphest pages) I did some microoptimization. Lisk is hot enough that it's one of the few places where it's worthwhile (see D1291).

Test Plan: Loaded homepage, no longer got warnings about `viewerIsMember` from Project queries. Browsed ~10 apps, didn't see any issues.

Reviewers: vrana

Reviewed By: vrana

CC: aran

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

src/infrastructure/storage/lisk/LiskDAO.php

@@ -656,11 +656,33 @@ public function reload() {
    * @task   load
    */
   public function loadFromArray(array $row) {
-
-    // TODO: We should load only valid properties.
+    static $valid_properties = array();
 
     $map = array();
     foreach ($row as $k => $v) {
+      // We permit (but ignore) extra properties in the array because a
+      // common approach to building the array is to issue a raw SELECT query
+      // which may include extra explicit columns or joins.
+
+      // This pathway is very hot on some pages, so we're inlining a cache
+      // and doing some microoptimization to avoid a strtolower() call for each
+      // assignment. The common path (assigning a valid property which we've
+      // already seen) always incurs only one empty(). The second most common
+      // path (assigning an invalid property which we've already seen) costs
+      // an empty() plus an isset().
+
+      if (empty($valid_properties[$k])) {
+        if (isset($valid_properties[$k])) {
+          // The value is set but empty, which means it's false, so we've
+          // already determined it's not valid. We don't need to check again.
+          continue;
+        }
+        $valid_properties[$k] = (bool)$this->checkProperty($k);
+        if (!$valid_properties[$k]) {
+          continue;
+        }
+      }
+
       $map[$k] = $v;
     }