Issue #2664016 by hchonov, gease, DuaelFr, morsok, tstoeckler, Daniele...

Issue #2664016 by hchonov, gease, DuaelFr, morsok, tstoeckler, Daniele Nicastro, alexpott, plach, larowlan, dawehner, cspitzlay: Adding a new batch set while the batch is running breaks batch order

core/includes/batch.inc

@@ -418,8 +418,10 @@ function &_batch_current_set() {
  */
 function _batch_next_set() {
   $batch = &batch_get();
-  if (isset($batch['sets'][$batch['current_set'] + 1])) {
-    $batch['current_set']++;
+  $set_indexes = array_keys($batch['sets']);
+  $current_set_index_key = array_search($batch['current_set'], $set_indexes);
+  if (isset($set_indexes[$current_set_index_key + 1])) {
+    $batch['current_set'] = $set_indexes[$current_set_index_key + 1];
     $current_set = &_batch_current_set();
     if (isset($current_set['form_submit']) && ($callback = $current_set['form_submit']) && is_callable($callback)) {
       // We use our stored copies of $form and $form_state to account for

core/includes/form.inc

@@ -776,18 +776,70 @@ function batch_set($batch_definition) {
       $batch['sets'][] = $batch_set;
     }
     else {
-      // The set is being added while the batch is running. Insert the new set
-      // right after the current one to ensure execution order, and store its
-      // operations in a queue.
-      $index = $batch['current_set'] + 1;
-      $slice1 = array_slice($batch['sets'], 0, $index);
-      $slice2 = array_slice($batch['sets'], $index);
-      $batch['sets'] = array_merge($slice1, [$batch_set], $slice2);
-      _batch_populate_queue($batch, $index);
+      // The set is being added while the batch is running.
+      _batch_append_set($batch, $batch_set);
     }
   }
 }
 
+/**
+ * Appends a batch set to a running batch.
+ *
+ * Inserts the new set right after the current one to ensure execution order,
+ * and stores its operations in a queue. If the current batch has already
+ * inserted a new set, additional sets will be inserted after the last inserted
+ * set.
+ *
+ * @param &$batch
+ *   The batch array.
+ * @param $batch_set
+ *   The batch set.
+ */
+function _batch_append_set(&$batch, $batch_set) {
+  $append_after_index = $batch['current_set'];
+  $reached_current_set = FALSE;
+  foreach ($batch['sets'] as $index => $set) {
+    // As the indexes are not ordered numerically we need to first reach the
+    // index of the current set and then search for the proper place to append
+    // the new batch set.
+    if (!$reached_current_set) {
+      if ($index == $batch['current_set']) {
+        $reached_current_set = TRUE;
+      }
+      continue;
+    }
+    if ($index > $append_after_index) {
+      if (isset($set['appended_after_index'])) {
+        $append_after_index = $index;
+      }
+      else {
+        break;
+      }
+    }
+  }
+  $batch_set['appended_after_index'] = $append_after_index;
+
+  // Iterate by reference over the existing batch sets and assign them by
+  // reference in the new batch sets array in order not to break a retrieved
+  // reference to the current set. Among other places a reference to the current
+  // set is being retrieved in _batch_process(). Additionally, we have to
+  // preserve the original indexes, as they are used to generate the queue name
+  // of each batch set, otherwise the operations of the new batch set will be
+  // queued in the queue of a previous batch set.
+  // @see _batch_populate_queue().
+  $new_sets = [];
+  foreach ($batch['sets'] as $index => &$set) {
+    $new_sets[$index] = &$set;
+    if ($index == $append_after_index) {
+      $new_set_index = count($batch['sets']);
+      $new_sets[$new_set_index] = $batch_set;
+    }
+  }
+
+  $batch['sets'] = $new_sets;
+  _batch_populate_queue($batch, $new_set_index);
+}
+
 /**
  * Processes the batch.
  *

core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc

@@ -15,7 +15,7 @@
  * Performs a simple batch operation.
  */
 function _batch_test_callback_1($id, $sleep, &$context) {
-  // No-op, but ensure the batch take a couple iterations.
+  // No-op, but ensure the batch takes a couple iterations.
   // Batch needs time to run for the test, so sleep a bit.
   usleep($sleep);
   // Track execution, and store some result for post-processing in the
@@ -39,7 +39,7 @@ function _batch_test_callback_2($start, $total, $sleep, &$context) {
   // Process by groups of 5 (arbitrary value).
   $limit = 5;
   for ($i = 0; $i < $limit && $context['sandbox']['count'] < $total; $i++) {
-    // No-op, but ensure the batch take a couple iterations.
+    // No-op, but ensure the batch takes a couple iterations.
     // Batch needs time to run for the test, so sleep a bit.
     usleep($sleep);
     // Track execution, and store some result for post-processing in the
@@ -63,7 +63,7 @@ function _batch_test_callback_2($start, $total, $sleep, &$context) {
  * Implements callback_batch_operation().
  */
 function _batch_test_callback_5($id, $sleep, &$context) {
-  // No-op, but ensure the batch take a couple iterations.
+  // No-op, but ensure the batch takes a couple iterations.
   // Batch needs time to run for the test, so sleep a bit.
   usleep($sleep);
   // Track execution, and store some result for post-processing in the
@@ -77,15 +77,50 @@ function _batch_test_callback_5($id, $sleep, &$context) {
 /**
  * Implements callback_batch_operation().
  *
- * Performs a batch operation setting up its own batch.
+ * Performs a simple batch operation.
+ */
+function _batch_test_callback_6($id, $sleep, &$context) {
+  // No-op, but ensure the batch takes a couple iterations.
+  // Batch needs time to run for the test, so sleep a bit.
+  usleep($sleep);
+  // Track execution, and store some result for post-processing in the
+  // 'finished' callback.
+  batch_test_stack("op 6 id $id");
+  $context['results'][6][] = $id;
+}
+
+/**
+ * Implements callback_batch_operation().
+ *
+ * Performs a simple batch operation.
  */
-function _batch_test_nested_batch_callback() {
-  batch_test_stack('setting up batch 2');
-  batch_set(_batch_test_batch_2());
+function _batch_test_callback_7($id, $sleep, &$context) {
+  // No-op, but ensure the batch takes a couple iterations.
+  // Batch needs time to run for the test, so sleep a bit.
+  usleep($sleep);
+  // Track execution, and store some result for post-processing in the
+  // 'finished' callback.
+  batch_test_stack("op 7 id $id");
+  $context['results'][7][] = $id;
 }
 
 /**
- * Provides a common 'finished' callback for batches 1 to 4.
+ * Implements callback_batch_operation().
+ *
+ * Performs a batch operation setting up its own batch(es).
+ */
+function _batch_test_nested_batch_callback(array $batches = []) {
+  foreach ($batches as $batch) {
+    batch_test_stack("setting up batch $batch");
+    $function = '_batch_test_batch_' . $batch;
+    batch_set($function());
+  }
+  \Drupal::state()
+    ->set('batch_test_nested_order_multiple_batches', batch_get());
+}
+
+/**
+ * Provides a common 'finished' callback for batches 1 to 7.
  */
 function _batch_test_finished_helper($batch_id, $success, $results, $operations) {
   if ($results) {
@@ -182,3 +217,21 @@ function _batch_test_finished_4($success, $results, $operations) {
 function _batch_test_finished_5($success, $results, $operations) {
   _batch_test_finished_helper(5, $success, $results, $operations);
 }
+
+/**
+ * Implements callback_batch_finished().
+ *
+ * Triggers 'finished' callback for batch 6.
+ */
+function _batch_test_finished_6($success, $results, $operations) {
+  _batch_test_finished_helper(6, $success, $results, $operations);
+}
+
+/**
+ * Implements callback_batch_finished().
+ *
+ * Triggers 'finished' callback for batch 7.
+ */
+function _batch_test_finished_7($success, $results, $operations) {
+  _batch_test_finished_helper(7, $success, $results, $operations);
+}

core/modules/system/tests/modules/batch_test/batch_test.module

@@ -24,6 +24,7 @@ function _batch_test_batch_0() {
     'operations' => [],
     'finished' => '_batch_test_finished_0',
     'file' => drupal_get_path('module', 'batch_test') . '/batch_test.callbacks.inc',
+    'batch_test_id' => 'batch_0',
   ];
   return $batch;
 }
@@ -46,6 +47,7 @@ function _batch_test_batch_1() {
     'operations' => $operations,
     'finished' => '_batch_test_finished_1',
     'file' => drupal_get_path('module', 'batch_test') . '/batch_test.callbacks.inc',
+    'batch_test_id' => 'batch_1',
   ];
   return $batch;
 }
@@ -67,6 +69,7 @@ function _batch_test_batch_2() {
     'operations' => $operations,
     'finished' => '_batch_test_finished_2',
     'file' => drupal_get_path('module', 'batch_test') . '/batch_test.callbacks.inc',
+    'batch_test_id' => 'batch_2',
   ];
   return $batch;
 }
@@ -98,6 +101,7 @@ function _batch_test_batch_3() {
     'operations' => $operations,
     'finished' => '_batch_test_finished_3',
     'file' => drupal_get_path('module', 'batch_test') . '/batch_test.callbacks.inc',
+    'batch_test_id' => 'batch_3',
   ];
   return $batch;
 }
@@ -119,7 +123,7 @@ function _batch_test_batch_4() {
   for ($i = 1; $i <= round($total / 2); $i++) {
     $operations[] = ['_batch_test_callback_1', [$i, $sleep]];
   }
-  $operations[] = ['_batch_test_nested_batch_callback', []];
+  $operations[] = ['_batch_test_nested_batch_callback', [[2]]];
   for ($i = round($total / 2) + 1; $i <= $total; $i++) {
     $operations[] = ['_batch_test_callback_1', [$i, $sleep]];
   }
@@ -127,6 +131,7 @@ function _batch_test_batch_4() {
     'operations' => $operations,
     'finished' => '_batch_test_finished_4',
     'file' => drupal_get_path('module', 'batch_test') . '/batch_test.callbacks.inc',
+    'batch_test_id' => 'batch_4',
   ];
   return $batch;
 }
@@ -149,6 +154,61 @@ function _batch_test_batch_5() {
     'operations' => $operations,
     'finished' => '_batch_test_finished_5',
     'file' => drupal_get_path('module', 'batch_test') . '/batch_test.callbacks.inc',
+    'batch_test_id' => 'batch_5',
+  ];
+  return $batch;
+}
+
+/**
+ * Batch 6: Repeats a simple operation.
+ *
+ * Operations: op 6 from 1 to 10.
+ */
+function _batch_test_batch_6() {
+  // Ensure the batch takes at least two iterations.
+  $total = 10;
+  $sleep = (1000000 / $total) * 2;
+
+  $operations = [];
+  for ($i = 1; $i <= $total; $i++) {
+    $operations[] = ['_batch_test_callback_6', [$i, $sleep]];
+  }
+  $batch = [
+    'operations' => $operations,
+    'finished' => '_batch_test_finished_6',
+    'file' => drupal_get_path('module', 'batch_test') . '/batch_test.callbacks.inc',
+    'batch_test_id' => 'batch_6',
+  ];
+  return $batch;
+}
+
+/**
+ * Batch 7: Performs two batches within a batch.
+ *
+ * Operations:
+ * - op 7 from 1 to 5,
+ * - set batch 5 (op 5 from 1 to 10, should run at the end before batch 2)
+ * - set batch 6 (op 6 from 1 to 10, should run at the end after batch 1)
+ * - op 7 from 6 to 10,
+ */
+function _batch_test_batch_7() {
+  // Ensure the batch takes at least two iterations.
+  $total = 10;
+  $sleep = (1000000 / $total) * 2;
+
+  $operations = [];
+  for ($i = 1; $i <= $total / 2; $i++) {
+    $operations[] = ['_batch_test_callback_7', [$i, $sleep]];
+  }
+  $operations[] = ['_batch_test_nested_batch_callback', [[6, 5]]];
+  for ($i = ($total / 2) + 1; $i <= $total; $i++) {
+    $operations[] = ['_batch_test_callback_7', [$i, $sleep]];
+  }
+  $batch = [
+    'operations' => $operations,
+    'finished' => '_batch_test_finished_7',
+    'file' => drupal_get_path('module', 'batch_test') . '/batch_test.callbacks.inc',
+    'batch_test_id' => 'batch_7',
   ];
   return $batch;
 }
@@ -187,13 +247,14 @@ function _batch_test_title_callback() {
  * Helper function: Stores or retrieves traced execution data.
  */
 function batch_test_stack($data = NULL, $reset = FALSE) {
+  $state = \Drupal::state();
   if ($reset) {
-    \Drupal::state()->delete('batch_test.stack');
+    $state->delete('batch_test.stack');
   }
   if (!isset($data)) {
-    return \Drupal::state()->get('batch_test.stack');
+    return $state->get('batch_test.stack');
   }
-  $stack = \Drupal::state()->get('batch_test.stack');
+  $stack = $state->get('batch_test.stack');
   $stack[] = $data;
-  \Drupal::state()->set('batch_test.stack', $stack);
+  $state->set('batch_test.stack', $stack);
 }

core/modules/system/tests/modules/batch_test/src/Form/BatchTestSimpleForm.php

@@ -32,7 +32,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
         'batch_2' => 'batch 2',
         'batch_3' => 'batch 3',
         'batch_4' => 'batch 4',
+        'batch_6' => 'batch 6',
+        'batch_7' => 'batch 7',
       ],
+      '#multiple' => TRUE,
     ];
     $form['submit'] = [
       '#type' => 'submit',
@@ -48,8 +51,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   public function submitForm(array &$form, FormStateInterface $form_state) {
     batch_test_stack(NULL, TRUE);
 
-    $function = '_batch_test_' . $form_state->getValue('batch');
-    batch_set($function());
+    foreach ($form_state->getValue('batch') as $batch) {
+      $function = '_batch_test_' . $batch;
+      batch_set($function());
+    }
 
     $form_state->setRedirect('batch_test.redirect');
   }

core/modules/system/tests/src/Functional/Batch/ProcessingTest.php

@@ -89,6 +89,31 @@ public function testBatchForm() {
     $this->assertBatchMessages($this->_resultMessages('batch_4'), 'Nested batch performed successfully.');
     $this->assertEqual(batch_test_stack(), $this->_resultStack('batch_4'), 'Execution order was correct.');
     $this->assertText('Redirection successful.', 'Redirection after batch execution is correct.');
+
+    // Submit batches 4 and 7. Batch 4 will trigger batch 2. Batch 7 will
+    // trigger batches 6 and 5.
+    $edit = ['batch' => ['batch_4', 'batch_7']];
+    $this->drupalPostForm('batch-test', $edit, 'Submit');
+    $this->assertSession()->assertNoEscaped('<');
+    $this->assertSession()->responseContains('Redirection successful.');
+    $this->assertBatchMessages($this->_resultMessages('batch_4'), 'Nested batch performed successfully.');
+    $this->assertBatchMessages($this->_resultMessages('batch_7'), 'Nested batch performed successfully.');
+    $expected_stack = array_merge($this->_resultStack('batch_4'), $this->_resultStack('batch_7'));
+    $this->assertEquals($expected_stack, batch_test_stack(), 'Execution order was correct.');
+    $batch = \Drupal::state()->get('batch_test_nested_order_multiple_batches');
+    $this->assertEquals(5, count($batch['sets']));
+    // Ensure correct queue mapping.
+    foreach ($batch['sets'] as $index => $batch_set) {
+      $this->assertEquals('drupal_batch:' . $batch['id'] . ':' . $index, $batch_set['queue']['name']);
+    }
+    // Ensure correct order of the nested batches. We reset the indexes in
+    // order to directly access the batches by their order.
+    $batch_sets = array_values($batch['sets']);
+    $this->assertEquals('batch_4', $batch_sets[0]['batch_test_id']);
+    $this->assertEquals('batch_2', $batch_sets[1]['batch_test_id']);
+    $this->assertEquals('batch_7', $batch_sets[2]['batch_test_id']);
+    $this->assertEquals('batch_6', $batch_sets[3]['batch_test_id']);
+    $this->assertEquals('batch_5', $batch_sets[4]['batch_test_id']);
   }
 
   /**
@@ -246,6 +271,25 @@ public function _resultStack($id, $value = 0) {
         }
         break;
 
+      case 'batch_6':
+        for ($i = 1; $i <= 10; $i++) {
+          $stack[] = "op 6 id $i";
+        }
+        break;
+
+      case 'batch_7':
+        for ($i = 1; $i <= 5; $i++) {
+          $stack[] = "op 7 id $i";
+        }
+        $stack[] = 'setting up batch 6';
+        $stack[] = 'setting up batch 5';
+        for ($i = 6; $i <= 10; $i++) {
+          $stack[] = "op 7 id $i";
+        }
+        $stack = array_merge($stack, $this->_resultStack('batch_6'));
+        $stack = array_merge($stack, $this->_resultStack('batch_5'));
+        break;
+
       case 'chained':
         $stack[] = 'submit handler 1';
         $stack[] = 'value = ' . $value;
@@ -295,6 +339,16 @@ public function _resultMessages($id) {
         $messages[] = 'results for batch 5<div class="item-list"><ul><li>op 5: processed 10 elements</li></ul></div>';
         break;
 
+      case 'batch_6':
+        $messages[] = 'results for batch 6<div class="item-list"><ul><li>op 6: processed 10 elements</li></ul></div>';
+        break;
+
+      case 'batch_7':
+        $messages[] = 'results for batch 7<div class="item-list"><ul><li>op 7: processed 10 elements</li></ul></div>';
+        $messages = array_merge($messages, $this->_resultMessages('batch_6'));
+        $messages = array_merge($messages, $this->_resultMessages('batch_5'));
+        break;
+
       case 'chained':
         $messages = array_merge($messages, $this->_resultMessages('batch_1'));
         $messages = array_merge($messages, $this->_resultMessages('batch_2'));