Remove callable type from BatchBuilder ::addOperation and ::setFinishCallback

Not sure if we want to just expand it to include strings or remove it entirely.

For now I expanded it so we could convert some.

I did the ones that only check the literal type and the ones actually calling the batch.

The approach needs review.

I think I saw each one call to array so isn't it implicitly testing that?

I'm not sure about updating existing tests to use the format service:method notation. I expect to see new case usage alongside existing test cases.

You're right I think we need coverage of both ways, but I don't think we need brand new tests. I can revert one or two of them so we have coverage of both ways.

We'll have good coverage since we test so many cases already and we won't slow down the actual runtime.

I set it to needs work so I can do that, great idea!

I reverted a couple!

This is a real failure

The reverted/updated definitions still had a : in the method names in the array, fixed that.

Oh thank you! I forgot to circle back to this when I got in front of my computer.

I've reviewed your changes and they look like what I expected.

If you want to review my changes I think we can co rtbc this

Went through the MR again, looks good to me, my changes essentially just correct the revert back to HEAD and do not actually show up in the MR anymore.

This unblocks further cleanup in #3533299: Deprecate node access rebuild functions

This should probably be backported to 11.3 too I think.

I'd argue against that. There are workarounds (not using batch builder, using static methods, ...) and this is a type change, even if it's for a class that nobody is supposed to subclass, but who knows, in theory it would break a subclass overriding that method.

Fair enough, I won't push back too hard, but this enables the intent of adding callable resolver which is why I thought it should be backported.

I'm happy as long as it gets into main.

I think it would be clearer if BatchBuilderTest explicitly tested the [$class, $method] format as well for the two methods in question as well as the two string formats.

Is there a reason the BetchTestDefinitions tests are not enough? We do have this:

public function batch1(): array {
    // Ensure the batch takes at least two iterations.
    $total = 10;
    $sleep = (int) (1000000 / $total) * 2;

    $batch_builder = (new BatchBuilder())
      ->setFinishCallback(BatchTestCallbacks::class . ':finished1');

    for ($i = 1; $i <= $total; $i++) {
      $batch_builder->addOperation(BatchTestCallbacks::class . ':callback1', [$i, $sleep]);
    }

    return $batch_builder->toArray() + ['batch_test_id' => 'batch1'];
  }

Also I'm not quite sure what you mean by two string formats?
Do mean this:
->setFinishCallback(static::class . ':finishedCallback')
and this:
->setFinishCallback('Drupal\Tests\Core\Batch\BatchBuilderTest:finishedCallback')

Or do you mean something else, because that does not seem to work.

Mix and matching in BatchTestDefinitions has the possibility of being lost or refactored in the future to be consistent (and ideally, it should consistently use the recommended best practice now if there is one, so copy-pasters and LLMs can learn from it).

Testing each type of callable (array, single colon string, double colon string) in the actual method unit tests shows explicit intent of the test coverage and is less likely to be lost.

Fair enough, will do!

I pushed up some additional test cases to check the various forms.

Committed and pushed bec4e5cafcc to main and 668beff98d2 to 11.x and 07c059cad23 to 11.3.x. Thanks!

Nice fix.