Merge pull request #1282 from CybotTM/fix/empty-filename-url-generator

[BUGFIX] Improve URL generation for empty filenames and known documents

Author: jaapio

packages/guides/src/Renderer/UrlGenerator/AbstractUrlGenerator.php

@@ -13,10 +13,10 @@
 
 namespace phpDocumentor\Guides\Renderer\UrlGenerator;
 
-use Exception;
 use League\Uri\BaseUri;
 use phpDocumentor\Guides\ReferenceResolvers\DocumentNameResolverInterface;
 use phpDocumentor\Guides\RenderContext;
+use phpDocumentor\Guides\Renderer\UrlGenerator\Exception\InvalidUrlException;
 
 use function filter_var;
 use function sprintf;
@@ -32,8 +32,13 @@ public function __construct(private readonly DocumentNameResolverInterface $docu
 
     public function createFileUrl(RenderContext $context, string $filename, string|null $anchor = null): string
     {
-        return $filename . '.' . $context->getOutputFormat() .
-            ($anchor !== null ? '#' . $anchor : '');
+        $anchorSuffix = $anchor !== null && $anchor !== '' ? '#' . $anchor : '';
+
+        if ($filename === '') {
+            return $anchorSuffix !== '' ? $anchorSuffix : '#';
+        }
+
+        return $filename . '.' . $context->getOutputFormat() . $anchorSuffix;
     }
 
     abstract public function generateInternalPathFromRelativeUrl(
@@ -50,15 +55,20 @@ public function generateCanonicalOutputUrl(RenderContext $context, string $refer
             return $reference;
         }
 
+        // Pass through email addresses (for mailto: link generation)
         if (filter_var($reference, FILTER_VALIDATE_EMAIL) !== false) {
             return $reference;
         }
 
+        // If reference is already a known document, it's already canonical - use directly
         if ($context->getProjectNode()->findDocumentEntry($reference) !== null) {
-            // todo: this is a hack, existing documents are expected to be handled like absolute links in some places
-            $reference = '/' . $reference;
+            return $this->generateInternalUrl(
+                $context,
+                $this->createFileUrl($context, $reference, $anchor),
+            );
         }
 
+        // Otherwise, resolve relative reference to canonical path
         $canonicalUrl = $this->documentNameResolver->canonicalUrl(
             $context->getDirName(),
             $reference,
@@ -75,7 +85,7 @@ public function generateInternalUrl(
         string $canonicalUrl,
     ): string {
         if (!$this->isRelativeUrl($canonicalUrl)) {
-            throw new Exception(sprintf('%s::%s may only be applied to relative URLs, %s cannot be handled', self::class, __METHOD__, $canonicalUrl));
+            throw new InvalidUrlException(sprintf('%s::%s may only be applied to relative URLs, %s cannot be handled', self::class, __METHOD__, $canonicalUrl));
         }
 
         return $this->generateInternalPathFromRelativeUrl($renderContext, $canonicalUrl);

packages/guides/src/Renderer/UrlGenerator/Exception/InvalidUrlException.php

@@ -0,0 +1,20 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * This file is part of phpDocumentor.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ *
+ * @link https://phpdoc.org
+ */
+
+namespace phpDocumentor\Guides\Renderer\UrlGenerator\Exception;
+
+use Exception;
+
+final class InvalidUrlException extends Exception
+{
+}

packages/guides/tests/unit/Renderer/UrlGenerator/RelativeUrlGeneratorTest.php

@@ -15,6 +15,7 @@
 
 use phpDocumentor\Guides\ReferenceResolvers\DocumentNameResolverInterface;
 use phpDocumentor\Guides\RenderContext;
+use phpDocumentor\Guides\Renderer\UrlGenerator\Exception\InvalidUrlException;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
 
@@ -72,15 +73,10 @@ public static function generateRelativeInternalUrlProvider(): array
     }
 
     #[DataProvider('fileUrlProvider')]
-    public function testCreateFileUrl(string $expected, string $filename, string $outputFormat = 'html', string|null $anchor = null, string $skip = ''): void
+    public function testCreateFileUrl(string $expected, string $filename, string $outputFormat = 'html', string|null $anchor = null): void
     {
-        if ($skip !== '') {
-            self::markTestSkipped($skip);
-        }
-
         $urlGenerator = new RelativeUrlGenerator(self::createStub(DocumentNameResolverInterface::class));
         $renderContext = $this->createMock(RenderContext::class);
-        $renderContext->method('getCurrentFileName')->willReturn($filename);
         $renderContext->method('getOutputFormat')->willReturn($outputFormat);
         self::assertSame($expected, $urlGenerator->createFileUrl($renderContext, $filename, $anchor));
     }
@@ -113,15 +109,34 @@ public static function fileUrlProvider(): array
                 'filename' => '',
                 'outputFormat' => 'html',
                 'anchor' => 'anchor',
-                'skip' => 'Empty filenames are not supported',
             ],
-            'Empty File with empty anchor' => [
+            'Empty File with null anchor' => [
                 'expected' => '#',
                 'filename' => '',
                 'outputFormat' => 'html',
                 'anchor' => null,
-                'skip' => 'Empty filenames are not supported',
+            ],
+            'Empty File with empty string anchor' => [
+                'expected' => '#',
+                'filename' => '',
+                'outputFormat' => 'html',
+                'anchor' => '',
+            ],
+            'File with empty string anchor' => [
+                'expected' => 'file.html',
+                'filename' => 'file',
+                'outputFormat' => 'html',
+                'anchor' => '',
             ],
         ];
     }
+
+    public function testGenerateInternalUrlThrowsOnAbsoluteUrl(): void
+    {
+        $urlGenerator = new RelativeUrlGenerator(self::createStub(DocumentNameResolverInterface::class));
+        $renderContext = $this->createMock(RenderContext::class);
+
+        $this->expectException(InvalidUrlException::class);
+        $urlGenerator->generateInternalUrl($renderContext, 'https://example.com/page.html');
+    }
 }

tests/Integration/tests-full/bootstrap/bootstrap-menu-external-linktargets/expected/index.html

@@ -87,7 +87,7 @@ <h1>Document Title</h1>
 
 </li>
     <li class="toc-item">
-    <a href="/page1.html#">Some Page</a>
+    <a href="/page1.html">Some Page</a>
 
 
 </li>

tests/Integration/tests-full/bootstrap/bootstrap-menu-external-on-subpage/expected/index.html

@@ -109,7 +109,7 @@ <h1>Document Title</h1>
 
 </li>
     <li class="toc-item">
-    <a href="/page1.html#">Some Page</a>
+    <a href="/page1.html">Some Page</a>
         <ul class="menu-level-1">
             <li class="toc-item">
     <a href="https://example.com/index.html#another">Title 2</a>

tests/Integration/tests/toctree/toctree-external-linktargets/expected/index.html

@@ -15,7 +15,7 @@ <h1>Document Title</h1>
 
 </li>
     <li class="toc-item">
-    <a href="/page1.html#">Some Page</a>
+    <a href="/page1.html">Some Page</a>
 
 
 </li>