Make more explicit what the operations array index passed to batch_set() must contain

Created on 5 June 2023, over 1 year ago
Updated 21 April 2024, 9 months ago

Problem/Motivation

Changes to the call_user_func_array() function require that the args array is presented with positional arguments first, followed by named arguments.

Steps to reproduce

Consider a batch callback function with the following signature:

function lovely_module_batch_process(?string $product_type, array &$context): void

The following part of batch.inc contains the PHP 8 incompatible-code

      list($function, $args) = $item->data;

      // Build the 'context' array and execute the function call.
      $batch_context = array(
        'sandbox'  => &$current_set['sandbox'],
        'results'  => &$current_set['results'],
        'finished' => &$finished,
        'message'  => &$task_message,
      );
      call_user_func_array($function, array_merge($args, array(&$batch_context)));

Where if a named argument is supplied from $item->data, this is then spliced with the context array and fails.

Proposed resolution

Replacing the reflective call to the callback fixes the issue:

call_user_func_array($function, array_merge($args, array('context' => &$batch_context)));

However this relies on the context array being named context. This appears to be the widely-followed convention; however there is a risk that some modules use different signatures that could be affected by this fix.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

7.0 ⚰️

Component
Batch 

Last updated about 7 hours ago

Created by

🇬🇧United Kingdom davidgould

Live updates comments and jobs are added and updated live.
  • PHP 8.0

    The issue particularly affects sites running on PHP version 8.0.0 or later.

Sign in to follow issues

Comments & Activities

  • Issue created by @davidgould
  • 🇮🇹Italy apaderno Brescia, 🇮🇹
  • 🇮🇹Italy apaderno Brescia, 🇮🇹

    The correct code that uses batch_set() is similar to the following one. (It has been taken from node_mass_update().)

        $batch = array(
          'operations' => array(array('_node_mass_update_batch_process', array($nodes, $updates))),
          'finished' => '_node_mass_update_batch_finished',
          'title' => t('Processing'),
          // We use a single multi-pass operation, so the default
          // 'Remaining x of y operations' message will be confusing here.
          'progress_message' => '',
          'error_message' => t('The update has encountered an error.'),
          // The operations do not live in the .module file, so we need to
          // tell the batch engine which file to load before calling them.
          'file' => drupal_get_path('module', 'node') . '/node.admin.inc',
        );
        batch_set($batch);
    

    Notice how the 'operations' array index is defined, in particular the array($nodes, $updates) part.
    If you replace that with array($nodes, 'updates' => $updates), you will get an error message described in the issue summary.

    batch_set() describes 'operations' as:

    operations: (required) Array of operations to be performed, where each item is an array consisting of the name of an implementation of callback_batch_operation() and an array of parameter.

    The example shown does not use an associate array, but the documentation does not say an associative array must not be used. That should be probably made explicit.

  • 🇮🇹Italy apaderno Brescia, 🇮🇹
Production build 0.71.5 2024