Port isTargetAllowed to share 2.0 #2847
Conversation
|
@LukasReschke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @schiessle, @nickvergessen and @Xenopathic to be potential reviewers. |
|
Tests are failing. 🚀 |
LukasReschke
added
2. developing
| * | ||
| * @return bool true if allowed, false otherwise | ||
| * | ||
| * @since 11.0 |
rullzer
force-pushed
the
downstream-share20-istargetallowed
branch
from
5d5dc64 to
1df37ac
Compare
|
@icewind1991 I'd like to get your opinion on this as well |
icewind1991
left a comment
icewind1991
left a comment
There was a problem hiding this comment.
I'm not happy with the way this checks if the target is shared (do we really not have an existing method to do that?), getting the node and iterating over parents is needlessly expensive
| $shareManager = \OC::$server->getShareManager(); | ||
| $targetNode = $targetNodes[0]; | ||
| // FIXME: make it stop earlier in '/$userId/files' | ||
| while (!is_null($targetNode) && $targetNode->getPath() !== '/') { |
There was a problem hiding this comment.
Why does byPath want a Node instead of a path 🙈
There was a problem hiding this comment.
We use nodes everywhere. Was just naming stuff to be somewhat siilar to the old one (you should have complained when the code was reviewed 😉 )
|
@icewind1991 well give me my closure table and I can optimize it properly.... |
rullzer
force-pushed
the
downstream-share20-istargetallowed
branch
from
1df37ac to
5c81884
Compare
|
@icewind1991 could you have a look at the failing tests so we can get this in? I know you have some objections but lets fix them separatly. |
|
Let me rebase this and get #3944 into this. |
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
| $fileId = (int)$targetStorage->getCache()->getId(dirname($targetInternalPath)); | ||
| } | ||
|
|
||
| $targetNodes = \OC::$server->getRootFolder()->getById($fileId); |
There was a problem hiding this comment.
The first unit test fails here.
1) OCA\Files_Sharing\Tests\UpdaterTest::testDeleteParentFolder
Failed asserting that true is false.
/drone/src/github.com/nextcloud/server/apps/files_sharing/tests/UpdaterTest.php:105
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke
force-pushed
the
downstream-share20-istargetallowed
branch
from
5c81884 to
a6f4096
Compare
|
@icewind1991 Please have a look at the failing tests. Would be nice to get this in and I don't properly understand this :/ |
|
Nothing for 12 anymore |
From owncloud/core#26615