Fixes #4288. Added setIncrementOffset() to ElggBatch.

2 files changed, 137 insertions, 21 deletions

diff --git a/engine/classes/ElggBatch.php b/engine/classes/ElggBatch.php
index 3d01133fa..0cb13eb32 100644
--- a/engine/classes/ElggBatch.php
+++ b/engine/classes/ElggBatch.php
@@ -3,47 +3,51 @@
  * Efficiently run operations on batches of results for any function
  * that supports an options array.
  *
- * This is usually used with elgg_get_entities() and friends, elgg_get_annotations()
- * and elgg_get_metadata().
+ * This is usually used with elgg_get_entities() and friends,
+ * elgg_get_annotations(), and elgg_get_metadata().
  *
- * If you pass a valid PHP callback, all results will be run through that callback.
- * You can still foreach() through the result set after.  Valid PHP callbacks
- * can be a string, an array, or a closure.
+ * If you pass a valid PHP callback, all results will be run through that
+ * callback. You can still foreach() through the result set after.  Valid
+ * PHP callbacks can be a string, an array, or a closure.
  * {@link http://php.net/manual/en/language.pseudo-types.php}
  *
- * The callback function must accept 3 arguments: an entity, the getter used, and the options used.
+ * The callback function must accept 3 arguments: an entity, the getter
+ * used, and the options used.
  *
- * Results from the callback are stored in callbackResult.
- * If the callback returns only booleans, callbackResults will be the combined
- * result of all calls.
+ * Results from the callback are stored in callbackResult. If the callback
+ * returns only booleans, callbackResults will be the combined result of
+ * all calls.
  *
- * If the callback returns anything else, callbackresult will be an indexed array
- * of whatever the callback returns.  If returning error handling information,
- * you should include enough information to determine which result you're referring
- * to.
+ * If the callback returns anything else, callbackresult will be an indexed
+ * array of whatever the callback returns.  If returning error handling
+ * information, you should include enough information to determine which
+ * result you're referring to.
  *
  * Don't combine returning bools and returning something else.
  *
  * Note that returning false will not stop the foreach.
  *
+ * @warning If your callback or foreach loop deletes or disable entities
+ * you MUST call setIncrementOffset(false) or set that when instantiating.
+ * This forces the offset to stay what it was in the $options array.
+ *
  * @example
  * <code>
+ * // using foreach
  * $batch = new ElggBatch('elgg_get_entities', array());
+ * $batch->setIncrementOffset(false);
  *
  * foreach ($batch as $entity) {
  * 	$entity->disable();
  * }
  *
+ * // using both a callback
  * $callback = function($result, $getter, $options) {
- * 	var_dump("Going to delete annotation id: $result->id");
+ * 	var_dump("Looking at annotation id: $result->id");
  *  return true;
  * }
  *
  * $batch = new ElggBatch('elgg_get_annotations', array('guid' => 2), $callback);
- *
- * foreach ($batch as $annotation) {
- * 	$annotation->delete();
- * }
  * </code>
  *
  * @package    Elgg.Core
@@ -139,6 +143,13 @@ class ElggBatch
 	public $callbackResult = null;
 
 	/**
+	 * If false, offset will not be incremented. This is used for callbacks/loops that delete.
+	 *
+	 * @var bool
+	 */
+	private $incrementOffset = true;
+
+	/**
 	 * Batches operations on any elgg_get_*() or compatible function that supports
 	 * an options array.
 	 *
@@ -156,12 +167,18 @@ class ElggBatch
 	 * @param int    $chunk_size The number of entities to pull in before requesting more.
 	 *                           You have to balance this between running out of memory in PHP
 	 *                           and hitting the db server too often.
+	 * @param bool   $inc_offset Increment the offset on each fetch. This must be false for
+	 *                           callbacks that delete rows. You can set this after the
+	 *                           object is created with {@see ElggBatch::setIncrementOffset()}.
 	 */
-	public function __construct($getter, $options, $callback = null, $chunk_size = 25) {
+	public function __construct($getter, $options, $callback = null, $chunk_size = 25,
+			$inc_offset = true) {
+		
 		$this->getter = $getter;
 		$this->options = $options;
 		$this->callback = $callback;
 		$this->chunkSize = $chunk_size;
+		$this->setIncrementOffset($inc_offset);
 
 		if ($this->chunkSize <= 0) {
 			$this->chunkSize = 25;
@@ -174,7 +191,7 @@ class ElggBatch
 		// if passed a callback, create a new ElggBatch with the same options
 		// and pass each to the callback.
 		if ($callback && is_callable($callback)) {
-			$batch = new ElggBatch($getter, $options, null, $chunk_size);
+			$batch = new ElggBatch($getter, $options, null, $chunk_size, $inc_offset);
 
 			$all_results = null;
 
@@ -245,9 +262,15 @@ class ElggBatch
 			}
 		}
 
+		if ($this->incrementOffset) {
+			$offset = $this->offset + $this->retrievedResults;
+		} else {
+			$offset = $this->offset;
+		}
+
 		$current_options = array(
 			'limit' => $limit,
-			'offset' => $this->offset + $this->retrievedResults
+			'offset' => $offset
 		);
 
 		$options = array_merge($this->options, $current_options);
@@ -270,6 +293,16 @@ class ElggBatch
 	}
 
 	/**
+	 * Increment the offset from the original options array? Setting to
+	 * false is required for callbacks that delete rows.
+	 *
+	 * @param bool $increment
+	 */
+	public function setIncrementOffset($increment = true) {
+		$this->incrementOffset = (bool) $increment;
+	}
+
+	/**
 	 * Implements Iterator
 	 */
 
diff --git a/engine/tests/api/helpers.php b/engine/tests/api/helpers.php
index 77205138d..a615be0c0 100644
--- a/engine/tests/api/helpers.php
+++ b/engine/tests/api/helpers.php
@@ -518,4 +518,87 @@ class ElggCoreHelpersTest extends ElggCoreUnitTest {
 
 		$this->assertIdentical($elements_sorted_string, $test_elements);
 	}
+
+	// see http://trac.elgg.org/ticket/4288
+	public function testElggBatchIncOffset() {
+		// normal increment
+		$options = array(
+			'offset' => 0,
+			'limit' => 11
+		);
+		$batch = new ElggBatch(array('ElggCoreHelpersTest', 'test_elgg_batch_callback'), $options,
+				null, 5);
+		$j = 0;
+		foreach ($batch as $e) {
+			$offset = floor($j / 5) * 5;
+			$this->assertEqual($offset, $e['offset']);
+			$this->assertEqual($j + 1, $e['index']);
+			$j++;
+		}
+
+		$this->assertEqual(11, $j);
+
+		// no increment, 0 start
+		ElggCoreHelpersTest::test_elgg_batch_callback(array(), true);
+		$options = array(
+			'offset' => 0,
+			'limit' => 11
+		);
+		$batch = new ElggBatch(array('ElggCoreHelpersTest', 'test_elgg_batch_callback'), $options,
+				null, 5);
+		$batch->setIncrementOffset(false);
+
+		$j = 0;
+		foreach ($batch as $e) {
+			$this->assertEqual(0, $e['offset']);
+			// should always be the same 5
+			$this->assertEqual($e['index'], $j + 1 - (floor($j / 5) * 5));
+			$j++;
+		}
+		$this->assertEqual(11, $j);
+
+		// no increment, 3 start
+		ElggCoreHelpersTest::test_elgg_batch_callback(array(), true);
+		$options = array(
+			'offset' => 3,
+			'limit' => 11
+		);
+		$batch = new ElggBatch(array('ElggCoreHelpersTest', 'test_elgg_batch_callback'), $options,
+				null, 5);
+		$batch->setIncrementOffset(false);
+
+		$j = 0;
+		foreach ($batch as $e) {
+			$this->assertEqual(3, $e['offset']);
+			// same 5 results
+			$this->assertEqual($e['index'], $j + 4 - (floor($j / 5) * 5));
+			$j++;
+		}
+
+		$this->assertEqual(11, $j);
+	}
+
+	static function test_elgg_batch_callback($options, $reset = false) {
+		static $count = 1;
+
+		if ($reset) {
+			$count = 1;
+			return true;
+		}
+
+		if ($count > 20) {
+			return false;
+		}
+
+		for ($j = 0; ($options['limit'] < 5) ? $j < $options['limit'] : $j < 5; $j++) {
+			$return[] = array(
+				'offset' => $options['offset'],
+				'limit' => $options['limit'],
+				'count' => $count++,
+				'index' => 1 + $options['offset'] + $j
+			);
+		}
+
+		return $return;
+	}
 }
\ No newline at end of file