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

Issue fork drupal-3401873

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

joachim created an issue. See original summary.

joachim’s picture

Title: Use callback resolver in batch API to allow callbacks in service notation » Use callable resolver in batch API to allow callbacks in service notation
Issue summary: View changes
Issue tags: +Novice

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

alexbaron77’s picture

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

joachim’s picture

Status: Active » Needs work

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

However, this also needs a corresponding docs change.

alexbaron77’s picture

Status: Needs work » Needs review

The docs has been updated .

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

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

camilo.escobar’s picture

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!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

alexbaron77’s picture

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

pooja_sharma made their first commit to this issue’s fork.

pooja_sharma’s picture

Spell checking error fixed by rebasing the MR, pipeline passed for spell check errors (2 stage-pass)

I have analysed the spell check error: concluded that
The command run during spell check:
git diff --name-only refs/heads/$TARGET_BRANCH : is used to get a list of modified files in that branch.

As current branch behind 1157 commits, however too many files being passed as arguments(i.e. too long)
By rebasing, only changes files in MR pass for pipeline.

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

I believe on code alteration in files, these respective test files needs to be addressed (as test failure occurs)

pooja_sharma’s picture

Status: Needs work » Needs review

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

joachim’s picture

Status: Needs review » Needs work

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

pooja_sharma’s picture

Assigned: Unassigned » pooja_sharma
pooja_sharma’s picture

Assigned: pooja_sharma » Unassigned

Refactored the code logic for improved clarity and cleanliness

Please review , moved NR.

pooja_sharma’s picture

Status: Needs work » Needs review
pooja_sharma’s picture

Rebased the MR with latest code, seems fine

pooja_sharma’s picture

Rebased the MR with latest code, seems fine

smustgrave’s picture

Status: Needs review » Needs work

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.

pooja_sharma’s picture

Status: Needs work » Needs review

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 clarify a bit? , 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.

smustgrave’s picture

Status: Needs review » Needs work

Appears to be open threads.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

nicxvan’s picture

Status: Needs work » Closed (outdated)

Thank you everyone for your contribution, I just found this, this was resolved in: #3539919: Convert batch callbacks to CallableResolver

I'm going to close this as outdated, I've updated credit.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.