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. |
|
|
||
| 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 🤔 |
| @@ -136,6 +136,15 @@ public static function isEnabled() { | |||
| * 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…
| @@ -347,6 +347,15 @@ public function getName() { | |||
| * @return \OCP\Files\Folder | |||
| */ | |||
| public function getUserFolder($userId) { | |||
| $userObject = \OC::$server->getUserManager()->get($userId); | |||
| $userObject = \OC::$server->getUserManager()->get($userId); | ||
|
|
||
| if (is_null($userObject)) { | ||
| \OCP\Util::writeLog('files', 'Backends provided no user object for ' . $userId, \OCP\Util::ERROR); |
| @@ -136,6 +136,15 @@ public static function isEnabled() { | |||
| * 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…
| $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 |
|
😭 |
@icewind1991 Could you have a look at this, because it involves filesystem stuff
cc @rullzer @nickvergessen