Skip to content

Commit be8d211

Browse files
jaapiojaapio
authored
Merge pull request #1322 from wouterj/section-id-deduplication
Fix section ID deduplication by looping through the whole node tree
2 parents cf6e155 + 4c92985 commit be8d211

6 files changed

Lines changed: 127 additions & 94 deletions

File tree

‎packages/guides/src/Compiler/Passes/ImplicitHyperlinkTargetPass.php‎

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,7 @@
2525
use function array_merge;
2626
use function current;
2727
use function in_array;
28-
use function key;
2928
use function next;
30-
use function prev;
3129

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

5250
$nodes = $document->getNodes();
53-
$node = current($nodes);
54-
do {
55-
if ($node instanceof AnchorNode) {
56-
// override implicit section reference if an anchor precedes the section
57-
$key = key($nodes);
58-
$section = next($nodes);
59-
if (!$section instanceof SectionNode) {
60-
prev($nodes);
61-
continue;
62-
}
63-
64-
$section->getTitle()->setId($node->getValue());
65-
if ($key !== null) {
66-
$document = $document->removeNode($key);
67-
}
68-
69-
continue;
70-
}
51+
$this->deduplicateSectionIds($nodes, $knownReferences);
7152

72-
if (!($node instanceof SectionNode)) {
73-
continue;
74-
}
53+
return $document;
54+
}, $documents);
55+
}
7556

57+
/**
58+
* @param array<int, Node> $nodes
59+
* @param list<string> $knownIds
60+
*
61+
* @return list<string>
62+
*/
63+
private function deduplicateSectionIds(array $nodes, array $knownIds): array
64+
{
65+
$node = current($nodes);
66+
do {
67+
if ($node instanceof SectionNode) {
7668
$realId = $sectionId = $node->getTitle()->getId();
7769

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

8476
$node->getTitle()->setId($realId);
85-
$knownReferences[] = $realId;
86-
//phpcs:ignore SlevomatCodingStandard.ControlStructures.AssignmentInCondition.AssignmentInCondition
87-
} while ($node = next($nodes));
77+
$knownIds[] = $realId;
78+
}
8879

89-
return $document;
90-
}, $documents);
80+
if ($node instanceof CompoundNode) {
81+
$knownIds = $this->deduplicateSectionIds($node->getChildren(), $knownIds);
82+
}
83+
//phpcs:ignore SlevomatCodingStandard.ControlStructures.AssignmentInCondition.AssignmentInCondition
84+
} while ($node = next($nodes));
85+
86+
return $knownIds;
9187
}
9288

9389
/** @return string[] */

‎packages/guides/tests/unit/Compiler/Passes/ImplicitHyperlinkTargetPassTest.php‎

Lines changed: 54 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -21,85 +21,75 @@
2121
use phpDocumentor\Guides\Nodes\SectionNode;
2222
use phpDocumentor\Guides\Nodes\TitleNode;
2323
use PHPUnit\Framework\TestCase;
24-
use Symfony\Component\String\Slugger\AsciiSlugger;
2524

2625
final class ImplicitHyperlinkTargetPassTest extends TestCase
2726
{
2827
public function testAllImplicitUniqueSections(): void
2928
{
3029
$document = new DocumentNode('1', 'index');
31-
$expected = new DocumentNode('1', 'index');
32-
$slugger = new AsciiSlugger();
33-
foreach (['Document 1', 'Section A', 'Section B'] as $titles) {
34-
$document->addChildNode(
30+
$documentSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
31+
$document->addChildNode($documentSection);
32+
foreach (['Document 1' => 'document-1', 'Section A' => 'section-a', 'Section B' => 'section-b'] as $title => $id) {
33+
$documentSection->addChildNode(
3534
new SectionNode(
36-
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($titles), 1, $slugger->slug($titles)->lower()->toString()),
35+
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($title), 1, $id),
3736
),
3837
);
39-
$expected->addChildNode(
38+
}
39+
40+
$expected = new DocumentNode('1', 'index');
41+
$expectedSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
42+
$expected->addChildNode($expectedSection);
43+
foreach (['Document 1' => 'document-1', 'Section A' => 'section-a', 'Section B' => 'section-b'] as $title => $id) {
44+
$expectedSection->addChildNode(
4045
new SectionNode(
41-
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($titles), 1, $slugger->slug($titles)->lower()->toString()),
46+
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($title), 1, $id),
4247
),
4348
);
4449
}
4550

4651
$pass = new ImplicitHyperlinkTargetPass();
47-
$resultDocuments = $pass->run([clone $document], new CompilerContext(new ProjectNode()));
52+
$resultDocuments = $pass->run([$document], new CompilerContext(new ProjectNode()));
4853

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

5257
public function testImplicitWithConflict(): void
5358
{
5459
$document = new DocumentNode('1', 'index');
55-
$expected = new DocumentNode('1', 'index');
56-
$slugger = new AsciiSlugger();
57-
58-
foreach (['Document 1', 'Section A', 'Section A'] as $titles) {
59-
$document->addChildNode(
60+
$documentSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
61+
$document->addChildNode($documentSection);
62+
foreach (['Document 1' => 'document-1', 'Section A' => 'section-a'] as $title => $id) {
63+
$documentSection->addChildNode(
6064
new SectionNode(
61-
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($titles), 1, $slugger->slug($titles)->lower()->toString()),
62-
),
63-
);
64-
$expected->addChildNode(
65-
new SectionNode(
66-
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($titles), 1, $slugger->slug($titles)->lower()->toString()),
65+
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($title), 1, $id),
6766
),
6867
);
6968
}
7069

71-
$pass = new ImplicitHyperlinkTargetPass();
72-
$resultDocuments = $pass->run([$document], new CompilerContext(new ProjectNode()));
73-
74-
$section = $expected->getNodes()[2];
75-
self::assertInstanceOf(SectionNode::class, $section);
76-
$section->getTitle()->setId('section-a-1');
77-
78-
self::assertEquals([$expected], $resultDocuments);
79-
}
70+
$documentSection->addChildNode(
71+
new SectionNode(
72+
new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a'),
73+
),
74+
);
8075

81-
public function testExplicit(): void
82-
{
83-
$document = new DocumentNode('1', 'index');
8476
$expected = new DocumentNode('1', 'index');
77+
$expectedSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
78+
$expected->addChildNode($expectedSection);
79+
foreach (['Document 1' => 'document-1', 'Section A' => 'section-a'] as $title => $id) {
80+
$expectedSection->addChildNode(
81+
new SectionNode(
82+
new TitleNode(InlineCompoundNode::getPlainTextInlineNode($title), 1, $id),
83+
),
84+
);
85+
}
8586

86-
$document->addChildNode(new SectionNode(
87-
new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1'),
88-
));
89-
$expected->addChildNode(new SectionNode(
90-
new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1'),
91-
));
92-
93-
$document->addChildNode(new AnchorNode('custom-anchor'));
94-
$expected->addChildNode(new AnchorNode('removed'));
95-
$expected = $expected->removeNode(1);
96-
97-
$document->addChildNode(
98-
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a')),
87+
$expectedSection->addChildNode(
88+
new SectionNode(
89+
// conflict in ID, "-1" is added
90+
new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a-1'),
91+
),
9992
);
100-
$expectedTitle = new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a');
101-
$expectedTitle->setId('custom-anchor');
102-
$expected->addChildNode(new SectionNode($expectedTitle));
10393

10494
$pass = new ImplicitHyperlinkTargetPass();
10595
$resultDocuments = $pass->run([$document], new CompilerContext(new ProjectNode()));
@@ -110,25 +100,27 @@ public function testExplicit(): void
110100
public function testExplicitHasPriorityOverImplicit(): void
111101
{
112102
$document = new DocumentNode('1', 'index');
113-
$expected = new DocumentNode('1', 'index');
114-
115-
$document->addChildNode(
103+
$documentSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
104+
$document->addChildNode($documentSection);
105+
$documentSection->addChildNode(
116106
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1')),
117107
);
118-
$expectedTitle = new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1');
119-
$expectedTitle->setId('document-1-1');
120-
$expected->addChildNode(new SectionNode($expectedTitle));
121-
122-
$document->addChildNode(new AnchorNode('document-1'));
123-
$expected->addChildNode(new AnchorNode('removed'));
124-
$expected = $expected->removeNode(1);
108+
$documentSection->addChildNode(new AnchorNode('document-1'));
109+
$documentSection->addChildNode(
110+
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a')),
111+
);
125112

126-
$document->addChildNode(
113+
$expected = new DocumentNode('1', 'index');
114+
$expectedSection = new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Index'), 1, 'index'));
115+
$expected->addChildNode($expectedSection);
116+
$expectedSection->addChildNode(
117+
// "document-1" is claimed by an explicit reference anchor, implicit reference gets the "-1" suffix
118+
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Document 1'), 1, 'document-1-1')),
119+
);
120+
$expectedSection->addChildNode(new AnchorNode('document-1'));
121+
$expectedSection->addChildNode(
127122
new SectionNode(new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a')),
128123
);
129-
$expectedTitle = new TitleNode(InlineCompoundNode::getPlainTextInlineNode('Section A'), 1, 'section-a');
130-
$expectedTitle->setId('document-1');
131-
$expected->addChildNode(new SectionNode($expectedTitle));
132124

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

‎tests/Functional/tests/titles-ids/titles-ids.html‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<div class="section" id="main-title">
2+
<h1>
3+
Main title
4+
</h1>
5+
<div class="section" id="subtitle">
6+
<h2>
7+
Subtitle
8+
</h2>
9+
</div>
10+
<div class="section" id="other-subtitle">
11+
<h2>
12+
Other Subtitle
13+
</h2>
14+
<div class="section" id="subtitle-1">
15+
<h3>
16+
Subtitle
17+
</h3>
18+
<p>Duplicate titles should get a unique ID through a <code>-1</code> suffix.</p>
19+
</div>
20+
</div>
21+
<div class="section" id="other-subtitle-1">
22+
<h2>
23+
Other <code>Subtitle</code>
24+
</h2>
25+
<p>This also works for titles with markup.</p>
26+
</div>
27+
</div>

‎tests/Functional/tests/titles-ids/titles-ids.rst‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
Main title
2+
==========
3+
4+
Subtitle
5+
--------
6+
7+
Other Subtitle
8+
--------------
9+
10+
Subtitle
11+
~~~~~~~~
12+
13+
Duplicate titles should get a unique ID through a ``-1`` suffix.
14+
15+
Other ``Subtitle``
16+
------------------
17+
18+
This also works for titles with markup.

‎tests/Integration/tests/images/image-target/expected/index.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ <h2>Link image to reference</h2>
3333
width="400" alt="Alternative text" />
3434
</a>
3535
</div>
36-
<div class="section" id="link-image-to-reference">
36+
<div class="section" id="link-image-to-reference-1">
3737
<h2>Link image to reference</h2>
3838
<a href="#start"><img
3939
src="images/hero2-illustration.svg"

‎tests/Integration/tests/images/image-target/input/index.rst‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ Link image to external URL
1616
:target: https://example.org/
1717

1818
Link image to email
19-
==========================
19+
===================
2020

2121
.. image:: /images/hero2-illustration.svg
2222
:width: 400
@@ -52,4 +52,4 @@ Link image to reference
5252

5353
.. toctree::
5454

55-
subfolder/subpage
55+
subfolder/subpage

0 commit comments

Comments
 (0)