Conversation
… Storage use) owncloud/core/pull/19235 Adds the ability to define directories in config.php which will not be further processed (scanned, shown, created ect). These directories exist but will not handled in nextcloud. Usecase: eg. filesystem which have the ability of snapshots have a defined naming for them. For details pls see also the documentation PR coming asap.
|
@mmattel, thanks for your PR! By analyzing the history of the files in this pull request, we identified @icewind1991, @LukasReschke and @DeepDiver1975 to be potential reviewers. |
| */ | ||
| public function createFile($name, $data = null) { | ||
|
|
||
| # the check here is necessary, because createFile uses put covered in sabre/file.php |
There was a problem hiding this comment.
# comments are deprecated in php 🙊
LukasReschke
left a comment
LukasReschke
left a comment
There was a problem hiding this comment.
I think that should be handled on storage system level and not a global setting. I'm not really in favour of that as it is quite an unexpected behaviour for users and likely won't be tested properly since it's a setting for edge cases.
If that would be handled at least on the storage level layer than that would at least only apply to the minimum storages.
Also what's the use-case behind this? What is the hardware in case exactly? Can't it be configured in another way?
cc @icewind1991
| # the check here is necessary, because createFile uses put covered in sabre/file.php | ||
| # and not touch covered in files/view.php | ||
| if (\OC\Files\Filesystem::isForbiddenFileOrDir($name)) { | ||
| throw new \Sabre\DAV\Exception\Forbidden(); |
There was a problem hiding this comment.
either this or the call above is wrong
| throw new \Sabre\DAV\Exception\Forbidden(); | ||
| } | ||
|
|
||
| $path = trim($path, '/'); |
There was a problem hiding this comment.
Swap the order of this assignment and the check before, then you can get rid of the second check below
| } | ||
|
|
||
| // check the path, also called when the path has been entered manually eg via a file explorer | ||
| if (\OC\Files\Filesystem::isForbiddenFileOrDir($path)) { |
| * | ||
| * @param array $data from hook | ||
| */ | ||
| static public function isBlacklisted($data) { |
There was a problem hiding this comment.
this method might be used by other apps?
There was a problem hiding this comment.
Yes, when I originally created the PR in owncloud, I have found one app (but I do not remember anymore...) that was using this call. Therefore I left it and forwarded the call to the new function.
| } | ||
|
|
||
| /** | ||
| /** |
nickvergessen
added
enhancement
2. developing
|
And I happen to have then not been happy with this approach nor am I yet. This adds more static code that is barely unit-test covered. This is not the way we should go.
I aimed to refer to StorageWrappers. See https://github.com/nextcloud/files_accesscontrol/blob/master/lib/StorageWrapper.php for example. This should be done using a storage-level approach in Nextcloud and not put directly into the Nextcloud core filesystem classes. |
|
@icewind1991 Please comment on the storage wrapper approach. THX. |
|
I agree. This solution is way to hacky for my taste. And it is not something that core should take care of. A simple POC to do this with an app in https://github.com/rullzer/files_excludedirs |
Current coverage is 30.59% (diff: 37.28%)@@ master #1627 diff @@
==========================================
Files 1081 1081
Lines 60023 60350 +327
Methods 6808 6846 +38
Messages 0 0
Branches 0 0
==========================================
+ Hits 18378 18465 +87
- Misses 41645 41885 +240
Partials 0 0
|
|
Closing as it should be done via an app as the PoC linked from #1627 (comment) shows. |
6 participants
Feature PR, references: Exclude Directories (merged, for professional…… Storage use) in owncloud/core/pull/19235
Adds the ability to define directories in config.php which will not be further processed (scanned, shown, created ect). These directories exist in the source filesystem but will not handled in nextcloud.
Usecase: eg. filesystems which have the ability of snapshots have a defined folder naming for these snapshots. Processing them is in most cases not sensful.
For a detailed description pls see also the documentation PR.