Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Undefined variable $item used in Drupal\Tests\system\Functional\FileTransfer\TestFileTransfer
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#21 | Screenshot from 2020-02-13 17-43-21.png | 285.51 KB | dhirendra.mishra |
#5 | Screen Shot 2020-02-10 at 2.18.32 PM.png | 47.52 KB | hash6 |
#2 | 3106455-2.patch | 818 bytes | Hardik_Patel_12 |
Comments
Comment #2
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedKindly review a patch.
Comment #3
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #4
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #5
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedThanks @Hardik_Patel_12 for the patch, it applies successfully without any failure.
PHPUnit Tests assertions also passed successfully.
Comment #6
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #7
longwaveIssue title says CronRunTest but the file in the patch is different?
Comment #8
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #9
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commented@longwave updated the filename in the title.
Comment #10
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #11
alexpottIf 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 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.
Comment #12
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #13
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedUpdated the code by using
MockTestConnection()
to run commands instead offtp_delete
, used$this->connection->run
Thanks @alexpott
Comment #14
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #15
longwaveComment #16
alexpottThis 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.Comment #17
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #18
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedI have updated the patch, thanks @longwave, @alexpott and @Hardik_Patel_12 .
Comment #19
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #20
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedNot 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
Comment #21
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedComment #22
alexpott@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.Comment #23
xjm#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!
Comment #24
longwavePatch 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?
Comment #25
xjmActually 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.
Comment #26
xjm(Didn't mean to tag yet, just to raise the question.)
Comment #28
longwaveRevisiting this following #3178534: Start running PHPStan on Drupal core (level 0) which has also picked up on this:
The docblock at the top of TestFileTransfer does already note why it exists:
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.
Comment #29
longwaveComment #30
alexpottCommitted 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.
Removed unused use on commit.
@dhirendra.mishra screenshots of your cli provide no more information than the copy and paste in #20 and thereby I removed issue credit as the comment is unrelated to the issue. It's about your phpunit.xml configuration and dev dependency installation.