Validate file exists in Batch::setFile()

Should we just check the existence of the file or throw an exception if it's not found. In batch.inc it does not throw an exception

      // Check if the set requires an additional file for function definitions.
      if (isset($batch_set['file']) && is_file($batch_set['file'])) {
        include_once \Drupal::root() . '/' . $batch_set['file'];
      }

I think we should retain the existing functionality

Can we use assert so developers can see their error, but it doesn't impact site users?

Thanks, @larowlan, using assert is good to me. Another question.

BatchBuilder:

  public function setFile($filename) {
    include_once $filename;

batch.inc:

function _batch_finished() {
  $batch = &batch_get();
  $batch_finished_redirect = NULL;

  // Execute the 'finished' callbacks for each batch set, if defined.
  foreach ($batch['sets'] as $batch_set) {
    if (isset($batch_set['finished'])) {
      // Check if the set requires an additional file for function definitions.
      if (isset($batch_set['file']) && is_file($batch_set['file'])) {
        include_once \Drupal::root() . '/' . $batch_set['file'];
      }

include_once $filename; vs include_once \Drupal::root() . '/' . $batch_set['file'];, $filename in setFile() expects an absolute path, $batch_set['file'] in _batch_finished() expects a relatvie path. Should we unify them?

Yes, because I think as it stands, this will break

is_file() Returns TRUE if the filename exists and is a regular file, FALSE otherwise.

      if (isset($batch_set['file']) && is_file($batch_set['file'])) {
        include_once \Drupal::root() . '/' . $batch_set['file'];
      }

I doubt include_once \Drupal::root() . '/' . $batch_set['file']; never gets executed. (batch.inc's path is core/includes/batch.inc.)

For example: is_file('core/foo/bar.inc'); it might lookup core/includes/core/foo/bar.inc. It can not be TRUE in general.

So I propose to support both the absolute path and the relative path. See the patch, then we don't need to worry broken the existing sites.

(Pending change the title/scope)

Self-review:

+++ b/core/includes/batch.inc
@@ -447,8 +447,14 @@ function _batch_finished() {
+        assert(is_file($batch_set['file']) || is_file(\Drupal::root() . '/' . $batch_set['file']));

+++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
@@ -215,8 +215,13 @@ public function setErrorMessage($message) {
+    assert(is_file($filename) || \Drupal::root() . '/' . $filename);

would be better adding a description to assert().

A quick review:

1. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -215,8 +215,13 @@ public function setErrorMessage($message) {
   +    elseif (\Drupal::root() . '/' . $filename) {
   

   You are missing the is_file() on this line.

2. Missing testing for these changes

Thanks for your quick review, @daffie!

Addressing #15.1, and tagging "Needs tests"

Adding tests.

Fixing #17

Tagging Bug Smash Initiative

1. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -215,9 +215,15 @@ public function setErrorMessage($message) {
   +    assert(is_file($filename) || \Drupal::root() . '/' . $filename);
   

   Missing the is_file call around the second path. The fact that this didn't fail tests maybe indicates missing test coverage? I don't think that's the case.

2. +++ b/core/tests/Drupal/Tests/Core/Batch/BatchBuilderTest.php
   @@ -108,20 +109,56 @@ public function testSetErrorMessage() {
   +   * Tests setFile() with non-existant file.
   ...
   +  public function testSetFileNonexistant() {
   

   Spelling and camel casing need fixing here

3. +++ b/core/tests/Drupal/Tests/Core/Batch/BatchBuilderTest.php
   @@ -108,20 +109,56 @@ public function testSetErrorMessage() {
   +   * Tests setFile() with a file which file path is relative.
   ...
   +   * Tests setFile() with a file which file path is absolute.
   

   I think these could do with some re-wording.

Fixed these up myself :)

Missed the new fixture file 🤦‍♂️

@acbramley, many thanks for reviewing and correcting.

Fixing #22/#24

+++ b/core/tests/Drupal/Tests/Core/Batch/BatchBuilderTest.php
@@ -118,8 +118,11 @@ public function testSetFileNonExistent() {
+    $this->expectException('AssertionError');

Changing to $this->expectException(AssertionError::class);

Patch fails testbot.

Thanks, @daffie!

+++ b/core/includes/batch.inc
@@ -447,8 +447,15 @@ function _batch_finished() {
+      if (isset($batch_set['file'])) {
+        $filename = $batch['file'];

Should be $filename = $batch_set['file']; my bad.

As far as I can see there is no testing added for the change to the function _batch_finished().

The added testing for BatchBuilder::setFile() look good to me.

@jungle: "My bad" happens to every programmer. :)

Thanks @daffie!

No existing test i could find to cover _batch_finished(), that means, no test coverage to _batch_finished() as i know. And there is an issue #2875151: [META] Implement Batch API as a service.

Meanwhile, per the issue title, touching _batch_finished() is not in scope. So I am reverting the changes to _batch_finished() keeping it untouched.

Not the solution that I expected, but is ok too.
The added solution looks good to me.
There is testing added for the solution.
For me it is RTBC.

From what I can see \_batch_process always appends the `\Drupal::root` to the passed file.

So if we support the option with a full path, that will end up in \Drupal::root being appended twice.

I.e. I think the batch handler only supports relative paths.

Thoughts?

Thanks @larowlan!

    // If this is the first time we iterate this batch set in the current
    // request, we check if it requires an additional file for functions
    // definitions.
    if ($set_changed && isset($current_set['file']) && is_file($current_set['file'])) {
      include_once \Drupal::root() . '/' . $current_set['file'];
    }

Code snippet from \_batch_process().

is_file() makes sure $current_set['file']) is a valid file path. include_once $current_set['file']; should work for both relative path and absolute path.

Using BatchBuilder, include_once is unnecessary here in \_batch_process() and other _batch_* functions. But it's no harm if it does.

\Drupal\Core\Batch\BatchBuilder::setFile() supports both relative path and absolute path which should be fine and good, IMO.

In all, I am going to try removing prepended \Drupal::root() in \_batch_process() so that full path in BatchBuilder::setFile should work.

If we are going to do this then we should add support for absolute and relative paths. Now we are changing support for absolute paths with support relative paths. To me it would be better to support both. If we cannot add testing for it, then so be it. In the file core/includes/batch.inc we have 2 instances of it. The first on line 277 and the second on line 451. Lets fix it for both.

Any reason to support absolute path? it looks insecure

If we cannot add testing for it, then so be it.

Almost all _batch_* functions in batch.inc are missing tests coverage. So re-uploading #30 for review. For here, only If there is an existed test to modify, I am trending to add testing for it.

Any reason to support absolute path? it looks insecure

Maybe it's fine to support absolute path in BatchBuilder, not sure. No idea to the security.

A known random fail.

Hi @jungle: The change that you make in the file core/lib/Drupal/Core/Batch/BatchBuilder.php, can you make the same change in the functions: _batch_process() and _batch_finished() in the file core/includes/batch.inc?

Hi @daffie, do you mean this -- see the patch?

@jungle: Yes, that is the change I was looking for.

For the changes in core/lib/Drupal/Core/Batch/BatchBuilder.php and core/includes/batch.inc there is no testing added, because AFAIK we cannot test those changes.
For me we should support both absolute paths and relative paths.
Back to RTBC.

Updated the IS.

1. +++ b/core/includes/batch.inc
   @@ -273,8 +273,15 @@ function _batch_process() {
   -    if ($set_changed && isset($current_set['file']) && is_file($current_set['file'])) {
   -      include_once \Drupal::root() . '/' . $current_set['file'];
   +    if ($set_changed && isset($current_set['file'])) {
   +      $filename = $current_set['file'];
   +      assert(is_file($filename) || is_file(\Drupal::root() . '/' . $filename), "File '$filename' is non-existent.");
   +      if (is_file($filename)) {
   +        include_once $filename;
   +      }
   +      elseif (is_file(\Drupal::root() . '/' . $filename)) {
   +        include_once \Drupal::root() . '/' . $filename;
   +      }
   
   @@ -447,8 +454,15 @@ function _batch_finished() {
   -      if (isset($batch_set['file']) && is_file($batch_set['file'])) {
   -        include_once \Drupal::root() . '/' . $batch_set['file'];
   +      if (isset($batch_set['file'])) {
   +        $filename = $batch_set['file'];
   +        assert(is_file($filename) || is_file(\Drupal::root() . '/' . $filename), "File '$filename' is non-existent.");
   +        if (is_file($filename)) {
   +          include_once $filename;
   +        }
   +        elseif (is_file(\Drupal::root() . '/' . $filename)) {
   +          include_once \Drupal::root() . '/' . $filename;
   +        }
   

   I think these changes are out-of-scope and unnecessary. Widening the API to include any file is a security concern.

2. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -215,9 +215,15 @@ public function setErrorMessage($message) {
   +    assert(is_file($filename) || is_file(\Drupal::root() . '/' . $filename), "File '$filename' is non-existent.");
   +    if (is_file($filename)) {
   +      include_once $filename;
   +      $this->file = $filename;
   +    }
   +    elseif (is_file(\Drupal::root() . '/' . $filename)) {
   +      include_once \Drupal::root() . '/' . $filename;
   +      $this->file = \Drupal::root() . '/' . $filename;
   +    }
   

   There is no need for this to be including the file already. Why is this change being made.

I've always been a bit dubious about the value of this change. PHP will error very nicely when you try to include a file that does not exist. The asserts are meaningless because if the file doesn't exist then PHP is going to fail and fail hard.

php -r "include 'file_does_not_exist.php';"                                                                                                                                         5.3s  Sat  8 Aug 23:28:50 2020
PHP Warning:  include(file_does_not_exist.php): failed to open stream: No such file or directory in Command line code on line 1

Personally I think we should close this as works as designed.

Furthermore, once all the operations and callbacks are static methods on classes - or we support services via the class resolver in the batch system then setFile() become meaningless because everything is being autoloaded.

Thanks @alexpott!

Personally I think we should close this as works as designed.

+1, setting to NR for closing this if applicable.

PHP will error very nicely when you try to include a file that does not exist. The asserts are meaningless because if the file doesn't exist then PHP is going to fail and fail hard.

I am not sure it is true, because here is the actual code:

      if (isset($batch_set['file']) && is_file($batch_set['file'])) {
        include_once \Drupal::root() . '/' . $batch_set['file'];
      }

If the file doesn't exists it will not be included and it will fail silently (except in the logs)... In my case, I spent too much time to understand that the batch process didn't occurs (looking at the progress bar, everything is fine but in fact it is not fine)

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.