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

Created on 15 November 2023, over 1 year ago
Updated 11 August 2024, 8 months 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 work

Version

11.0 🔥

Component
Batch 

Last updated 23 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
    over 1 year ago
    Total: 56s
    #75882
  • Pipeline finished with Success
    over 1 year ago
    Total: 686s
    #75883
  • Status changed to Needs work over 1 year 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
    about 1 year ago
    Total: 45s
    #79395
  • Pipeline finished with Success
    about 1 year ago
    Total: 564s
    #79398
  • Status changed to Needs review about 1 year ago
  • Status changed to RTBC about 1 year 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 about 1 year 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
    11 months ago
    Total: 229s
    #190143
  • Pipeline finished with Failed
    11 months 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
    10 months 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
    10 months ago
    Total: 542s
    #195882
  • Pipeline finished with Failed
    10 months ago
    Total: 554s
    #195891
  • Pipeline finished with Success
    10 months ago
    Total: 180s
    #195901
  • Pipeline finished with Failed
    10 months ago
    Total: 608s
    #195903
  • Pipeline finished with Failed
    10 months ago
    Total: 184s
    #196608
  • Pipeline finished with Success
    10 months ago
    Total: 646s
    #196612
  • Status changed to Needs review 10 months 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 10 months 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
    10 months 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 10 months ago
  • Pipeline finished with Success
    10 months ago
    Total: 641s
    #204957
  • Rebased the MR with latest code, seems fine

  • Pipeline finished with Success
    10 months ago
    Total: 543s
    #213106
  • Rebased the MR with latest code, seems fine

  • Status changed to Needs work 9 months ago
  • 🇺🇸United States smustgrave

    Can understand wanting to address small stuff later but the issue can't be RTBC (shouldn't at least) till it's truly ready.

    Also

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

    Should we add test coverage for this now? per #9 also.

  • Pipeline finished with Failed
    9 months ago
    Total: 155s
    #228540
  • Pipeline finished with Success
    9 months ago
    Total: 461s
    #228558
  • Status changed to Needs review 9 months ago
  • Thanks for reviewing , Applied the suggestions on the MR, can we remove the Novoice tag as this issue not seems to be straightforward

    we can also allow callbacks in service notation, that is, in the form 'my_service:methodName': this is not clear to me can you please elaborate it , however I can able to recall, I have called the func in this specified format here : core/includes/batch.inc at line no. 436

    It seems test coverage already added in the Drupal\Tests\system\Functional\Batch\ProcessingTest which cover these newly added changes.

  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

    Appears to be open threads.

Production build 0.71.5 2024