diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php index 93f85a888b..5e2426ecc7 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -182,7 +182,7 @@ class MountProvider implements IMountProvider { // such issue can usually happen when dealing with // null groups which usually appear with group backend // caching inconsistencies - \OC::$server->getLogger()->debug( + $this->logger->debug( 'Could not adjust share target for share ' . $share->getId() . ' to make it consistent: ' . $e->getMessage(), ['app' => 'files_sharing'] ); diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 8b18abd5e9..40009dbfd8 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -37,7 +37,10 @@ namespace OC\Group; use OC\Hooks\PublicEmitter; use OCP\GroupInterface; +use OCP\IGroup; use OCP\IGroupManager; +use OCP\ILogger; +use OCP\IUser; /** * Class Manager @@ -78,11 +81,16 @@ class Manager extends PublicEmitter implements IGroupManager { /** @var \OC\SubAdmin */ private $subAdmin = null; + /** @var ILogger */ + private $logger; + /** * @param \OC\User\Manager $userManager + * @param ILogger $logger */ - public function __construct(\OC\User\Manager $userManager) { + public function __construct(\OC\User\Manager $userManager, ILogger $logger) { $this->userManager = $userManager; + $this->logger = $logger; $cachedGroups = & $this->cachedGroups; $cachedUserGroups = & $this->cachedUserGroups; $this->listen('\OC\Group', 'postDelete', function ($group) use (&$cachedGroups, &$cachedUserGroups) { @@ -186,7 +194,7 @@ class Manager extends PublicEmitter implements IGroupManager { * @return bool */ public function groupExists($gid) { - return !is_null($this->get($gid)); + return $this->get($gid) instanceof IGroup; } /** @@ -194,7 +202,7 @@ class Manager extends PublicEmitter implements IGroupManager { * @return \OC\Group\Group */ public function createGroup($gid) { - if ($gid === '' || is_null($gid)) { + if ($gid === '' || $gid === null) { return false; } else if ($group = $this->get($gid)) { return $group; @@ -224,10 +232,10 @@ class Manager extends PublicEmitter implements IGroupManager { $groupIds = $backend->getGroups($search, $limit, $offset); foreach ($groupIds as $groupId) { $aGroup = $this->get($groupId); - if (!is_null($aGroup)) { + if ($aGroup instanceof IGroup) { $groups[$groupId] = $aGroup; } else { - \OC::$server->getLogger()->debug('Group "' . $groupId . '" was returned by search but not found through direct access', array('app' => 'core')); + $this->logger->debug('Group "' . $groupId . '" was returned by search but not found through direct access', ['app' => 'core']); } } if (!is_null($limit) and $limit <= 0) { @@ -242,7 +250,7 @@ class Manager extends PublicEmitter implements IGroupManager { * @return \OC\Group\Group[] */ public function getUserGroups($user) { - if (is_null($user)) { + if (!$user instanceof IUser) { return []; } return $this->getUserIdGroups($user->getUID()); @@ -262,10 +270,10 @@ class Manager extends PublicEmitter implements IGroupManager { if (is_array($groupIds)) { foreach ($groupIds as $groupId) { $aGroup = $this->get($groupId); - if (!is_null($aGroup)) { + if ($aGroup instanceof IGroup) { $groups[$groupId] = $aGroup; } else { - \OC::$server->getLogger()->debug('User "' . $uid . '" belongs to deleted group: "' . $groupId . '"', array('app' => 'core')); + $this->logger->debug('User "' . $uid . '" belongs to deleted group: "' . $groupId . '"', ['app' => 'core']); } } } diff --git a/lib/private/Server.php b/lib/private/Server.php index bcbe23b018..2ee612a0ad 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -227,7 +227,7 @@ class Server extends ServerContainer implements IServerContainer { return new \OC\User\Manager($config); }); $this->registerService('GroupManager', function (Server $c) { - $groupManager = new \OC\Group\Manager($this->getUserManager()); + $groupManager = new \OC\Group\Manager($this->getUserManager(), $this->getLogger()); $groupManager->listen('\OC\Group', 'preCreate', function ($gid) { \OC_Hook::emit('OC_Group', 'pre_createGroup', array('run' => true, 'gid' => $gid)); }); diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index c57ef0b34c..1fb8119b92 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -23,17 +23,21 @@ namespace Test\Group; use OC\User\Manager; +use OCP\ILogger; use OCP\IUser; use OCP\GroupInterface; class ManagerTest extends \Test\TestCase { /** @var Manager|\PHPUnit_Framework_MockObject_MockObject $userManager */ protected $userManager; + /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject $userManager */ + protected $logger; protected function setUp() { parent::setUp(); $this->userManager = $this->createMock(Manager::class); + $this->logger = $this->createMock(ILogger::class); } private function getTestUser($userId) { @@ -91,7 +95,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $group = $manager->get('group1'); @@ -100,7 +104,7 @@ class ManagerTest extends \Test\TestCase { } public function testGetNoBackend() { - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $this->assertNull($manager->get('group1')); } @@ -115,7 +119,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(false)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $this->assertNull($manager->get('group1')); @@ -125,7 +129,7 @@ class ManagerTest extends \Test\TestCase { $backend = new \Test\Util\Group\Dummy(); $backend->createGroup('group1'); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $group = $manager->get('group1'); @@ -152,7 +156,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -179,7 +183,7 @@ class ManagerTest extends \Test\TestCase { $backendGroupCreated = true; }));; - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -198,7 +202,7 @@ class ManagerTest extends \Test\TestCase { $backend->expects($this->never()) ->method('createGroup'); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -219,7 +223,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $groups = $manager->search('1'); @@ -253,7 +257,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -290,7 +294,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -321,7 +325,7 @@ class ManagerTest extends \Test\TestCase { */ $userManager = $this->createMock('\OC\User\Manager'); - $manager = new \OC\Group\Manager($userManager); + $manager = new \OC\Group\Manager($userManager, $this->logger); $manager->addBackend($backend); $groups = $manager->search('1'); @@ -342,7 +346,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $groups = $manager->getUserGroups($this->getTestUser('user1')); @@ -396,7 +400,7 @@ class ManagerTest extends \Test\TestCase { */ $userManager = $this->createMock('\OC\User\Manager'); $userBackend = $this->createMock('\OC_User_Backend'); - $manager = new \OC\Group\Manager($userManager); + $manager = new \OC\Group\Manager($userManager, $this->logger); $manager->addBackend($backend); /** @var \OC\User\User $user */ @@ -420,7 +424,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $this->assertTrue($manager->isInGroup('user1', 'group1')); @@ -439,7 +443,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $this->assertTrue($manager->isAdmin('user1')); @@ -458,7 +462,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $this->assertFalse($manager->isAdmin('user1')); @@ -489,7 +493,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -552,7 +556,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3'); @@ -616,7 +620,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1); @@ -684,7 +688,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1, 1); @@ -728,7 +732,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', ''); @@ -771,7 +775,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1); @@ -814,7 +818,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1, 1); @@ -842,7 +846,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); // prime cache @@ -885,7 +889,7 @@ class ManagerTest extends \Test\TestCase { ->method('removeFromGroup') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); // prime cache @@ -915,7 +919,7 @@ class ManagerTest extends \Test\TestCase { ->with('user1') ->will($this->returnValue(null)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $groups = $manager->getUserIdGroups('user1'); @@ -941,7 +945,7 @@ class ManagerTest extends \Test\TestCase { ['group2', ['gid' => 'group2']], ])); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); // group with display name