fix(search): validate comment ownership in format hook

Make sure comment has an owner before trying to render an icon

engine/tests/classes/Elgg/BaseTestCase.php

@@ -18,6 +18,7 @@ abstract class BaseTestCase extends TestCase implements Seedable, Testable {
 
 	use Testing;
 	use PluginTesting;
+	use HookTesting;
 
 	static $_instance;
 	static $_settings;

engine/tests/classes/Elgg/HookTesting.php

@@ -0,0 +1,20 @@
+<?php
+
+namespace Elgg;
+
+trait HookTesting {
+
+	/**
+	 * @return TestableHook
+	 */
+	public function registerTestingHook($name, $type, $handler, $priority = 500) {
+
+		$hook = new TestableHook();
+		$hook->name = $name;
+		$hook->type = $type;
+		$hook->handler = $handler;
+		$hook->priority = $priority;
+
+		return $hook->register($this);
+	}
+}

engine/tests/classes/Elgg/TestableHook.php

@@ -0,0 +1,158 @@
+<?php
+
+namespace Elgg;
+
+class TestableHook {
+
+	/**
+	 * @var string
+	 */
+	public $name;
+
+	/**
+	 * @var string
+	 */
+	public $type;
+
+	/**
+	 * @var callable
+	 */
+	public $handler;
+
+	/**
+	 * @var int
+	 */
+	public $priority = 500;
+
+	/**
+	 * @var BaseTestCase
+	 */
+	protected $test_case;
+
+	/**
+	 * @var int
+	 */
+	protected $calls = 0;
+
+	/**
+	 * @var Hook
+	 */
+	protected $before_state;
+
+	/**
+	 * @var mixed
+	 */
+	protected $result;
+
+	/**
+	 * @var Hook
+	 */
+	protected $after_state;
+
+	/**
+	 * Register a new testing hook handler
+	 *
+	 * @param BaseTestCase $test_case Test case
+	 *
+	 * @return TestableHook
+	 */
+	public function register(BaseTestCase $test_case) {
+		$this->test_case = $test_case;
+
+		elgg_register_plugin_hook_handler($this->name, $this->type, [$this, 'handler'], $this->priority);
+
+		return $this;
+	}
+
+	/**
+	 * Unregister handler
+	 * @return void
+	 */
+	public function unregister() {
+		elgg_unregister_plugin_hook_handler($this->name, $this->type, [$this, 'handler']);
+	}
+
+	/**
+	 * Handler hook call
+	 *
+	 * @param Hook $hook Hook
+	 *
+	 * @return mixed
+	 */
+	public function handler(\Elgg\Hook $hook) {
+		$this->calls++;
+		$this->before_state = $hook;
+
+		list($success, $return, $hook) = _elgg_services()->handlers->call($this->handler, $hook, [
+			$this->name,
+			$this->type,
+			$hook->getValue(),
+			$hook->getParams(),
+		]);
+
+		$this->after_state = $hook;
+
+		if ($return !== null) {
+			$this->result = $return;
+		} else {
+			$this->result = $hook->getValue();
+		}
+
+		return $return;
+	}
+
+	/**
+	 * @return mixed
+	 */
+	public function getResult() {
+		return $this->result;
+	}
+
+	/**
+	 * Assert that hook was called expected number of times
+	 *
+	 * @param int $expected Expectation
+	 */
+	public function assertNumberOfCalls($expected) {
+		$this->test_case->assertEquals($expected, $this->calls);
+	}
+
+	/**
+	 * Assert that before hook handler is called the named parameter had a specific value
+	 *
+	 * @param string $name  Name
+	 * @param mixed  $value Value
+	 */
+	public function assertParamBefore($name, $value) {
+		$this->test_case->assertEquals($this->before_state->getParam($name), $value);
+	}
+
+	/**
+	 * Assert that before hook handler is called the named parameter has a specific
+	 *
+	 * @param string $name  Name
+	 * @param mixed  $value Value
+	 */
+	public function assertParamAfter($name, $value) {
+		$this->test_case->assertEquals($this->after_state->getParam($name), $value);
+	}
+
+	/**
+	 * Assert hook value before the handler was called
+	 *
+	 * @param mixed $value Value
+	 */
+	public function assertValueBefore($value) {
+		$this->test_case->assertEquals($this->before_state->getValue(), $value);
+	}
+
+	/**
+	 * Assert hook value after the handler was called
+	 *
+	 * @param mixed $value Value
+	 */
+	public function assertValueAfter($value) {
+		$this->test_case->assertEquals($this->after_state->getValue(), $value);
+	}
+
+}

mod/search/classes/Elgg/Search/FormatComentEntityHook.php

@@ -29,10 +29,12 @@ public function __invoke(Hook $hook) {
 			return;
 		}
 
+		$icon = '';
 		$owner = $entity->getOwnerEntity();
-		$size = $hook->getParam('size', 'small');
-		$icon = elgg_view_entity_icon($owner, $size);
-
+		if ($owner) {
+			$size = $hook->getParam('size', 'small');
+			$icon = elgg_view_entity_icon($owner, $size);
+		}
 		$container = $entity->getContainerEntity();
 		if (!$container instanceof ElggEntity) {
 			return;

mod/search/classes/Elgg/Search/Formatter.php

@@ -144,43 +144,45 @@ protected function getPropertyMatches() {
 		}
 
 		$matches = [];
-		foreach ($fields as $property_type => $property_type_fields) {
-			foreach ($property_type_fields as $field) {
-				if (!empty($exclude[$property_type]) && in_array($field, $exclude[$property_type])) {
-					continue;
-				}
-
-				switch ($property_type) {
-					case 'attributes' :
-					case 'metadata' :
-						$property_values = $this->entity->$field;
-						break;
-
-					case 'annotations' :
-						$property_values = [];
-						$annotations = $this->entity->getAnnotations([
-							'annotation_names' => $field,
-							'limit' => 0,
-						]);
-						foreach ($annotations as $annotation) {
-							$property_values[] = $annotation->value;
-						}
-						break;
+		if (!empty($fields)) {
+			foreach ($fields as $property_type => $property_type_fields) {
+				foreach ($property_type_fields as $field) {
+					if (!empty($exclude[$property_type]) && in_array($field, $exclude[$property_type])) {
+						continue;
+					}
 
-					case 'private_settings' :
-						$property_values = $this->entity->getPrivateSetting($field);
-						break;
-				}
+					switch ($property_type) {
+						case 'attributes' :
+						case 'metadata' :
+							$property_values = $this->entity->$field;
+							break;
+
+						case 'annotations' :
+							$property_values = [];
+							$annotations = $this->entity->getAnnotations([
+								'annotation_names' => $field,
+								'limit' => 0,
+							]);
+							foreach ($annotations as $annotation) {
+								$property_values[] = $annotation->value;
+							}
+							break;
+
+						case 'private_settings' :
+							$property_values = $this->entity->getPrivateSetting($field);
+							break;
+					}
 
-				if (is_array($property_values)) {
-					foreach ($property_values as $text) {
-						if (stristr($text, $query)) {
-							$matches[$property_type][$field][] = $this->highlighter->highlight($text, 1, 300);
+					if (is_array($property_values)) {
+						foreach ($property_values as $text) {
+							if (stristr($text, $query)) {
+								$matches[$property_type][$field][] = $this->highlighter->highlight($text, 1, 300);
+							}
+						}
+					} else {
+						if (stristr($property_values, $query)) {
+							$matches[$property_type][$field][] = $this->highlighter->highlight($property_values, 1, 300);
 						}
-					}
-				} else {
-					if (stristr($property_values, $query)) {
-						$matches[$property_type][$field][] = $this->highlighter->highlight($property_values, 1, 300);
 					}
 				}
 			}

mod/search/tests/phpunit/unit/Elgg/Search/FormatCommentEntityHookTest.php

@@ -0,0 +1,58 @@
+<?php
+
+namespace Elgg\Search;
+use Elgg\IntegratedUnitTestCase;
+
+/**
+ * @group Search
+ * @group Hooks
+ */
+class FormatCommentEntityHookTest extends IntegratedUnitTestCase {
+
+	public function up() {
+		_elgg_services()->hooks->backup();
+	}
+
+	public function down() {
+		_elgg_services()->hooks->restore();
+	}
+
+	public function testCommentVolatileDataIsPopulated() {
+		$hook = $this->registerTestingHook('search:format', 'entity', FormatComentEntityHook::class);
+
+		$object = $this->createObject([], [
+			'title' => 'Container',
+		]);
+		$comment = $this->createObject([
+			'subtype' => 'comment',
+			'owner_guid' => 0,
+			'container_guid' => $object->guid,
+		], [
+			'title' => 'Comment',
+		]);
+
+		$search = new Search([
+			'query' => 'comment',
+			'search_type' => 'entities',
+			'entity_type' => 'object',
+			'entity_subtype' => 'comment',
+		]);
+
+		$formatter = new Formatter($comment, $search);
+		$formatter->format();
+
+		$hook->assertNumberOfCalls(1);
+
+		$comment = $hook->getResult();
+
+		$this->assertEmpty($comment->getVolatileData('search_matched_icon'));
+
+		$this->assertEquals(
+			elgg_echo('search:comment_on', ['Container']),
+			$comment->getVolatileData('search_matched_title')
+		);
+
+		$hook->unregister();
+
+	}
+}