- 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 - Merge request !6130Issue #3401873: Use callable resolver in batch API to allow callbacks in service notation → (Open) created by Unnamed author
- Status changed to Needs work
over 1 year ago 8:05am 14 January 2024 - 🇬🇧United Kingdom joachim
Looks good -- all the calls to call_user_func_array() have been changed.
However, this also needs a corresponding docs change.
- Status changed to Needs review
about 1 year ago 6:23pm 5 February 2024 - Status changed to RTBC
about 1 year ago 10:23pm 13 February 2024 - 🇺🇸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 2:48pm 8 March 2024 - 🇬🇧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.
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.
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
- Status changed to Needs review
10 months ago 7:17pm 11 June 2024 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 3:27pm 13 June 2024 - 🇬🇧United Kingdom joachim
There's some logic that could be cleaned up, and some unrelated changes.
- Assigned to pooja_sharma
Refactored the code logic for improved clarity and cleanliness
Please review , moved NR.
- Issue was unassigned.
- Status changed to Needs review
10 months ago 7:25pm 13 June 2024 - Status changed to Needs work
9 months ago 12:49am 19 July 2024 - 🇺🇸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.
- Status changed to Needs review
9 months ago 11:09am 19 July 2024 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 6:03pm 11 August 2024