Undefined variable used in /core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php

Thanks @Hardik_Patel_12 for the patch, it applies successfully without any failure.
PHPUnit Tests assertions also passed successfully.

Issue title says CronRunTest but the file in the patch is different?

@longwave updated the filename in the title.

+++ b/core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php
@@ -47,8 +47,8 @@ public function createDirectoryJailed($directory) {
   public function removeFileJailed($destination) {
-    if (!ftp_delete($this->connection, $item)) {
-      throw new FileTransferException('Unable to remove the file @file.', NULL, ['@file' => $item]);
+    if (!ftp_delete($this->connection, $destination)) {
+      throw new FileTransferException('Unable to remove the file @file.', NULL, ['@file' => $destination]);
     }
   }

If we're going to fix this then we should actually fix the implementation. We can't call ftp_delete here. This method is never ever called. A "correct" implementation would be to do what the other methods do and record the command... so something like

$this->connection->run("rm $destination");

This method is never actually called hence tests not failing. But the current implementation is not right as we can't ftp_delete() anything in the test.

Updated the code by using MockTestConnection() to run commands instead of ftp_delete , used $this->connection->run

Thanks @alexpott

+++ b/core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php
@@ -47,8 +47,8 @@ public function createDirectoryJailed($directory) {
+    if (!$this->connection->run("rm $destination")) {
+      throw new FileTransferException('Unable to remove the file @file.', NULL, ['@file' => $destination]);
     }

This can never throw an exception...

The code should be only $this->connection->run("rm $destination"). $this->connnection is a \Drupal\Tests\system\Functional\FileTransfer\MockTestConnection object that records all the commands.

I have updated the patch, thanks @longwave, @alexpott and @Hardik_Patel_12 .

Not sure if this is related issue i got wen i manually tried running file with phpunit.

lenovo@lenovo-ThinkPad-L450:/var/www/html/core_drupal$ vendor/bin/phpunit -c core core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php
PHP Warning: Class 'PHPUnit\Framework\MockObject\Matcher\InvokedRecorder' not found in /var/www/html/core_drupal/core/tests/bootstrap.php on line 197
PHP Warning: Class 'Drupal\TestTools\PhpUnitCompatibility\PhpUnit8\HtmlOutputPrinter' not found in /var/www/html/core_drupal/core/tests/Drupal/Tests/Listeners/HtmlOutputPrinter.php on line 15
PHPUnit 8.5.2 by Sebastian Bergmann and contributors.

Could not use "\Drupal\Tests\Listeners\HtmlOutputPrinter" as printer: class does not exist

@dhirendra.mishra how have you got PHPUnit 8 - Drupal 8/9 doesn't support it yet - see #3063887: Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7 - I think you need to run composer install to get the correct dependencies.

#18 is still NR I think; I don't think #20 is related.

Since we should fix this bug in D8 too, I'm filing it against 8.8.x (which is the current bugfix support branch). Patches can be tested against other branches as needed when they're uploaded without changing the issue's version selector.

The issue will be automatically updated to 8.9.x after the last 8.8.x bugfix release. Thanks!

Patch in #18 follows @alexpott's suggestion in #16. There is obviously no test coverage here or it would catch it, but do we need any?

Actually I was just about to mark it NW for test coverage. This thing is a test fixture so there's no functional bug, but then, clearly something's missing from the test that uses this fixture. So we should either add that coverage, or explicitly document why the test using the fixture doesn't need to test that method.

(Didn't mean to tag yet, just to raise the question.)

Revisiting this following #3178534: Start running PHPStan on Drupal core (level 0) which has also picked up on this:

  Line   core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php  
 ------ ---------------------------------------------------------------------------- 
  50     Undefined variable: $item                                                   
  51     Undefined variable: $item                                                   

The docblock at the top of TestFileTransfer does already note why it exists:

   * This is for testing the CopyRecursive logic.

This test class only exists to test the recursive copy functionality, and removeFileJailed() isn't needed to do this, but as it's declared as an abstract method in the parent class then we need to write something here and it might as well be valid code.

Committed and pushed 3c692541be to 9.2.x and 6925f752bc to 9.1.x. Thanks!

Backported to 9.1.x since this is only changing test code.

diff --git a/core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php b/core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php
index bd6b90aa2b..f9806fd8b1 100644
--- a/core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php
+++ b/core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php
@@ -3,7 +3,6 @@
 namespace Drupal\Tests\system\Functional\FileTransfer;
 
 use Drupal\Core\FileTransfer\FileTransfer;
-use Drupal\Core\FileTransfer\FileTransferException;
 
 /**
  * Mock FileTransfer object for test case.

Removed unused use on commit.