Add using casing check/fix for initMountPoints#1915
Conversation
|
@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @icewind1991 and @DeepDiver1975 to be potential reviewers. |
|
I tested this code and at least my tests didn't revealed anything bad. |
lib/private/Share/Share.php
Outdated
|
|
||
| if (is_null($ownerUser)) { | ||
| \OCP\Util::writeLog('files', ' Backends provided no user object for ' . $ownerUser, \OCP\Util::ERROR); | ||
| throw new \OC\User\NoUserException('Backends provided no user object for ' . $ownerUser); |
There was a problem hiding this comment.
I'd prefer not adding potential user controlled content to the exception.
There was a problem hiding this comment.
Btw shouldnt the isnull check gp againsr userObject?
There was a problem hiding this comment.
… that probably too … or be below L147. Good catch ❗️
There was a problem hiding this comment.
be below L147. Good catch
This makes no sense, because if the userObject is null then the method will crash. 😉
There was a problem hiding this comment.
Yeah. But this checks for $ownerUser and not $userObject? $ownerUser is according to PHPDoc a string.
There was a problem hiding this comment.
yep - that's the reason why @nickvergessen comment is correct ;)
There was a problem hiding this comment.
Ah. Gotcha. I thought you referred to our comments 🙈
MorrisJobke
added
2. developing
MorrisJobke
added
3. to review
|
@nickvergessen @LukasReschke I fixed the issues. Please review again 🤔 |
lib/private/Share/Share.php
Outdated
| * not '/admin/data/file.txt' | ||
| */ | ||
| public static function getUsersSharingFile($path, $ownerUser, $includeOwner = false, $returnUserPaths = false, $recursive = true) { | ||
| $userManager = \OC::$server->getUserManager(); |
There was a problem hiding this comment.
Let me also look into that…
LukasReschke
left a comment
LukasReschke
left a comment
There was a problem hiding this comment.
Let me try to do some changes here to make the code cleaner.
lib/private/Files/Node/Root.php
Outdated
| * @return \OCP\Files\Folder | ||
| */ | ||
| public function getUserFolder($userId) { | ||
| $userObject = \OC::$server->getUserManager()->get($userId); |
lib/private/Files/Node/Root.php
Outdated
| $userObject = \OC::$server->getUserManager()->get($userId); | ||
|
|
||
| if (is_null($userObject)) { | ||
| \OCP\Util::writeLog('files', 'Backends provided no user object for ' . $userId, \OCP\Util::ERROR); |
lib/private/Share/Share.php
Outdated
| * not '/admin/data/file.txt' | ||
| */ | ||
| public static function getUsersSharingFile($path, $ownerUser, $includeOwner = false, $returnUserPaths = false, $recursive = true) { | ||
| $userManager = \OC::$server->getUserManager(); |
There was a problem hiding this comment.
Let me also look into that…
lib/private/Share/Share.php
Outdated
| $userObject = $userManager->get($ownerUser); | ||
|
|
||
| if (is_null($userObject)) { | ||
| \OCP\Util::writeLog('files', ' Backends provided no user object for ' . $ownerUser, \OCP\Util::ERROR); |
Signed-off-by: Morris Jobke <[email protected]>
Signed-off-by: Lukas Reschke <[email protected]>
Signed-off-by: Lukas Reschke <[email protected]>
LukasReschke
force-pushed
the
downstream-26398
branch
from
ac8f5f1 to
6920e60
Compare
LukasReschke
dismissed
their stale review
Adjusted the code to use DI and added some tests where possible.
|
@MorrisJobke @nickvergessen Did some changes so we have proper DI. Best to go through commits, 90% of it are test changes. Please review. LGTM for me on the original changes. Also cc @icewind1991 since it's filesystem stuff. |
|
👍 for @LukasReschke changes |
|
😭 |
5 participants
@icewind1991 Could you have a look at this, because it involves filesystem stuff
cc @rullzer @nickvergessen