From 74ddd89a71d47edf5ff861e2beb643c055b751e2 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Fri, 8 Mar 2024 21:27:24 +0100 Subject: [PATCH 1/7] Add ability to proactively close FTP and SFTP connections. --- src/Ftp/FtpAdapter.php | 12 +++++++-- src/Ftp/FtpAdapterTest.php | 22 +++++++++++++++- src/Ftp/FtpAdapterTestCase.php | 6 +++++ src/Ftp/StubConnectionProvider.php | 18 +++++++++++++ src/PhpseclibV3/ConnectionProvider.php | 3 +++ src/PhpseclibV3/SftpAdapter.php | 5 ++++ src/PhpseclibV3/SftpAdapterTest.php | 26 +++++++++++++++---- src/PhpseclibV3/SftpConnectionProvider.php | 7 +++++ .../StubSftpConnectionProvider.php | 11 ++++++-- 9 files changed, 100 insertions(+), 10 deletions(-) create mode 100644 src/Ftp/StubConnectionProvider.php diff --git a/src/Ftp/FtpAdapter.php b/src/Ftp/FtpAdapter.php index 2f0e6dc8e..76a36442d 100644 --- a/src/Ftp/FtpAdapter.php +++ b/src/Ftp/FtpAdapter.php @@ -30,6 +30,7 @@ use function error_clear_last; use function error_get_last; use function ftp_chdir; +use function ftp_close; use function is_string; class FtpAdapter implements FilesystemAdapter @@ -37,7 +38,7 @@ class FtpAdapter implements FilesystemAdapter private const SYSTEM_TYPE_WINDOWS = 'windows'; private const SYSTEM_TYPE_UNIX = 'unix'; - private FtpConnectionProvider $connectionProvider; + private ConnectionProvider $connectionProvider; private ConnectivityChecker $connectivityChecker; /** @@ -55,7 +56,7 @@ class FtpAdapter implements FilesystemAdapter public function __construct( private FtpConnectionOptions $connectionOptions, - FtpConnectionProvider $connectionProvider = null, + ConnectionProvider $connectionProvider = null, ConnectivityChecker $connectivityChecker = null, VisibilityConverter $visibilityConverter = null, MimeTypeDetector $mimeTypeDetector = null, @@ -104,6 +105,13 @@ private function connection() return $this->connection; } + public function disconnect(): void + { + if ($this->hasFtpConnection()) { + ftp_close($this->connection); + } + } + private function isPureFtpdServer(): bool { if ($this->isPureFtpdServer !== null) { diff --git a/src/Ftp/FtpAdapterTest.php b/src/Ftp/FtpAdapterTest.php index 2656b139e..aa8c498d0 100644 --- a/src/Ftp/FtpAdapterTest.php +++ b/src/Ftp/FtpAdapterTest.php @@ -26,8 +26,13 @@ protected static function createFilesystemAdapter(): FilesystemAdapter ]); static::$connectivityChecker = new ConnectivityCheckerThatCanFail(new NoopCommandConnectivityChecker()); + static::$connectionProvider = new StubConnectionProvider(new FtpConnectionProvider()); - return new FtpAdapter($options, null, static::$connectivityChecker); + return new FtpAdapter( + $options, + static::$connectionProvider, + static::$connectivityChecker, + ); } /** @@ -50,6 +55,21 @@ public function disconnect_after_destruct(): void $this->assertFalse((new NoopCommandConnectivityChecker())->isConnected($connection)); } + /** + * @test + */ + public function it_can_disconnect(): void + { + /** @var FtpAdapter $adapter */ + $adapter = $this->adapter(); + + $this->assertFalse($adapter->fileExists('not-existing.file')); + + self::assertTrue(static::$connectivityChecker->isConnected(static::$connectionProvider->connection)); + $adapter->disconnect(); + self::assertFalse(static::$connectivityChecker->isConnected(static::$connectionProvider->connection)); + } + /** * @test */ diff --git a/src/Ftp/FtpAdapterTestCase.php b/src/Ftp/FtpAdapterTestCase.php index 49c9b3f76..85bef62a2 100644 --- a/src/Ftp/FtpAdapterTestCase.php +++ b/src/Ftp/FtpAdapterTestCase.php @@ -4,6 +4,7 @@ namespace League\Flysystem\Ftp; +use Ftp\StubConnectionProvider; use Generator; use League\Flysystem\AdapterTestUtilities\FilesystemAdapterTestCase; use League\Flysystem\Config; @@ -37,6 +38,11 @@ protected function setUp(): void */ protected static $connectivityChecker; + /** + * @var StubConnectionProvider + */ + protected static $connectionProvider; + /** * @after */ diff --git a/src/Ftp/StubConnectionProvider.php b/src/Ftp/StubConnectionProvider.php new file mode 100644 index 000000000..f7c2ae075 --- /dev/null +++ b/src/Ftp/StubConnectionProvider.php @@ -0,0 +1,18 @@ +connection = $this->provider->createConnection($options); + } +} diff --git a/src/PhpseclibV3/ConnectionProvider.php b/src/PhpseclibV3/ConnectionProvider.php index d7182cc99..6f15bffe0 100644 --- a/src/PhpseclibV3/ConnectionProvider.php +++ b/src/PhpseclibV3/ConnectionProvider.php @@ -6,6 +6,9 @@ use phpseclib3\Net\SFTP; +/** + * @method void disconnect() + */ interface ConnectionProvider { public function provideConnection(): SFTP; diff --git a/src/PhpseclibV3/SftpAdapter.php b/src/PhpseclibV3/SftpAdapter.php index b47ed0695..5fa5f9dee 100644 --- a/src/PhpseclibV3/SftpAdapter.php +++ b/src/PhpseclibV3/SftpAdapter.php @@ -58,6 +58,11 @@ public function fileExists(string $path): bool } } + public function disconnect(): void + { + $this->connectionProvider->disconnect(); + } + public function directoryExists(string $path): bool { $location = $this->prefixer->prefixDirectoryPath($path); diff --git a/src/PhpseclibV3/SftpAdapterTest.php b/src/PhpseclibV3/SftpAdapterTest.php index bbcfc4b22..696b4a67c 100644 --- a/src/PhpseclibV3/SftpAdapterTest.php +++ b/src/PhpseclibV3/SftpAdapterTest.php @@ -30,7 +30,7 @@ public static function setUpBeforeClass(): void } /** - * @var ConnectionProvider + * @var StubSftpConnectionProvider */ private static $connectionProvider; @@ -214,9 +214,26 @@ public function list_contents_directory_does_not_exist(): void $this->assertCount(0, iterator_to_array($contents)); } - private static function connectionProvider(): ConnectionProvider + /** + * @test + */ + public function it_can_proactively_close_a_connection(): void + { + /** @var SftpAdapter $adapter */ + $adapter = $this->adapter(); + + self::assertFalse($adapter->fileExists('does not exists at all')); + + self::assertTrue(static::$connectionProvider->connection->isConnected()); + + $adapter->disconnect(); + + self::assertFalse(static::$connectionProvider->connection->isConnected()); + } + + private static function connectionProvider(): StubSftpConnectionProvider { - if ( ! static::$connectionProvider instanceof ConnectionProvider) { + if ( ! static::$connectionProvider instanceof StubSftpConnectionProvider) { static::$connectionProvider = new StubSftpConnectionProvider('localhost', 'foo', 'pass', 2222); } @@ -229,8 +246,7 @@ private static function connectionProvider(): ConnectionProvider private function adapterWithInvalidRoot(): SftpAdapter { $provider = static::connectionProvider(); - $adapter = new SftpAdapter($provider, '/invalid'); - return $adapter; + return new SftpAdapter($provider, '/invalid'); } } diff --git a/src/PhpseclibV3/SftpConnectionProvider.php b/src/PhpseclibV3/SftpConnectionProvider.php index 996169f51..5869f3226 100644 --- a/src/PhpseclibV3/SftpConnectionProvider.php +++ b/src/PhpseclibV3/SftpConnectionProvider.php @@ -83,6 +83,13 @@ public function provideConnection(): SFTP return $this->connection = $connection; } + public function disconnect(): void + { + if ($this->connection) { + $this->connection->disconnect(); + } + } + private function setupConnection(): SFTP { $connection = new SFTP($this->host, $this->port, $this->timeout); diff --git a/src/PhpseclibV3/StubSftpConnectionProvider.php b/src/PhpseclibV3/StubSftpConnectionProvider.php index e75dc15c2..2dc5258ef 100644 --- a/src/PhpseclibV3/StubSftpConnectionProvider.php +++ b/src/PhpseclibV3/StubSftpConnectionProvider.php @@ -9,9 +9,9 @@ class StubSftpConnectionProvider implements ConnectionProvider { /** - * @var SftpStub + * @var SftpStub|null */ - private $connection; + public $connection; public function __construct( private string $host, @@ -21,6 +21,13 @@ public function __construct( ) { } + public function disconnect(): void + { + if ($this->connection) { + $this->connection->disconnect(); + } + } + public function provideConnection(): SFTP { if ( ! $this->connection instanceof SFTP) { From e089524dfda3e32711543bc137743700d4de9f76 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Sat, 9 Mar 2024 14:16:40 +0100 Subject: [PATCH 2/7] Cleanup ftp impls --- src/Ftp/FtpAdapter.php | 8 +++----- src/Ftp/FtpAdapterTestCase.php | 7 ++++++- src/Ftp/FtpConnectionProvider.php | 5 ++++- src/Ftp/NoopCommandConnectivityCheckerTest.php | 2 +- src/Ftp/UnableToConnectToFtpHost.php | 4 ++-- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/Ftp/FtpAdapter.php b/src/Ftp/FtpAdapter.php index 76a36442d..09c14f71e 100644 --- a/src/Ftp/FtpAdapter.php +++ b/src/Ftp/FtpAdapter.php @@ -75,10 +75,7 @@ public function __construct( */ public function __destruct() { - if ($this->hasFtpConnection()) { - @ftp_close($this->connection); - } - $this->connection = false; + $this->disconnect(); } /** @@ -108,8 +105,9 @@ private function connection() public function disconnect(): void { if ($this->hasFtpConnection()) { - ftp_close($this->connection); + @ftp_close($this->connection); } + $this->connection = false; } private function isPureFtpdServer(): bool diff --git a/src/Ftp/FtpAdapterTestCase.php b/src/Ftp/FtpAdapterTestCase.php index 85bef62a2..aeea202d4 100644 --- a/src/Ftp/FtpAdapterTestCase.php +++ b/src/Ftp/FtpAdapterTestCase.php @@ -4,7 +4,6 @@ namespace League\Flysystem\Ftp; -use Ftp\StubConnectionProvider; use Generator; use League\Flysystem\AdapterTestUtilities\FilesystemAdapterTestCase; use League\Flysystem\Config; @@ -51,6 +50,12 @@ public function resetFunctionMocks(): void reset_function_mocks(); } + public static function clearFilesystemAdapterCache(): void + { + parent::clearFilesystemAdapterCache(); + static::$connectionProvider = null; + } + /** * @test */ diff --git a/src/Ftp/FtpConnectionProvider.php b/src/Ftp/FtpConnectionProvider.php index 09e12a631..67bbaa68d 100644 --- a/src/Ftp/FtpConnectionProvider.php +++ b/src/Ftp/FtpConnectionProvider.php @@ -4,6 +4,8 @@ namespace League\Flysystem\Ftp; +use function error_clear_last; +use function error_get_last; use const FTP_USEPASVADDRESS; class FtpConnectionProvider implements ConnectionProvider @@ -40,10 +42,11 @@ public function createConnection(FtpConnectionOptions $options) */ private function createConnectionResource(string $host, int $port, int $timeout, bool $ssl) { + error_clear_last(); $connection = $ssl ? @ftp_ssl_connect($host, $port, $timeout) : @ftp_connect($host, $port, $timeout); if ($connection === false) { - throw UnableToConnectToFtpHost::forHost($host, $port, $ssl); + throw UnableToConnectToFtpHost::forHost($host, $port, $ssl, error_get_last()['message'] ?? ''); } return $connection; diff --git a/src/Ftp/NoopCommandConnectivityCheckerTest.php b/src/Ftp/NoopCommandConnectivityCheckerTest.php index e15753865..e8299f686 100644 --- a/src/Ftp/NoopCommandConnectivityCheckerTest.php +++ b/src/Ftp/NoopCommandConnectivityCheckerTest.php @@ -26,7 +26,7 @@ public function detecting_a_good_connection(): void { $options = FtpConnectionOptions::fromArray([ 'host' => 'localhost', - 'port' => 2122, + 'port' => 2121, 'root' => '/home/foo/upload', 'username' => 'foo', 'password' => 'pass', diff --git a/src/Ftp/UnableToConnectToFtpHost.php b/src/Ftp/UnableToConnectToFtpHost.php index 0543498bd..8d66908d7 100644 --- a/src/Ftp/UnableToConnectToFtpHost.php +++ b/src/Ftp/UnableToConnectToFtpHost.php @@ -8,10 +8,10 @@ final class UnableToConnectToFtpHost extends RuntimeException implements FtpConnectionException { - public static function forHost(string $host, int $port, bool $ssl): UnableToConnectToFtpHost + public static function forHost(string $host, int $port, bool $ssl, string $reason = ''): UnableToConnectToFtpHost { $usingSsl = $ssl ? ', using ssl' : ''; - return new UnableToConnectToFtpHost("Unable to connect to host $host at port $port$usingSsl."); + return new UnableToConnectToFtpHost("Unable to connect to host $host at port $port$usingSsl. $reason"); } } From 4d5aaca96a03d0b892149016579209977de78452 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Sat, 9 Mar 2024 14:25:09 +0100 Subject: [PATCH 3/7] Call destruct, there is a reference keeping it from destroying itself. --- src/Ftp/FtpAdapterTest.php | 2 +- src/Ftp/FtpAdapterTestCase.php | 10 ++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/Ftp/FtpAdapterTest.php b/src/Ftp/FtpAdapterTest.php index aa8c498d0..20aaa09e0 100644 --- a/src/Ftp/FtpAdapterTest.php +++ b/src/Ftp/FtpAdapterTest.php @@ -50,7 +50,7 @@ public function disconnect_after_destruct(): void unset($reflection); $this->assertTrue(false !== ftp_pwd($connection)); - unset($adapter); + $adapter->__destruct(); static::clearFilesystemAdapterCache(); $this->assertFalse((new NoopCommandConnectivityChecker())->isConnected($connection)); } diff --git a/src/Ftp/FtpAdapterTestCase.php b/src/Ftp/FtpAdapterTestCase.php index aeea202d4..2a0c15a81 100644 --- a/src/Ftp/FtpAdapterTestCase.php +++ b/src/Ftp/FtpAdapterTestCase.php @@ -32,15 +32,9 @@ protected function setUp(): void $this->retryOnException(UnableToConnectToFtpHost::class); } - /** - * @var ConnectivityCheckerThatCanFail - */ - protected static $connectivityChecker; + protected static ConnectivityCheckerThatCanFail $connectivityChecker; - /** - * @var StubConnectionProvider - */ - protected static $connectionProvider; + protected static ?StubConnectionProvider $connectionProvider; /** * @after From e0196771030aeeb2909e56a6613ba3d2c08168c2 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Sat, 9 Mar 2024 16:32:51 +0100 Subject: [PATCH 4/7] Use ping as connectivity check --- src/PhpseclibV3/SimpleConnectivityChecker.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/PhpseclibV3/SimpleConnectivityChecker.php b/src/PhpseclibV3/SimpleConnectivityChecker.php index 2e198f2ec..fddcdb566 100644 --- a/src/PhpseclibV3/SimpleConnectivityChecker.php +++ b/src/PhpseclibV3/SimpleConnectivityChecker.php @@ -5,11 +5,16 @@ namespace League\Flysystem\PhpseclibV3; use phpseclib3\Net\SFTP; +use Throwable; class SimpleConnectivityChecker implements ConnectivityChecker { public function isConnected(SFTP $connection): bool { - return $connection->isConnected(); + try { + return $connection->ping(); + } catch (Throwable) { + return false; + } } } From 13a489c09a602ddef728804ed4e1f983ffa56009 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Sat, 9 Mar 2024 16:40:53 +0100 Subject: [PATCH 5/7] Revert seemingly insignificant part. --- src/PhpseclibV3/SftpAdapterTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PhpseclibV3/SftpAdapterTest.php b/src/PhpseclibV3/SftpAdapterTest.php index 696b4a67c..92d9a505f 100644 --- a/src/PhpseclibV3/SftpAdapterTest.php +++ b/src/PhpseclibV3/SftpAdapterTest.php @@ -233,7 +233,7 @@ public function it_can_proactively_close_a_connection(): void private static function connectionProvider(): StubSftpConnectionProvider { - if ( ! static::$connectionProvider instanceof StubSftpConnectionProvider) { + if ( ! static::$connectionProvider instanceof ConnectionProvider) { static::$connectionProvider = new StubSftpConnectionProvider('localhost', 'foo', 'pass', 2222); } From 6f7baa0eda7d871a94203145cde6d69acf9c1342 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Sat, 9 Mar 2024 16:55:49 +0100 Subject: [PATCH 6/7] Set connection to null on disconnect. --- src/PhpseclibV3/StubSftpConnectionProvider.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/PhpseclibV3/StubSftpConnectionProvider.php b/src/PhpseclibV3/StubSftpConnectionProvider.php index 2dc5258ef..0e673a7ee 100644 --- a/src/PhpseclibV3/StubSftpConnectionProvider.php +++ b/src/PhpseclibV3/StubSftpConnectionProvider.php @@ -25,6 +25,7 @@ public function disconnect(): void { if ($this->connection) { $this->connection->disconnect(); + $this->connection = null; } } From 13969affae52fafbfe1da1a2423217f841925bb9 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Sat, 9 Mar 2024 16:59:56 +0100 Subject: [PATCH 7/7] Keep connection but improve the check --- src/PhpseclibV3/StubSftpConnectionProvider.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/PhpseclibV3/StubSftpConnectionProvider.php b/src/PhpseclibV3/StubSftpConnectionProvider.php index 0e673a7ee..3818b876b 100644 --- a/src/PhpseclibV3/StubSftpConnectionProvider.php +++ b/src/PhpseclibV3/StubSftpConnectionProvider.php @@ -25,13 +25,12 @@ public function disconnect(): void { if ($this->connection) { $this->connection->disconnect(); - $this->connection = null; } } public function provideConnection(): SFTP { - if ( ! $this->connection instanceof SFTP) { + if ( ! $this->connection instanceof SFTP || ! $this->connection->isConnected()) { $connection = new SftpStub($this->host, $this->port); $connection->login($this->username, $this->password);