Merge pull request #1636 from hydephp/create-new-navigation-destinati…

…on-class

[2.x] Create new navigation destination class to replace external routes

RELEASE_NOTES.md

@@ -11,7 +11,6 @@ This serves two purposes:
 
 ### Added
 - Added a new `\Hyde\Framework\Actions\PreBuildTasks\TransferMediaAssets` build task handle media assets transfers for site builds.
-- Added a new `ExternalRoute` class to represent external routes.
 - Added a new `NavigationItem::getLink()` method contain the previous `NavigationItem::getDestination()` logic, to return the link URL.
 
 ### Changed

packages/framework/src/Framework/Features/Navigation/NavigationDestination.php

@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Hyde\Framework\Features\Navigation;
+
+use Stringable;
+use Hyde\Support\Models\Route;
+use Hyde\Foundation\Facades\Routes;
+
+use function is_string;
+
+class NavigationDestination implements Stringable
+{
+    protected Route|string $destination;
+
+    public function __construct(Route|string $destination)
+    {
+        if (is_string($destination) && Routes::has($destination)) {
+            $destination = Routes::get($destination);
+        }
+
+        $this->destination = $destination;
+    }
+
+    public function __toString(): string
+    {
+        return $this->getLink();
+    }
+
+    public function getLink(): string
+    {
+        return (string) $this->destination;
+    }
+
+    /** @experimental */
+    public function getRoute(): ?Route
+    {
+        return $this->destination instanceof Route ? $this->destination : null;
+    }
+}

packages/framework/src/Framework/Features/Navigation/NavigationGroup.php

@@ -7,7 +7,6 @@
 use Illuminate\Support\Str;
 use Illuminate\Support\Arr;
 use Hyde\Pages\DocumentationPage;
-use Hyde\Support\Models\ExternalRoute;
 
 use function min;
 use function collect;
@@ -79,7 +78,7 @@ protected function addItem(NavigationItem $item): void
     protected function containsOnlyDocumentationPages(): bool
     {
         return count($this->getItems()) && collect($this->getItems())->every(function (NavigationItem $child): bool {
-            return (! $child->getRoute() instanceof ExternalRoute) && $child->getRoute()->getPage() instanceof DocumentationPage;
+            return ($child->getRoute() !== null) && $child->getRoute()->getPage() instanceof DocumentationPage;
         });
     }
 }

packages/framework/src/Framework/Features/Navigation/NavigationItem.php

@@ -9,7 +9,6 @@
 use Hyde\Support\Models\Route;
 use Illuminate\Support\Str;
 use Stringable;
-use Hyde\Support\Models\ExternalRoute;
 
 use function is_string;
 
@@ -23,7 +22,7 @@
  */
 class NavigationItem implements NavigationElement, Stringable
 {
-    protected Route $route;
+    protected NavigationDestination $destination;
     protected string $label;
     protected int $priority;
 
@@ -40,11 +39,8 @@ class NavigationItem implements NavigationElement, Stringable
      */
     public function __construct(Route|string $destination, string $label, int $priority = NavigationMenu::DEFAULT, ?string $group = null)
     {
-        if (is_string($destination)) {
-            $destination = Routes::get($destination) ?? new ExternalRoute($destination);
-        }
+        $this->destination = new NavigationDestination($destination);
 
-        $this->route = $destination;
         $this->label = $label;
         $this->priority = $priority;
 
@@ -63,11 +59,11 @@ public function __construct(Route|string $destination, string $label, int $prior
      */
     public static function create(Route|string $destination, ?string $label = null, ?int $priority = null, ?string $group = null): self
     {
-        if (is_string($destination)) {
-            $destination = Routes::get($destination) ?? new ExternalRoute($destination);
+        if (is_string($destination) && Routes::has($destination)) {
+            $destination = Routes::get($destination);
         }
 
-        if ($destination instanceof Route && ! $destination instanceof ExternalRoute) {
+        if ($destination instanceof Route) {
             return new self(
                 $destination,
                 $label ?? $destination->getPage()->navigationMenuLabel(),
@@ -89,18 +85,22 @@ public function __toString(): string
 
     /**
      * Get the destination route of the navigation item. For dropdowns, this will return null.
+     *
+     * @deprecated To simplify the class, we may remove this as we probably don't need it.
      */
     public function getRoute(): ?Route
     {
-        return $this->route;
+        return $this->destination->getRoute();
     }
 
     /**
      * Resolve the destination link of the navigation item.
+     *
+     * @deprecated May be renamed to getLink() in the future to better match its usage, and to match the Route class.
      */
     public function getUrl(): string
     {
-        return (string) $this->route;
+        return $this->destination->getLink();
     }
 
     /**
@@ -137,7 +137,7 @@ public function getGroupKey(): ?string
      */
     public function isActive(): bool
     {
-        return Hyde::currentRoute()->getLink() === $this->route->getLink();
+        return Hyde::currentRoute()->getLink() === $this->getUrl();
     }
 
     /** @return ($group is null ? null : string) */

packages/framework/tests/Feature/NavigationMenuTest.php

@@ -7,7 +7,6 @@
 use Hyde\Hyde;
 use Hyde\Support\Models\Route;
 use Hyde\Foundation\Facades\Routes;
-use Hyde\Support\Models\ExternalRoute;
 use Hyde\Framework\Features\Navigation\NavigationGroup;
 use Hyde\Framework\Features\Navigation\MainNavigationMenu;
 use Hyde\Framework\Features\Navigation\NavigationItem;
@@ -268,7 +267,7 @@ public function testCanAddItemsToMainNavigationMenuResolvedFromContainer()
         Hyde::boot();
 
         $navigation = app('navigation.main');
-        $navigation->add(new NavigationItem(new ExternalRoute('/foo'), 'Foo'));
+        $navigation->add(new NavigationItem('/foo', 'Foo'));
 
         $this->assertCount(2, $navigation->getItems());
         $this->assertSame('Foo', $navigation->getItems()->last()->getLabel());

packages/framework/tests/Unit/DocumentationSidebarUnitTest.php

@@ -6,7 +6,6 @@
 
 use Hyde\Testing\UnitTestCase;
 use Illuminate\Support\Collection;
-use Hyde\Support\Models\ExternalRoute;
 use Hyde\Framework\Features\Navigation\NavigationItem;
 use Hyde\Framework\Features\Navigation\NavigationGroup;
 use Hyde\Framework\Features\Navigation\DocumentationSidebar;
@@ -21,6 +20,9 @@
  */
 class DocumentationSidebarUnitTest extends UnitTestCase
 {
+    protected static bool $needsKernel = true;
+    protected static bool $needsConfig = true;
+
     // Base menu tests
 
     public function testCanConstruct()
@@ -207,9 +209,6 @@ public function testHasGroupsReturnsFalseWhenNoItemsHaveChildren()
 
     public function testHasGroupsReturnsTrueWhenAtLeastOneItemIsNavigationGroupInstance()
     {
-        self::mockConfig();
-        self::setupKernel();
-
         $menu = new DocumentationSidebar([
             new NavigationGroup('foo', []),
         ]);
@@ -228,6 +227,6 @@ protected function getItems(): array
 
     protected function item(string $destination, string $label, int $priority = 500): NavigationItem
     {
-        return new NavigationItem(new ExternalRoute($destination), $label, $priority);
+        return new NavigationItem($destination, $label, $priority);
     }
 }

packages/framework/tests/Unit/NavigationItemTest.php

@@ -5,7 +5,6 @@
 namespace Hyde\Framework\Testing\Unit;
 
 use Hyde\Foundation\Facades\Routes;
-use Hyde\Support\Models\ExternalRoute;
 use Hyde\Framework\Features\Navigation\NavigationItem;
 use Hyde\Pages\InMemoryPage;
 use Hyde\Pages\MarkdownPage;
@@ -66,20 +65,18 @@ public function testPassingRouteKeyToConstructorUsesRouteInstance()
         $this->assertSame($route, (new NavigationItem('index', 'Home'))->getRoute());
     }
 
-    public function testPassingUrlToConstructorUsesExternalRoute()
+    public function testPassingUrlToConstructorSetsRouteToNull()
     {
         $item = new NavigationItem('https://example.com', 'Home');
-        $this->assertInstanceOf(ExternalRoute::class, $item->getRoute());
-        $this->assertEquals(new ExternalRoute('https://example.com'), $item->getRoute());
-        $this->assertSame('https://example.com', (string) $item->getRoute());
+        $this->assertNull($item->getRoute());
+        $this->assertSame('https://example.com', $item->getUrl());
     }
 
-    public function testPassingUnknownRouteKeyToConstructorUsesExternalRoute()
+    public function testPassingUnknownRouteKeyToConstructorSetsRouteToNull()
     {
         $item = new NavigationItem('foo', 'Home');
-        $this->assertInstanceOf(ExternalRoute::class, $item->getRoute());
-        $this->assertEquals(new ExternalRoute('foo'), $item->getRoute());
-        $this->assertSame('foo', (string) $item->getRoute());
+        $this->assertNull($item->getRoute());
+        $this->assertSame('foo', $item->getUrl());
     }
 
     public function testGetDestination()
@@ -133,7 +130,8 @@ public function testCreateWithLink()
     {
         $item = NavigationItem::create('foo', 'bar');
 
-        $this->assertEquals(new ExternalRoute('foo'), $item->getRoute());
+        $this->assertNull($item->getRoute());
+        $this->assertSame('foo', $item->getUrl());
         $this->assertSame('bar', $item->getLabel());
         $this->assertSame(500, $item->getPriority());
     }
@@ -173,7 +171,7 @@ public function testCreateWithRouteKey()
 
     public function testCreateWithMissingRouteKey()
     {
-        $this->assertInstanceOf(ExternalRoute::class, NavigationItem::create('foo', 'foo')->getRoute());
+        $this->assertNull(NavigationItem::create('foo', 'foo')->getRoute());
     }
 
     public function testCreateWithCustomPriority()
@@ -245,7 +243,7 @@ public function testIsCurrent()
         $this->assertFalse(NavigationItem::create(new Route(new InMemoryPage('bar')))->isActive());
     }
 
-    public function testIsCurrentWithExternalRoute()
+    public function testIsCurrentWithLink()
     {
         Render::swap(Mockery::mock(RenderData::class, [
             'getRoute' => new Route(new InMemoryPage('foo')),

packages/framework/tests/Unit/NavigationMenuUnitTest.php

@@ -6,7 +6,6 @@
 
 use Hyde\Testing\UnitTestCase;
 use Illuminate\Support\Collection;
-use Hyde\Support\Models\ExternalRoute;
 use Hyde\Framework\Features\Navigation\NavigationItem;
 use Hyde\Framework\Features\Navigation\MainNavigationMenu;
 
@@ -19,6 +18,9 @@
  */
 class NavigationMenuUnitTest extends UnitTestCase
 {
+    protected static bool $needsKernel = true;
+    protected static bool $needsConfig = true;
+
     // Base menu tests
 
     public function testCanConstruct()
@@ -159,6 +161,6 @@ protected function getItems(): array
 
     protected function item(string $destination, string $label, int $priority = 500): NavigationItem
     {
-        return new NavigationItem(new ExternalRoute($destination), $label, $priority);
+        return new NavigationItem($destination, $label, $priority);
     }
 }