Added deeper content id de-duplication

Closes #1393

diff --git a/app/Entities/Repos/PageRepo.php b/app/Entities/Repos/PageRepo.php
index aad540adba9a7e4b649acb10a2d163b11a259abc..1aeee8daeab9bc66c694f112995f0074ecee74fb 100644 (file)
--- a/app/Entities/Repos/PageRepo.php
+++ b/app/Entities/Repos/PageRepo.php
@@ -7,6 +7,7 @@ use BookStack\Entities\Page;
 use BookStack\Entities\PageRevision;
 use Carbon\Carbon;
 use DOMDocument;
 use BookStack\Entities\PageRevision;
 use Carbon\Carbon;
 use DOMDocument;

+use DOMElement;

 use DOMXPath;
 
 class PageRepo extends EntityRepo
 use DOMXPath;
 
 class PageRepo extends EntityRepo


@@ -129,8 +130,7 @@ class PageRepo extends EntityRepo
     }
 
     /**
     }
 
     /**

-     * Formats a page's html to be tagged correctly
-     * within the system.
+     * Formats a page's html to be tagged correctly within the system.

      * @param string $htmlText
      * @return string
      */
      * @param string $htmlText
      * @return string
      */


@@ -148,37 +148,17 @@ class PageRepo extends EntityRepo
         $body = $container->childNodes->item(0);
         $childNodes = $body->childNodes;
 
         $body = $container->childNodes->item(0);
         $childNodes = $body->childNodes;
 

-        // Ensure no duplicate ids are used
-        $idArray = [];
-
+        // Set ids on top-level nodes
+        $idMap = [];

         foreach ($childNodes as $index => $childNode) {
         foreach ($childNodes as $index => $childNode) {

-            /** @var \DOMElement $childNode */
-            if (get_class($childNode) !== 'DOMElement') {
-                continue;
-            }
-
-            // Overwrite id if not a BookStack custom id
-            if ($childNode->hasAttribute('id')) {
-                $id = $childNode->getAttribute('id');
-                if (strpos($id, 'bkmrk') === 0 && array_search($id, $idArray) === false) {
-                    $idArray[] = $id;
-                    continue;
-                };
-            }
-
-            // Create an unique id for the element
-            // Uses the content as a basis to ensure output is the same every time
-            // the same content is passed through.
-            $contentId = 'bkmrk-' . substr(strtolower(preg_replace('/\s+/', '-', trim($childNode->nodeValue))), 0, 20);
-            $newId = urlencode($contentId);
-            $loopIndex = 0;
-            while (in_array($newId, $idArray)) {
-                $newId = urlencode($contentId . '-' . $loopIndex);
-                $loopIndex++;
-            }
+            $this->setUniqueId($childNode, $idMap);
+        }

 
 

-            $childNode->setAttribute('id', $newId);
-            $idArray[] = $newId;
+        // Ensure no duplicate ids within child items
+        $xPath = new DOMXPath($doc);
+        $idElems = $xPath->query('//body//*//*[@id]');
+        foreach ($idElems as $domElem) {
+            $this->setUniqueId($domElem, $idMap);

         }
 
         // Generate inner html as a string
         }
 
         // Generate inner html as a string


@@ -190,6 +170,41 @@ class PageRepo extends EntityRepo
         return $html;
     }
 
         return $html;
     }
 

+    /**
+     * Set a unique id on the given DOMElement.
+     * A map for existing ID's should be passed in to check for current existence.
+     * @param DOMElement $element
+     * @param array $idMap
+     */
+    protected function setUniqueId($element, array &$idMap)
+    {
+        if (get_class($element) !== 'DOMElement') {
+            return;
+        }
+
+        // Overwrite id if not a BookStack custom id
+        $existingId = $element->getAttribute('id');
+        if (strpos($existingId, 'bkmrk') === 0 && !isset($idMap[$existingId])) {
+            $idMap[$existingId] = true;
+            return;
+        }
+
+        // Create an unique id for the element
+        // Uses the content as a basis to ensure output is the same every time
+        // the same content is passed through.
+        $contentId = 'bkmrk-' . substr(strtolower(preg_replace('/\s+/', '-', trim($element->nodeValue))), 0, 20);
+        $newId = urlencode($contentId);
+        $loopIndex = 0;
+
+        while (isset($idMap[$newId])) {
+            $newId = urlencode($contentId . '-' . $loopIndex);
+            $loopIndex++;
+        }
+
+        $element->setAttribute('id', $newId);
+        $idMap[$newId] = true;
+    }
+

     /**
      * Get the plain text version of a page's content.
      * @param \BookStack\Entities\Page $page
     /**
      * Get the plain text version of a page's content.
      * @param \BookStack\Entities\Page $page

diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php
index 03cf03956db0bf534fb097c4687fab5e4207eca1..88169c50da486c13ef710213d83b79609e021911 100644 (file)
--- a/tests/Entity/PageContentTest.php
+++ b/tests/Entity/PageContentTest.php
@@ -120,7 +120,7 @@ class PageContentTest extends TestCase
         $this->asEditor();
         $page = Page::first();
 
         $this->asEditor();
         $page = Page::first();
 

-        $content = '<p id="bkmrk-test">test a</p>'."\n".'<p id="bkmrk-test">test b</p>';
+        $content = '<ul id="bkmrk-test"><li>test a</li><li><ul id="bkmrk-test"><li>test b</li></ul></li></ul>';

         $pageSave = $this->put($page->getUrl(), [
             'name' => $page->name,
             'html' => $content,
         $pageSave = $this->put($page->getUrl(), [
             'name' => $page->name,
             'html' => $content,