[BUGFIX] Fix unique slug when counter suffix fails

A loop in the Slug "uniqueIn*" check is broken because the comparison uses post-increment, and in case no valid slug with a counter suffix is found, $counter will be 101, not 100. Resolves: #91547 Releases: master, 10.4 Change-Id: If96974b52c6254b4b8298df24403ff3941489fbc Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/65429 Tested-by: TYPO3com <noreply@typo3.com> Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de> Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
TYPO3 · Aug 25, 2020 · 9203ef8 · 9203ef8
1 parent e854a6a
commit 9203ef8
Show file tree

Hide file tree

Showing 3 changed files with 251 additions and 38 deletions.
diff --git a/typo3/sysext/core/Classes/DataHandling/SlugHelper.php b/typo3/sysext/core/Classes/DataHandling/SlugHelper.php
@@ -32,6 +32,7 @@
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Core\Utility\MathUtility;
 use TYPO3\CMS\Core\Utility\RootlineUtility;
+use TYPO3\CMS\Core\Utility\StringUtility;
 use TYPO3\CMS\Core\Versioning\VersionState;
 
 /**
@@ -370,28 +371,42 @@ protected function flushRootLineCaches(): void
      *
      * @param string $slug proposed slug
      * @param RecordState $state
+     * @param callable $isUnique Callback to check for uniqueness
      * @return string
-     * @throws \TYPO3\CMS\Core\Exception\SiteNotFoundException
+     * @throws SiteNotFoundException
      */
-    public function buildSlugForUniqueInSite(string $slug, RecordState $state): string
+    protected function buildSlug(string $slug, RecordState $state, callable $isUnique): string
     {
         $slug = $this->sanitize($slug);
         $rawValue = $this->extract($slug);
         $newValue = $slug;
         $counter = 0;
-        while (!$this->isUniqueInSite(
-            $newValue,
-            $state
-        ) && $counter++ < 100
+        while (
+            !call_user_func($isUnique, $newValue, $state)
+            && ++$counter < 100
         ) {
             $newValue = $this->sanitize($rawValue . '-' . $counter);
         }
         if ($counter === 100) {
-            $newValue = $this->sanitize($rawValue . '-' . GeneralUtility::shortMD5($rawValue));
+            $uniqueId = StringUtility::getUniqueId();
+            $newValue = $this->sanitize($rawValue . '-' . GeneralUtility::shortMD5($uniqueId));
         }
         return $newValue;
     }
 
+    /**
+     * Generate a slug with a suffix "/mytitle-1" if that is in use already.
+     *
+     * @param string $slug proposed slug
+     * @param RecordState $state
+     * @return string
+     * @throws SiteNotFoundException
+     */
+    public function buildSlugForUniqueInSite(string $slug, RecordState $state): string
+    {
+        return $this->buildSlug($slug, $state, [$this, 'isUniqueInSite']);
+    }
+
     /**
      * Generate a slug with a suffix "/mytitle-1" if the suggested slug is in use already.
      *
@@ -401,21 +416,7 @@ public function buildSlugForUniqueInSite(string $slug, RecordState $state): stri
      */
     public function buildSlugForUniqueInPid(string $slug, RecordState $state): string
     {
-        $slug = $this->sanitize($slug);
-        $rawValue = $this->extract($slug);
-        $newValue = $slug;
-        $counter = 0;
-        while (!$this->isUniqueInPid(
-            $newValue,
-            $state
-        ) && $counter++ < 100
-        ) {
-            $newValue = $this->sanitize($rawValue . '-' . $counter);
-        }
-        if ($counter === 100) {
-            $newValue = $this->sanitize($rawValue . '-' . GeneralUtility::shortMD5($rawValue));
-        }
-        return $newValue;
+        return $this->buildSlug($slug, $state, [$this, 'isUniqueInPid']);
     }
 
     /**
@@ -424,25 +425,11 @@ public function buildSlugForUniqueInPid(string $slug, RecordState $state): strin
      * @param string $slug proposed slug
      * @param RecordState $state
      * @return string
-     * @throws \TYPO3\CMS\Core\Exception\SiteNotFoundException
+     * @throws SiteNotFoundException
      */
     public function buildSlugForUniqueInTable(string $slug, RecordState $state): string
     {
-        $slug = $this->sanitize($slug);
-        $rawValue = $this->extract($slug);
-        $newValue = $slug;
-        $counter = 0;
-        while (!$this->isUniqueInTable(
-            $newValue,
-            $state
-        ) && $counter++ < 100
-        ) {
-            $newValue = $this->sanitize($rawValue . '-' . $counter);
-        }
-        if ($counter === 100) {
-            $newValue = $this->sanitize($rawValue . '-' . GeneralUtility::shortMD5($rawValue));
-        }
-        return $newValue;
+        return $this->buildSlug($slug, $state, [$this, 'isUniqueInTable']);
     }
 
     /**

diff --git a/typo3/sysext/core/Tests/Functional/DataHandling/Slug/DataSet/PagesForSlugsUnique.csv b/typo3/sysext/core/Tests/Functional/DataHandling/Slug/DataSet/PagesForSlugsUnique.csv
@@ -0,0 +1,103 @@
+pages,,,,,,,,
+,uid,pid,deleted,sys_language_uid,l10n_parent,l10n_source,title,slug
+,1,0,0,0,0,0,Root,/
+,2,1,0,0,0,0,unique slug,/unique-slug
+,3,1,0,0,0,0,unique slug,/unique-slug-1
+,4,1,0,0,0,0,unique slug,/unique-slug-2
+,5,1,0,0,0,0,unique slug,/unique-slug-3
+,6,1,0,0,0,0,unique slug,/unique-slug-4
+,7,1,0,0,0,0,unique slug,/unique-slug-5
+,8,1,0,0,0,0,unique slug,/unique-slug-6
+,9,1,0,0,0,0,unique slug,/unique-slug-7
+,10,1,0,0,0,0,unique slug,/unique-slug-8
+,11,1,0,0,0,0,unique slug,/unique-slug-9
+,12,1,0,0,0,0,unique slug,/unique-slug-10
+,13,1,0,0,0,0,unique slug,/unique-slug-11
+,14,1,0,0,0,0,unique slug,/unique-slug-12
+,15,1,0,0,0,0,unique slug,/unique-slug-13
+,16,1,0,0,0,0,unique slug,/unique-slug-14
+,17,1,0,0,0,0,unique slug,/unique-slug-15
+,18,1,0,0,0,0,unique slug,/unique-slug-16
+,19,1,0,0,0,0,unique slug,/unique-slug-17
+,20,1,0,0,0,0,unique slug,/unique-slug-18
+,21,1,0,0,0,0,unique slug,/unique-slug-19
+,22,1,0,0,0,0,unique slug,/unique-slug-20
+,23,1,0,0,0,0,unique slug,/unique-slug-21
+,24,1,0,0,0,0,unique slug,/unique-slug-22
+,25,1,0,0,0,0,unique slug,/unique-slug-23
+,26,1,0,0,0,0,unique slug,/unique-slug-24
+,27,1,0,0,0,0,unique slug,/unique-slug-25
+,28,1,0,0,0,0,unique slug,/unique-slug-26
+,29,1,0,0,0,0,unique slug,/unique-slug-27
+,30,1,0,0,0,0,unique slug,/unique-slug-28
+,31,1,0,0,0,0,unique slug,/unique-slug-29
+,32,1,0,0,0,0,unique slug,/unique-slug-30
+,33,1,0,0,0,0,unique slug,/unique-slug-31
+,34,1,0,0,0,0,unique slug,/unique-slug-32
+,35,1,0,0,0,0,unique slug,/unique-slug-33
+,36,1,0,0,0,0,unique slug,/unique-slug-34
+,37,1,0,0,0,0,unique slug,/unique-slug-35
+,38,1,0,0,0,0,unique slug,/unique-slug-36
+,39,1,0,0,0,0,unique slug,/unique-slug-37
+,40,1,0,0,0,0,unique slug,/unique-slug-38
+,41,1,0,0,0,0,unique slug,/unique-slug-39
+,42,1,0,0,0,0,unique slug,/unique-slug-40
+,43,1,0,0,0,0,unique slug,/unique-slug-41
+,44,1,0,0,0,0,unique slug,/unique-slug-42
+,45,1,0,0,0,0,unique slug,/unique-slug-43
+,46,1,0,0,0,0,unique slug,/unique-slug-44
+,47,1,0,0,0,0,unique slug,/unique-slug-45
+,48,1,0,0,0,0,unique slug,/unique-slug-46
+,49,1,0,0,0,0,unique slug,/unique-slug-47
+,50,1,0,0,0,0,unique slug,/unique-slug-48
+,51,1,0,0,0,0,unique slug,/unique-slug-49
+,52,1,0,0,0,0,unique slug,/unique-slug-50
+,53,1,0,0,0,0,unique slug,/unique-slug-51
+,54,1,0,0,0,0,unique slug,/unique-slug-52
+,55,1,0,0,0,0,unique slug,/unique-slug-53
+,56,1,0,0,0,0,unique slug,/unique-slug-54
+,57,1,0,0,0,0,unique slug,/unique-slug-55
+,58,1,0,0,0,0,unique slug,/unique-slug-56
+,59,1,0,0,0,0,unique slug,/unique-slug-57
+,60,1,0,0,0,0,unique slug,/unique-slug-58
+,61,1,0,0,0,0,unique slug,/unique-slug-59
+,62,1,0,0,0,0,unique slug,/unique-slug-60
+,63,1,0,0,0,0,unique slug,/unique-slug-61
+,64,1,0,0,0,0,unique slug,/unique-slug-62
+,65,1,0,0,0,0,unique slug,/unique-slug-63
+,66,1,0,0,0,0,unique slug,/unique-slug-64
+,67,1,0,0,0,0,unique slug,/unique-slug-65
+,68,1,0,0,0,0,unique slug,/unique-slug-66
+,69,1,0,0,0,0,unique slug,/unique-slug-67
+,70,1,0,0,0,0,unique slug,/unique-slug-68
+,71,1,0,0,0,0,unique slug,/unique-slug-69
+,72,1,0,0,0,0,unique slug,/unique-slug-70
+,73,1,0,0,0,0,unique slug,/unique-slug-71
+,74,1,0,0,0,0,unique slug,/unique-slug-72
+,75,1,0,0,0,0,unique slug,/unique-slug-73
+,76,1,0,0,0,0,unique slug,/unique-slug-74
+,77,1,0,0,0,0,unique slug,/unique-slug-75
+,78,1,0,0,0,0,unique slug,/unique-slug-76
+,79,1,0,0,0,0,unique slug,/unique-slug-77
+,80,1,0,0,0,0,unique slug,/unique-slug-78
+,81,1,0,0,0,0,unique slug,/unique-slug-79
+,82,1,0,0,0,0,unique slug,/unique-slug-80
+,83,1,0,0,0,0,unique slug,/unique-slug-81
+,84,1,0,0,0,0,unique slug,/unique-slug-82
+,85,1,0,0,0,0,unique slug,/unique-slug-83
+,86,1,0,0,0,0,unique slug,/unique-slug-84
+,87,1,0,0,0,0,unique slug,/unique-slug-85
+,88,1,0,0,0,0,unique slug,/unique-slug-86
+,89,1,0,0,0,0,unique slug,/unique-slug-87
+,90,1,0,0,0,0,unique slug,/unique-slug-88
+,91,1,0,0,0,0,unique slug,/unique-slug-89
+,92,1,0,0,0,0,unique slug,/unique-slug-90
+,93,1,0,0,0,0,unique slug,/unique-slug-91
+,94,1,0,0,0,0,unique slug,/unique-slug-92
+,95,1,0,0,0,0,unique slug,/unique-slug-93
+,96,1,0,0,0,0,unique slug,/unique-slug-94
+,97,1,0,0,0,0,unique slug,/unique-slug-95
+,98,1,0,0,0,0,unique slug,/unique-slug-96
+,99,1,0,0,0,0,unique slug,/unique-slug-97
+,100,1,0,0,0,0,unique slug,/unique-slug-98
+,101,1,0,0,0,0,unique slug,/unique-slug-99
diff --git a/typo3/sysext/core/Tests/Functional/DataHandling/Slug/SlugHelperUniqueTest.php b/typo3/sysext/core/Tests/Functional/DataHandling/Slug/SlugHelperUniqueTest.php
@@ -0,0 +1,123 @@
+<?php
+
+declare(strict_types=1);
+
+namespace TYPO3\CMS\Core\Tests\Functional\DataHandling\Slug;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use TYPO3\CMS\Core\DataHandling\Model\RecordStateFactory;
+use TYPO3\CMS\Core\DataHandling\SlugHelper;
+use TYPO3\CMS\Core\Tests\Functional\DataHandling\AbstractDataHandlerActionTestCase;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
+
+class SlugHelperUniqueTest extends AbstractDataHandlerActionTestCase
+{
+    /**
+     * @var string
+     */
+    protected $scenarioDataSetDirectory = 'typo3/sysext/core/Tests/Functional/DataHandling/Slug/DataSet/';
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->importScenarioDataSet('PagesForSlugsUnique');
+        $this->setUpFrontendSite(1);
+        $this->setUpFrontendRootPage(1, ['typo3/sysext/core/Tests/Functional/Fixtures/Frontend/JsonRenderer.typoscript']);
+    }
+
+    /**
+     * @test
+     */
+    public function buildSlugForUniqueInSiteRespectsMaxRetryOverflow()
+    {
+        $subject = GeneralUtility::makeInstance(
+            SlugHelper::class,
+            'pages',
+            'slug',
+            [
+                'generatorOptions' => [
+                    'fields' => ['title'],
+                    'prefixParentPageSlug' => true,
+                ],
+            ]
+        );
+
+        $state = RecordStateFactory::forName('pages')->fromArray(['uid' => 'NEW102', 'pid' => 1]);
+        $overflowSlug = $subject->buildSlugForUniqueInSite('/unique-slug', $state);
+        $parts = explode('-', $overflowSlug);
+        if (count($parts) !== 3) {
+            self::fail('No suffix to the slug was created');
+        }
+        $variablePartOfSlug = end($parts);
+        // shordMd5 return value is 10 chars long, so we can use this to assure the function has been called and returned a value
+        self::assertSame(10, strlen($variablePartOfSlug));
+    }
+
+    /**
+     * @test
+     */
+    public function buildSlugForUniqueInPidRespectsMaxRetryOverflow()
+    {
+        $subject = GeneralUtility::makeInstance(
+            SlugHelper::class,
+            'pages',
+            'slug',
+            [
+                'generatorOptions' => [
+                    'fields' => ['title'],
+                    'prefixParentPageSlug' => true,
+                ],
+            ]
+        );
+        $state = RecordStateFactory::forName('pages')->fromArray(['uid' => 'NEW102', 'pid' => 1]);
+        $overflowSlug = $subject->buildSlugForUniqueInPid('/unique-slug', $state);
+        $parts = explode('-', $overflowSlug);
+        if (count($parts) !== 3) {
+            self::fail('No suffix to the slug was created');
+        }
+        $variablePartOfSlug = end($parts);
+        // shordMd5 return value is 10 chars long, so we can use this to assure the function has been called and returned a value
+        self::assertSame(10, strlen($variablePartOfSlug));
+    }
+
+    /**
+     * @test
+     */
+    public function buildSlugForUniqueInTableRespectsMaxRetryOverflow()
+    {
+        $subject = GeneralUtility::makeInstance(
+            SlugHelper::class,
+            'pages',
+            'slug',
+            [
+                'generatorOptions' => [
+                    'fields' => ['title'],
+                    'prefixParentPageSlug' => true,
+                ],
+            ]
+        );
+
+        $state = RecordStateFactory::forName('pages')->fromArray(['uid' => 'NEW102', 'pid' => 1]);
+        $overflowSlug = $subject->buildSlugForUniqueInTable('/unique-slug', $state);
+        $parts = explode('-', $overflowSlug);
+        if (count($parts) !== 3) {
+            self::fail('No suffix to the slug was created');
+        }
+        $variablePartOfSlug = end($parts);
+        // shordMd5 return value is 10 chars long, so we can use this to assure the function has been called and returned a value
+        self::assertSame(10, strlen($variablePartOfSlug));
+    }
+}