Files sharing ported to Share API 2 #4382
Conversation
…d, this will be replaced by the controller using the share types directly
|
I just wanted to have a look at the encryption app issues. But I stopped at another problem (also with encryption disabled): If I want to share a file or folder I get a "The file does not exist in the filesystem" error in the web interface. No errors in the apache error log, the owncloud.log or the JS console. Of course the file/folder exists, I can edit it, download it, etc. |
|
While reading and seeing the diffstats this does not look like a »careful refactoring« but more like a »crazy rewrite«. I see 10k lines of new code, the upgrade procedure sounds like a second class citizen (will it explode in our faces? Esp. on installations with large amounts of users?) Commit message says just files upgrade is fixed – what about calendars? Or any other type of share that can exists in other (maybe even unknown) apps? Upgrade is most critical. We have deprecated methods in the Share API (without changes in their body, will they even work?) despite we want to have a stable API. Sorry, but I do not have a good feeling with it so far. |
|
I agree. Let's see if we have a working and stable codebase in a week. If not than we will drop this and go with the old sharing code. We can't introduce new bugs and instability here. |
… respect variable types
|
@schiesbn Sorry about that, I didn't catch that because I was using the new UI for testing and didn't notice that the old one was passing strings in. It's fixed now. I also added this method: https://github.com/owncloud/core/blob/files_sharing-ported/apps/files_sharing/lib/share/filesharefetcher.php#L281 which I believe is what you wanted for encryption. I'm going to try fixing encryption again. |
|
Test failed. |
Conflicts: lib/public/share.php
|
@schiesbn I ported getUsersSharingFile, but that is not the only old dependence. There are multiple dangerous manual queries in Util that should be removed.
I can't follow what kind of path you're trying to generate for the hooks: https://github.com/owncloud/core/blob/master/apps/files_encryption/hooks/hooks.php#L257 And the users with access won't change for different children files: https://github.com/owncloud/core/blob/master/apps/files_encryption/hooks/hooks.php#L354 That is probably a huge performance drain. It would probably be better/easier to just switch to the new hooks. |
|
Test failed. |
I'm not entirely sure but if I remember correctly it was introduced because getPath() is not unique. For example if the file is located in my Shared folder I will get my Shared-path and not the path from the original owner, but that's the path we need here.
They where introduced because the where no equivalent method in the share API. As you can see at the code where this functions are used it is not that easy to generate always the correct path for re-re-shares, etc. Maybe we can clean this up with the new share API, I don't know yet. Which methods from the new share API would you suggest as a replacement? But I would prefer to do this in a second step. First update the queries (if necessary) so that it works as before and than clean it up directly after OC6 so that we really have some time for testing.
Yes, it looks like this function is no longer in use. I think we can remove it.
I didn't looked at the new hooks until now, so I don't know how complex the changes are. But I don't think it is a valid option at this point in time. We already have enough work to do. Let's avoid to add things which are not necessary. |
|
I played a bit around with the file sharing. Tried some re-re-shares with nested folders. Shared files from external storages, public link share... Everything worked as expected, that's great! I just noticed two thing, If I activate the "Password protect" checkbox the password field appears but immediately disappears again. This only happens in this branch, master works fine. The drop-down to adjust the permissions still doesn't appear, as I already mentioned above |
|
@MTGap Can you give us an update about the upgrade mechanism? About three weeks ago you wrote:
Thomas and I provided some ideas. What's the state here? Do you have a solution for the upgrade in case not all share managers are registered when the update starts? What currently works and what doesn't work? |
|
I found one more issue while working on #4499, you have three users: user1, user2 and user3 user1 shares file test.txt to user3 User3 will see two files both named test.txt and both will point to the same file. |
The update is working fine, but should have more testing. It doesn't update shares if an app is enabled after the initial update and will log messages that some shares weren't updated. I'll probably just add a hook for when apps are enabled to trigger another update.
I'm aware, it's an easy fix.
It works fine for me using Chromium. |
It used to be the opposite and I just changed it, so that's not true. I'm pretty sure you do want the path that the current logged in user has access to the file inside the hooks. Can you look at that again? I'll be trying it tomorrow. |
|
Test failed. |
Doesn't work in Firefox 23 and Chromium 29.0.1547.57 |
The port of the files_sharing app to the Share API 2. This is a branch of #3969 so you can just checkout this branch to test it.
Things that don't work:
The old share ajax file was hacked a bit to get it working, that's why some of the above things are broken. The new controller has already been written and is much cleaner. It's just the UI changes that are slowing things down now.
Classes that you should look at to understand the Share API changes:
@karlitschek @DeepDiver1975