Skip to content

Commit

Permalink
review adjustments
Browse files Browse the repository at this point in the history
  • Loading branch information
mrow4a committed Jan 15, 2023
1 parent b6c240b commit c30556b
Showing 1 changed file with 10 additions and 9 deletions.
19 changes: 10 additions & 9 deletions apps/files_sharing/lib/External/Storage.php
Expand Up @@ -226,10 +226,10 @@ public function test() {
*/
public function checkStorageAvailability() {
// WARNING: Disabling this setting is recommended only in tightly integrated federated setups as temporarly setting,
// should NOT be used in decentralized federation setup. It could cause bad user experience when dealing
// with deleted external shares
// should NOT be used in decentralized federation setup. It could cause bad user experience when dealing
// with deleted external shares
// NOTE: This allows administrator to disable cleanup of invalid shares so only manual removal by user or admin is possible,
// this is particularly useful when dealing with complex migrations that could cause unexpected server responses and instabilities.
// this is particularly useful when dealing with complex migrations that could cause unexpected server responses and instabilities.
$cleanupInvalidEnabled = $this->config->getAppValue('files_sharing', 'enable_cleanup_invalid_external_shares', 'yes');

// see if we can find out why the share is unavailable
Expand All @@ -242,7 +242,7 @@ public function checkStorageAvailability() {

if ($cleanupInvalidEnabled === 'yes') {
// FIXME: for now delete, but maybe provide a dialog in the future to the user
// to inform that share no longer valid and should contact owner? Likely big refactor
// to inform that share no longer valid and should contact owner? Likely big refactor
$this->manager->removeShare($this->mountPoint);
$this->manager->getMountManager()->removeMount($this->mountPoint);
$this->logger->error(
Expand Down Expand Up @@ -277,21 +277,22 @@ public function checkStorageAvailability() {
// can either mean that the share no longer exists or there is no ownCloud on
// the remote (proxy could return 404 for that path), check remote if accessible
if ($this->testRemote()) {
// valid ownCloud instance means that the external share no longer exists
// valid ownCloud instance means that the external share no longer exists,
// since this is permanent (re-sharing the file will create a new token)

// we remove the invalid storage
if ($cleanupInvalidEnabled === 'yes') {
// FIXME: for now delete, but maybe provide a dialog in the future to the user
// to inform that share no longer valid and should contact owner? Likely big refactor
// WARNING: we remove here to improve user experience for 99,9% of cases, but it can happen that
// server is misbehaving/unstable (returning wrong responses) and valid share gets removed
// to inform that share no longer valid and should contact owner? Likely big refactor
// NOTE: we remove share here to improve user experience for 99,9% of cases, however it can happen that
// server is misbehaving/unstable (returning wrong responses) and valid share gets removed
$this->manager->removeShare($this->mountPoint);
$this->manager->getMountManager()->removeMount($this->mountPoint);
$this->logger->error(
'Storage for external share {shareId} returns not found error. Removing share as testing of remote succeeded.',
['shareId' => $this->getId()]
);
} else {
// Ignore removal due to app config
$this->logger->error(
'Storage for external share {shareId} returns not found error. Ignoring due to app config files_sharing.enable_cleanup_invalid_external_shares={cleanupInvalidEnabled}.',
['shareId' => $this->getId(), 'cleanupInvalidEnabled' => $cleanupInvalidEnabled]
Expand Down

0 comments on commit c30556b

Please sign in to comment.