When adding a callback in an included file, can error if the function hasn't already been included.

TypeError: Argument 1 passed to Drupal\Core\Batch\BatchBuilder::addOperation() must be callable, string given...

To fix Drupal\Core\Batch\BatchBuilder::setFile() needs to include the file that contains the callback function.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

John Cook created an issue. See original summary.

John Cook’s picture

John Cook’s picture

Status: Active » Needs review
FileSize
1.39 KB

I've made a failing test to show the problem.

Status: Needs review » Needs work

The last submitted patch, 3: 3028621-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

John Cook’s picture

Status: Needs work » Needs review
FileSize
2.7 KB
1.59 KB

I've created a patch that includes the requested file. The testSetFile() test has been updated to reference an actual file as an error occurs if PHP is requested to include a file that does not exist.

John Cook’s picture

John Cook’s picture

Moved batch_test_callback_in_file.php to reflect @alexpott's comment in #2875279: Update core modules to use the new batch builder #24.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jungle’s picture

Thanks @John Cook!

  1. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
    @@ -205,12 +205,18 @@ public function setErrorMessage($message) {
    diff --git a/core/modules/system/tests/fixtures/batch/batch_test_callback_in_file.php b/core/modules/system/tests/fixtures/batch/batch_test_callback_in_file.php
    
    +++ b/core/tests/Drupal/Tests/Core/Batch/BatchBuilderTest.php
    @@ -113,11 +113,12 @@ public function testSetErrorMessage() {
    +    $filename = './modules/system/tests/fixtures/batch/batch_test_callback_in_file.php';
    

    An existed callbacks file could be reused, instead of adding a new one.

  2. +++ b/core/tests/Drupal/Tests/Core/Batch/BatchBuilderTest.php
    @@ -113,11 +113,12 @@ public function testSetErrorMessage() {
       public function testSetFile() {
    ...
         $batch = (new BatchBuilder())
    ...
    +      ->setFile($filename)
           ->toArray();
     
    -    $this->assertEquals('filename.php', $batch['file']);
    +    $this->assertEquals( $filename, $batch['file']);
    

    This test does not cover that callbacks in the file are callables.

  3. +++ b/core/tests/Drupal/Tests/Core/Batch/BatchBuilderTest.php
    @@ -233,6 +234,17 @@ public function testAddOperation() {
    +    $filename = './modules/system/tests/fixtures/batch/batch_test_callback_in_file.php';
    +    $batch_builder = new BatchBuilder();
    ...
    +    ], $batch['operations']);
    

    The test here is unnecessary to me. which could be a part of ::testSetFile()

  4. Tagging "Bug Smash Initiative"

The last submitted patch, 11: 3028621-11-test-only.patch, failed testing. View results

pratik_kamble’s picture

Assigned: Unassigned » pratik_kamble
jungle’s picture

pratik_kamble’s picture

Assigned: pratik_kamble » Unassigned
Status: Needs review » Reviewed & tested by the community

@jungle, I have reviewed the patch in comment #11 for file 3028621-11.patch.
Using the existing batch_test.callbacks.inc make sense. Verified the code in testSetFile() function it LGTM.

Marking it as RTBC.

kim.pepper’s picture

Nit:

public function testSetFile() {
+    $filename = __DIR__ . '/../../../../../../core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc';

You should use dirname(__DIR__, 6) here.

jungle’s picture

Thanks, @pratik_kamble and @kim.pepper for reviewing!

Addressing #16.

jungle’s picture

FileSize
2.1 KB
760 bytes

Self review:

+++ b/core/tests/Drupal/Tests/Core/Batch/BatchBuilderTest.php
@@ -113,7 +113,7 @@ public function testSetErrorMessage() {
+    $filename = dirname(__DIR__, 6) .  '/core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc';
     $this->assertIsNotCallable('_batch_test_callback_1');

One redundant whitespace, fixing.

jungle’s picture

  • larowlan committed ad50981 on 9.1.x
    Issue #3028621 by jungle, John Cook: BatchBuilder included files fails
    

  • larowlan committed f63bd1c on 9.0.x
    Issue #3028621 by jungle, John Cook: BatchBuilder included files fails...
larowlan’s picture

Title: BatchBuilder included files fails » [backport] BatchBuilder included files fails
Version: 9.1.x-dev » 8.9.x-dev

Looked into this for my curiosity.

This was only causing issues from the callable type-hint on the BatchBuilder::addOperation and ::setFinishCallback.

The added include_once is in order for those type-hints to resolve.

The actual loading of the file for the sake of processing the batch happens in _batch_process and _batch_finished

Committed ad50981 and pushed to 9.1.x. Thanks!

Cherry-picked to 9.0.x

Queued an 8.9.x test-run and flagging this for [backport]

  • larowlan committed 978e4b5 on 8.9.x
    Issue #3028621 by jungle, John Cook: BatchBuilder included files fails...
larowlan’s picture

Title: [backport] BatchBuilder included files fails » BatchBuilder included files fails
Status: Reviewed & tested by the community » Fixed

#18 passes on 8.9.x.

As this is a bug and there is little disruption, cherry-picked to 8.9.x

Status: Fixed » Closed (fixed)

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