Update core modules to use the new batch builder

Please let's do the sub issues here in one patch rather than multiple patches. They are the same task - see https://www.drupal.org/core/scope

Here's a combined patch from all the closed child issues. It'll probably fail tests and get back to Needs Work based on some of the child issues.

I added contributors from the child issues to the issue summary and removed the Meta since it's no longer a meta.

Adding some of my reviews:

I found that _locale_translation_batch_status_operations is a wrapper for generating multiple operations using the 'locale_translation_batch_status_check' callback. I think we may need to think of a way to keep adding multiple operations?

Maybe BatchBuilder should be able to add multiple operations in a new addOperations method?

1. +++ b/core/modules/locale/locale.compare.inc
   @@ -236,17 +237,14 @@ function locale_translation_batch_status_build($projects = [], $langcodes = [])
   +    ->addOperation('_locale_translation_batch_status_operations', [$projects, $langcodes, $options]);
   

   I'm not sure if this is the correct callback for this batch.

   _locale_translation_batch_status_operations() returns an array of operations based on some logic and the operation is locale_translation_batch_status_check.

   Additionally I think that we would need to add a @deprecated annotation into the _locale_translation_batch_status_operations function if it's no longer needed.

2. +++ b/core/modules/locale/locale.fetch.inc
   @@ -33,20 +35,19 @@ function locale_translation_batch_update_build($projects = [], $langcodes = [],
   -  $operations = array_merge($operations, _locale_translation_fetch_operations($projects, $langcodes, $options));
   +  $batch_builder->addOperation('_locale_translation_fetch_operations', [$projects, $langcodes, $options]);
   

   Same as above.

3. +++ b/core/modules/locale/locale.fetch.inc
   @@ -67,15 +68,14 @@ function locale_translation_batch_fetch_build($projects = [], $langcodes = [], $
   +    ->addOperation('_locale_translation_fetch_operations', [$projects, $langcodes, $options])
   

   Same as above. Operation is locale_translation_batch_status_check?

4. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -617,13 +618,14 @@ protected function triggerBatch(Request $request) {
   +      ->setTitle(t('Updating'))
   

   Missing a $this->t() instead of t().

5. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -617,13 +618,14 @@ protected function triggerBatch(Request $request) {
   +      ->addOperation($operations[])
   
   +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
   @@ -42,11 +43,11 @@ function _batch_test_batch_1() {
   +    ->addOperation($operations[])
   
   @@ -94,11 +92,11 @@ function _batch_test_batch_3() {
   +    ->addOperation($operations[])
   
   @@ -123,11 +121,11 @@ function _batch_test_batch_4() {
   +    ->addOperation($operations[])
   
   @@ -145,11 +143,11 @@ function _batch_test_batch_5() {
   +    ->addOperation($operations[])
   

   I think these need multiple addOperation calls unless BatchBuilder can accept multiple operations in a new method.

6. +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
   @@ -60,14 +61,11 @@ function _batch_test_batch_2() {
   +    ->addOperation(['_batch_test_callback_2', [1, $total, $sleep]])
   
   +++ b/core/modules/system/tests/modules/batch_test/src/Controller/BatchTestController.php
   @@ -47,9 +48,9 @@ public function testLargePercentage() {
   +      ->addOperation(['_batch_test_nested_drupal_form_submit_callback', [$value]])
   

   Shouldn't this be

   addOperation('_batch_test_Callback_2', [1, $total, $sleep]) instead?

+++ b/core/modules/system/tests/src/Unit/Batch/BatchBuilderTest.php
@@ -114,10 +114,10 @@ public function testSetErrorMessage() {
     $batch = (new BatchBuilder())
-      ->setFile('filename.php')
+      ->setFile(__DIR__ . '/../../../modules/batch_test/batch_test.set_file_test.inc')
       ->toArray();
 
-    $this->assertEquals('filename.php', $batch['file']);
+    $this->assertEquals(__DIR__ . '/../../../modules/batch_test/batch_test.set_file_test.inc', $batch['file']);

Let's put this file in core/modules/system/tests/fixtures/batch

it's not part of that test module really.

Also this test shouldn't be in system. It should be in core/tests/Drupal/Tests/Core/Batch - can you open an issue to move it there?

+++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
@@ -205,12 +205,17 @@ public function setErrorMessage($message) {
+   * The file needs to be set before using ::addOperation(),
+   * ::setFinishCallback(), or any other function that uses callbacks from the
+   * file. This is so that PHP knows about the included functions.
+   *
    * @param string $filename
    *   The path to the file.
    *
    * @return $this
    */
   public function setFile($filename) {
+    include_once $filename;

This change looks like it's fixing a bug in BatchBuilder itself. That's out of scope for this issue, surely?

Changing to NW for the question in #31.

Rerolled patch #36 for 9.1.x, please review.

Code sniffer has been updated..

Let's postpone this on #3028621: BatchBuilder included files fails

The blocker is in, re-queued a test.

+++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
@@ -411,19 +411,16 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-          'finished' => [ConfigImporterBatch::class, 'finish'],
...
+          ->setFinishCallback([ConfigSync::class, 'finishBatch'])
...
-          $batch['operations'][] = [[ConfigImporterBatch::class, 'process'], [$config_importer, $sync_step]];
+          $batch_builder->addOperation([ConfigSync::class, 'processBatch'], [$config_importer, $sync_step]);

Class/callbacks should not be changed.

+++ b/core/modules/system/tests/modules/batch_test/batch_test.module
@@ -119,21 +111,18 @@ function _batch_test_batch_4() {
-  $operations[] = ['_batch_test_nested_batch_callback', [[2]]];
+  $batch_builder->addOperation('_batch_test_nested_batch_callback');

Parameter lost

+++ b/core/modules/system/tests/modules/batch_test/batch_test.module
@@ -88,22 +82,20 @@ function _batch_test_batch_3() {
   $total = 10;
   $sleep = (1000000 / $total) * 2;
...
   for ($i = 1; $i <= round($total / 2); $i++) {
-    $operations[] = ['_batch_test_callback_1', [$i, $sleep]];

Room to improve/micro optimization in _batch_test_batch_*(). could do in a separate issue. Fewer iterations $total < 10, smaller $sleep if possible?

Added some missing replacements and removed the redundant variable.

@voleger, many thanks for fixing the tests.

Refactoring a bit to keep them consistent like below.

$batch_builder = (new BatchBuilder())
  // Call `setFile()` first, even it's not mandatory.
  ->setFile()
  // The rest.
  ...

batch_set($batch_builder->toArray());

Continue with #53

+++ b/core/modules/system/tests/modules/batch_test/batch_test.module
@@ -38,7 +37,6 @@ function _batch_test_batch_1() {
-  /** @var \Drupal\Core\Batch\BatchBuilder $batch_builder */

@@ -82,11 +80,9 @@ function _batch_test_batch_3() {
-  /** @var \Drupal\Core\Batch\BatchBuilder $batch_builder */

@@ -111,7 +108,6 @@ function _batch_test_batch_4() {
-  /** @var \Drupal\Core\Batch\BatchBuilder $batch_builder */

@@ -135,13 +132,13 @@ function _batch_test_batch_5() {
-  /** @var \Drupal\Core\Batch\BatchBuilder $batch_builder */

@@ -155,13 +152,13 @@ function _batch_test_batch_6() {
-  /** @var \Drupal\Core\Batch\BatchBuilder $batch_builder */

@@ -179,7 +176,6 @@ function _batch_test_batch_7() {
-  /** @var \Drupal\Core\Batch\BatchBuilder $batch_builder */

Type hints are unnecessary here. Unlike other variables, without it, autocomplete feature in PHPStorm still works.

#3028621 is relevant to this, which is a bug, so tagging Bug Smash Initiative as well.

Due to my interest in migrate I started at the migrate drupal ui form and reviewed to the end, missing the beginning of the patch. Now, my eyes have had enough so not looking at the first part of the patch.

The tests are passing and there are no coding standard notifications. Nice.

I noticed that the added use statement is inconsistently placed, sometimes it is in alphabetical order and sometimes not. I find it easier to skim that list if they are in alphabetical order so, if possible, let's make that consistent. Another inconsistency is the change from t() to $this->t(). I think changing those is out of scope for this issue so should be removed. Unless, of course, I am wrong about that.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -5,6 +5,7 @@
   +use Drupal\Core\Batch\BatchBuilder;
   

   Please keep in alphabetical order.

2. +++ b/core/modules/node/node.admin.inc
   @@ -6,6 +6,7 @@
   +use Drupal\Core\Batch\BatchBuilder;
   

   Can we keep this is alphabetical order?

3. +++ b/core/modules/node/node.admin.inc
   @@ -34,21 +35,15 @@ function node_mass_update(array $nodes, array $updates, $langcode = NULL, $load
   -      // We use a single multi-pass operation, so the default
   -      // 'Remaining x of y operations' message will be confusing here.
   -      'progress_message' => '',
   

   Why is this removed?

4. +++ b/core/modules/node/node.module
   @@ -1039,14 +1040,11 @@ function node_access_rebuild($batch_mode = FALSE) {
   +        ->addOperation('_node_access_rebuild_batch_operation')
   

   Why is the second parameter removed?

5. +++ b/core/modules/update/src/Controller/UpdateController.php
   @@ -59,16 +60,13 @@ public function updateStatus() {
   +      ->addOperation([$this->updateManager, 'fetchDataBatch'])
   

   Why is the second parameter removed?

6. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -12,6 +12,7 @@
   +use Drupal\Core\Batch\BatchBuilder;
   
   +++ b/core/modules/update/update.authorize.inc
   @@ -12,6 +12,7 @@
   +use Drupal\Core\Batch\BatchBuilder;
   

   Also out of alpha order.

7. +++ b/core/modules/update/update.authorize.inc
   @@ -91,25 +90,18 @@ function update_authorize_run_update($filetransfer, $projects) {
   -    // @todo Use a different finished callback for different messages?
   

   Should this todo stay?

8. +++ b/core/modules/user/user.module
   @@ -614,13 +615,6 @@ function user_cancel($edit, $uid, $method) {
   -  // Initialize batch (to set title).
   -  $batch = [
   -    'title' => t('Cancelling account'),
   -    'operations' => [],
   -  ];
   -  batch_set($batch);
   

   Why is this batch no longer needed?

@quietone, many thanks for your detailed review!

1. Fixed use statements order in 12 files related, including #56.1, 2 and 6
2. #56.3. added back
3. #56.4, 5, the 2nd parameter is optional, but added back.
   

     public function addOperation(callable $callback, array $arguments = []) {
     

4. #56.7 Added back.
5. #56.8 Added back.

6. Another inconsistency is the change from t() to $this->t()

   I think it's ok. in classes/traits we use $this->t() if possible, otherwise, use t().

+++ b/core/modules/user/user.module
@@ -615,11 +616,9 @@ function user_cancel($edit, $uid, $method) {
   // Initialize batch (to set title).
-  $batch = [
-    'title' => t('Cancelling account'),
-    'operations' => [],
-  ];
-  batch_set($batch);
+  $batch_builder = (new BatchBuilder())
+    ->setTitle(t('Cancelling account'));
+  batch_set($batch_builder->toArray());

@@ -630,22 +629,19 @@ function user_cancel($edit, $uid, $method) {
-  // Finish the batch and actually cancel the account.
-  $batch = [
-    'title' => t('Cancelling user account'),
-    'operations' => [
-      ['_user_cancel', [$edit, $account, $method]],
-    ],
-  ];
+  // Initialize batch (to set title).
+  $batch_builder = (new BatchBuilder())

Self review, the 2nd comment is wrong.

@jungle, oh, sorry. I think I didn't explain the alphabetization of the use statement clearly. I mean just add theuse Drupal\Core\Batch\BatchBuilder; in the 'best' alphabetical position possible. That is, where the use statements are already ordered it should be easy to add this new line. When this list is not order, do the best one can. Alphabetizing the whole list of use statements is out of scope.

Re #59 @quietone, no worries, however, as I already did, I do not think it's necessary to revert them, or if you think it's necessary, setting back to NW, please, thanks!

1. +++ b/core/modules/locale/locale.compare.inc
   @@ -260,20 +261,20 @@ function locale_translation_batch_status_build($projects = [], $langcodes = [])
   + * @internal
   

   Out of scope?

2. +++ b/core/modules/locale/locale.fetch.inc
   @@ -33,20 +35,19 @@ function locale_translation_batch_update_build($projects = [], $langcodes = [],
   +    ->setErrorMessage('Error importing translation files')
   ...
   +    ->setErrorMessage(t('Error importing translation files'));
   

   Duplicate setErrorMessage

3. +++ b/core/modules/locale/locale.fetch.inc
   @@ -33,20 +35,19 @@ function locale_translation_batch_update_build($projects = [], $langcodes = [],
   -  $operations = array_merge($operations, _locale_translation_fetch_operations($projects, $langcodes, $options));
   

   Is the effect of this array_merge replicated?

NW for #61 and #59 (to stay strictly in scope).

@quietone, @jonathanshaw, thanks for reviewing!

Addressing #59 and #61.1, 2.

Re #61.3. Should be ok, array_merge() is not used anymore.

  // Check status of local and remote translation files.
  _locale_translation_batch_status_operations($batch_builder, $projects, $langcodes, $status_options);
  // Download and import translations.
  _locale_translation_fetch_operations($batch_builder, $projects, $langcodes, $options);

And, BTW, $batch_builder is an object. no need to prepend &

+++ b/core/modules/node/node.admin.inc
@@ -34,21 +35,19 @@ function node_mass_update(array $nodes, array $updates, $langcode = NULL, $load
+    ;

Leftover, removing.

1. +++ b/core/modules/locale/locale.fetch.inc
   @@ -89,20 +90,28 @@ function locale_translation_batch_fetch_build($projects = [], $langcodes = [], $
   + * @internal
   

   out of scope?

2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -240,20 +241,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      'progress_message' => '',
   ...
   +    $batch_builder = (new BatchBuilder())
   +      ->setTitle($this->t('Running upgrade'))
   +      ->addOperation([MigrateUpgradeImportBatch::class, 'run'], [array_keys($this->migrations), $config])
   +      ->setFinishCallback([MigrateUpgradeImportBatch::class, 'finished']);
   +    batch_set($batch_builder->toArray());
   

   Missing setProgressMessage('')

Otherwise RTBC I think.

I've tried to read through it all and check every batch builder call perfectly duplicates the original.

@jonathanshaw many thanks for the great review!

Hopefully, this is the last iteration. I did a self-review as well.

Addressing #65

@jonathanshaw, thanks for RTBC-ing.

Uploading a patch for 8.9.x which should be applicable to 9.0.x as well,

1. +++ b/core/modules/locale/locale.compare.inc
   @@ -236,44 +237,42 @@ function locale_translation_batch_status_build($projects = [], $langcodes = [])
   -function _locale_translation_batch_status_operations($projects, $langcodes, $options = []) {
   -  $operations = [];
   -
   +function _locale_translation_batch_status_operations(BatchBuilder $batch_builder, $projects, $langcodes, $options = []) {
   
   +++ b/core/modules/locale/locale.fetch.inc
   @@ -88,21 +89,27 @@ function locale_translation_batch_fetch_build($projects = [], $langcodes = [], $
   -function _locale_translation_fetch_operations($projects, $langcodes, $options) {
   -  $operations = [];
   -
   +function _locale_translation_fetch_operations(BatchBuilder $batch_builder, $projects, $langcodes, $options) {
   

   These methods are marked @internal but I don't think we should optimise this here. We should continue to call them and do something like

   array_walk($operations, function ($operation) use ($batch_builder) {
   call_user_func_array([$batch_builder, 'addOperation'], $operation);
   }
   

   And then file a follow-up to do this - where can discus any BC implications properly. I've not found any contrib usages. So we might decide to go forward as is but having this as part of this change makes it harder to have that discussion.

2. +++ b/core/modules/locale/locale.compare.inc
   @@ -236,44 +237,42 @@ function locale_translation_batch_status_build($projects = [], $langcodes = [])
   -      $operations[] = ['locale_translation_batch_status_check', [$project, $langcode, $options]];
   +      $batch_builder->addOperation(
   +        'locale_translation_batch_status_check',
   +        [
   +          $project, $langcode, $options,
   +        ]
   +      );
   
   +++ b/core/modules/locale/locale.fetch.inc
   @@ -88,21 +89,27 @@ function locale_translation_batch_fetch_build($projects = [], $langcodes = [], $
   -        $operations[] = ['locale_translation_batch_fetch_download', [$project, $langcode]];
   +        $batch_builder->addOperation(
   +          'locale_translation_batch_fetch_download',
   +          [
   +            $project,
   +            $langcode,
   +          ]
   +        );
   ...
   -      $operations[] = ['locale_translation_batch_fetch_import', [$project, $langcode, $options]];
   +      $batch_builder->addOperation(
   +        'locale_translation_batch_fetch_import',
   +        [
   +          $project,
   +          $langcode,
   +          $options,
   +        ]
   +      );
   
   +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -604,7 +610,14 @@ protected function triggerBatch(Request $request) {
   -        $operations[] = ['update_do_one', [$update['module'], $update['number'], $dependency_map[$function]]];
   +        $batch_builder->addOperation(
   +          'update_do_one',
   +          [
   +            $update['module'],
   +            $update['number'],
   +            $dependency_map[$function],
   +          ]
   +        );
   

   These changes make it header to read than it was and in other places we've kept the single line. Why are we doing this here?

Note as a task and one that should not result in any underlying change this will only get committed to the latest development branch.

Thanks @alexpott!

#69.1, a great feedback!

Re #69.2

If the line declaring an array spans longer than 80 characters, each element should be broken into its own line

Per Drupal.Arrays.Array.LongLineDeclaration, for example, even it's an unapplied rule ATM. i am going to changing,

$batch_builder->addOperation(
+        'locale_translation_batch_fetch_import',
+        [
+          $project,
+          $langcode,
+          $options,
+        ]
+      );

to

$batch_builder->addOperation('locale_translation_batch_fetch_import', [
   $project,
   $langcode,
   $options,
]);

Instead of a single line.

$batch_builder->addOperation('locale_translation_batch_fetch_import', [$project, $langcode, $options]);

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -613,20 +626,13 @@ protected function triggerBatch(Request $request) {
-      $operations[] = ['drupal_flush_all_caches', []];
+      $batch_builder->addOperation('drupal_flush_all_caches');

Although, by default, the 2nd parameter is [], adding back still. And addressing #69.

Really nice to see the progress here. Just a few things I noticed.

In #51, this was tagged as needs followup. I could not find where the followup was made. Setting to NW.

I reviewed the patch and notice two inconsistencies, one is that sometimes t() is replaced with $this-t() and sometimes when an array spans longer that 80 characters it is broken into multiple lines and sometimes that is not done (also reported in $69). For me, both of those changes are out of scope and do add to the time it takes to review the patch. There is enough to review here that adding those changes, and done inconsistently, makes this harder to review.

1. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -411,19 +412,16 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +          $batch_builder->addOperation([ConfigImporterBatch::class, 'process'], [$config_importer, $sync_step]);
   

   There are many changes like this where the line is greater than 80 characters and the array is not split onto multiple lines. Which is correct here as that would be out of scope.

2. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -604,7 +610,11 @@ protected function triggerBatch(Request $request) {
   -        $operations[] = ['update_do_one', [$update['module'], $update['number'], $dependency_map[$function]]];
   +        $batch_builder->addOperation('update_do_one', [
   +          $update['module'],
   +          $update['number'],
   +          $dependency_map[$function],
   +        ]);
   

   And yet, as pointed out in #69 this one is changed. I think this is out of scope.

3. +++ b/core/modules/config/src/Form/ConfigSync.php
   @@ -364,19 +365,16 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +          ->setTitle($this->t('Synchronizing configuration'))
   ...
   +          ->setInitMessage($this->t('Starting configuration synchronization.'))
   +          ->setProgressMessage($this->t('Completed step @current of @total.'))
   +          ->setErrorMessage($this->t('Configuration synchronization has encountered an error.'));
   

   The changes to using $this->t are also out of scope.

4. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -180,19 +181,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      ->setTitle($this->t('Deleting @entity_type_plural', ['@entity_type_plural' => $entity_type_plural]))
   

   As above.

5. +++ b/core/modules/update/src/Controller/UpdateController.php
   @@ -59,16 +60,13 @@ public function updateStatus() {
   +      ->setTitle($this->t('Checking available update data'))
   ...
   +      ->setProgressMessage($this->t('Trying to check available update data ...'))
   +      ->setErrorMessage($this->t('Error checking available update data.'))
   

   As above.

Could have collision with #3215707: [META] Modernize Locale module

Used 2875279-71.patch for a reroll on 9.3.x in the new MR.

Not addressed anything from #72 (yet).

- Addressed #72.2
- Found the same issue in core/modules/config/src/Form/ConfigSync.php, addressed that as well.
- Addressed #72.5

Not sure if this is because of changes between the review and now, but I found that both #72.3 and #72.4 are currently in both before and after using $this->t.
So have not addressed those.

Back to NR.

I'm worried that creating a follow-up for #51 just clogs the issue queue with a task of minor benefit that will never get done. Optimizing the test suite is probably best done systematically using profiling to identify the best places to concentrate attention. It's a possible follow-up but not a NECESSARY one, so no "needs follow-up".

In which case this is RTBC AFAIK.

This looks good to me, but it needs a rebase.

Setting back to RTBC

Adding commit credit from the issue summary (thanks for including that).

Committed c5886a7 and pushed to 9.3.x. Thanks!

The follow-up is related, needs to get rid of batch_set() #2875151: [META] Implement Batch API as a service