[BUGFIX] Remove runtime cache and early return from TemplatePaths 32/53932/2
authorClaus Due <claus@namelesscoder.net>
Tue, 5 Sep 2017 15:10:18 +0000 (17:10 +0200)
committerAndreas Fernandez <typo3@scripting-base.de>
Thu, 7 Sep 2017 05:04:57 +0000 (07:04 +0200)
This patch removes the previously introduced runtime cache
and early returns from TemplatePaths, both of which were
implemented in an attempt to prevent excessive TypoScript
parsing - an issue which has since been solved by optimising
the TypoScript parsing enough that a cache and early return
is no longer necessary (no longer constitutes a significant
performance increase).

The early return and caching introduced regressions described
in the related forge issues. Removing both solves those problems.

In addition, the method resolving TypoScript paths is now
covered by extensive unit tests confirming everything from
merging to sorting of template paths. An average of 8 tests
cover the method's lines. Each of the expected behaviors
is now declared as specific test.

Change-Id: Ia6d505dcec7d77ad7aaeea9094d7d85a58553c63
Resolves: #82196
Resolves: #82181
Related: #79662
Releases: master, 8.7
Reviewed-on: https://review.typo3.org/53932
Reviewed-by: Susanne Moog <susanne.moog@typo3.org>
Tested-by: Susanne Moog <susanne.moog@typo3.org>
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Andreas Fernandez <typo3@scripting-base.de>
Tested-by: Andreas Fernandez <typo3@scripting-base.de>
typo3/sysext/fluid/Classes/View/TemplatePaths.php
typo3/sysext/fluid/Tests/Unit/View/TemplatePathsTest.php

index 1a61b56..7bba39f 100644 (file)
@@ -14,8 +14,6 @@ namespace TYPO3\CMS\Fluid\View;
  * The TYPO3 project - inspiring people to share!
  */
 
-use TYPO3\CMS\Core\Cache\CacheManager;
-use TYPO3\CMS\Core\Cache\Frontend\VariableFrontend;
 use TYPO3\CMS\Core\Utility\ArrayUtility;
 use TYPO3\CMS\Core\Utility\ExtensionManagementUtility;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
@@ -79,23 +77,8 @@ class TemplatePaths extends \TYPO3Fluid\Fluid\View\TemplatePaths
         if (empty($extensionKey)) {
             return [];
         }
-        $cache = $this->getRuntimeCache();
-        $cacheIdentifier = 'viewpaths_' . $extensionKey;
-        $configuration = $cache->get($cacheIdentifier);
-        if (is_array($configuration)) {
-            return $configuration;
-        }
 
         $configuredPaths = [];
-        if (!empty($this->templateRootPaths) || !empty($this->partialRootPaths) || !empty($this->layoutRootPaths)) {
-            // The view was manually configured - early return to avoid caching this configuration.
-            return [
-                self::CONFIG_TEMPLATEROOTPATHS => $this->templateRootPaths,
-                self::CONFIG_PARTIALROOTPATHS => $this->partialRootPaths,
-                self::CONFIG_LAYOUTROOTPATHS => $this->layoutRootPaths,
-            ];
-        }
-
         $resources = $this->getExtensionPrivateResourcesPath($extensionKey);
         $paths = [
             self::CONFIG_TEMPLATEROOTPATHS => [$resources . 'Templates/'],
@@ -121,8 +104,6 @@ class TemplatePaths extends \TYPO3Fluid\Fluid\View\TemplatePaths
             }
         }
 
-        $cache->set($cacheIdentifier, $paths);
-
         return $paths;
     }
 
@@ -231,12 +212,4 @@ class TemplatePaths extends \TYPO3Fluid\Fluid\View\TemplatePaths
     {
         return TYPO3_MODE === 'FE';
     }
-
-    /**
-     * @return VariableFrontend
-     */
-    protected function getRuntimeCache()
-    {
-        return GeneralUtility::makeInstance(CacheManager::class)->getCache('cache_runtime');
-    }
 }
index 58a7643..fa6518e 100644 (file)
@@ -14,7 +14,6 @@ namespace TYPO3\CMS\Fluid\Tests\Unit\View;
  * The TYPO3 project - inspiring people to share!
  */
 
-use TYPO3\CMS\Core\Cache\Frontend\VariableFrontend;
 use TYPO3\CMS\Extbase\Configuration\ConfigurationManagerInterface;
 use TYPO3\CMS\Fluid\View\TemplatePaths;
 
@@ -102,9 +101,312 @@ class TemplatePathsTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
     }
 
     /**
+     * Bulk test to confirm that configuration is returned correctly based on
+     * different TypoScript and FE/BE mode, and mixed sorting results in a
+     * correctly sorted set of template paths.
+     *
+     * @param bool $frontendMode
+     * @param array $typoScript
+     * @param array $expectedPaths
      * @test
+     * @dataProvider getContextSpecificViewConfigurationTestValues
      */
-    public function getContextSpecificViewConfigurationSortsTypoScriptConfiguredPathsCorrectlyInFrontendMode()
+    public function getContextSpecificViewConfigurationMergesAndSortsPaths($frontendMode, array $typoScript, array $expectedPaths)
+    {
+        $configurationManager = $this->getMockBuilder(ConfigurationManagerInterface::class)->getMockForAbstractClass();
+        $configurationManager->expects($this->once())->method('getConfiguration')->willReturn($typoScript);
+        $subject = $this->getMockBuilder(TemplatePaths::class)->setMethods(['getConfigurationManager', 'getExtensionPrivateResourcesPath', 'isBackendMode', 'isFrontendMode'])->getMock();
+        $subject->expects($this->once())->method('getExtensionPrivateResourcesPath')->with('test')->willReturn('test/');
+        $subject->expects($this->once())->method('getConfigurationManager')->willReturn($configurationManager);
+        $subject->expects($this->any())->method('isBackendMode')->willReturn(!$frontendMode);
+        $subject->expects($this->any())->method('isFrontendMode')->willReturn($frontendMode);
+        $result = $this->callInaccessibleMethod($subject, 'getContextSpecificViewConfiguration', 'test');
+        $this->assertSame($expectedPaths, $result);
+    }
+
+    /**
+     * Confirmation that if an empty extension key context is passed,
+     * TemplatePaths will be unable to return even fallback paths and
+     * will instead return an empty array and never call any methods
+     * to get TypoScript etc.
+     *
+     * @test
+     */
+    public function getContextSpecificViewConfigurationReturnsEmptyPathsForEmptyExtensionKey()
+    {
+        $subject = $this->getMockBuilder(TemplatePaths::class)->setMethods(['getConfigurationManager'])->getMock();
+        $subject->expects($this->never())->method('getConfigurationManager');
+        $this->callInaccessibleMethod($subject, 'getContextSpecificViewConfiguration', '');
+    }
+
+    /**
+     * @return array
+     */
+    public function getContextSpecificViewConfigurationTestValues()
+    {
+        return [
+            'complete TypoScript configuration with mixed-sorting paths get sorted correctly in backend mode' => [
+                false,
+                [
+                    'module.' => [
+                        'tx_test.' => [
+                            'view.' => [
+                                'templateRootPaths.' => [
+                                    '30' => 'third',
+                                    '10' => 'first',
+                                    '20' => 'second'
+                                ],
+                                'partialRootPaths.' => [
+                                    '20' => '2',
+                                    '30' => '3',
+                                    '10' => '1'
+                                ],
+                                'layoutRootPaths.' => [
+                                    '130' => '3.',
+                                    '10' => '1.',
+                                    '120' => '2.'
+                                ],
+                            ]
+                        ]
+                    ]
+                ],
+                [
+                    'templateRootPaths' => [
+                        'test/Templates/',
+                        'first',
+                        'second',
+                        'third'
+                    ],
+                    'partialRootPaths' => [
+                        'test/Partials/',
+                        '1',
+                        '2',
+                        '3'
+                    ],
+                    'layoutRootPaths' => [
+                        'test/Layouts/',
+                        '1.',
+                        '2.',
+                        '3.'
+                    ]
+                ]
+            ],
+            'complete TypoScript configuration with mixed-sorting paths get sorted correctly in frontend mode' => [
+                true,
+                [
+                    'plugin.' => [
+                        'tx_test.' => [
+                            'view.' => [
+                                'templateRootPaths.' => [
+                                    '30' => 'third',
+                                    '10' => 'first',
+                                    '20' => 'second'
+                                ],
+                                'partialRootPaths.' => [
+                                    '20' => '2',
+                                    '30' => '3',
+                                    '10' => '1'
+                                ],
+                                'layoutRootPaths.' => [
+                                    '130' => '3.',
+                                    '10' => '1.',
+                                    '120' => '2.'
+                                ],
+                            ]
+                        ]
+                    ]
+                ],
+                [
+                    'templateRootPaths' => [
+                        'test/Templates/',
+                        'first',
+                        'second',
+                        'third'
+                    ],
+                    'partialRootPaths' => [
+                        'test/Partials/',
+                        '1',
+                        '2',
+                        '3'
+                    ],
+                    'layoutRootPaths' => [
+                        'test/Layouts/',
+                        '1.',
+                        '2.',
+                        '3.'
+                    ]
+                ]
+            ],
+            'partial TypoScript configuration merges fallback templateRootPaths' => [
+                true,
+                [
+                    'plugin.' => [
+                        'tx_test.' => [
+                            'view.' => [
+                                'partialRootPaths.' => [
+                                    '20' => '2',
+                                    '30' => '3',
+                                    '10' => '1'
+                                ],
+                                'layoutRootPaths.' => [
+                                    '130' => '3.',
+                                    '10' => '1.',
+                                    '120' => '2.'
+                                ],
+                            ]
+                        ]
+                    ]
+                ],
+                [
+                    'templateRootPaths' => [
+                        'test/Templates/'
+                    ],
+                    'partialRootPaths' => [
+                        'test/Partials/',
+                        '1',
+                        '2',
+                        '3'
+                    ],
+                    'layoutRootPaths' => [
+                        'test/Layouts/',
+                        '1.',
+                        '2.',
+                        '3.'
+                    ]
+                ]
+            ],
+            'partial TypoScript configuration merges fallback partialRootPaths' => [
+                true,
+                [
+                    'plugin.' => [
+                        'tx_test.' => [
+                            'view.' => [
+                                'templateRootPaths.' => [
+                                    '20' => '2',
+                                    '30' => '3',
+                                    '10' => '1'
+                                ],
+                                'layoutRootPaths.' => [
+                                    '130' => '3.',
+                                    '10' => '1.',
+                                    '120' => '2.'
+                                ],
+                            ]
+                        ]
+                    ]
+                ],
+                [
+                    'templateRootPaths' => [
+                        'test/Templates/',
+                        '1',
+                        '2',
+                        '3'
+                    ],
+                    'partialRootPaths' => [
+                        'test/Partials/'
+                    ],
+                    'layoutRootPaths' => [
+                        'test/Layouts/',
+                        '1.',
+                        '2.',
+                        '3.'
+                    ]
+                ]
+            ],
+            'partial TypoScript configuration merges fallback layoutRootPaths' => [
+                true,
+                [
+                    'plugin.' => [
+                        'tx_test.' => [
+                            'view.' => [
+                                'templateRootPaths.' => [
+                                    '20' => '2',
+                                    '30' => '3',
+                                    '10' => '1'
+                                ],
+                                'partialRootPaths.' => [
+                                    '130' => '3.',
+                                    '10' => '1.',
+                                    '120' => '2.'
+                                ],
+                            ]
+                        ]
+                    ]
+                ],
+                [
+                    'templateRootPaths' => [
+                        'test/Templates/',
+                        '1',
+                        '2',
+                        '3'
+                    ],
+                    'partialRootPaths' => [
+                        'test/Partials/',
+                        '1.',
+                        '2.',
+                        '3.'
+                    ],
+                    'layoutRootPaths' => [
+                        'test/Layouts/'
+                    ]
+                ]
+            ],
+            'partial TypoScript configuration merges fallback layoutRootPaths and partialRootPaths' => [
+                true,
+                [
+                    'plugin.' => [
+                        'tx_test.' => [
+                            'view.' => [
+                                'templateRootPaths.' => [
+                                    '20' => '2',
+                                    '30' => '3',
+                                    '10' => '1'
+                                ]
+                            ]
+                        ]
+                    ]
+                ],
+                [
+                    'templateRootPaths' => [
+                        'test/Templates/',
+                        '1',
+                        '2',
+                        '3'
+                    ],
+                    'partialRootPaths' => [
+                        'test/Partials/'
+                    ],
+                    'layoutRootPaths' => [
+                        'test/Layouts/'
+                    ]
+                ]
+            ],
+            'missing TypoScript configuration returns fallback path collection' => [
+                true,
+                [],
+                [
+                    'templateRootPaths' => [
+                        'test/Templates/'
+                    ],
+                    'partialRootPaths' => [
+                        'test/Partials/'
+                    ],
+                    'layoutRootPaths' => [
+                        'test/Layouts/'
+                    ]
+                ]
+            ],
+        ];
+    }
+
+    /**
+     * Test to confirm that regardless of TypoScript configuration, an unknown FE/BE mode
+     * results in purely fallback template paths being returned (since not knowing the
+     * mode means inability to resolve correct TS TLO plugin./module.)
+     *
+     * @test
+     */
+    public function getContextSpecificViewConfigurationDoesNotResolveFromTypoScriptAndDoesNotSortInUnspecifiedMode()
     {
         $configurationManager = $this->getMockBuilder(ConfigurationManagerInterface::class)->getMockForAbstractClass();
         $configurationManager->expects($this->once())->method('getConfiguration')->willReturn([
@@ -130,47 +432,40 @@ class TemplatePathsTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
                 ]
             ]
         ]);
-        $cache = $this->getMockBuilder(VariableFrontend::class)->setMethods(['get', 'set'])->disableOriginalConstructor()->getMock();
-        $cache->expects($this->once())->method('get')->willReturn(false);
-        $cache->expects($this->once())->method('set');
-        $subject = $this->getMockBuilder(TemplatePaths::class)->setMethods(['getConfigurationManager', 'getExtensionPrivateResourcesPath', 'getRuntimeCache', 'isBackendMode', 'isFrontendMode'])->getMock();
+        $subject = $this->getMockBuilder(TemplatePaths::class)->setMethods(['getConfigurationManager', 'getExtensionPrivateResourcesPath', 'isBackendMode', 'isFrontendMode'])->getMock();
         $subject->expects($this->once())->method('getExtensionPrivateResourcesPath')->with('test')->willReturn('test/');
         $subject->expects($this->once())->method('getConfigurationManager')->willReturn($configurationManager);
-        $subject->expects($this->once())->method('getRuntimeCache')->willReturn($cache);
         $subject->expects($this->once())->method('isBackendMode')->willReturn(false);
-        $subject->expects($this->once())->method('isFrontendMode')->willReturn(true);
+        $subject->expects($this->once())->method('isFrontendMode')->willReturn(false);
         $result = $this->callInaccessibleMethod($subject, 'getContextSpecificViewConfiguration', 'test');
         $this->assertSame([
             'templateRootPaths' => [
-                'test/Templates/',
-                'first',
-                'second',
-                'third'
+                'test/Templates/'
             ],
             'partialRootPaths' => [
-                'test/Partials/',
-                '1',
-                '2',
-                '3'
+                'test/Partials/'
             ],
             'layoutRootPaths' => [
-                'test/Layouts/',
-                '1.',
-                '2.',
-                '3.'
+                'test/Layouts/'
             ]
         ], $result);
     }
 
     /**
+     * Test to confirm that regardless which setter methods for path collections
+     * were called, the getContextSpecificViewConfiguration() method always returns
+     * the paths that were configured in TypoScript and does not change any of the
+     * paths set via the setter methods.
+     *
+     * @dataProvider getContextSpecificViewConfigurationIgnoresValues
      * @test
      */
-    public function getContextSpecificViewConfigurationSortsTypoScriptConfiguredPathsCorrectlyInBackendMode()
+    public function getContextSpecificViewConfigurationIgnoresValuesSetInPathSetterMethods($frontendMode, array $paths)
     {
         $configurationManager = $this->getMockBuilder(ConfigurationManagerInterface::class)->getMockForAbstractClass();
         $configurationManager->expects($this->once())->method('getConfiguration')->willReturn([
-            'module.' => [
-                'tx_test2.' => [
+            'plugin.' => [
+                'tx_test.' => [
                     'view.' => [
                         'templateRootPaths.' => [
                             '30' => 'third',
@@ -191,16 +486,20 @@ class TemplatePathsTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
                 ]
             ]
         ]);
-        $cache = $this->getMockBuilder(VariableFrontend::class)->setMethods(['get', 'set'])->disableOriginalConstructor()->getMock();
-        $cache->expects($this->once())->method('get')->willReturn(false);
-        $cache->expects($this->once())->method('set');
-        $subject = $this->getMockBuilder(TemplatePaths::class)->setMethods(['getConfigurationManager', 'getExtensionPrivateResourcesPath', 'getRuntimeCache', 'isBackendMode', 'isFrontendMode'])->getMock();
-        $subject->expects($this->once())->method('getExtensionPrivateResourcesPath')->with('test2')->willReturn('test/');
+        $subject = $this->getMockBuilder(TemplatePaths::class)->setMethods(['getConfigurationManager', 'getExtensionPrivateResourcesPath', 'isBackendMode', 'isFrontendMode', 'sanitizePath'])->getMock();
+        $subject->expects($this->once())->method('getExtensionPrivateResourcesPath')->with('test')->willReturn('test/');
         $subject->expects($this->once())->method('getConfigurationManager')->willReturn($configurationManager);
-        $subject->expects($this->once())->method('getRuntimeCache')->willReturn($cache);
-        $subject->expects($this->once())->method('isBackendMode')->willReturn(true);
-        $subject->expects($this->never())->method('isFrontendMode');
-        $result = $this->callInaccessibleMethod($subject, 'getContextSpecificViewConfiguration', 'test2');
+        $subject->expects($this->once())->method('isBackendMode')->willReturn(!$frontendMode);
+        $subject->expects($this->once())->method('isFrontendMode')->willReturn($frontendMode);
+
+        // Set paths and expect sanitizePath() to be called, emulate return from sanitizePath()
+        $subject->expects($this->atLeastOnce())->method('sanitizePath')->willReturnArgument(0);
+        foreach ($paths as $key => $pathSet) {
+            $setter = 'set' . ucfirst($key);
+            $subject->$setter($pathSet);
+        }
+
+        $result = $this->callInaccessibleMethod($subject, 'getContextSpecificViewConfiguration', 'test');
         $this->assertSame([
             'templateRootPaths' => [
                 'test/Templates/',
@@ -221,57 +520,44 @@ class TemplatePathsTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
                 '3.'
             ]
         ], $result);
+        foreach ($paths as $key => $pathSet) {
+            $this->assertAttributeSame($pathSet, $key, $subject);
+        }
     }
 
     /**
-     * @test
+     * @return array
      */
-    public function getContextSpecificViewConfigurationDoesNotResolveFromTypoScriptAndDoesNotSortInUnspecifiedMode()
+    public function getContextSpecificViewConfigurationIgnoresValues()
     {
-        $configurationManager = $this->getMockBuilder(ConfigurationManagerInterface::class)->getMockForAbstractClass();
-        $configurationManager->expects($this->once())->method('getConfiguration')->willReturn([
-            'plugin.' => [
-                'tx_test.' => [
-                    'view.' => [
-                        'templateRootPaths.' => [
-                            '30' => 'third',
-                            '10' => 'first',
-                            '20' => 'second'
-                        ],
-                        'partialRootPaths.' => [
-                            '20' => '2',
-                            '30' => '3',
-                            '10' => '1'
-                        ],
-                        'layoutRootPaths.' => [
-                            '130' => '3.',
-                            '10' => '1.',
-                            '120' => '2.'
-                        ],
+        return [
+            'has templateRootPaths set with setter method' => [
+                true,
+                [
+                    'templateRootPaths' => [
+                        '1.',
+                        '2.'
                     ]
                 ]
-            ]
-        ]);
-        $cache = $this->getMockBuilder(VariableFrontend::class)->setMethods(['get', 'set'])->disableOriginalConstructor()->getMock();
-        $cache->expects($this->once())->method('get')->willReturn(false);
-        $cache->expects($this->once())->method('set');
-        $subject = $this->getMockBuilder(TemplatePaths::class)->setMethods(['getConfigurationManager', 'getExtensionPrivateResourcesPath', 'getRuntimeCache', 'isBackendMode', 'isFrontendMode'])->getMock();
-        $subject->expects($this->once())->method('getExtensionPrivateResourcesPath')->with('test')->willReturn('test/');
-        $subject->expects($this->once())->method('getConfigurationManager')->willReturn($configurationManager);
-        $subject->expects($this->once())->method('getRuntimeCache')->willReturn($cache);
-        $subject->expects($this->once())->method('isBackendMode')->willReturn(false);
-        $subject->expects($this->once())->method('isFrontendMode')->willReturn(false);
-        $result = $this->callInaccessibleMethod($subject, 'getContextSpecificViewConfiguration', 'test');
-        $this->assertSame([
-            'templateRootPaths' => [
-                'test/Templates/'
             ],
-            'partialRootPaths' => [
-                'test/Partials/'
+            'has partialRootPaths set with setter method' => [
+                true,
+                [
+                    'partialRootPaths' => [
+                        '1.',
+                        '2.'
+                    ]
+                ]
             ],
-            'layoutRootPaths' => [
-                'test/Layouts/'
-            ]
-        ], $result);
+            'has layoutRootPaths set with setter method' => [
+                true,
+                [
+                    'layoutRootPaths' => [
+                        '1.',
+                        '2.'
+                    ]
+                ]
+            ],
+        ];
     }
 }