[BUGFIX] Keep navigation menu in toctree order when mixing internal and external entries

[CybotTM]

Problem

A toctree that mixes internal pages and external links builds the navigation menu (the document entry's menu entries, used for the sidebar/navbar) with all external entries grouped first, even though the on-page toctree keeps the authored order. The navigation therefore disagrees with the page. It shows up for nested toctrees.

Root cause

Internal and external menu entries are attached to the document entry by two separate transformers — InternalMenuEntryNodeTransformer and ExternalMenuEntryNodeTransformer — and the compiler runs one full tree traversal per transformer at a given priority (both attach at 4500). So every external entry is appended to DocumentEntryNode::getMenuEntries() in one pass and every internal entry in another, and the list ends up grouped by type instead of following the toctree. The on-page toctree renders from the TocNode value (authored order) and stays correct.

Fix

ToctreeSortingTransformer (priority 3200, after the attach passes) now realigns the document entry's menu entries with the authored toctree order, matching entries by file/URL. It emits each toctree's entries as one contiguous block at the position of its first entry, so a document with several mixed toctrees is ordered correctly too. It already ran at the right point to handle the reversed option. Globbed toctrees are skipped — their order comes from the glob expansion, not an authored sequence. That sentence was wrong and the code it described is gone; see the last section.

Before / after

Sidebar of a nested subpage whose toctree interleaves internal pages and external links (Bootstrap theme):

Before	After

Tests

Seven integration fixtures under tests-full/bootstrap/, one per toctree shape, each failing against main: bootstrap-menu-nested-external-order (one toctree interleaving internal and external), bootstrap-menu-multiple-toctrees-order (two mixed toctrees), bootstrap-menu-separate-toctrees-order (an internal-only toctree followed by an external-only one), bootstrap-menu-glob-option-and-duplicate (the :glob: option without a *, plus a duplicated internal entry), bootstrap-menu-duplicate-external-order (a duplicated external link), bootstrap-menu-glob-external-order (a real * glob followed by an external toctree) and bootstrap-menu-reversed-external-order (:reversed: mixing both kinds).

ToctreeSortingTransformerTest covers what no .rst input can produce, including the fallback branch. Each of its five cases was seen failing on its own injected defect before it was kept — fallback removed, one position per url instead of per occurrence, skipping an unplaceable entry instead of bailing out, the last position instead of the first, and only the visited toctree — five mutations, five distinct failures.

Verified on the final tree: PHPUnit 841 tests green, PHPStan (level max + baseline), PHPCS (Doctrine standard) and deptrac clean.

Context

Reported downstream at TYPO3-Documentation/render-guides#1175.

Update: the first commit did not fix the case it set out to fix

A review of this branch found that anchoring each toctree's block at the lowest index it matched in the menu entries uses a position in the type-grouped list, not a position in the document. With one toctree holding only internal entries and the next only an external one — the ordinary "chapters" plus "external links" split — the navigation still contradicted the page:

Navigation: External B, Alpha, Beta
Page:       alpha, beta, External B

Both fixtures above use toctrees that mix internal and external entries, which is precisely the shape that hides this. Reproduced by rendering, then pinned as bootstrap-menu-separate-toctrees-order.

The second commit takes the order from all toctrees of the document at once, in document order, and sorts the whole menu entry list by it; when the entries cannot be mapped one to one the list is left untouched rather than partially reordered. The toctrees are collected from the document tree, because DocumentNode::getTocNodes() is filled at priority 1000 — after this pass — and getNodes() only sees direct children while a toctree usually sits inside a section.

Three further corrections fall out of that. The :reversed: handling no longer reverses the document's whole menu entry list when the order can be derived, so it stops displacing entries of other toctrees; the wholesale reversal remains for glob toctrees, where there is no authored sequence to sort by. The glob guard tested hasOption('glob'), but ToctreeBuilder creates a GlobMenuEntryNode from a * in the reference and never reads that option, so the guard was wrong in both directions — it now looks at the node type. And an entry listed twice keeps its first authored position, where the last one used to win while the counts still matched, so nothing noticed.

Two more fixtures cover the separate-toctrees case and the glob-option-plus-duplicate case; both fail against the previous state of this branch.

Update: two review findings from @linawolf, both confirmed and fixed

A duplicated external link reverted the whole fix. ExternalMenuEntryNodeTransformer builds a fresh ExternalEntryNode per occurrence, and the dedup guard in MenuEntryManagement::attachDocumentEntriesToParents only covers DocumentEntryNode — so a link listed twice is attached twice while the position map collapsed it to one slot. The second entry overwrote the first, the count check tripped, and the sorting bailed out for the whole page, back to the type-grouped order this branch exists to fix. Reproduced by rendering a toctree of alpha, External A, beta, External A: sidebar External A, External A, Alpha, Beta against page Alpha, External A, Beta, External A. The map now records one position per occurrence and hands them out in turn; an internal entry is deduplicated on attach so it takes the first occurrence, the two menu entries of a duplicated external link take both. Every menu entry now consumes a distinct position, so the count check has nothing left to catch and is gone. Pinned as bootstrap-menu-duplicate-external-order, which fails on the previous commit of this branch with exactly the order above.

The glob guard was unreachable. Compiler drives its passes off an SplPriorityQueue, a max-heap, so GlobMenuEntryNodeTransformer (4000) has already replaced every GlobMenuEntryNode with the entries it expands to before this pass runs at 3200. Instrumenting the guard across the integration suite recorded zero hits against 138 sortings — the protection claimed in the "Fix" section above never existed. The guard is removed rather than relocated: the expanded entries carry the urls this matches on, and they sort correctly, which is what the suite was already showing. bootstrap-menu-glob-external-order pins that a glob toctree followed by a toctree with an external link produces a navigation menu matching the page; it fails against main.

So the sentence "globbed toctrees are skipped" in the Fix section is wrong and no longer describes the code — glob toctrees are sorted like any other, on the entries the expansion produced. The remaining null return, and with it the wholesale :reversed: fallback, now fires only when a menu entry is attached that no toctree of the document accounts for — a condition nothing in the test suite reaches. The branch stays because its unreachability is not proven, and its comment now says that rather than naming a case as if it had been observed.

Rebased onto main at 382485e. Verified on the final tree: full PHPUnit suite 835 tests green (integration 234), PHPStan level max clean, PHPCS clean.

Assisted by claude-code:claude-opus-5 — Session

[linawolf]

Reviewed ToctreeSortingTransformer.php. Two correctness issues found:

1. A duplicate external link silently reverts the whole fix (ToctreeSortingTransformer.php:130)

ExternalMenuEntryNodeTransformer creates a brand-new ExternalEntryNode for every occurrence of the same external link — unlike internal entries, which are deduped via the getParent() guard in MenuEntryManagement::attachDocumentEntriesToParents. If a toctree lists External A <https://example.com/a> twice, $documentEntry->getMenuEntries() ends up with two distinct ExternalEntryNode objects sharing one URL, while $order collapses that URL to a single position. In the second loop, $ordered[$order[$key]] = $menuEntry; runs twice for the same key, silently overwriting the first entry at that array index, so count($ordered) < count($menuEntries). The count check then triggers return null, and the whole page's menu falls back to the old, wrong type-grouped order — silently, with no log — directly contradicting the method's own comment that "an entry listed twice keeps its first authored position." If that toctree is also :reversed:, the fallback branch then reverses the document's entire menu-entry list wholesale, reintroducing the cross-toctree displacement bug this PR sets out to fix.

2. The glob-toctree guard is unreachable dead code (ToctreeSortingTransformer.php:109)

ToctreeSortingTransformer runs at priority 3200, but GlobMenuEntryNodeTransformer runs at priority 4000 and unconditionally expands/removes every GlobMenuEntryNode in every document first — Compiler.php drives passes off an SplPriorityQueue, which is a max-heap in PHP and dequeues highest priority first, confirmed locally:

$q = new SplPriorityQueue();
$q->insert("low", 3200);
$q->insert("high", 4000);
foreach ($q as $item) { echo $item . "\n"; }
// prints: high, low

So by the time sortMenuEntriesByToctrees() iterates $tocNode->getValue(), no GlobMenuEntryNode instances remain to trigger the return null guard — the protection described in the PR body ("globbed toctrees are skipped... it now looks at the node type") never actually fires in a normal build. It's not visibly corrupting order today only because the glob-expansion append order and the document-tree attach order happen to be produced by the same loop in GlobMenuEntryNodeTransformer::handleMenuEntry(), so this is a latent/documented-intent defect rather than a currently-observable bug — but the explicit glob-safety net the PR claims to add provides no actual protection against future refactors of the glob-expansion ordering.

[CybotTM]

Thanks — both findings are correct. I reproduced each one rather than reading the code back, and both are fixed as of f04526e. Details below, including one place where your second finding cannot be turned into a test and why.

1. The duplicated external link. Confirmed exactly as described. ExternalMenuEntryNodeTransformer builds a fresh ExternalEntryNode per occurrence, and the dedup guard in MenuEntryManagement::attachDocumentEntriesToParents only covers DocumentEntryNode — ExternalEntryNode extends EntryNode, so it falls through and is attached twice. Rendering a toctree of alpha, External A <…/a>, beta, External A <…/a> gave sidebar External A, External A, Alpha, Beta against page Alpha, External A, Beta, External A: the count check tripped, null came back, and the whole page fell back to the type-grouped order. The position map now records one position per occurrence and hands them out in turn with array_shift. An internal entry is deduplicated on attach, so its single menu entry takes the first occurrence and the later ones stay unused — "listed twice keeps its first authored position" still holds. The two menu entries of a duplicated external link take both occurrences. Every menu entry now consumes a distinct position, so the count check has nothing left to catch and is gone. Pinned as bootstrap-menu-duplicate-external-order, verified failing on the previous commit of this branch with the order above.

2. The glob guard. Also confirmed, and your reading of SplPriorityQueue is right. Rather than argue it from the priorities I instrumented the branch and ran the whole integration suite: zero hits against 138 sortings. The guard is removed rather than relocated, because the expanded entries carry the urls the sorting matches on and they sort correctly — which is what those 138 sortings were already showing.

One consequence worth stating plainly: there is no test that fails without that removal, and there cannot be one. The guard was unreachable, so output with and without it is identical. bootstrap-menu-glob-external-order therefore is not a regression test for this finding — it passes against the previous commit of this branch. What it does pin is that a glob toctree followed by a toctree with an external link produces a navigation menu matching the page; it fails against main, and it is what would catch a future change to the glob-expansion ordering. That is the protection the dead guard claimed to provide, now actually wired to an assertion.

The PR body's "Fix" section has been corrected in place — the sentence "globbed toctrees are skipped" was wrong and is struck through there now.

3. A gap your review made me look for. The sorted plus :reversed: path is new in this branch and had no coverage anywhere: my fixtures never set :reversed:, and toctree-level-2-reversed and toctree-glob-reverse never mix in an external link. bootstrap-menu-reversed-external-order closes that; it fails against main.

The wholesale :reversed: fallback is kept, but nothing in the suite reaches it — all three call sites of attachDocumentEntriesToParents attach only under a TocNode of the current document. I could not construct an input that gets there and could not prove it unreachable either, so the branch stays and its comment now says exactly that instead of naming a case as though it had been observed. If you know of a path that reaches it, I would rather have a test than a comment.

Verified on the final tree: PHPUnit 836 tests green (integration 235), PHPStan level max clean, PHPCS clean. Rebased onto main at 382485e.

Assisted by claude-code:claude-opus-5 — Session