From fe6022cd97f53db60bd144429b5d258c9ffc5403 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 3 Oct 2012 15:42:44 -0700
Subject: [PATCH] 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 | 26 +++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php
index a1e65df2ac..86b61b58f2 100644
--- a/src/infrastructure/storage/lisk/LiskDAO.php
+++ b/src/infrastructure/storage/lisk/LiskDAO.php
@@ -656,11 +656,33 @@ abstract class LiskDAO {
    * @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;
     }