[BUGFIX] substituteMarkerArrayCached() must accept special chars 19/45319/6
authorMarkus Klein <markus.klein@typo3.org>
Wed, 16 Dec 2015 18:04:07 +0000 (19:04 +0100)
committerAndreas Fernandez <typo3@scripting-base.de>
Thu, 17 Dec 2015 14:28:27 +0000 (15:28 +0100)
Add a bunch of unittests and streamline the code as well
by removing a useless preg_match_all() call.
Rename some variables and add comments.

Resolves: #72252
Releases: master, 6.2
Change-Id: I2a31a1c2ab6d83528428693213b922f0e1bc6fe5
Reviewed-on: https://review.typo3.org/45319
Reviewed-by: Tobias Klepp <tobias.klepp@stimme-der-hoffnung.de>
Tested-by: Tobias Klepp <tobias.klepp@stimme-der-hoffnung.de>
Reviewed-by: Andreas Fernandez <typo3@scripting-base.de>
Tested-by: Andreas Fernandez <typo3@scripting-base.de>
typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php
typo3/sysext/frontend/Tests/Unit/ContentObject/ContentObjectRendererTest.php
typo3/sysext/frontend/Tests/Unit/ContentObject/Fixtures/PageRepositoryFixture.php [new file with mode: 0644]

index 7ede0ea..f64531c 100644 (file)
@@ -2136,7 +2136,7 @@ class ContentObjectRenderer
      * but only the splitting of the template in various parts. So if you
      * want only one cache-entry per template, make sure you always pass the
      * exact same set of marker/subpart keys. Else you will be flooding the
-     * users cache table.
+     * user's cache table.
      *
      * This function takes three kinds of substitutions in one:
      * $markContentArray is a regular marker-array where the 'keys' are
@@ -2176,16 +2176,13 @@ class ContentObjectRenderer
         // Finding keys and check hash:
         $sPkeys = array_keys($subpartContentArray);
         $wPkeys = array_keys($wrappedSubpartContentArray);
-        $aKeys = array_merge(array_keys($markContentArray), $sPkeys, $wPkeys);
-        if (empty($aKeys)) {
+        $keysToReplace = array_merge(array_keys($markContentArray), $sPkeys, $wPkeys);
+        if (empty($keysToReplace)) {
             $timeTracker->pull();
             return $content;
         }
-        asort($aKeys);
-        $storeKey = md5('substituteMarkerArrayCached_storeKey:' . serialize(array(
-            $content,
-            $aKeys
-        )));
+        asort($keysToReplace);
+        $storeKey = md5('substituteMarkerArrayCached_storeKey:' . serialize(array($content, $keysToReplace)));
         if ($this->substMarkerCache[$storeKey]) {
             $storeArr = $this->substMarkerCache[$storeKey];
             $timeTracker->setTSlogMessage('Cached', 0);
@@ -2210,31 +2207,26 @@ class ContentObjectRenderer
                 }
 
                 $storeArr = array();
-                $result = preg_match_all('/###([\w:-]+)###/', $content, $usedMarkers);
-                if ($result !== false && !empty($usedMarkers[1])) {
-                    $tagArray = array_flip($usedMarkers[1]);
-
-                    $aKeys = array_flip($aKeys);
-                    $bKeys = array();
+                // search all markers in the content
+                $result = preg_match_all('/###([^#](?:[^#]*+|#{1,2}[^#])+)###/', $content, $markersInContent);
+                if ($result !== false && !empty($markersInContent[1])) {
+                    $keysToReplaceFlipped = array_flip($keysToReplace);
+                    $regexKeys = array();
+                    $wrappedKeys = array();
                     // Traverse keys and quote them for reg ex.
-                    foreach ($tagArray as $tV => $tK) {
-                        $tV = '###' . $tV . '###';
-                        if (isset($aKeys[$tV])) {
-                            $bKeys[$tK] = preg_quote($tV, '/');
+                    foreach ($markersInContent[1] as $key) {
+                        if (isset($keysToReplaceFlipped['###' . $key . '###'])) {
+                            $regexKeys[] = preg_quote($key, '/');
+                            $wrappedKeys[] = '###' . $key . '###';
                         }
                     }
-                    $regex = '/' . implode('|', $bKeys) . '/';
-                    // Doing regex's
-                    if (preg_match_all($regex, $content, $keyList) !== false) {
-                        $storeArr['c'] = preg_split($regex, $content);
-                        $storeArr['k'] = $keyList[0];
-                    }
-                    if (!empty($storeArr)) {
-                        // Setting cache:
-                        $this->substMarkerCache[$storeKey] = $storeArr;
-                        // Storing the cached data:
-                        $this->getTypoScriptFrontendController()->sys_page->storeHash($storeKey, $storeArr, 'substMarkArrayCached');
-                    }
+                    $regex = '/###(?:' . implode('|', $regexKeys) . ')###/';
+                    $storeArr['c'] = preg_split($regex, $content); // contains all content parts around markers
+                    $storeArr['k'] = $wrappedKeys; // contains all markers incl. ###
+                    // Setting cache:
+                    $this->substMarkerCache[$storeKey] = $storeArr;
+                    // Storing the cached data:
+                    $this->getTypoScriptFrontendController()->sys_page->storeHash($storeKey, $storeArr, 'substMarkArrayCached');
                 }
                 $timeTracker->setTSlogMessage('Parsing', 0);
             }
@@ -2247,14 +2239,21 @@ class ContentObjectRenderer
             $content = '';
             // Traversing the keyList array and merging the static and dynamic content
             foreach ($storeArr['k'] as $n => $keyN) {
+                // add content before marker
                 $content .= $storeArr['c'][$n];
                 if (!is_array($valueArr[$keyN])) {
+                    // fetch marker replacement from $markContentArray or $subpartContentArray
                     $content .= $valueArr[$keyN];
                 } else {
-                    $content .= $valueArr[$keyN][(int)$wSCA_reg[$keyN] % 2];
+                    if (!isset($wSCA_reg[$keyN])) {
+                        $wSCA_reg[$keyN] = 0;
+                    }
+                    // fetch marker replacement from $wrappedSubpartContentArray
+                    $content .= $valueArr[$keyN][$wSCA_reg[$keyN] % 2];
                     $wSCA_reg[$keyN]++;
                 }
             }
+            // add remaining content
             $content .= $storeArr['c'][count($storeArr['k'])];
         }
         $timeTracker->pull();
index 709ee70..f7ac169 100644 (file)
@@ -18,9 +18,11 @@ use Psr\Log\LoggerInterface;
 use TYPO3\CMS\Core\Charset\CharsetConverter;
 use TYPO3\CMS\Core\Core\ApplicationContext;
 use TYPO3\CMS\Core\Log\LogManager;
+use TYPO3\CMS\Core\TimeTracker\NullTimeTracker;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Frontend\ContentObject\AbstractContentObject;
 use TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer;
+use TYPO3\CMS\Frontend\Tests\Unit\ContentObject\Fixtures\PageRepositoryFixture;
 
 /**
  * Testcase for TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer
@@ -92,7 +94,7 @@ class ContentObjectRendererTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
         $this->createMockedLoggerAndLogManager();
 
         $this->templateServiceMock = $this->getMock(\TYPO3\CMS\Core\TypoScript\TemplateService::class, array('getFileName', 'linkData'));
-        $pageRepositoryMock = $this->getMock(\TYPO3\CMS\Frontend\Page\PageRepository::class, array('getRawRecord'));
+        $pageRepositoryMock = $this->getMock(PageRepositoryFixture::class, array('getRawRecord'));
 
         $this->typoScriptFrontendControllerMock = $this->getAccessibleMock(\TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController::class, array('dummy'), array(), '', false);
         $this->typoScriptFrontendControllerMock->tmpl = $this->templateServiceMock;
@@ -102,7 +104,7 @@ class ContentObjectRendererTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
         $this->typoScriptFrontendControllerMock->csConvObj = new CharsetConverter();
         $this->typoScriptFrontendControllerMock->renderCharset = 'utf-8';
         $GLOBALS['TSFE'] = $this->typoScriptFrontendControllerMock;
-
+        $GLOBALS['TT'] = new NullTimeTracker();
         $GLOBALS['TYPO3_DB'] = $this->getMock(\TYPO3\CMS\Core\Database\DatabaseConnection::class, array());
         $GLOBALS['TYPO3_CONF_VARS']['SYS']['t3lib_cs_utils'] = 'mbstring';
 
@@ -4694,10 +4696,222 @@ class ContentObjectRendererTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     }
 
     /**
+     * @return array
+     */
+    public function substituteMarkerArrayCachedReturnsExpectedContentDataProvider() {
+        return array(
+            'no markers defined' => array(
+                'dummy content with ###UNREPLACED### marker',
+                array(),
+                array(),
+                array(),
+                'dummy content with ###UNREPLACED### marker',
+                false,
+                false
+            ),
+            'no markers used' => array(
+                'dummy content with no marker',
+                array(
+                    '###REPLACED###' => '_replaced_'
+                ),
+                array(),
+                array(),
+                'dummy content with no marker',
+                true,
+                false
+            ),
+            'one marker' => array(
+                'dummy content with ###REPLACED### marker',
+                array(
+                    '###REPLACED###' => '_replaced_'
+                ),
+                array(),
+                array(),
+                'dummy content with _replaced_ marker'
+            ),
+            'one marker with lots of chars' => array(
+                'dummy content with ###RE.:##-=_()LACED### marker',
+                array(
+                    '###RE.:##-=_()LACED###' => '_replaced_'
+                ),
+                array(),
+                array(),
+                'dummy content with _replaced_ marker'
+            ),
+            'markers which are special' => array(
+                'dummy ###aa##.#######A### ######',
+                array(
+                    '###aa##.###' => 'content ',
+                    '###A###' => 'is',
+                    '######' => '-is not considered-'
+                ),
+                array(),
+                array(),
+                'dummy content #is ######'
+            ),
+            'two markers in content, but more defined' => array(
+                'dummy ###CONTENT### with ###REPLACED### marker',
+                array(
+                    '###REPLACED###' => '_replaced_',
+                    '###CONTENT###' => 'content',
+                    '###NEVERUSED###' => 'bar'
+                ),
+                array(),
+                array(),
+                'dummy content with _replaced_ marker'
+            ),
+            'one subpart' => array(
+                'dummy content with ###ASUBPART### around some text###ASUBPART###.',
+                array(),
+                array(
+                    '###ASUBPART###' => 'some other text'
+                ),
+                array(),
+                'dummy content with some other text.'
+            ),
+            'one wrapped subpart' => array(
+                'dummy content with ###AWRAPPEDSUBPART### around some text###AWRAPPEDSUBPART###.',
+                array(),
+                array(),
+                array(
+                    '###AWRAPPEDSUBPART###' => array(
+                        'more content',
+                        'content'
+                    )
+                ),
+                'dummy content with more content around some textcontent.'
+            ),
+            'one subpart with markers, not replaced recursively' => array(
+                'dummy ###CONTENT### with ###ASUBPART### around ###SOME### text###ASUBPART###.',
+                array(
+                    '###CONTENT###' => 'content',
+                    '###SOME###' => '-this should never make it into output-',
+                    '###OTHER_NOT_REPLACED###' => '-this should never make it into output-'
+                ),
+                array(
+                    '###ASUBPART###' => 'some ###OTHER_NOT_REPLACED### text'
+                ),
+                array(),
+                'dummy content with some ###OTHER_NOT_REPLACED### text.'
+            ),
+        );
+    }
+
+    /**
+     * @test
+     * @dataProvider substituteMarkerArrayCachedReturnsExpectedContentDataProvider
+     *
+     * @param string $content
+     * @param array $markContentArray
+     * @param array $subpartContentArray
+     * @param array $wrappedSubpartContentArray
+     * @param string $expectedContent
+     * @param bool $shouldQueryCache
+     * @param bool $shouldStoreCache
+     */
+    public function substituteMarkerArrayCachedReturnsExpectedContent($content, array $markContentArray, array $subpartContentArray, array $wrappedSubpartContentArray, $expectedContent, $shouldQueryCache = true, $shouldStoreCache = true) {
+        /** @var PageRepositoryFixture|\PHPUnit_Framework_MockObject_MockObject $pageRepo */
+        $pageRepo = $this->typoScriptFrontendControllerMock->sys_page;
+        $pageRepo->resetCallCount();
+
+        $resultContent = $this->subject->substituteMarkerArrayCached($content, $markContentArray, $subpartContentArray, $wrappedSubpartContentArray);
+
+        $this->assertSame((int)$shouldQueryCache, $pageRepo::$getHashCallCount, 'getHash call count mismatch');
+        $this->assertSame((int)$shouldStoreCache, $pageRepo::$storeHashCallCount, 'storeHash call count mismatch');
+        $this->assertSame($expectedContent, $resultContent);
+    }
+
+    /**
+     * @test
+     */
+    public function substituteMarkerArrayCachedRetrievesCachedValueFromRuntimeCache() {
+        /** @var PageRepositoryFixture|\PHPUnit_Framework_MockObject_MockObject $pageRepo */
+        $pageRepo = $this->typoScriptFrontendControllerMock->sys_page;
+        $pageRepo->resetCallCount();
+
+        $content = 'Please tell me this ###FOO###.';
+        $markContentArray = array(
+            '###FOO###' => 'foo',
+            '###NOTUSED###' => 'blub'
+        );
+        $storeKey = md5('substituteMarkerArrayCached_storeKey:' . serialize(array($content, array_keys($markContentArray))));
+        $this->subject->substMarkerCache[$storeKey] = array(
+            'c' => array(
+                'Please tell me this ',
+                '.'
+            ),
+            'k' => array(
+                '###FOO###'
+            ),
+        );
+        $resultContent = $this->subject->substituteMarkerArrayCached($content, $markContentArray);
+        $this->assertSame(0, $pageRepo::$getHashCallCount);
+        $this->assertSame('Please tell me this foo.', $resultContent);
+    }
+
+    /**
+     * @test
+     */
+    public function substituteMarkerArrayCachedRetrievesCachedValueFromDbCache() {
+        /** @var PageRepositoryFixture|\PHPUnit_Framework_MockObject_MockObject $pageRepo */
+        $pageRepo = $this->typoScriptFrontendControllerMock->sys_page;
+        $pageRepo->resetCallCount();
+
+        $content = 'Please tell me this ###FOO###.';
+        $markContentArray = array(
+            '###FOO###' => 'foo',
+            '###NOTUSED###' => 'blub'
+        );
+        $pageRepo::$dbCacheContent = array(
+            'c' => array(
+                'Please tell me this ',
+                '.'
+            ),
+            'k' => array(
+                '###FOO###'
+            ),
+        );
+        $resultContent = $this->subject->substituteMarkerArrayCached($content, $markContentArray);
+        $this->assertSame(1, $pageRepo::$getHashCallCount, 'getHash call count mismatch');
+        $this->assertSame(0, $pageRepo::$storeHashCallCount, 'storeHash call count mismatch');
+        $this->assertSame('Please tell me this foo.', $resultContent);
+    }
+
+    /**
+     * @test
+     */
+    public function substituteMarkerArrayCachedStoresResultInCaches() {
+        /** @var PageRepositoryFixture|\PHPUnit_Framework_MockObject_MockObject $pageRepo */
+        $pageRepo = $this->typoScriptFrontendControllerMock->sys_page;
+        $pageRepo->resetCallCount();
+
+        $content = 'Please tell me this ###FOO###.';
+        $markContentArray = array(
+            '###FOO###' => 'foo',
+            '###NOTUSED###' => 'blub'
+        );
+        $resultContent = $this->subject->substituteMarkerArrayCached($content, $markContentArray);
+
+        $storeKey = md5('substituteMarkerArrayCached_storeKey:' . serialize(array($content, array_keys($markContentArray))));
+        $storeArr = array(
+            'c' => array(
+                'Please tell me this ',
+                '.'
+            ),
+            'k' => array(
+                '###FOO###'
+            ),
+        );
+        $this->assertSame(1, $pageRepo::$getHashCallCount);
+        $this->assertSame('Please tell me this foo.', $resultContent);
+        $this->assertSame($storeArr, $this->subject->substMarkerCache[$storeKey]);
+        $this->assertSame(1, $pageRepo::$storeHashCallCount);
+    }
+
+    /**
      * @return \TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController
      */
     protected function getFrontendController() {
         return $GLOBALS['TSFE'];
     }
-
 }
diff --git a/typo3/sysext/frontend/Tests/Unit/ContentObject/Fixtures/PageRepositoryFixture.php b/typo3/sysext/frontend/Tests/Unit/ContentObject/Fixtures/PageRepositoryFixture.php
new file mode 100644 (file)
index 0000000..d51370e
--- /dev/null
@@ -0,0 +1,46 @@
+<?php
+namespace TYPO3\CMS\Frontend\Tests\Unit\ContentObject\Fixtures;
+
+/*
+ * 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\Frontend\Page\PageRepository;
+
+/**
+ * Fixture for TYPO3\CMS\Core\Utility\GeneralUtility
+ */
+class PageRepositoryFixture extends PageRepository
+{
+
+    public static $getHashCallCount = 0;
+    public static $storeHashCallCount = 0;
+    public static $dbCacheContent = null;
+
+    public static function getHash($hash)
+    {
+        static::$getHashCallCount++;
+        return static::$dbCacheContent;
+    }
+
+    public static function storeHash($hash, $data, $ident, $lifetime = 0)
+    {
+        static::$storeHashCallCount++;
+    }
+
+    public static function resetCallCount()
+    {
+        static::$getHashCallCount = 0;
+        static::$storeHashCallCount = 0;
+        static::$dbCacheContent = null;
+    }
+}