[regression] FTPExtension class can no longer connect as of 9.5.x

Patch based on running FTPExtension->connect() ; once.

+++ b/core/lib/Drupal/Core/FileTransfer/FTPExtension.php
@@ -11,6 +11,10 @@ class FTPExtension extends FTP implements ChmodInterface {
+    // Ensure `$this->connect()` is only called once.
+    if ($this->setConnectRunState()) {

+++ b/core/lib/Drupal/Core/FileTransfer/FileTransfer.php
@@ -62,6 +62,13 @@ abstract class FileTransfer {
+  private $hasConnectRunState = FALSE;

@@ -439,6 +452,19 @@ public function setChroot() {
+  public function setConnectRunState() {
+    $hasConnectRunState = $this->hasConnectRunState;
+    $this->hasConnectRunState = TRUE;
+    return $hasConnectRunState;

+++ b/core/lib/Drupal/Core/FileTransfer/SSH.php
@@ -22,6 +22,10 @@ public function __construct($jail, $username, $password, $hostname = "localhost"
+    // Ensure `$this->connect()` is only called once.
+    if ($this->setConnectRunState()) {

I think there's $connectionHandle private variable which could be used instead

Interesting, I see that ftp_connect() and ssh2_connect() both return a non-NULL value, so FALSE or a resource object so an isset() check could work. I can redo patch later, or feel free to do yourself if you get to it first, no worries.

Updated patch to use a do a null check as per suggested : isset($this->connection) .

Didn't use isset($this->connectionHandle) in subclass is it is private (no subclass access to parent) and all sub-classes still use naming convention of $this->connection

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 under control, following Review a patch or merge request → as a guide.

Next step would be to add a test case to show the issue.

Hi,
As one of the few remaining users who can't use composer, I was somewhat interested in getting this fixed.
So if I gave you a working solution, would anybody be willing to implement it?

What I did was (in FileTransfer.php)

• Kept the added property declarations but renamed connection back (removed -Handle) and made it public
• Added code to make sure chrootHandle is always set when needed (that part broke with the addition of the declaration) and fixed some leftover uses of just chroot (without 'Path')

Im adding adding this as a .zip file, I know that's not the right way but learning how to create a merge request seems like a lot of work...

Btw, I tested this on a site with shared hosting with a chrroted FTP. I also verified that the 'Local' subclass still works.

Some more Details on what I dit, and why:
The change made with Drupal 9.5 was to add property declarations for $connection, $croot and $jail.
connection and chroot were also renamed and made private respectively protected.
Some more 'magic' were function added, probably to make it (unit tests?) work again.

Renaming connection was imho ill-advised. Because that means the __get() has two different things to do: providing access from the outside and the FTPExension subclass, and connecting when $connection is absent in the class itself.That just leads to unnecessary complications.

By the way, connection is deliberatley unset in core\modules\update\update.authorize.inc
See "unset($filetransfer->connection);" (with an explanation added)

So what happens is: From the form where the ftp credentials are entered, a connection is established, but probably just to test it.
When we come back wih a task, connection is therefore completely gone. Also, chrootPath has somehow lost it's value.
But because it is declared, it is null, not absent. So while connection reestablishes itself, the 'magic' __get is nottriggered by it. Apparently, null is not unaccessible for the sake of this.

So I had to find a location where to call setChroot() and found it in fixRemotePath().

After that, all the other magic functions were no longer needed.

The latest patch works wonderfully. Tested in Drupal 10.2.5, it fixed the issue for me with the SSH library. Beforehand I was too was getting that undefined `connection` property, but this patch resolves that and eliminates the recursive behaviour that would lead to an infinite loop if the exception weren't being thrown.

Unsure how we can add a test for this, this class doesn't have any existing test coverage and without the ssh2 extension or an FTP server to connect to I'm not sure what value any test would have.

The fix and explanation make sense to me, the additional comments in this (untested) code are welcome too, going to mark this as RTBC. Although it does also point to the fact that very few people probably use this if it's been broken since 9.5...

This is testable and the change in #3295821: Ignore: patch testing issue for PHP 8.2 attributes → was implemented incorrectly. We can fix this without changing the logic of things that extend \Drupal\Core\FileTransfer\FileTransfer. In fact the changes to \Drupal\Tests\system\Functional\FileTransfer\TestFileTransfer in #3295821: Ignore: patch testing issue for PHP 8.2 attributes → should have warned us that we were doing this wrong.

Appears to have a test failure

    1) Drupal\Tests\system\Functional\FileTransfer\FileTransferTest::testJail
    Error: Call to a member function run() on null
    
    /builds/project/drupal/core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php:62
    /builds/project/drupal/core/lib/Drupal/Core/FileTransfer/FileTransfer.php:216
    /builds/project/drupal/core/lib/Drupal/Core/FileTransfer/FileTransfer.php:335
    /builds/project/drupal/core/lib/Drupal/Core/FileTransfer/FileTransfer.php:180
    /builds/project/drupal/core/modules/system/tests/src/Functional/FileTransfer/FileTransferTest.php:95
    /builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

Hiding patches for clarity

Tests are all green so switch to chroot and connection didn't break anything.

Not sure how else to test but seems fine to me.

Added a comment that shows we have coverage of the problem in the issue summary by reverting a change to the test code that was made in the earlier issue.

Also the test only job fails as expected - https://git.drupalcode.org/project/drupal/-/jobs/1534956

This one needs a reroll for phpstan baseline folks. Thanks

kim.pepper → made their first commit to this issue’s fork.

Rebased on 11.x

I thought we could remove the #[\AllowDynamicProperties] because we had the @property tags - it's good enough for PHPStan but not for PHP itself... so we still need that attribute.

Setting back to rtbc because @kim.pepper's fix was only phpstan and comments.

larowlan → closed merge request !7905

Committed to 11.x, 11.0.x, 10.4.x and 10.3.x

Doesn't apply to 10.2.x - moving to there for a possible backport

Same changes locally; this should also fix the PHPStan fails in 10.2.x at https://git.drupalcode.org/project/drupal/-/pipelines/191534

That's a nice benefit. There were conflicts in the baseline so I rebuilt it.

I did not intend to change the status

Committed as this is just a backport and also fixes PHPStan in time for a 10.2.x release this week.

Committed c141f9e and pushed to 10.2.x. Thanks!

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