Skip to content

Fix section ID deduplication by looping through the whole node tree #1322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jaapio merged 1 commit into from
Mar 22, 2026
Merged

Fix section ID deduplication by looping through the whole node tree #1322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 25 additions & 29 deletions packages/guides/src/Compiler/Passes/ImplicitHyperlinkTargetPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@
use function array_merge;
use function current;
use function in_array;
use function key;
use function next;
use function prev;

/**
* Resolves the hyperlink target for each section in the document.
Expand All @@ -50,44 +48,42 @@ public function run(array $documents, CompilerContextInterface $compilerContext)
$knownReferences = $this->fetchExplicitReferences($document);

$nodes = $document->getNodes();
$node = current($nodes);
do {
if ($node instanceof AnchorNode) {
// override implicit section reference if an anchor precedes the section
$key = key($nodes);
$section = next($nodes);
if (!$section instanceof SectionNode) {
prev($nodes);
continue;
}

$section->getTitle()->setId($node->getValue());
if ($key !== null) {
$document = $document->removeNode($key);
}

continue;
}
Comment on lines -55 to -70
Copy link
Copy Markdown
Contributor Author

@wouterj wouterj Mar 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is redundant: Explicit anchors don't override implicit section IDs. They add additional section IDs.

    <div class="section" id="another-subtitle">
        <a id="something-different"></a>

The menu logic can keep using the implicit references, reducing complexity in this compiler pass.

$this->deduplicateSectionIds($nodes, $knownReferences);

if (!($node instanceof SectionNode)) {
continue;
}
return $document;
}, $documents);
}

/**
* @param array<int, Node> $nodes
* @param list<string> $knownIds
*
* @return list<string>
*/
private function deduplicateSectionIds(array $nodes, array $knownIds): array
{
$node = current($nodes);
do {
if ($node instanceof SectionNode) {
$realId = $sectionId = $node->getTitle()->getId();

// resolve conflicting references by appending an increasing number
$i = 1;
while (in_array($realId, $knownReferences, true)) {
while (in_array($realId, $knownIds, true)) {
$realId = $sectionId . '-' . ($i++);
}

$node->getTitle()->setId($realId);
$knownReferences[] = $realId;
//phpcs:ignore SlevomatCodingStandard.ControlStructures.AssignmentInCondition.AssignmentInCondition
} while ($node = next($nodes));
$knownIds[] = $realId;
}

return $document;
}, $documents);
if ($node instanceof CompoundNode) {
$knownIds = $this->deduplicateSectionIds($node->getChildren(), $knownIds);
}
//phpcs:ignore SlevomatCodingStandard.ControlStructures.AssignmentInCondition.AssignmentInCondition
} while ($node = next($nodes));

return $knownIds;
}

/** @return string[] */
Expand Down
116 changes: 54 additions & 62 deletions packages/guides/tests/unit/Compiler/Passes/ImplicitHyperlinkTargetPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,85 +21,75 @@
use phpDocumentor\Guides\Nodes\SectionNode;
use phpDocumentor\Guides\Nodes\TitleNode;
use PHPUnit\Framework\TestCase;
use Symfony\Component\String\Slugger\AsciiSlugger;

final class ImplicitHyperlinkTargetPassTest extends TestCase
{
public function testAllImplicitUniqueSections(): void
{
$document = new DocumentNode('1', 'index');
$expected = new DocumentNode('1', 'index');
$slugger = new AsciiSlugger();
foreach (['Document 1', 'Section A', 'Section B'] as $titles) {
$document->addChildNode(
$documentSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
$document->addChildNode($documentSection);
foreach (['Document 1' => 'document-1', 'Section A' => 'section-a', 'Section B' => 'section-b'] as $title => $id) {
$documentSection->addChildNode(
new SectionNode(
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($titles), 1, $slugger->slug($titles)->lower()->toString()),
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($title), 1, $id),
),
);
$expected->addChildNode(
}

$expected = new DocumentNode('1', 'index');
$expectedSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
$expected->addChildNode($expectedSection);
foreach (['Document 1' => 'document-1', 'Section A' => 'section-a', 'Section B' => 'section-b'] as $title => $id) {
$expectedSection->addChildNode(
new SectionNode(
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($titles), 1, $slugger->slug($titles)->lower()->toString()),
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($title), 1, $id),
),
);
}

$pass = new ImplicitHyperlinkTargetPass();
$resultDocuments = $pass->run([clone $document], new CompilerContext(new ProjectNode()));
$resultDocuments = $pass->run([$document], new CompilerContext(new ProjectNode()));

self::assertEquals([$expected], $resultDocuments);
}

public function testImplicitWithConflict(): void
{
$document = new DocumentNode('1', 'index');
$expected = new DocumentNode('1', 'index');
$slugger = new AsciiSlugger();

foreach (['Document 1', 'Section A', 'Section A'] as $titles) {
$document->addChildNode(
$documentSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
$document->addChildNode($documentSection);
foreach (['Document 1' => 'document-1', 'Section A' => 'section-a'] as $title => $id) {
$documentSection->addChildNode(
new SectionNode(
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($titles), 1, $slugger->slug($titles)->lower()->toString()),
),
);
$expected->addChildNode(
new SectionNode(
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($titles), 1, $slugger->slug($titles)->lower()->toString()),
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($title), 1, $id),
),
);
}

$pass = new ImplicitHyperlinkTargetPass();
$resultDocuments = $pass->run([$document], new CompilerContext(new ProjectNode()));

$section = $expected->getNodes()[2];
self::assertInstanceOf(SectionNode::class, $section);
$section->getTitle()->setId('section-a-1');

self::assertEquals([$expected], $resultDocuments);
}
$documentSection->addChildNode(
new SectionNode(
new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a'),
),
);

public function testExplicit(): void
{
$document = new DocumentNode('1', 'index');
$expected = new DocumentNode('1', 'index');
$expectedSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
$expected->addChildNode($expectedSection);
foreach (['Document 1' => 'document-1', 'Section A' => 'section-a'] as $title => $id) {
$expectedSection->addChildNode(
new SectionNode(
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($title), 1, $id),
),
);
}

$document->addChildNode(new SectionNode(
new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1'),
));
$expected->addChildNode(new SectionNode(
new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1'),
));

$document->addChildNode(new AnchorNode('custom-anchor'));
$expected->addChildNode(new AnchorNode('removed'));
$expected = $expected->removeNode(1);

$document->addChildNode(
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a')),
$expectedSection->addChildNode(
new SectionNode(
// conflict in ID, "-1" is added
new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a-1'),
),
);
$expectedTitle = new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a');
$expectedTitle->setId('custom-anchor');
$expected->addChildNode(new SectionNode($expectedTitle));

$pass = new ImplicitHyperlinkTargetPass();
$resultDocuments = $pass->run([$document], new CompilerContext(new ProjectNode()));
Expand All @@ -110,25 +100,27 @@ public function testExplicit(): void
public function testExplicitHasPriorityOverImplicit(): void
{
$document = new DocumentNode('1', 'index');
$expected = new DocumentNode('1', 'index');

$document->addChildNode(
$documentSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
$document->addChildNode($documentSection);
$documentSection->addChildNode(
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1')),
);
$expectedTitle = new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1');
$expectedTitle->setId('document-1-1');
$expected->addChildNode(new SectionNode($expectedTitle));

$document->addChildNode(new AnchorNode('document-1'));
$expected->addChildNode(new AnchorNode('removed'));
$expected = $expected->removeNode(1);
$documentSection->addChildNode(new AnchorNode('document-1'));
$documentSection->addChildNode(
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a')),
);

$document->addChildNode(
$expected = new DocumentNode('1', 'index');
$expectedSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
$expected->addChildNode($expectedSection);
$expectedSection->addChildNode(
// "document-1" is claimed by an explicit reference anchor, implicit reference gets the "-1" suffix
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1-1')),
);
$expectedSection->addChildNode(new AnchorNode('document-1'));
$expectedSection->addChildNode(
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a')),
);
$expectedTitle = new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a');
$expectedTitle->setId('document-1');
$expected->addChildNode(new SectionNode($expectedTitle));

$pass = new ImplicitHyperlinkTargetPass();
$resultDocuments = $pass->run([$document], new CompilerContext(new ProjectNode()));
Expand Down
27 changes: 27 additions & 0 deletions tests/Functional/tests/titles-ids/titles-ids.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<div class="section" id="main-title">
<h1>
Main title
</h1>
<div class="section" id="subtitle">
<h2>
Subtitle
</h2>
</div>
<div class="section" id="other-subtitle">
<h2>
Other Subtitle
</h2>
<div class="section" id="subtitle-1">
<h3>
Subtitle
</h3>
<p>Duplicate titles should get a unique ID through a <code>-1</code> suffix.</p>
</div>
</div>
<div class="section" id="other-subtitle-1">
<h2>
Other <code>Subtitle</code>
</h2>
<p>This also works for titles with markup.</p>
</div>
</div>
18 changes: 18 additions & 0 deletions tests/Functional/tests/titles-ids/titles-ids.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
Main title
==========

Subtitle
--------

Other Subtitle
--------------

Subtitle
~~~~~~~~

Duplicate titles should get a unique ID through a ``-1`` suffix.

Other ``Subtitle``
------------------

This also works for titles with markup.
2 changes: 1 addition & 1 deletion tests/Integration/tests/images/image-target/expected/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ <h2>Link image to reference</h2>
width="400" alt="Alternative text" />
</a>
</div>
<div class="section" id="link-image-to-reference">
<div class="section" id="link-image-to-reference-1">
<h2>Link image to reference</h2>
<a href="#start"><img
src="images/hero2-illustration.svg"
Expand Down
4 changes: 2 additions & 2 deletions tests/Integration/tests/images/image-target/input/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Link image to external URL
:target: https://example.org/

Link image to email
==========================
===================

.. image:: /images/hero2-illustration.svg
:width: 400
Expand Down Expand Up @@ -52,4 +52,4 @@ Link image to reference

.. toctree::

subfolder/subpage
subfolder/subpage
Loading