Skip to content

refactor(cache-storage): Make Storage and StorageGlobal psalm strict #58826

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.

Sign up for GitHub

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

Merged
come-nc merged 2 commits into from
Mar 10, 2026
Merged

refactor(cache-storage): Make Storage and StorageGlobal psalm strict #58826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
140bfa2
refactor(cache-storage): Make Storage and StorageGlobal psalm strict
CarlSchwan Mar 10, 2026
783e2ac
refactor(external-storage-services): Modernize code
CarlSchwan Mar 10, 2026
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
39 changes: 11 additions & 28 deletions apps/files_external/lib/Service/GlobalStoragesService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,14 @@
use OCA\Files_External\Lib\StorageConfig;
use OCA\Files_External\MountConfig;
use OCP\IGroup;
use Override;

/**
* Service class to manage global external storage
*/
class GlobalStoragesService extends StoragesService {
/**
* Triggers $signal for all applicable users of the given
* storage
*
* @param StorageConfig $storage storage data
* @param string $signal signal to trigger
*/
protected function triggerHooks(StorageConfig $storage, $signal) {
#[Override]
protected function triggerHooks(StorageConfig $storage, $signal): void {
// FIXME: Use as expression in empty once PHP 5.4 support is dropped
$applicableUsers = $storage->getApplicableUsers();
$applicableGroups = $storage->getApplicableGroups();
Expand Down Expand Up @@ -55,15 +50,8 @@ protected function triggerHooks(StorageConfig $storage, $signal) {
);
}

/**
* Triggers signal_create_mount or signal_delete_mount to
* accommodate for additions/deletions in applicableUsers
* and applicableGroups fields.
*
* @param StorageConfig $oldStorage old storage config
* @param StorageConfig $newStorage new storage config
*/
protected function triggerChangeHooks(StorageConfig $oldStorage, StorageConfig $newStorage) {
#[Override]
protected function triggerChangeHooks(StorageConfig $oldStorage, StorageConfig $newStorage): void {
// if mount point changed, it's like a deletion + creation
if ($oldStorage->getMountPoint() !== $newStorage->getMountPoint()) {
$this->eventDispatcher->dispatchTyped(new StorageDeletedEvent($oldStorage));
Expand Down Expand Up @@ -139,16 +127,13 @@ protected function triggerChangeHooks(StorageConfig $oldStorage, StorageConfig $
}
}

/**
* Get the visibility type for this controller, used in validation
*
* @return int BackendService::VISIBILITY_* constants
*/
public function getVisibilityType() {
#[Override]
public function getVisibilityType(): int {
return BackendService::VISIBILITY_ADMIN;
}

protected function isApplicable(StorageConfig $config) {
#[Override]
protected function isApplicable(StorageConfig $config): bool {
return true;
}

Expand All @@ -157,16 +142,14 @@ protected function isApplicable(StorageConfig $config) {
*
* @return StorageConfig[] map of storage id to storage config
*/
public function getStorageForAllUsers() {
public function getStorageForAllUsers(): array {
$mounts = $this->dbConfig->getAllMounts();
$configs = array_map([$this, 'getStorageConfigFromDBMount'], $mounts);
$configs = array_filter($configs, function ($config) {
return $config instanceof StorageConfig;
});

$keys = array_map(function (StorageConfig $config) {
return $config->getId();
}, $configs);
$keys = array_map(static fn (StorageConfig $config) => $config->getId(), $configs);

return array_combine($keys, $configs);
}
Expand Down
117 changes: 36 additions & 81 deletions apps/files_external/lib/Service/StoragesService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,10 @@

/**
* Service class to manage external storage
*
* @psalm-import-type ExternalMountInfo from DBConfigService
*/
abstract class StoragesService {

/**
* @param BackendService $backendService
* @param DBConfigService $dbConfig
* @param IEventDispatcher $eventDispatcher
*/
public function __construct(
protected BackendService $backendService,
protected DBConfigService $dbConfig,
Expand All @@ -47,24 +43,21 @@ public function __construct(
) {
}

protected function readDBConfig() {
/**
* @return list<ExternalMountInfo>
*/
protected function readDBConfig(): array {
return $this->dbConfig->getAdminMounts();
}

protected function getStorageConfigFromDBMount(array $mount) {
$applicableUsers = array_filter($mount['applicable'], function ($applicable) {
return $applicable['type'] === DBConfigService::APPLICABLE_TYPE_USER;
});
$applicableUsers = array_map(function ($applicable) {
return $applicable['value'];
}, $applicableUsers);

$applicableGroups = array_filter($mount['applicable'], function ($applicable) {
return $applicable['type'] === DBConfigService::APPLICABLE_TYPE_GROUP;
});
$applicableGroups = array_map(function ($applicable) {
return $applicable['value'];
}, $applicableGroups);
protected function getStorageConfigFromDBMount(array $mount): ?StorageConfig {
$applicableUsers = array_filter($mount['applicable'], static fn (array $applicable): bool
=> $applicable['type'] === DBConfigService::APPLICABLE_TYPE_USER);
$applicableUsers = array_map(static fn (array $applicable) => $applicable['value'], $applicableUsers);

$applicableGroups = array_filter($mount['applicable'], static fn (array $applicable): bool
=> $applicable['type'] === DBConfigService::APPLICABLE_TYPE_GROUP);
$applicableGroups = array_map(static fn (array $applicable) => $applicable['value'], $applicableGroups);

try {
$config = $this->createStorage(
Expand Down Expand Up @@ -99,18 +92,14 @@ protected function getStorageConfigFromDBMount(array $mount) {
/**
* Read the external storage config
*
* @return array map of storage id to storage config
* @return array<int, StorageConfig> map of storage id to storage config
*/
protected function readConfig() {
protected function readConfig(): array {
$mounts = $this->readDBConfig();
$configs = array_map([$this, 'getStorageConfigFromDBMount'], $mounts);
$configs = array_filter($configs, function ($config) {
return $config instanceof StorageConfig;
});
$configs = array_map($this->getStorageConfigFromDBMount(...), $mounts);
$configs = array_filter($configs);

$keys = array_map(function (StorageConfig $config) {
return $config->getId();
}, $configs);
$keys = array_map(static fn (StorageConfig $config): int => $config->getId(), $configs);

return array_combine($keys, $configs);
}
Expand Down Expand Up @@ -139,18 +128,15 @@ public function getStorage(int $id): StorageConfig {

/**
* Check whether this storage service should provide access to a storage
*
* @param StorageConfig $config
* @return bool
*/
abstract protected function isApplicable(StorageConfig $config);
abstract protected function isApplicable(StorageConfig $config): bool;

/**
* Gets all storages, valid or not
*
* @return StorageConfig[] array of storage configs
*/
public function getAllStorages() {
public function getAllStorages(): array {
return $this->readConfig();
}

Expand All @@ -159,18 +145,16 @@ public function getAllStorages() {
*
* @return StorageConfig[]
*/
public function getStorages() {
return array_filter($this->getAllStorages(), [$this, 'validateStorage']);
public function getStorages(): array {
return array_filter($this->getAllStorages(), $this->validateStorage(...));
}

/**
* Validate storage
* FIXME: De-duplicate with StoragesController::validate()
*
* @param StorageConfig $storage
* @return bool
*/
protected function validateStorage(StorageConfig $storage) {
protected function validateStorage(StorageConfig $storage): bool {
/** @var Backend */
$backend = $storage->getBackend();
/** @var AuthMechanism */
Expand All @@ -180,25 +164,19 @@ protected function validateStorage(StorageConfig $storage) {
// not permitted to use backend
return false;
}
if (!$authMechanism->isVisibleFor($this->getVisibilityType())) {
// not permitted to use auth mechanism
return false;
}

return true;
// permitted to use auth mechanism
return $authMechanism->isVisibleFor($this->getVisibilityType());
}

/**
* Get the visibility type for this controller, used in validation
*
* @return int BackendService::VISIBILITY_* constants
* @return BackendService::VISIBILITY_*
*/
abstract public function getVisibilityType();
abstract public function getVisibilityType(): int;

/**
* @return integer
*/
protected function getType() {
protected function getType(): int {
return DBConfigService::MOUNT_TYPE_ADMIN;
}

Expand All @@ -209,7 +187,7 @@ protected function getType() {
*
* @return StorageConfig storage config, with added id
*/
public function addStorage(StorageConfig $newStorage) {
public function addStorage(StorageConfig $newStorage): StorageConfig {
$allStorages = $this->readConfig();

$configId = $this->dbConfig->addMount(
Expand Down Expand Up @@ -275,7 +253,7 @@ public function createStorage(
?array $applicableUsers = null,
?array $applicableGroups = null,
?int $priority = null,
) {
): StorageConfig {
$backend = $this->backendService->getBackend($backendIdentifier);
if (!$backend) {
$backend = new InvalidBackend($backendIdentifier);
Expand Down Expand Up @@ -313,7 +291,7 @@ public function createStorage(
* @param string $mountType hook mount type param
* @param array $applicableArray array of applicable users/groups for which to trigger the hook
*/
protected function triggerApplicableHooks($signal, $mountPoint, $mountType, $applicableArray): void {
protected function triggerApplicableHooks(string $signal, string $mountPoint, string $mountType, array $applicableArray): void {
$this->eventDispatcher->dispatchTyped(new InvalidateMountCacheEvent(null));
foreach ($applicableArray as $applicable) {
Util::emitHook(
Expand All @@ -335,7 +313,7 @@ protected function triggerApplicableHooks($signal, $mountPoint, $mountType, $app
* @param StorageConfig $storage storage data
* @param string $signal signal to trigger
*/
abstract protected function triggerHooks(StorageConfig $storage, $signal);
abstract protected function triggerHooks(StorageConfig $storage, string $signal): void;

/**
* Triggers signal_create_mount or signal_delete_mount to
Expand All @@ -345,7 +323,7 @@ abstract protected function triggerHooks(StorageConfig $storage, $signal);
* @param StorageConfig $oldStorage old storage data
* @param StorageConfig $newStorage new storage data
*/
abstract protected function triggerChangeHooks(StorageConfig $oldStorage, StorageConfig $newStorage);
abstract protected function triggerChangeHooks(StorageConfig $oldStorage, StorageConfig $newStorage): void;

/**
* Update storage to the configuration
Expand All @@ -355,7 +333,7 @@ abstract protected function triggerChangeHooks(StorageConfig $oldStorage, Storag
* @return StorageConfig storage config
* @throws NotFoundException if the given storage does not exist in the config
*/
public function updateStorage(StorageConfig $updatedStorage) {
public function updateStorage(StorageConfig $updatedStorage): StorageConfig {
$id = $updatedStorage->getId();

$existingMount = $this->dbConfig->getMountById($id);
Expand Down Expand Up @@ -435,7 +413,7 @@ public function updateStorage(StorageConfig $updatedStorage) {
*
* @throws NotFoundException if no storage was found with the given id
*/
public function removeStorage(int $id) {
public function removeStorage(int $id): void {
$existingMount = $this->dbConfig->getMountById($id);

if (!is_array($existingMount)) {
Expand All @@ -454,29 +432,6 @@ public function removeStorage(int $id) {
$this->updateOverwriteHomeFolders();
}

/**
* Construct the storage implementation
*
* @param StorageConfig $storageConfig
* @return int
*/
private function getStorageId(StorageConfig $storageConfig) {
try {
$class = $storageConfig->getBackend()->getStorageClass();
/** @var \OC\Files\Storage\Storage $storage */
$storage = new $class($storageConfig->getBackendOptions());

// auth mechanism should fire first
$storage = $storageConfig->getBackend()->wrapStorage($storage);
$storage = $storageConfig->getAuthMechanism()->wrapStorage($storage);

/** @var \OC\Files\Storage\Storage $storage */
return $storage->getStorageCache()->getNumericId();
} catch (\Exception $e) {
return -1;
}
}

public function updateOverwriteHomeFolders(): void {
$appIdsList = $this->appConfig->getValueArray(FilesApplication::APP_ID, ConfigLexicon::OVERWRITES_HOME_FOLDERS);

Expand Down
Loading
Loading