Merge pull request #63524 from nextcloud/followup/63134/allow-to-hide-navigation-closures

fix(navigation): Allow apps to hide registered navigation entries

Author: nickvergessen

build/psalm-baseline.xml

@@ -4080,6 +4080,11 @@
       <code><![CDATA[DEFAULT_TTL]]></code>
     </AmbiguousConstantInheritance>
   </file>
+  <file src="ocs/v1.php">
+    <InternalMethod>
+      <code><![CDATA[setup]]></code>
+    </InternalMethod>
+  </file>
   <file src="public.php">
     <DeprecatedMethod>
       <code><![CDATA[getAppValue]]></code>

lib/private/NavigationManager.php

@@ -57,7 +57,7 @@ class NavigationManager implements INavigationManager {
 	protected ?string $activeEntry = null;
 	/** @var array<string, NavigationEntryOutput> */
 	protected array $entries = [];
-	/** @var list<callable(): NavigationEntry> */
+	/** @var list<callable(): ?NavigationEntry> */
 	protected array $closureEntries = [];
 	/** User defined app order (cached for the `add` function) */
 	protected ?array $customAppOrder = null;
@@ -357,7 +357,12 @@ private function resolveAppNavigationEntries(): void {
 			// as apps might add new navigation entries via closures at any time
 			while ($c = array_pop($this->closureEntries)) {
 				try {
-					$this->add($c());
+					$entry = $c();
+					if ($entry === null) {
+						$this->logger->debug('Closure of navigation entry returned null, skipping');
+						continue;
+					}
+					$this->add($entry);
 				} catch (\Throwable $e) {
 					$this->logger->error('Failed to add navigation entry from closure', ['exception' => $e]);
 				}

lib/public/INavigationManager.php

@@ -80,9 +80,9 @@ interface INavigationManager {
 	/**
 	 * Creates a new navigation entry
 	 *
-	 * @param NavigationEntry|callable():NavigationEntry $entry If a menu entry (type = 'link') is added, you shall also set app to the app that
-	 *                                                          added the entry. The use of a closure is preferred, because it will avoid loading
-	 *                                                          the routing of your app, unless required.
+	 * @param NavigationEntry|callable():?NavigationEntry $entry If a menu entry (type = 'link') is added, you shall also set app to the app that
+	 *                                                           added the entry. The use of a closure is preferred, because it will avoid loading
+	 *                                                           the routing of your app, unless required.
 	 * @return void
 	 * @since 6.0.0
 	 */

ocs/v1.php

@@ -79,6 +79,10 @@
 			$appManager->loadApps(['core']);
 		}
 
+		// All apps are now loaded to handle the request
+		Server::get(\OC\NavigationManager::class)->setup();
+		Server::get(\OCP\EventDispatcher\IEventDispatcher::class)->dispatchTyped(new \OCP\App\Events\AppsLoadedEvent());
+
 		Server::get(Router::class)->match('/ocsapp' . $request->getRawPathInfo());
 	} catch (MaxDelayReached $ex) {
 		ApiHelper::respond(Http::STATUS_TOO_MANY_REQUESTS, $ex->getMessage());

tests/lib/NavigationManagerTest.php

@@ -173,6 +173,36 @@ public function testAddClosure(array $entry, array $expectedEntry): void {
 		$this->assertEmpty($this->navigationManager->getAll('all'), 'Expected no navigation entry exists after clear()');
 	}
 
+	/**
+	 * Entry points that never call setup() (e.g. the OCS dispatch in ocs/v1.php)
+	 * must not silently lose closure-registered entries such as an app's nav
+	 * link: getAll() should only resolve what it can, not resolve nothing.
+	 */
+	public function testGetAllDoesNotResolveClosureBeforeSetup(): void {
+		$numberOfCalls = 0;
+		$this->navigationManager->add(function () use (&$numberOfCalls) {
+			$numberOfCalls++;
+
+			return [
+				'id' => 'entry id',
+				'name' => 'link text',
+				'order' => 1,
+				'href' => 'url',
+			];
+		});
+
+		$navigationEntries = $this->navigationManager->getAll('all');
+
+		$this->assertEquals(0, $numberOfCalls, 'Expected that the closure is not called by getAll() before setup()');
+		$this->assertEmpty($navigationEntries, 'Expected no navigation entry exists before setup()');
+
+		$this->navigationManager->setup();
+		$navigationEntries = $this->navigationManager->getAll('all');
+
+		$this->assertEquals(1, $numberOfCalls, 'Expected that the closure is called by getAll() once setup() has run');
+		$this->assertArrayHasKey('entry id', $navigationEntries);
+	}
+
 	public function testAddClosureAfterSetup(): void {
 		$this->navigationManager->setup();
 		$this->assertEmpty($this->navigationManager->getAll('all'), 'Expected no navigation entry exists');