From bd7fc29d578f765cae6cb28f7284baccbaf6cecc Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sun, 19 Jul 2026 13:35:25 +0200 Subject: [PATCH] fix(NavigationManager): resolve closure and event entries when all apps loaded Signed-off-by: Ferdinand Thiessen --- lib/OC.php | 3 + lib/private/NavigationManager.php | 43 ++++++--- tests/lib/NavigationManagerTest.php | 131 +++++++++++++++++++++++++++- 3 files changed, 161 insertions(+), 16 deletions(-) diff --git a/lib/OC.php b/lib/OC.php index a39f15f0bba..1f696846b35 100644 --- a/lib/OC.php +++ b/lib/OC.php @@ -7,6 +7,7 @@ declare(strict_types=1); * SPDX-License-Identifier: AGPL-3.0-only */ +use OC\NavigationManager; use OC\Profiler\BuiltInProfiler; use OC\Security\CSP\ContentSecurityPolicyNonceManager; use OC\Share20\GroupDeletedListener; @@ -1203,6 +1204,8 @@ class OC { } // All apps are now loaded to handle the request + Server::get(NavigationManager::class)->setup(); + // if we are not on CLI, try to match the request to a route and handle it if (!self::$CLI) { try { diff --git a/lib/private/NavigationManager.php b/lib/private/NavigationManager.php index 468e2617d67..4942ba47493 100644 --- a/lib/private/NavigationManager.php +++ b/lib/private/NavigationManager.php @@ -166,10 +166,14 @@ class NavigationManager implements INavigationManager { /** * removes all the entries */ - public function clear(bool $loadDefaultLinks = true): void { + public function clear(bool $resetInit = true): void { $this->entries = []; $this->closureEntries = []; - $this->init = !$loadDefaultLinks; + + if ($resetInit) { + $this->loadedAppInfo = []; + $this->init = false; + } } #[Override] @@ -203,20 +207,25 @@ class NavigationManager implements INavigationManager { } /** - * Resolve the app navigation entries from closures and info.xml files. + * Setup the navigation manager. + * @internal - This is only used by Nextcloud core to setup the navigation manager. It is not intended for use by apps. */ - private function resolveAppNavigationEntries(): void { + public function setup(): void { // Resolve app navigation closures while ($c = array_pop($this->closureEntries)) { $this->add($c()); } // Resolve dynamically added navigation entries via event listeners - if ($this->loadedAppInfo === []) { - $this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent()); - } + $this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent()); + } - // Resolve classic info.xml based navigation entries + /** + * Resolve classic info.xml based navigation entires. + * Some code relies on this to be available earlier then the app loading finished. + * So we need to resolve the navigation entries here, even if not all apps are loaded yet. + */ + private function resolveAppNavigationEntries(): void { if ($this->userSession->isLoggedIn()) { $user = $this->userSession->getUser(); $apps = $this->appManager->getEnabledAppsForUser($user); @@ -225,16 +234,23 @@ class NavigationManager implements INavigationManager { } foreach ($apps as $app) { - // skip already loaded apps - if (in_array($app, $this->loadedAppInfo)) { + if (in_array($app, $this->loadedAppInfo, true)) { + // already loaded + continue; + } + if (!$this->appManager->isAppLoaded($app)) { + // app is not loaded yet, skip it continue; } // load plugins and collections from info.xml $info = $this->appManager->getAppInfo($app); if (!isset($info['navigations']['navigation'])) { + // this app does not have any navigation entries, skip it + $this->loadedAppInfo[] = $app; continue; } + foreach ($info['navigations']['navigation'] as $key => $nav) { $nav['type'] = $nav['type'] ?? 'link'; if (!isset($nav['name'])) { @@ -250,8 +266,11 @@ class NavigationManager implements INavigationManager { } $id = $nav['id'] ?? $app . ($key === 0 ? '' : $key); $order = $nav['order'] ?? 100; - $type = $nav['type']; - $route = !empty($nav['route']) ? $this->urlGenerator->linkToRoute($nav['route']) : ''; + $type = $nav['type'] ?? 'link'; + $route = $nav['route'] ?? ''; + if ($route !== '') { + $route = $this->urlGenerator->linkToRoute($route); + } $icon = $nav['icon'] ?? null; if ($icon !== null) { try { diff --git a/tests/lib/NavigationManagerTest.php b/tests/lib/NavigationManagerTest.php index 1b2805f9c08..fbd10b71682 100644 --- a/tests/lib/NavigationManagerTest.php +++ b/tests/lib/NavigationManagerTest.php @@ -157,6 +157,7 @@ class NavigationManagerTest extends TestCase { $this->assertEquals(0, $testAddClosureNumberOfCalls, 'Expected that the closure is not called by add()'); + $this->navigationManager->setup(); $navigationEntries = $this->navigationManager->getAll('all'); $this->assertEquals(1, $testAddClosureNumberOfCalls, 'Expected that the closure is called by getAll()'); $this->assertCount(1, $navigationEntries, 'Expected that 1 navigation entry exists'); @@ -233,6 +234,12 @@ class NavigationManagerTest extends TestCase { ->method('getAppInfo') ->with('test') ->willReturn($navigation); + $this->appManager->expects($this->any()) + ->method('isAppLoaded') + ->willReturnMap([ + ['test', true], + ['files', true], + ]); $this->urlGenerator->expects($this->any()) ->method('imagePath') ->willReturnCallback(function ($appName, $file) { @@ -259,11 +266,12 @@ class NavigationManagerTest extends TestCase { $this->groupManager->expects($this->any())->method('isAdmin')->willReturn($isAdmin); $this->navigationManager->clear(); - $this->dispatcher->expects($this->once()) + $this->dispatcher->expects($this->atLeastOnce()) ->method('dispatchTyped') ->willReturnCallback(function ($event): void { $this->assertInstanceOf(LoadAdditionalEntriesEvent::class, $event); }); + $this->navigationManager->setup(); $entries = $this->navigationManager->getAll('all'); $this->assertEquals($expected, $entries); } @@ -427,8 +435,20 @@ class NavigationManagerTest extends TestCase { ->method('isEnabledForUser') ->with('theming') ->willReturn(true); - $this->appManager->expects($this->once())->method('getAppInfo')->with('test')->willReturn($navigation); - $this->appManager->expects($this->once())->method('getAppIcon')->with('test')->willReturn('/apps/test/img/app.svg'); + $this->appManager->expects($this->once()) + ->method('getAppIcon') + ->with('test') + ->willReturn('/apps/test/img/app.svg'); + $this->appManager->expects($this->once()) + ->method('getAppInfo') + ->with('test') + ->willReturn($navigation); + $this->appManager->expects($this->atLeastOnce()) + ->method('isAppLoaded') + ->willReturnMap([ + ['test', true], + ['files', true], + ]); $this->l10nFac->expects($this->any())->method('get')->willReturn($l); $this->urlGenerator->expects($this->any())->method('imagePath')->willReturnCallback(function ($appName, $file) { return "/apps/$appName/img/$file"; @@ -455,10 +475,106 @@ class NavigationManagerTest extends TestCase { ->willReturnCallback(function ($event): void { $this->assertInstanceOf(LoadAdditionalEntriesEvent::class, $event); }); + $this->navigationManager->setup(); $entries = $this->navigationManager->getAll(); $this->assertEquals($expected, $entries); } + /** + * Navigation entries of enabled apps that are not booted yet must not be resolved. + */ + public function testResolveOnlyLoadedApps(): void { + /* Return default value */ + $this->config->method('getUserValue')->willReturnArgument(3); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user001'); + $this->userSession->method('getUser')->willReturn($user); + $this->userSession->method('isLoggedIn')->willReturn(true); + $this->appManager->method('getEnabledAppsForUser')->with($user)->willReturn(['test']); + + // The app is enabled but not booted yet ... + $this->appManager->expects($this->atLeastOnce()) + ->method('isAppLoaded') + ->with('test') + ->willReturn(false); + // ... so its info.xml navigation entries must never be read + $this->appManager->expects($this->never())->method('getAppInfo'); + + $this->navigationManager->clear(); + $this->assertEquals([], $this->navigationManager->getAll('all')); + } + + /** + * The LoadAdditionalEntriesEvent is only dispatched by setup(), not by getAll(). + */ + public function testGetAllDoesNotDispatchAdditionalEntries(): void { + $this->userSession->method('isLoggedIn')->willReturn(false); + $this->appManager->method('getEnabledApps')->willReturn([]); + + $this->dispatcher->expects($this->never())->method('dispatchTyped'); + + $this->navigationManager->clear(); + $this->assertEquals([], $this->navigationManager->getAll('all')); + } + + /** + * An app's info.xml must only be resolved once, even across multiple getAll() calls + * and even when the app does not provide any navigation entries. + */ + public function testAppInfoResolvedOnlyOnce(): void { + $this->config->method('getUserValue')->willReturnArgument(3); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user001'); + $this->userSession->method('getUser')->willReturn($user); + $this->userSession->method('isLoggedIn')->willReturn(true); + $this->appManager->method('getEnabledAppsForUser')->with($user)->willReturn(['test']); + $this->appManager->method('isAppLoaded')->with('test')->willReturn(true); + + // App has no navigation entries; info.xml must only be read once + $this->appManager->expects($this->once()) + ->method('getAppInfo') + ->with('test') + ->willReturn(['navigations' => []]); + + $this->navigationManager->clear(); + $this->assertEquals([], $this->navigationManager->getAll('all')); + $this->assertEquals([], $this->navigationManager->getAll('all')); + } + + /** + * clear(false) keeps the resolved state, so already loaded apps are not resolved again; + * clear(true) resets it, forcing a fresh resolve. + */ + public function testClearResetsResolvedStateOnlyWhenRequested(): void { + $this->config->method('getUserValue')->willReturnArgument(3); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user001'); + $this->userSession->method('getUser')->willReturn($user); + $this->userSession->method('isLoggedIn')->willReturn(true); + $this->appManager->method('getEnabledAppsForUser')->with($user)->willReturn(['test']); + $this->appManager->method('isAppLoaded')->with('test')->willReturn(true); + + // Resolved once for the initial getAll(), then again after clear(true) resets the state + $this->appManager->expects($this->exactly(2)) + ->method('getAppInfo') + ->with('test') + ->willReturn(['navigations' => []]); + + $this->navigationManager->clear(); + $this->assertEquals([], $this->navigationManager->getAll('all')); + + // Soft clear keeps the resolved state, so getAppInfo is not called again + $this->navigationManager->clear(false); + $this->assertEquals([], $this->navigationManager->getAll('all')); + + // Full clear resets the resolved state, so the app is resolved again + $this->navigationManager->clear(true); + $this->assertEquals([], $this->navigationManager->getAll('all')); + } + public static function provideDefaultEntries(): array { return [ // none specified, default to files @@ -630,7 +746,13 @@ class NavigationManagerTest extends TestCase { ]; }); - $this->appManager->method('getEnabledApps')->willReturn([]); + $this->appManager->method('getEnabledApps')->willReturn(['files']); + $this->appManager->expects($this->atLeastOnce()) + ->method('isAppLoaded') + ->willReturnMap([ + ['test', true], + ['files', true], + ]); $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user1'); @@ -651,6 +773,7 @@ class NavigationManagerTest extends TestCase { ['user1', 'core', 'apporder', '[]', $userApporder], ]); + $this->navigationManager->setup(); $this->assertEquals($expectedApp, $this->navigationManager->getDefaultEntryIdForUser(null, $withFallbacks)); }