diff --git a/lib/private/NavigationManager.php b/lib/private/NavigationManager.php index 032de5266e5c8..6afcb6ab65f3e 100644 --- a/lib/private/NavigationManager.php +++ b/lib/private/NavigationManager.php @@ -34,7 +34,7 @@ class NavigationManager implements INavigationManager { * negative to keep this group in front of everything else. * Apps that are not listed keep their own order. */ - private const DEFAULT_APP_ORDER = [ + private const array DEFAULT_APP_ORDER = [ // Basics 'dashboard' => -100, 'files' => -99, @@ -54,15 +54,20 @@ class NavigationManager implements INavigationManager { 'activity' => -88, ]; + protected ?string $activeEntry = null; /** @var array */ protected array $entries = []; /** @var list */ protected array $closureEntries = []; - protected ?string $activeEntry = null; - protected array $unreadCounters = []; - protected bool $init = false; /** User defined app order (cached for the `add` function) */ - private ?array $customAppOrder = null; + protected ?array $customAppOrder = null; + /** @var array */ + protected array $unreadCounters = []; + + /** true if the internal state has been initialized */ + protected bool $initAppOrderDone = false; + /** true if all apps have been loaded by the App Manager */ + protected bool $initSetupDone = false; /** List of loaded app info */ private array $loadedAppInfo = []; @@ -80,11 +85,12 @@ public function __construct( #[Override] public function add(array|callable $entry): void { - if ($entry instanceof \Closure) { + if (is_callable($entry)) { $this->closureEntries[] = $entry; return; } - $this->init(); + // if needed initialize the internal state to allow setting app order and default app + $this->initCustomAppOrder(); $id = $entry['id']; @@ -201,7 +207,7 @@ public function clear(bool $resetInit = true): void { if ($resetInit) { $this->loadedAppInfo = []; - $this->init = false; + $this->initAppOrderDone = false; } } @@ -219,11 +225,11 @@ public function getActiveEntry(): ?string { * Initialize the internal state. * This loads the default app mapping and user mapping for app ordering. */ - private function init(): void { - if ($this->init) { + private function initCustomAppOrder(): void { + if ($this->initAppOrderDone) { return; } - $this->init = true; + $this->initAppOrderDone = true; if ($this->customAppOrder === null) { if ($this->userSession->isLoggedIn()) { @@ -237,20 +243,22 @@ private function init(): void { /** * 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. */ 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 $this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent()); + + // mark setup as done to allow performance optimizations + $this->initSetupDone = true; } /** - * Resolve classic info.xml based navigation entires. + * Resolve app navigation entries. + * + * This is called every time by any getter as some code for legacy reasons relies + * on the navigation entries being available before the app loading is finished. * 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. */ @@ -342,6 +350,19 @@ private function resolveAppNavigationEntries(): void { )); } } + + // once all apps are loaded we can resolve the app navigation closures + if ($this->initSetupDone) { + // This has to be done on every call, + // as apps might add new navigation entries via closures at any time + while ($c = array_pop($this->closureEntries)) { + try { + $this->add($c()); + } catch (\Throwable $e) { + $this->logger->error('Failed to add navigation entry from closure', ['exception' => $e]); + } + } + } } private function isAdmin(): bool { diff --git a/tests/lib/NavigationManagerTest.php b/tests/lib/NavigationManagerTest.php index 1ca5861bab5da..8120425655204 100644 --- a/tests/lib/NavigationManagerTest.php +++ b/tests/lib/NavigationManagerTest.php @@ -172,6 +172,35 @@ public function testAddClosure(array $entry, array $expectedEntry): void { $this->assertEmpty($this->navigationManager->getAll('all'), 'Expected no navigation entry exists after clear()'); } + public function testAddClosureAfterSetup(): void { + $this->navigationManager->setup(); + $this->assertEmpty($this->navigationManager->getAll('all'), 'Expected no navigation entry exists'); + + $numberOfCalls = 0; + $this->navigationManager->add(function () use (&$numberOfCalls) { + $numberOfCalls++; + + return [ + 'id' => 'late entry', + 'name' => 'link text', + 'order' => 1, + 'href' => 'url', + ]; + }); + + $this->assertEquals(0, $numberOfCalls, 'Expected that the closure is not called by add()'); + + $navigationEntries = $this->navigationManager->getAll('all'); + $this->assertEquals(1, $numberOfCalls, 'Expected that the closure added after setup() is called by getAll()'); + $this->assertCount(1, $navigationEntries, 'Expected that 1 navigation entry exists'); + $this->assertArrayHasKey('late entry', $navigationEntries); + + $navigationEntries = $this->navigationManager->getAll('all'); + $this->assertEquals(1, $numberOfCalls, 'Expected that the closure is only called once'); + $this->assertCount(1, $navigationEntries, 'Expected that 1 navigation entry exists'); + $this->assertArrayHasKey('late entry', $navigationEntries); + } + public function testAddArrayClearGetAll(): void { $entry = [ 'id' => 'entry id',