From fe2fa96d7632a9034127c6d8068e8e3c8f5242fa Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 2 Mar 2026 22:39:56 -0500 Subject: [PATCH 1/6] refactor(files_external/SMB): modernize properties And add some comments in the constructor Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/SMB.php | 42 +++++++++------------ 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/SMB.php b/apps/files_external/lib/Lib/Storage/SMB.php index 679e3ad791c4e..dce07de0c8b5a 100644 --- a/apps/files_external/lib/Lib/Storage/SMB.php +++ b/apps/files_external/lib/Lib/Storage/SMB.php @@ -1,7 +1,7 @@ */ protected CappedMemoryCache $statCache; - - /** @var LoggerInterface */ - protected $logger; - - /** @var bool */ - protected $showHidden; - + protected LoggerInterface $logger; + protected bool $showHidden; private bool $caseSensitive; - - /** @var bool */ - protected $checkAcl; + protected bool $checkAcl; public function __construct(array $parameters) { + // Validate required connection target if (!isset($parameters['host'])) { throw new \Exception('Invalid configuration, no host provided'); } + // Resolve SMB authentication: prefer prebuilt auth, otherwise build from user/password. if (isset($parameters['auth'])) { $auth = $parameters['auth']; } elseif (isset($parameters['user']) && isset($parameters['password']) && isset($parameters['share'])) { @@ -86,6 +70,8 @@ public function __construct(array $parameters) { throw new \Exception('Invalid configuration, no credentials provided'); } + // Use injected logger when provided; otherwise fall back to the server logger. + // @todo: I suspect $parameters['logger'] is legacy since I don't see it supported elsewhere... if (isset($parameters['logger'])) { if (!$parameters['logger'] instanceof LoggerInterface) { throw new \Exception( @@ -99,6 +85,7 @@ public function __construct(array $parameters) { $this->logger = \OCP\Server::get(LoggerInterface::class); } + // Build SMB client options from configuration. $options = new Options(); if (isset($parameters['timeout'])) { $timeout = (int)$parameters['timeout']; @@ -106,19 +93,24 @@ public function __construct(array $parameters) { $options->setTimeout($timeout); } } + + // Create server + share handles used by all subsequent filesystem operations. $system = \OCP\Server::get(SystemBridge::class); $serverFactory = new ServerFactory($options, $system); $this->server = $serverFactory->createServer($parameters['host'], $auth); $this->share = $this->server->getShare(trim($parameters['share'], '/')); + // Normalize root to canonical internal form: leading slash, trailing slash. $this->root = $parameters['root'] ?? '/'; $this->root = '/' . ltrim($this->root, '/'); $this->root = rtrim($this->root, '/') . '/'; + // Normalize root to canonical internal form: leading slash, trailing slash. $this->showHidden = isset($parameters['show_hidden']) && $parameters['show_hidden']; $this->caseSensitive = (bool)($parameters['case_sensitive'] ?? true); $this->checkAcl = isset($parameters['check_acl']) && $parameters['check_acl']; + // Per-instance metadata cache for stat() results. $this->statCache = new CappedMemoryCache(); parent::__construct($parameters); } From 7ef939b9f906bbfda631b789c5591fbee5d8ba74 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 2 Mar 2026 22:49:21 -0500 Subject: [PATCH 2/6] refactor(Storage/SMB): variable naming Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/SMB.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/SMB.php b/apps/files_external/lib/Lib/Storage/SMB.php index dce07de0c8b5a..17341d4dcf85c 100644 --- a/apps/files_external/lib/Lib/Storage/SMB.php +++ b/apps/files_external/lib/Lib/Storage/SMB.php @@ -95,8 +95,8 @@ public function __construct(array $parameters) { } // Create server + share handles used by all subsequent filesystem operations. - $system = \OCP\Server::get(SystemBridge::class); - $serverFactory = new ServerFactory($options, $system); + $systemBridge = \OCP\Server::get(SystemBridge::class); + $serverFactory = new ServerFactory($options, $systemBridge); $this->server = $serverFactory->createServer($parameters['host'], $auth); $this->share = $this->server->getShare(trim($parameters['share'], '/')); @@ -112,6 +112,7 @@ public function __construct(array $parameters) { // Per-instance metadata cache for stat() results. $this->statCache = new CappedMemoryCache(); + parent::__construct($parameters); } From d97ffa5442a62aa9dc2a0e33ddbf8763b926bee3 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 2 Mar 2026 23:01:00 -0500 Subject: [PATCH 3/6] refactor(Storage/SMB): unify instance cache management w/ S3 No behavior change; just unifying implementations Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/SMB.php | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/SMB.php b/apps/files_external/lib/Lib/Storage/SMB.php index 17341d4dcf85c..1eb25d1742110 100644 --- a/apps/files_external/lib/Lib/Storage/SMB.php +++ b/apps/files_external/lib/Lib/Storage/SMB.php @@ -43,7 +43,6 @@ use Psr\Log\LoggerInterface; class SMB extends Common implements INotifyStorage { - protected \Icewind\SMB\IServer $server; protected \Icewind\SMB\IShare $share; protected string $root; @@ -110,10 +109,16 @@ public function __construct(array $parameters) { $this->caseSensitive = (bool)($parameters['case_sensitive'] ?? true); $this->checkAcl = isset($parameters['check_acl']) && $parameters['check_acl']; - // Per-instance metadata cache for stat() results. + $this->initCaches(); + parent::__construct($parameters); + } + + private function initCaches(): void { $this->statCache = new CappedMemoryCache(); + } - parent::__construct($parameters); + private function clearCaches(): void { + $this->statCache->clear(); } private function splitUser(string $user): array { @@ -497,7 +502,7 @@ public function rmdir(string $path): bool { } try { - $this->statCache = new CappedMemoryCache(); + $this->clearCaches(); $content = $this->share->dir($this->buildPath($path)); foreach ($content as $file) { if ($file->isDirectory()) { From c4dcf7256d714796fbc151616eb778389dec89ce Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 2 Mar 2026 23:05:22 -0500 Subject: [PATCH 4/6] refactor(Storage/SMB): drop unused logger via $parameters code This doesn't exist in other storages and seems to be dead code. Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/SMB.php | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/SMB.php b/apps/files_external/lib/Lib/Storage/SMB.php index 1eb25d1742110..6144436b04ded 100644 --- a/apps/files_external/lib/Lib/Storage/SMB.php +++ b/apps/files_external/lib/Lib/Storage/SMB.php @@ -69,20 +69,7 @@ public function __construct(array $parameters) { throw new \Exception('Invalid configuration, no credentials provided'); } - // Use injected logger when provided; otherwise fall back to the server logger. - // @todo: I suspect $parameters['logger'] is legacy since I don't see it supported elsewhere... - if (isset($parameters['logger'])) { - if (!$parameters['logger'] instanceof LoggerInterface) { - throw new \Exception( - 'Invalid logger. Got ' - . get_class($parameters['logger']) - . ' Expected ' . LoggerInterface::class - ); - } - $this->logger = $parameters['logger']; - } else { - $this->logger = \OCP\Server::get(LoggerInterface::class); - } + $this->logger = \OCP\Server::get(LoggerInterface::class); // Build SMB client options from configuration. $options = new Options(); From 2a9591b4c0bef785e1edac2e28b8c387b6f95a5a Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 2 Mar 2026 23:16:44 -0500 Subject: [PATCH 5/6] refactor(Storage/SMB): move SMB auth resolution to helper Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/SMB.php | 32 +++++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/SMB.php b/apps/files_external/lib/Lib/Storage/SMB.php index 6144436b04ded..f004818dd1edc 100644 --- a/apps/files_external/lib/Lib/Storage/SMB.php +++ b/apps/files_external/lib/Lib/Storage/SMB.php @@ -59,15 +59,7 @@ public function __construct(array $parameters) { throw new \Exception('Invalid configuration, no host provided'); } - // Resolve SMB authentication: prefer prebuilt auth, otherwise build from user/password. - if (isset($parameters['auth'])) { - $auth = $parameters['auth']; - } elseif (isset($parameters['user']) && isset($parameters['password']) && isset($parameters['share'])) { - [$workgroup, $user] = $this->splitUser($parameters['user']); - $auth = new BasicAuth($user, $workgroup, $parameters['password']); - } else { - throw new \Exception('Invalid configuration, no credentials provided'); - } + $auth = $this->resolveAuth($parameters); $this->logger = \OCP\Server::get(LoggerInterface::class); @@ -97,9 +89,31 @@ public function __construct(array $parameters) { $this->checkAcl = isset($parameters['check_acl']) && $parameters['check_acl']; $this->initCaches(); + + // Call parent last: SMB-specific dependencies/state must be initialized first. parent::__construct($parameters); } + /** + * Resolve SMB authentication from config. + * + * Prefer prebuilt auth, otherwise build from user/password. + * + * @throws \Exception when credentials are missing/invalid + */ + private function resolveAuth(array $parameters) { + if (isset($parameters['auth'])) { + return $parameters['auth']; + } + + if (isset($parameters['user']) && isset($parameters['password']) && isset($parameters['share'])) { + [$workgroup, $username] = $this->splitUser($parameters['user']); + return new BasicAuth($username, $workgroup, $parameters['password']); + } + + throw new \Exception('Invalid configuration, no credentials provided'); + } + private function initCaches(): void { $this->statCache = new CappedMemoryCache(); } From 9b762092292c7bb879f665dcc55baa78849a4d63 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 2 Mar 2026 23:22:15 -0500 Subject: [PATCH 6/6] refactor(Storage/SMB): move SMB client options building to helper Signed-off-by: Josh --- apps/files_external/lib/Lib/Storage/SMB.php | 32 ++++++++++----------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/SMB.php b/apps/files_external/lib/Lib/Storage/SMB.php index f004818dd1edc..8109fdf27ef73 100644 --- a/apps/files_external/lib/Lib/Storage/SMB.php +++ b/apps/files_external/lib/Lib/Storage/SMB.php @@ -58,42 +58,42 @@ public function __construct(array $parameters) { if (!isset($parameters['host'])) { throw new \Exception('Invalid configuration, no host provided'); } - + // SMB auth $auth = $this->resolveAuth($parameters); - + // SMB client options + $options = $this->buildSmbOptions($parameters); $this->logger = \OCP\Server::get(LoggerInterface::class); - - // Build SMB client options from configuration. - $options = new Options(); - if (isset($parameters['timeout'])) { - $timeout = (int)$parameters['timeout']; - if ($timeout > 0) { - $options->setTimeout($timeout); - } - } - // Create server + share handles used by all subsequent filesystem operations. $systemBridge = \OCP\Server::get(SystemBridge::class); $serverFactory = new ServerFactory($options, $systemBridge); $this->server = $serverFactory->createServer($parameters['host'], $auth); $this->share = $this->server->getShare(trim($parameters['share'], '/')); - // Normalize root to canonical internal form: leading slash, trailing slash. $this->root = $parameters['root'] ?? '/'; $this->root = '/' . ltrim($this->root, '/'); $this->root = rtrim($this->root, '/') . '/'; - // Normalize root to canonical internal form: leading slash, trailing slash. $this->showHidden = isset($parameters['show_hidden']) && $parameters['show_hidden']; $this->caseSensitive = (bool)($parameters['case_sensitive'] ?? true); $this->checkAcl = isset($parameters['check_acl']) && $parameters['check_acl']; - $this->initCaches(); - // Call parent last: SMB-specific dependencies/state must be initialized first. parent::__construct($parameters); } + private function buildSmbOptions(array $parameters): Options { + $options = new Options(); + + if (isset($parameters['timeout'])) { + $timeout = (int)$parameters['timeout']; + if ($timeout > 0) { + $options->setTimeout($timeout); + } + } + + return $options; + } + /** * Resolve SMB authentication from config. *