Skip to content

Add proper path fallback to cache entries #16911

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

Closed
juliusknorr wants to merge 3 commits into from
Closed

Add proper path fallback to cache entries #16911

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
800123d
Add proper path fallback to cache entries
juliusknorr Aug 28, 2019
8ac2284
Merge remote-tracking branch 'origin/master' into bugfix/noid/unjaile…
rullzer Aug 29, 2019
58129ed
fixup! Merge remote-tracking branch 'origin/master' into bugfix/noid/…
rullzer Aug 29, 2019
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
4 changes: 4 additions & 0 deletions apps/files_sharing/lib/Cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ protected function getRoot() {
if (is_null($this->root)) {
$absoluteRoot = $this->sourceRootInfo->getPath();

if ($this->sourceRootInfo->getMimeType() === 'httpd/unix-directory') {
$absoluteRoot = rtrim($absoluteRoot, '/') . '/';

@juliusknorr juliusknorr Aug 30, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the tests don't seem to be too happy. When just applying the the normalization if we are inside a jail storage the tests run fine. But looking the failing tests it also feels like this is fixing a symptom as well 😉

This comment was marked as outdated.

Sign in to view

@juliusknorr juliusknorr Aug 30, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rullzer It seems that the existing Jail/CacheJail logic generally assumes that the root is a string, while the FilesSharing\Cache initializes it with null. That fix would also solve the issue:

diff --git a/apps/files_sharing/lib/Cache.php b/apps/files_sharing/lib/Cache.php
index 4dbe4b75b2..b0575a4a6b 100644
--- a/apps/files_sharing/lib/Cache.php
+++ b/apps/files_sharing/lib/Cache.php
@@ -67,7 +67,7 @@ class Cache extends CacheJail {
 
                parent::__construct(
                        null,
-                       null
+                       ''
                );
        }

@rullzer rullzer Sep 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo then lets do that!

@juliusknorr juliusknorr Sep 3, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#16985

}

// the sourceRootInfo path is the absolute path of the folder in the "real" storage
// in the case where a folder is shared from a Jail we need to ensure that the share Jail
// has it's root set relative to the source Jail
Expand Down