fix(database): Uncallable callback arguments now throw exceptions

Fixes #6937
Elgg · Apr 10, 2015 · 1e65aa1 · 1e65aa1
1 parent 7ae9194
commit 1e65aa1
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 10 deletions.
diff --git a/docs/guides/upgrading.rst b/docs/guides/upgrading.rst
@@ -12,6 +12,14 @@ See the administrator guides for :doc:`how to upgrade a live site </admin/upgrad
 From 1.11 to 2.0
 ================
 
+Callbacks in Queries
+--------------------
+
+Make sure to use only valid *callable* values for "callback" argument/options in the API.
+
+Querying functions will now will throw a ``RuntimeException`` if ``is_callable()`` returns ``false`` for the given
+callback value. This includes functions such as ``elgg_get_entities()``, ``get_data()``, and many more.
+
 Breadcrumbs
 -----------
 

diff --git a/engine/classes/Elgg/Database.php b/engine/classes/Elgg/Database.php
@@ -322,15 +322,11 @@ protected function getResults($query, $callback = null, $single = false) {
 		// https://github.com/elgg/elgg/issues/4049
 		$query_id = (int)$single . $query . '|';
 		if ($callback) {
-			$is_callable = is_callable($callback);
-			if ($is_callable) {
-				$query_id .= $this->fingerprintCallback($callback);
-			} else {
-				// TODO do something about invalid callbacks
-				$callback = null;
+			if (!is_callable($callback)) {
+				$inspector = new \Elgg\Debug\Inspector();
+				throw new \RuntimeException('$callback must be a callable function. Given ' . $inspector->describeCallable($callback));
 			}
-		} else {
-			$is_callable = false;
+			$query_id .= $this->fingerprintCallback($callback);
 		}
 		// MD5 yields smaller mem usage for cache and cleaner logs
 		$hash = md5($query_id);
@@ -348,7 +344,7 @@ protected function getResults($query, $callback = null, $single = false) {
 
 		if ($result = $this->executeQuery("$query", $dblink)) {
 			while ($row = mysql_fetch_object($result)) {
-				if ($is_callable) {
+				if ($callback) {
 					$row = call_user_func($callback, $row);
 				}
 

diff --git a/engine/classes/Elgg/Debug/Inspector.php b/engine/classes/Elgg/Debug/Inspector.php
@@ -344,7 +344,7 @@ public function describeCallable($callable, $file_root = '') {
 		if (is_object($callable)) {
 			return "(" . get_class($callable) . ")->__invoke()";
 		}
-		return "(unknown)";
+		return print_r($callable, true);
 	}
 
 	/**

diff --git a/engine/tests/phpunit/Elgg/DatabaseTest.php b/engine/tests/phpunit/Elgg/DatabaseTest.php
@@ -95,6 +95,12 @@ public function testFingerprintingOfCallbacks() {
 		$this->assertEquals($uniques, count($prints));
 	}
 
+	public function testInvalidCallbacksThrow() {
+		$this->setExpectedException('RuntimeException', '$callback must be a callable function. Given blorg!');
+
+		$this->getDbMock()->getData("SELECT 1", 'blorg!');
+	}
+
 	private function getFixture($filename) {
 		return dirname(__FILE__) .
 			DIRECTORY_SEPARATOR . '..' .