From 4f58273652838cfc963b6bc02439e80fb969c594 Mon Sep 17 00:00:00 2001
From: Guillaume Turri <guillaume.turri@gmail.com>
Date: Sat, 20 Mar 2021 19:12:19 +0100
Subject: [PATCH] Add tests on sectionID and fixes #3436

This commit fixes a bug on sectionID which could lead to having duplicated id.

Note that this commit changes the API a bit $check used to be a key-value array
(with key=the origin id, and value=the number of time this id has been generated)
and is now just an array of string. That's because the previous structure
couldn't work.

As far as Dokuwiki sources are concerned, this change should have no impact because:
- most call to sectionID are done with $check=false (and the behavior in this case
  isn't changed
- only Doku_Renderer->_headerToLink passes an actual array to this method. But this
  array is initialized empty, it is only filled and read by sectionID itself.
  (this Doku_Renderer->headers array is protected but the classes which extends it
  aren't referring this array).

It could still break usages of pluging that would call sectionID and rely on the
format of this array. But even if this commits kept $check as an array<string, bool>
(for instance setting the values to 1) the semantic wouldn't be the same.

To put it in a nutshell:
- this change of API has 0 impact on the core of Dokuwiki
- there is no evidence that it would impact other code
- there doesn't seem to be a clean way to avoid this change

This commit fixes #3436
---
 _test/tests/inc/pageutils_sectionid.test.php | 43 ++++++++++++++++++++
 inc/pageutils.php                            | 17 ++++----
 2 files changed, 52 insertions(+), 8 deletions(-)
 create mode 100644 _test/tests/inc/pageutils_sectionid.test.php

diff --git a/_test/tests/inc/pageutils_sectionid.test.php b/_test/tests/inc/pageutils_sectionid.test.php
new file mode 100644
index 000000000..f7cfcc68a
--- /dev/null
+++ b/_test/tests/inc/pageutils_sectionid.test.php
@@ -0,0 +1,43 @@
+<?php
+
+class sectionid_test extends Dokuwikitest
+{
+    /**
+     * DataProvider
+     *
+     * @return Generator|array
+     * @see testSectionidsAreUnique
+     */
+    public function provideTestData(){
+        // Each test case represents a sequence of sections title
+        return [
+            [['A', 'A', 'A1']],
+            [['A', 'A1', 'A']]
+        ];
+    }
+
+    /**
+     * @dataProvider provideTestData
+     * @param array $titles
+     */
+    function testSectionidsAreUnique($titles)
+    {
+        $check = array();
+        $alreadyGeneratedIds = array();
+        foreach($titles as $title){
+            $newId = sectionID($title, $check);
+            $this->assertNotContains($newId, $alreadyGeneratedIds, "id $newId has been generated twice. The 2nd time it was for the title $title");
+            $alreadyGeneratedIds []= $newId;
+        }
+    }
+
+    /**
+     * The convention in the code is to pass $check=false when we're not interested in having
+     * unique sectionID. This test ensures that this type of call is correctly handled
+     */
+    function testSectionIDCanBeCalledWithNonArrayCheck(){
+        $check = false;
+        $this->assertEquals("abc", sectionID("abc", $check), "Passing \$check=false shouldn't lead to an error");
+        $this->assertEquals("abc", sectionID("abc", $check), "Passing \$check=false shouldn't try to deduplicate id");
+    }
+}
diff --git a/inc/pageutils.php b/inc/pageutils.php
index d4a8bb7c4..c281ffc5b 100644
--- a/inc/pageutils.php
+++ b/inc/pageutils.php
@@ -226,7 +226,7 @@ function noNSorNS($id) {
  * Creates a XHTML valid linkid from a given headline title
  *
  * @param string  $title   The headline title
- * @param array|bool   $check   Existing IDs (title => number)
+ * @param array|bool   $check   Existing IDs
  * @return string the title
  *
  * @author Andreas Gohr <andi@splitbrain.org>
@@ -241,15 +241,16 @@ function sectionID($title,&$check) {
     }
 
     if(is_array($check)){
-        // make sure tiles are unique
-        if (!array_key_exists ($title,$check)) {
-            $check[$title] = 0;
-        } else {
-            $title .= ++ $check[$title];
+        $suffix=0;
+        $candidateTitle = $title;
+        while(in_array($candidateTitle, $check)){
+          $candidateTitle = $title . ++$suffix;
         }
+        $check []= $candidateTitle;
+        return $candidateTitle;
+    } else {
+      return $title;
     }
-
-    return $title;
 }
 
 /**