Skip to content

fix: resolve cross-page fragment links in markdown - #1300

Open
jonathanhefner wants to merge 1 commit into
phpDocumentor:mainfrom
jonathanhefner:fix-markdown-cross-page-fragment-links
Open

fix: resolve cross-page fragment links in markdown#1300
jonathanhefner wants to merge 1 commit into
phpDocumentor:mainfrom
jonathanhefner:fix-markdown-cross-page-fragment-links

Conversation

@jonathanhefner

Copy link
Copy Markdown
Contributor

⚠️ Disclaimer: This was vibe-coded. I am not a PHP developer, but I am helping https://github.com/modelcontextprotocol/php-sdk set up documentation using phpDocumentor, and I encountered this issue. (See also modelcontextprotocol/php-sdk#232.) I guided Claude through the investigation and self-review, but I am unable to personally judge the fix.


Markdown links with both a page reference and a fragment anchor like [text](page-b.md#section-two) failed to resolve, producing a warning: "Reference page-b.md#section-two could not be resolved".

Two independent bugs caused this:

  1. LinkParser checked str_ends_with($url, '.md') on the full URL including the fragment, so page-b.md#section-two did not match and the .md extension was never stripped. Fix: split the fragment off before the .md check, then reattach it afterward.

  2. PageHyperlinkResolver passed the full target reference (including #fragment) to findDocumentEntry(), which always returned null since no document is keyed with a fragment suffix. Fix: split the fragment before the document lookup and append it to the generated URL, matching the pattern already used by DocReferenceResolver.

Markdown links with both a page reference and a fragment anchor like
`[text](page-b.md#section-two)` failed to resolve, producing a warning:
"Reference page-b.md#section-two could not be resolved".

Two independent bugs caused this:

1. `LinkParser` checked `str_ends_with($url, '.md')` on the full URL
   including the fragment, so `page-b.md#section-two` did not match and
   the `.md` extension was never stripped. Fix: split the fragment off
   before the `.md` check, then reattach it afterward.

2. `PageHyperlinkResolver` passed the full target reference (including
   `#fragment`) to `findDocumentEntry()`, which always returned `null`
   since no document is keyed with a fragment suffix. Fix: split the
   fragment before the document lookup and append it to the generated
   URL, matching the pattern already used by `DocReferenceResolver`.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@wWzZb

wWzZb commented Aug 11, 2026

Copy link
Copy Markdown

I verified this against the latest upstream main (90594491) using the actual link shape from the PHP SDK docs.

The PHP SDK links from mcp-elements.md to anchors including:

  • server-builder.md#manual-capability-registration
  • server-builder.md#explicit-element-registration

On current main, guides logs Reference server-builder.md#manual-capability-registration could not be resolved and renders the link text without an <a> element. With this PR applied, the links render as /server-builder.html#... and no unresolved-reference warning is produced.

I also reproduced the old PHP 8.5 / lowest-dependencies failure on the original PR head. It was unrelated to this fix: PHPUnit exited non-zero on 100 dependency deprecations from the lowest Symfony and scrivo/highlight.php versions. Upstream main subsequently addressed that with the Symfony deprecation baseline (b844999e) and a higher highlighter floor (bb8bb544). After merging current main, the PHP 8.5 lowest-dependency unit suite passes (484 tests), and the locked full suite passes (829 tests / 6,032 assertions). PHPStan and the architecture check also pass.

Because I cannot push directly to the contributor branch, I opened a clean, mergeable refresh PR here: jonathanhefner#2

@jonathanhefner, could you merge that refresh PR into this branch? Once it lands, this PR will be current with upstream and ready for a fresh CI run. Maintainers, a review after that refresh would help unblock the PHP SDK documentation links affected by this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants