Use callable resolver in batch API to allow callbacks in service notation

Created on 15 November 2023, 8 months ago
Updated 21 June 2024, 7 days ago

Problem/Motivation

The batch API takes callbacks, but these have to be functions or static methods on classes.

If we use the callable resolver, we can also allow callbacks in service notation, that is, in the form 'my_service:methodName'.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

✨ Feature request
Status

Needs review

Version

11.0 πŸ”₯

Component
BatchΒ  β†’

Last updated 7 days ago

Created by

πŸ‡¬πŸ‡§United Kingdom joachim

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @joachim
  • πŸ‡¬πŸ‡§United Kingdom joachim

    Tentatively tagging as novice -- it's *reasonably* simple:

    1. find places in batch.inc where callbacks are called
    2. Use the callable resolver service to get a callable first

  • My team and i are starting to work on this issue.

  • Pipeline finished with Canceled
    6 months ago
    Total: 56s
    #75882
  • Pipeline finished with Success
    6 months ago
    Total: 686s
    #75883
  • Status changed to Needs work 6 months ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    Looks good -- all the calls to call_user_func_array() have been changed.

    However, this also needs a corresponding docs change.

  • Pipeline finished with Canceled
    5 months ago
    Total: 45s
    #79395
  • Pipeline finished with Success
    5 months ago
    Total: 564s
    #79398
  • Status changed to Needs review 5 months ago
  • Status changed to RTBC 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Change makes sense and searched in batch.inc only for call_user_func_array and all 3 instances have been replaced.

  • πŸ‡¨πŸ‡΄Colombia camilo.escobar

    This change makes a lot of sense. A reminder that Drush has its own implementation of batch API (batch.inc) which allows you to harness batch operations when running Drush commands, via drush_backend_batch_process().
    It would be very convenient to implement these changes in that file too. I opened an issue in the Drush list and hope to work soon on that patch: https://github.com/drush-ops/drush/issues/5880.
    Thanks!

  • Status changed to Needs work 4 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    This will need to update \Drupal\Core\Batch\BatchBuilder too so that
    \Drupal\Core\Batch\BatchBuilder::setFinishCallback()
    \Drupal\Core\Batch\BatchBuilder::addOperation()

    Also I think we need test coverage to prove that this actually works... I think there's code in \_batch_next_set() that makes this a bit trickier than the current MR.

  • Pipeline finished with Failed
    25 days ago
    Total: 229s
    #190143
  • Pipeline finished with Failed
    25 days ago
    Total: 2783s
    #190151
  • We did some modifications in the code according to @alexpott comments. Unfortunately , the pipeline failed and we don't know how to solve it.

    $ echo $MODIFIED | tr ' ' '\n' | yarn --cwd=./core run -s spellcheck:core --no-must-find-files --file-list stdin
    /scripts-126748-1767911/step_script: line 308: /usr/bin/tr: Argument list too long
    /scripts-126748-1767911/step_script: line 308: /usr/bin/yarn: Argument list too long
    Cleaning up project directory and file based variables
    00:01
    ERROR: Job failed: command terminated with exit code 1

  • First commit to issue fork.
  • Pipeline finished with Failed
    20 days ago
    Total: 524s
    #194432
  • Spell checking error fixed by rebasing the MR, pipeline passed (2 stage) Spell checking errors

    Some Functional test case fail w.r.rt to batch file change in MR due to which pipeline not able to pass (3 stage), those needs to be addressed

  • Pipeline finished with Failed
    18 days ago
    Total: 542s
    #195882
  • Pipeline finished with Failed
    18 days ago
    Total: 554s
    #195891
  • Pipeline finished with Success
    18 days ago
    Total: 180s
    #195901
  • Pipeline finished with Failed
    18 days ago
    Total: 608s
    #195903
  • Pipeline finished with Failed
    17 days ago
    Total: 184s
    #196608
  • Pipeline finished with Success
    17 days ago
    Total: 646s
    #196612
  • Status changed to Needs review 17 days ago
  • All mentioned changes are addressed in such way that test case failures w.r.t to batch that were previously failing are now passing!!

    Please review, moved to NR

  • Status changed to Needs work 15 days ago
  • πŸ‡¬πŸ‡§United Kingdom joachim

    There's some logic that could be cleaned up, and some unrelated changes.

  • Assigned to pooja_sharma
  • Pipeline finished with Success
    15 days ago
    Total: 710s
    #198277
  • Refactored the code logic for improved clarity and cleanliness

    Please review , moved NR.

  • Issue was unassigned.
  • Status changed to Needs review 15 days ago
  • Pipeline finished with Success
    7 days ago
    Total: 641s
    #204957
  • Rebased the MR with latest code, seems fine

Production build 0.69.0 2024