Problem/Motivation

These redirects are required to be in a .module file. Let's allow arbitrary callbacks.

Steps to reproduce

N/A

Proposed resolution

Convert to CallableResolver

Remaining tasks

Determine if we want an exception if it fails

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3539919

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:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Title: Convert batch redirect to CallbackResolver » Convert batch redirect to CallableResolver
Issue summary: View changes

nicxvan opened merge request !12921

nicxvan’s picture

nicxvan commented
Status: Active » Needs review

There was a comment here #3520416-30: [meta] Handle calls to functions that still require .module file autoloading.

I'm still not quite sure what @berdir meant there unless he means this should just call CallableResolver with no try catch so it dies if it fails.

I'm going to set this as needs review in the meantime.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Convert batch redirect to CallableResolver » Convert batch callbacks to CallableResolver
Status: Needs review » Needs work

> unless he means this should just call CallableResolver with no try catch so it dies if it fails.

That's exactly what he meant. That's what it should be IMHO. The only question is whether or not we need to worry about BC here. But I don't see a reason why you'd expect to be able to provide a callback here that doesn't actually work.

That said, the reason I pushed for this to be a separate issue is that I think all batch related callbacks should use this. Essentially all call_user_func_array() calls in batch.inc

nicxvan’s picture

nicxvan commented

Thanks for confirming!
I started with just pulling out this one try catch.

I then did a naive replacement in batch.inc, let's see what tests say.

nicxvan’s picture

nicxvan commented
Related issues: +#2875151: [META] Implement Batch API as a service
nicxvan’s picture

nicxvan commented

Some tests are failing there were a few related to the number of arguments splat operator took care of most of them but there are still a couple failures I haven't had a chance to review.

I think it's still the number of args calluserfuncarray handle it differently.

nicxvan’s picture

nicxvan commented

(Drupal\Tests\system\Functional\Batch\Processing)
✘ Batch form multiple batches
✘ Batch form programmatic

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

This is ready for review now!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

One comment.

nicxvan’s picture

nicxvan commented

I'm not sure how test only is passing, it's a service... maybe the bot set up the test wrong?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Looking at https://git.drupalcode.org/issue/drupal-3539919/-/jobs/6807564, it doesn't seem to run any tests, it also doesn't list the changed tests as changed files, so you might need to set up a separate MR. or we'll just have to verify locally, I'll try doing that when I find some time.

nicxvan’s picture

nicxvan commented

I can take care of that!

Edit: https://git.drupalcode.org/issue/drupal-3539919/-/pipelines/621754

nicxvan opened merge request !13434

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

The code is pretty complex in how the by-reference and callbacks works, but we have some tests I think this looks good.

This enables quite a bit of cleanup and removal and conversion of helper functions, non-injectable static methods and functions in remaining include files.

nicxvan’s picture

nicxvan commented
Related issues: +#3551734: [pp-1] deprecate the file key in batches and BatchBuilder::setFile(), +#3551735: [pp-1][Meta] convert batch callbacks to services with DI

Requested follow ups created.

  • catch committed 29242328 on 11.x 
    Issue #3539919 by nicxvan, berdir: Convert batch callbacks to...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Looks good. Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

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.

catch closed merge request !12921

  • catch committed 3d827c8a on 11.3.x 
    Issue #3539919 by nicxvan, berdir: Convert batch callbacks to...

nicxvan closed merge request !13434

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

acbramley’s picture

acbramley commented

I'm trying to follow the CR to update #3533299: Deprecate node access rebuild functions to use the callable syntax but I get 

TypeError: Drupal\Core\Batch\BatchBuilder::addOperation(): Argument #1 ($callback) must be of type callable, string given, called in /data/app/core/modules/node/src/NodeAccessRebuild.php on line 111

when using

->addOperation(NodeAccessRebuild::class . ':batchOperation')

Looking at the commits here it doesn't look like we have test coverage for the BatchBuilder approach?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes we didn't, I think we should remove the callable type there, this is not a language-level type that we can check there. I think that's OK because BatchBuilder isn't something that's meant to be subclassed and even if it is, an existing method definition could still add the type even if the parent doesn't have it since this is not an interface.

Can you create an issue for that?

acbramley’s picture

acbramley commented

Sure thing #3575096: Remove callable type from BatchBuilder ::addOperation and ::setFinishCallback

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented

#27: is not proper callable string - replace : with ::.

nicxvan’s picture

nicxvan commented

Single : is valid in this situation: https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co...

I moved the CR back and updated something that referenced form objects.

In general if it's a service I think we use the double ::, if it's not we use the single :.

acbramley’s picture

acbramley commented

In general if it's a service I think we use the double ::, if it's not we use the single :.

Isn't it the opposite? Single : for services, double for non services?

nicxvan’s picture

nicxvan commented

You're right, I always confuse the two and I literally read the comment: // Callable in the service:method notation. and still confused it :D

Bottom line is it should be valid, but we made a mistake, the follow up should be ready.