Closed (outdated)
Project:
Drupal core
Version:
main
Component:
batch system
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
15 Nov 2023 at 14:47 UTC
Updated:
1 Feb 2026 at 02:16 UTC
Jump to comment: Most recent
Comments
Comment #2
joachim commentedTentatively 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
Comment #3
alexbaron77My team and i are starting to work on this issue.
Comment #5
joachim commentedLooks good -- all the calls to call_user_func_array() have been changed.
However, this also needs a corresponding docs change.
Comment #6
alexbaron77The docs has been updated .
Comment #7
smustgrave commentedChange makes sense and searched in batch.inc only for call_user_func_array and all 3 instances have been replaced.
Comment #8
camilo.escobar commentedThis 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!
Comment #9
alexpottThis 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.
Comment #10
alexbaron77We did some modifications in the code according to @alexpott comments. Unfortunately , the pipeline failed and we don't know how to solve it.
Comment #12
pooja_sharma commentedSpell 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)
Comment #13
pooja_sharma commentedAll 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
Comment #14
joachim commentedThere's some logic that could be cleaned up, and some unrelated changes.
Comment #15
pooja_sharma commentedComment #16
pooja_sharma commentedRefactored the code logic for improved clarity and cleanliness
Please review , moved NR.
Comment #17
pooja_sharma commentedComment #18
pooja_sharma commentedRebased the MR with latest code, seems fine
Comment #19
pooja_sharma commentedRebased the MR with latest code, seems fine
Comment #20
smustgrave commentedCan understand wanting to address small stuff later but the issue can't be RTBC (shouldn't at least) till it's truly ready.
Also
Should we add test coverage for this now? per #9 also.
Comment #21
pooja_sharma commentedThanks 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.
Comment #22
smustgrave commentedAppears to be open threads.
Comment #24
nicxvan commentedThank 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.