Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to proactively close FTP and SFTP connections. #1763

Merged
merged 7 commits into from
Mar 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/Ftp/FtpAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@
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
{
private const SYSTEM_TYPE_WINDOWS = 'windows';
private const SYSTEM_TYPE_UNIX = 'unix';

private FtpConnectionProvider $connectionProvider;
private ConnectionProvider $connectionProvider;
private ConnectivityChecker $connectivityChecker;

/**
Expand All @@ -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,
Expand All @@ -74,10 +75,7 @@ public function __construct(
*/
public function __destruct()
{
if ($this->hasFtpConnection()) {
@ftp_close($this->connection);
}
$this->connection = false;
$this->disconnect();
}

/**
Expand All @@ -104,6 +102,14 @@ private function connection()
return $this->connection;
}

public function disconnect(): void
{
if ($this->hasFtpConnection()) {
@ftp_close($this->connection);
}
$this->connection = false;
}

private function isPureFtpdServer(): bool
{
if ($this->isPureFtpdServer !== null) {
Expand Down
24 changes: 22 additions & 2 deletions src/Ftp/FtpAdapterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
}

/**
Expand All @@ -45,11 +50,26 @@ 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));
}

/**
* @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
*/
Expand Down
13 changes: 9 additions & 4 deletions src/Ftp/FtpAdapterTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,9 @@ protected function setUp(): void
$this->retryOnException(UnableToConnectToFtpHost::class);
}

/**
* @var ConnectivityCheckerThatCanFail
*/
protected static $connectivityChecker;
protected static ConnectivityCheckerThatCanFail $connectivityChecker;

protected static ?StubConnectionProvider $connectionProvider;

/**
* @after
Expand All @@ -45,6 +44,12 @@ public function resetFunctionMocks(): void
reset_function_mocks();
}

public static function clearFilesystemAdapterCache(): void
{
parent::clearFilesystemAdapterCache();
static::$connectionProvider = null;
}

/**
* @test
*/
Expand Down
5 changes: 4 additions & 1 deletion src/Ftp/FtpConnectionProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/Ftp/NoopCommandConnectivityCheckerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
18 changes: 18 additions & 0 deletions src/Ftp/StubConnectionProvider.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php
declare(strict_types=1);

namespace League\Flysystem\Ftp;

class StubConnectionProvider implements ConnectionProvider
{
public mixed $connection;

public function __construct(private ConnectionProvider $provider)
{
}

public function createConnection(FtpConnectionOptions $options)
{
return $this->connection = $this->provider->createConnection($options);
}
}
4 changes: 2 additions & 2 deletions src/Ftp/UnableToConnectToFtpHost.php
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}
3 changes: 3 additions & 0 deletions src/PhpseclibV3/ConnectionProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

use phpseclib3\Net\SFTP;

/**
* @method void disconnect()
*/
interface ConnectionProvider
{
public function provideConnection(): SFTP;
Expand Down
5 changes: 5 additions & 0 deletions src/PhpseclibV3/SftpAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
24 changes: 20 additions & 4 deletions src/PhpseclibV3/SftpAdapterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public static function setUpBeforeClass(): void
}

/**
* @var ConnectionProvider
* @var StubSftpConnectionProvider
*/
private static $connectionProvider;

Expand Down Expand Up @@ -214,7 +214,24 @@ 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) {
static::$connectionProvider = new StubSftpConnectionProvider('localhost', 'foo', 'pass', 2222);
Expand All @@ -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');
}
}
7 changes: 7 additions & 0 deletions src/PhpseclibV3/SftpConnectionProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 6 additions & 1 deletion src/PhpseclibV3/SimpleConnectivityChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
13 changes: 10 additions & 3 deletions src/PhpseclibV3/StubSftpConnectionProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
class StubSftpConnectionProvider implements ConnectionProvider
{
/**
* @var SftpStub
* @var SftpStub|null
*/
private $connection;
public $connection;

public function __construct(
private string $host,
Expand All @@ -21,9 +21,16 @@ public function __construct(
) {
}

public function disconnect(): void
{
if ($this->connection) {
$this->connection->disconnect();
}
}

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);

Expand Down
Loading