ElggBatch with incrementOffset off now handles incomplete entities

2 files changed, 80 insertions, 14 deletions

diff --git a/engine/classes/ElggBatch.php b/engine/classes/ElggBatch.php
index 83963ccee..ac79cf084 100644
--- a/engine/classes/ElggBatch.php
+++ b/engine/classes/ElggBatch.php
@@ -242,9 +242,12 @@ class ElggBatch
 	/**
 	 * Fetches the next chunk of results
 	 *
+	 * @param int $num_incompletes_last_fetch When called recursively, this is the number of
+	 *                                        incomplete entities returned in the last fetch.
+	 *
 	 * @return bool
 	 */
-	private function getNextResultsChunk() {
+	private function getNextResultsChunk($num_incompletes_last_fetch = 0) {
 
 		// always reset results.
 		$this->results = array();
@@ -278,7 +281,7 @@ class ElggBatch
 		if ($this->incrementOffset) {
 			$offset = $this->offset + $this->retrievedResults;
 		} else {
-			$offset = $this->offset;
+			$offset = $this->offset + $num_incompletes_last_fetch;
 		}
 
 		$current_options = array(
@@ -292,17 +295,30 @@ class ElggBatch
 		$this->incompleteEntities = array();
 		$this->results = call_user_func_array($this->getter, array($options));
 
-		// If there were incomplete entities, we pretend they were at the beginning of the results,
-		// fool the local counter to think it's skipped by them already, and update the running
-		// total as if the results contained the incompletes.
-		if ($this->results || $this->incompleteEntities) {
+		$num_results = count($this->results);
+		$num_incomplete = count($this->incompleteEntities);
+
+		if ($this->incompleteEntities) {
+			// pad the front of the results with nulls representing the incompletes
+			array_splice($this->results, 0, 0, array_pad(array(), $num_incomplete, null));
+			// ...and skip past them
+			reset($this->results);
+			for ($i = 0; $i < $num_incomplete; $i++) {
+				next($this->results);
+			}
+		}
+
+		if ($this->results) {
 			$this->chunkIndex++;
-			$this->resultIndex = count($this->incompleteEntities);
-			$this->retrievedResults += (count($this->results) + count($this->incompleteEntities));
-			if (!$this->results) {
+
+			// let the system know we've jumped past the nulls
+			$this->resultIndex = $num_incomplete;
+
+			$this->retrievedResults += ($num_results + $num_incomplete);
+			if ($num_results == 0) {
 				// This fetch was *all* incompletes! We need to fetch until we can either
 				// offer at least one row to iterate over, or give up.
-				return $this->getNextResultsChunk();
+				return $this->getNextResultsChunk($num_incomplete);
 			}
 			return true;
 		} else {
diff --git a/engine/tests/api/helpers.php b/engine/tests/api/helpers.php
index 43244636b..06ef55138 100644
--- a/engine/tests/api/helpers.php
+++ b/engine/tests/api/helpers.php
@@ -578,16 +578,14 @@ class ElggCoreHelpersTest extends ElggCoreUnitTest {
 		$this->assertEqual(11, $j);
 	}
 
-	public function testElggBatchHandlesBrokenEntities() {
+	public function testElggBatchReadHandlesBrokenEntities() {
 		$num_test_entities = 6;
 		$guids = array();
-		$now = time();
 		for ($i = $num_test_entities; $i > 0; $i--) {
 			$entity = new ElggObject();
 			$entity->type = 'object';
 			$entity->subtype = 'test_5357_subtype';
 			$entity->access_id = ACCESS_PUBLIC;
-			$entity->time_created = ($now - $i);
 			$entity->save();
 			$guids[] = $entity->guid;
 			_elgg_invalidate_cache_for_entity($entity->guid);
@@ -604,11 +602,12 @@ class ElggCoreHelpersTest extends ElggCoreUnitTest {
 		$options = array(
 			'type' => 'object',
 			'subtype' => 'test_5357_subtype',
-			'order_by' => 'e.time_created ASC',
+			'order_by' => 'e.guid',
 		);
 
 		$entities_visited = array();
 		$batch = new ElggBatch('elgg_get_entities', $options, null, 2);
+		/* @var ElggEntity[] $batch */
 		foreach ($batch as $entity) {
 			$entities_visited[] = $entity->guid;
 		}
@@ -629,6 +628,57 @@ class ElggCoreHelpersTest extends ElggCoreUnitTest {
 		delete_data("DELETE FROM {$db_prefix}objects_entity WHERE guid IN (" . implode(',', $guids) . ")");
 	}
 
+	public function testElggBatchDeleteHandlesBrokenEntities() {
+		$num_test_entities = 6;
+		$guids = array();
+		for ($i = $num_test_entities; $i > 0; $i--) {
+			$entity = new ElggObject();
+			$entity->type = 'object';
+			$entity->subtype = 'test_5357_subtype';
+			$entity->access_id = ACCESS_PUBLIC;
+			$entity->save();
+			$guids[] = $entity->guid;
+			_elgg_invalidate_cache_for_entity($entity->guid);
+		}
+
+		// break entities such that the first fetch has one incomplete
+		// and the second fetch has only incompletes!
+		$db_prefix = elgg_get_config('dbprefix');
+		delete_data("
+			DELETE FROM {$db_prefix}objects_entity
+			WHERE guid IN ({$guids[1]}, {$guids[2]}, {$guids[3]})
+		");
+
+		$options = array(
+			'type' => 'object',
+			'subtype' => 'test_5357_subtype',
+			'order_by' => 'e.guid',
+		);
+
+		$entities_visited = array();
+		$batch = new ElggBatch('elgg_get_entities', $options, null, 2, false);
+		/* @var ElggEntity[] $batch */
+		foreach ($batch as $entity) {
+			$entities_visited[] = $entity->guid;
+			$entity->delete();
+		}
+
+		// The broken entities should not have been visited
+		$this->assertEqual($entities_visited, array($guids[0], $guids[4], $guids[5]));
+
+		// cleanup (including leftovers from previous tests)
+		$entity_rows = elgg_get_entities(array_merge($options, array(
+			'callback' => '',
+			'limit' => false,
+		)));
+		$guids = array();
+		foreach ($entity_rows as $row) {
+			$guids[] = $row->guid;
+		}
+		delete_data("DELETE FROM {$db_prefix}entities WHERE guid IN (" . implode(',', $guids) . ")");
+		delete_data("DELETE FROM {$db_prefix}objects_entity WHERE guid IN (" . implode(',', $guids) . ")");
+	}
+
 	static function elgg_batch_callback_test($options, $reset = false) {
 		static $count = 1;