Refs #1242, Fixes #2775. Pulled common code between get_data() and get_data_row() into elgg_query_runner(). Caching is now done in the query runner instead of spread across execute_query() and get_data*(). Namespacing cache for callback and single row request. I'm open to better names for that function. Not closing #1242 because the elgg_get_entities*() functions can still return false.

git-svn-id: http://code.elgg.org/elgg/trunk@7849 36083f99-b078-4883-b0ff-0f9b5a30f544

2 files changed, 67 insertions, 88 deletions

diff --git a/engine/lib/database.php b/engine/lib/database.php
index da068effd..d02427029 100644
--- a/engine/lib/database.php
+++ b/engine/lib/database.php
@@ -244,14 +244,11 @@ function explain_query($query, $link) {
  * @throws DatabaseException
  */
 function execute_query($query, $dblink) {
-	global $CONFIG, $dbcalls, $DB_QUERY_CACHE;
+	global $CONFIG, $dbcalls;
 
 	$dbcalls++;
 
 	$result = mysql_query($query, $dblink);
-	if ($DB_QUERY_CACHE) {
-		$DB_QUERY_CACHE[$query] = -1; // Set initial cache to -1
-	}
 
 	if (mysql_errno($dblink)) {
 		throw new DatabaseException(mysql_error($dblink) . "\n\n QUERY: " . $query);
@@ -327,59 +324,14 @@ function execute_delayed_read_query($query, $handler = "") {
  * argument to $callback.  If no callback function is defined, the
  * entire result set is returned as an array.
  *
- * If no results are matched, FALSE is returned.
- *
  * @param mixed  $query    The query being passed.
  * @param string $callback Optionally, the name of a function to call back to on each row
  *
- * @return array|false An array of database result objects or callback function results or false
+ * @return array An array of database result objects or callback function results. If the query
+ *               returned nothing, an empty array.
  */
 function get_data($query, $callback = "") {
-	global $CONFIG, $DB_QUERY_CACHE;
-
-	// Is cached?
-	if ($DB_QUERY_CACHE) {
-		$cached_query = $DB_QUERY_CACHE[$query];
-	}
-
-	if ((isset($cached_query)) && ($cached_query)) {
-		elgg_log("$query results returned from cache");
-
-		if ($cached_query === -1) {
-			// Last time this query returned nothing, so return an empty array
-			return array();
-		}
-
-		return $cached_query;
-	}
-
-	$dblink = get_db_link('read');
-	$resultarray = array();
-
-	if ($result = execute_query("$query", $dblink)) {
-		while ($row = mysql_fetch_object($result)) {
-			if (!empty($callback) && is_callable($callback)) {
-				$row = $callback($row);
-			}
-			if ($row) {
-				$resultarray[] = $row;
-			}
-		}
-	}
-
-	if (empty($resultarray)) {
-		elgg_log("DB query \"$query\" returned no results.");
-		// @todo consider changing this to return empty array #1242
-		return FALSE;
-	}
-
-	// Cache result
-	if ($DB_QUERY_CACHE) {
-		$DB_QUERY_CACHE[$query] = $resultarray;
-		elgg_log("$query results cached");
-	}
-
-	return $resultarray;
+	return elgg_query_runner($query, $callback, false);
 }
 
 /**
@@ -395,47 +347,74 @@ function get_data($query, $callback = "") {
  * @return mixed A single database result object or the result of the callback function.
  */
 function get_data_row($query, $callback = "") {
+	return elgg_query_runner($query, $callback, true);
+}
+
+/**
+ * Handles returning data from a query, running it through a callback function,
+ * and caching the results.
+ *
+ * @access private
+ *
+ * @param string $query    The query to execute
+ * @param string $callback An optional callback function to run on each row
+ * @param bool   $single   Return only a single result?
+ *
+ * @return array An array of database result objects or callback function results. If the query
+ *               returned nothing, an empty array.
+ * @since 1.8
+ */
+function elgg_query_runner($query, $callback = null, $single = false) {
 	global $CONFIG, $DB_QUERY_CACHE;
 
-	// Is cached
-	if ($DB_QUERY_CACHE) {
-		$cached_query = $DB_QUERY_CACHE[$query];
-	}
+	// since we want to cache results of running the callback, we need to
+	// need to namespace the query with the callback, and single result request.
+	$hash = (string)$callback . (string)$single . $query;
 
-	if ((isset($cached_query)) && ($cached_query)) {
-		elgg_log("$query results returned from cache");
+	// Is cached?
+	if ($DB_QUERY_CACHE) {
+		$cached_query = $DB_QUERY_CACHE[$hash];
 
-		if ($cached_query === -1) {
-			// Last time this query returned nothing, so return false
-			//@todo fix me this should return array().
-			return FALSE;
+		if ($cached_query !== FALSE) {
+			elgg_log("$query results returned from cache (hash: $hash)");
+			return $cached_query;
 		}
-
-		return $cached_query;
 	}
 
 	$dblink = get_db_link('read');
+	$return = array();
 
 	if ($result = execute_query("$query", $dblink)) {
-		$row = mysql_fetch_object($result);
 
-		// Cache result (even if query returned no data)
-		if ($DB_QUERY_CACHE) {
-			$DB_QUERY_CACHE[$query] = $row;
-			elgg_log("$query results cached");
-		}
-
-		if (!empty($callback) && is_callable($callback)) {
+		// test for callback once instead of on each iteration.
+		// @todo check profiling to see if this needs to be broken out into
+		// explicit cases instead of checking in the interation.
+		$is_callable = is_callable($callback);
+		while ($row = mysql_fetch_object($result)) {
+			if ($is_callable) {
 				$row = $callback($row);
-		}
+			}
 
-		if ($row) {
-			return $row;
+			if ($single) {
+				$return = $row;
+				break;
+			} else {
+				$return[] = $row;
+			}
 		}
 	}
 
-	elgg_log("$query returned no results.");
-	return FALSE;
+	if (empty($return)) {
+		elgg_log("DB query \"$query\" returned no results.");
+	}
+
+	// Cache result
+	if ($DB_QUERY_CACHE) {
+		$DB_QUERY_CACHE[$hash] = $return;
+		elgg_log("$query results cached (hash: $hash)");
+	}
+
+	return $return;
 }
 
 /**
diff --git a/engine/tests/objects/entities.php b/engine/tests/objects/entities.php
index 56c8f7947..8e7e30c5b 100644
--- a/engine/tests/objects/entities.php
+++ b/engine/tests/objects/entities.php
@@ -118,7 +118,7 @@ class ElggCoreEntityTest extends ElggCoreUnitTest {
 		$this->assertIsA($annotations[0], 'ElggAnnotation');
 		$this->assertIdentical($annotations[0]->name, 'non_existent');
 		$this->assertEqual($this->entity->countAnnotations('non_existent'), 1);
-		
+
 		$this->assertIdentical($annotations, get_annotations($this->entity->getGUID()));
 		$this->assertIdentical($annotations, get_annotations($this->entity->getGUID(), 'site'));
 		$this->assertIdentical($annotations, get_annotations($this->entity->getGUID(), 'site', 'testing'));
@@ -127,10 +127,10 @@ class ElggCoreEntityTest extends ElggCoreUnitTest {
 		//  clear annotation
 		$this->assertTrue($this->entity->clearAnnotations());
 		$this->assertEqual($this->entity->countAnnotations('non_existent'), 0);
-		
-		$this->assertIdentical(FALSE, get_annotations($this->entity->getGUID()));
-		$this->assertIdentical(FALSE, get_annotations($this->entity->getGUID(), 'site'));
-		$this->assertIdentical(FALSE, get_annotations($this->entity->getGUID(), 'site', 'testing'));
+
+		$this->assertIdentical(array(), get_annotations($this->entity->getGUID()));
+		$this->assertIdentical(array(), get_annotations($this->entity->getGUID(), 'site'));
+		$this->assertIdentical(array(), get_annotations($this->entity->getGUID(), 'site', 'testing'));
 
 		// clean up
 		$this->assertTrue($this->entity->delete());
@@ -201,30 +201,30 @@ class ElggCoreEntityTest extends ElggCoreUnitTest {
 		$this->assertTrue($this->entity->enable());
 		$this->assertTrue($this->entity->delete());
 	}
-	
+
 	public function testElggEntityMetadata() {
 		// let's delte a non-existent metadata
 		$this->assertFalse($this->entity->clearMetaData('important'));
-		
+
 		// let's add the meatadata
 		$this->assertTrue($this->entity->important = 'indeed!');
 		$this->assertTrue($this->entity->less_important = 'true, too!');
 		$this->save_entity();
-		
+
 		// test deleting incorrectly
 		// @link http://trac.elgg.org/ticket/2273
 		$this->assertFalse($this->entity->clearMetaData('impotent'));
 		$this->assertEqual($this->entity->important, 'indeed!');
-		
+
 		// get rid of one metadata
 		$this->assertEqual($this->entity->important, 'indeed!');
 		$this->assertTrue($this->entity->clearMetaData('important'));
 		$this->assertEqual($this->entity->important, '');
-		
+
 		// get rid of all metadata
 		$this->assertTrue($this->entity->clearMetaData());
 		$this->assertEqual($this->entity->less_important, '');
-		
+
 		// clean up database
 		$this->assertTrue($this->entity->delete());
 	}