-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix(files): Don't attempt to format a partial cache entry #43131
fix(files): Don't attempt to format a partial cache entry #43131
Conversation
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@@ -91,7 +91,7 @@ protected function formatCacheEntry($entry) { | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @return
here is still wrong 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I could return false
if $result is in array. But not sure if this fixes bugs or creates more …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To do or not to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 Not if you have no idea of the consequences. But at some point this return stack needs to be cleaned up and return the correct types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I would prefer to leave it
partial cache entries are the legacy gift that keep on giving and should really be removed |
/backport to stable28 |
/backport to stable27 |
/backport to stable26 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be continued…
Summary
\OC\Files\Cache\Wrapper\CacheWrapper::get
should return\OCP\Files\Cache\ICacheEntry
but home cache sometimes returns partial cache entries that are plain arrays. If they are processed in\OC\Files\Cache\Wrapper\CacheWrapper::formatCacheEntry
and overrides, information might be missing. This happens in\OCA\Files_Sharing\Cache::formatCacheEntry
when there is no path and\OC\Files\Cache\Wrapper\CacheJail::getJailedPath
returns null. Using null for the path passed to\OCA\Files_Sharing\SharedStorage::getPermissions
eventually leads to nextcloud/files_accesscontrol#464.So the theory and the tale. This is my first time touching cache wrappers. Double and tripple check what I'm doing 🙏
TODO
How to test
7.) Copy a file into S1 (due to [Bug]: Move or copy files and folders fails #42469 you might need cadaver)
Checklist