Method _batch_current_set is called without include core/includes/batch.inc

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Re-triggering for 10.1 and 9.5.x tests but don't expect failures

Followed the steps in the IS and can confirm the issue
Patch does fix the issue

The test being added here is kinda of incorrect. Batches are never requested at /batch/something - they are requested at /batch?id=12

Also the fix for the breadcrumbs is different than for the bug reported in the issue summary. We're never really going to have a breadcrumb for /batch so we should be exlcuding it like we do /user. I.e in \Drupal\system\PathBasedBreadcrumbBuilder::build() we could do something like:

    // /user is just a redirect, so skip it.
    // @todo Find a better way to deal with /user.
    $exclude['/user'] = TRUE;
    $exclude['/batch'] = TRUE;

But I think this issue points to a need to add a route option to disable the breadcrumb - that way contrib could make a similar fix. This probably should be split out into a separate fix.

So thinking about the original issue. The problem is you're not going to log the correct title with the current fix because in order to do that you need to load the batch. I think we need to do something like:

• Load core/includes/batch.inc in the constructor
• Change the batchPageTitle() to get batch and load it from storage if necessary.

I.e

  public function batchPageTitle(Request $request) {
    $batch = &batch_get();

    if (!($request_id = $request->query->get('id'))) {
      return '';
    }

    // Retrieve the current state of the batch.
    if (!$batch) {
      $batch = \Drupal::service('batch.storage')->load($request_id);
    }

    if (!$batch) {
      return '';
    }

    $current_set = _batch_current_set();
    return !empty($current_set['title']) ? $current_set['title'] : '';
  }

Ok let's see if that passes tests.

+++ b/core/modules/system/tests/src/Functional/Batch/PageTest.php
@@ -67,6 +67,11 @@ public function testBatchProgressPageTitle() {
+    // Test that the batch system emits a 404 page when appropriate.
+    $this->drupalPlaceBlock('system_breadcrumb_block');
+    $this->drupalGet('batch/foobar');
+    $this->assertSession()->pageTextContains('Page not found');

We should change this test. We should be getting the title for route like /batch?id=1234124123 ... we'll have to create a request and route match object to pass to the title resolver - see how \Drupal\system\PathBasedBreadcrumbBuilder::build() does it.

is it enough to just call the broken method in a test, like this?

If not, then yeah the test could call \Drupal::service('title_resolver')->getTitle($request, $route) as per the original issue summary.

For the record, I actually hit this bug as per the test I originally wrote - via an automated pentesting bot hitting random bogus paths including batch/[some string] - which tried and failed to build the breadcrumb.

@mfb so we need to fix the path breadcrumb builder issue in a separate related issue because even with this fix the breadcrumbs on batch/some_nonsense is incorrect. I think the test in #20 is okay - I think what'd be really nice is a unit test that tests getting the batch from storage (using a mock). Also we need to inject the batch storage service into the class.

THis part of #21

I think what'd be really nice is a unit test that tests getting the batch from storage (using a mock). Also we need to inject the batch storage service into the class.

Is something we should do on this issue. We also need to open another issue about excluding paths from the breadcrumb builder.

Did a bit more work on this - now injecting the service, and changed test from kernel test to unit test. But for the latter to work, I had to require form.inc, otherwise function batch_get() does not exist.

+++ b/core/modules/system/src/Controller/BatchController.php
--- /dev/null
+++ b/core/modules/system/tests/src/Unit/Batch/BatchControllerTest.php

+++ b/core/modules/system/tests/src/Unit/Batch/BatchControllerTest.php
@@ -0,0 +1,30 @@
+  /**
+   * Tests title callback.
+   *
+   * @covers ::batchPageTitle
+   */
+  public function testBatchPageTitle() {
+    $controller = new BatchController($this->root, $this->createMock(BatchStorageInterface::class));
+    require_once $this->root . '/core/includes/form.inc';
+    $this->assertSame('', $controller->batchPageTitle(new Request()));
+  }

Nice. So we should also test that a title is returned when batch_get() returns something. TO do this we can mock the load method on the batch storage.

Also as we're loading code and changing the state of the system let's add the @runTestsInSeparateProcesses annotation to the test class.

I opened the related issue - 🐛 PathBasedBreadcrumbBuilder needs to be able to exclude pointless paths Needs work

This seems to work as a minimal mock batch array

Seems #25 have been addressed in
+ $this->assertSame('foobar', $controller->batchPageTitle(new Request(['id' => 1234])));

This looks good. Adding 3344200 to my list also.

+++ b/core/modules/system/tests/src/Unit/Batch/BatchControllerTest.php
@@ -0,0 +1,34 @@
+  /**
+   * Tests title callback.
+   *
+   * @covers ::batchPageTitle
+   */
+  public function testBatchPageTitle() {
+    $batch_storage = $this->createMock(BatchStorageInterface::class);
+    $controller = new BatchController($this->root, $batch_storage);
+    require_once $this->root . '/core/includes/form.inc';
+    $this->assertSame('', $controller->batchPageTitle(new Request()));
+    $batch = ['sets' => [['title' => 'foobar']], 'current_set' => 0];
+    $batch_storage->method('load')->willReturn($batch);
+    $this->assertSame('foobar', $controller->batchPageTitle(new Request(['id' => 1234])));
+  }

This test should cover both situations - where the batch exists and where it does not.

Added another test case in there too - batch could be returned by &batch_get()

Going to try it again. If it's wrong I do apologize!

Given the constructor changes I think we can only commit this fix to 10.1.x - I considered asking for the deprecation dance and allow a NULL for batch storage but there's nothing in contrib that extends this class so I think we fine.

Committed f1dbd84 and pushed to 10.1.x. Thanks!

diff --git a/core/modules/system/tests/src/Unit/Batch/BatchControllerTest.php b/core/modules/system/tests/src/Unit/Batch/BatchControllerTest.php
index 172e2feae9..e68649fd37 100644
--- a/core/modules/system/tests/src/Unit/Batch/BatchControllerTest.php
+++ b/core/modules/system/tests/src/Unit/Batch/BatchControllerTest.php
@@ -32,7 +32,9 @@ public function testBatchPageTitle() {
     $this->assertSame('', $controller->batchPageTitle(new Request(['id' => 1234])));
     $this->assertSame('foobar', $controller->batchPageTitle(new Request(['id' => 1234])));
     // Test batch returned by &batch_get() call.
-    $this->assertSame('foobar', $controller->batchPageTitle(new Request(['id' => 1234])));
+    $batch = &batch_get();
+    $batch['sets']['0']['title'] = 'Updated title';
+    $this->assertSame('Updated title', $controller->batchPageTitle(new Request(['id' => 1234])));
   }
 
 }

Slightly improved the test on commit.

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