[TASK] Improve processing of mappable aspects 83/58483/3
authorOliver Hader <oliver@typo3.org>
Sun, 30 Sep 2018 08:40:21 +0000 (10:40 +0200)
committerFrank Naegler <frank.naegler@typo3.org>
Sun, 30 Sep 2018 16:09:32 +0000 (18:09 +0200)
Improve processing by deferring those mappers that invoke persistence
the latest possible time. In case mappers do not match the processing
is stopped earlier to avoid superfluous checks and invocations.

Resolves: #86464
Releases: master
Change-Id: I4f9ec66611e84b49a54223f77aee824033a2fb7a
Reviewed-on: https://review.typo3.org/58483
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Benni Mack <benni@typo3.org>
Tested-by: Benni Mack <benni@typo3.org>
Reviewed-by: Frank Naegler <frank.naegler@typo3.org>
Tested-by: Frank Naegler <frank.naegler@typo3.org>
typo3/sysext/core/Classes/Routing/Aspect/AspectFactory.php
typo3/sysext/core/Classes/Routing/Aspect/MappableProcessor.php
typo3/sysext/core/Classes/Routing/Aspect/PersistedAliasMapper.php
typo3/sysext/core/Classes/Routing/Aspect/PersistedMappableAspectInterface.php [new file with mode: 0644]
typo3/sysext/core/Classes/Routing/Aspect/PersistedPatternMapper.php
typo3/sysext/core/Tests/Unit/Routing/Aspect/AspectFactoryTest.php [new file with mode: 0644]

index 6ad5662..0c9c58a 100644 (file)
@@ -47,13 +47,15 @@ class AspectFactory
      */
     public function createAspects(array $aspects, SiteLanguage $language): array
     {
-        return array_map(
+        $aspects = array_map(
             function ($settings) use ($language) {
                 $type = (string)($settings['type'] ?? '');
                 return $this->create($type, $settings, $language);
             },
             $aspects
         );
+        uasort($aspects, [$this, 'sortAspects']);
+        return $aspects;
     }
 
     /**
@@ -102,4 +104,25 @@ class AspectFactory
         }
         return $aspect;
     }
+
+    /**
+     * Sorts aspects with putting persisted aspects to the end, thus
+     * non-persisted aspects can be executed earlier without invoking database.
+     *
+     * @param AspectInterface $first
+     * @param AspectInterface $second
+     * @return int
+     */
+    protected function sortAspects(AspectInterface $first, AspectInterface $second): int
+    {
+        // when first is persisted, move it to the end (>0)
+        $first = $first instanceof PersistedMappableAspectInterface ? 1 : 0;
+        // when second is persisted, move it to the beginning (<0)
+        $second = $second instanceof PersistedMappableAspectInterface ? -1 : 0;
+        // 0 + 0 =  0 - both are non-persisted
+        // 1 - 1 =  0 - both are persisted
+        // 1 + 0 =  1 - only first is persisted
+        // 0 - 1 = -1 - only second is persisted
+        return $first + $second;
+    }
 }
index e1202ad..fb2dd92 100644 (file)
@@ -40,13 +40,10 @@ class MappableProcessor
             $value = $mapper->resolve(
                 (string)($attributes[$variableName] ?? '')
             );
-            if ($value !== null) {
-                $values[$variableName] = $value;
+            if ($value === null) {
+                return false;
             }
-        }
-
-        if (count($mappers) !== count($values)) {
-            return false;
+            $values[$variableName] = $value;
         }
 
         $attributes = array_merge($attributes, $values);
@@ -70,13 +67,10 @@ class MappableProcessor
             $value = $mapper->generate(
                 (string)($attributes[$variableName] ?? '')
             );
-            if ($value !== null) {
-                $values[$variableName] = $value;
+            if ($value === null) {
+                return false;
             }
-        }
-
-        if (count($mappers) !== count($values)) {
-            return false;
+            $values[$variableName] = $value;
         }
 
         $attributes = array_merge($attributes, $values);
index a6e505c..9f07560 100644 (file)
@@ -43,7 +43,7 @@ use TYPO3\CMS\Frontend\Page\PageRepository;
  *           routeFieldName: 'path_segment'
  *           routeValuePrefix: '/'
  */
-class PersistedAliasMapper implements StaticMappableAspectInterface
+class PersistedAliasMapper implements PersistedMappableAspectInterface, StaticMappableAspectInterface
 {
     use SiteLanguageAwareTrait;
 
diff --git a/typo3/sysext/core/Classes/Routing/Aspect/PersistedMappableAspectInterface.php b/typo3/sysext/core/Classes/Routing/Aspect/PersistedMappableAspectInterface.php
new file mode 100644 (file)
index 0000000..e618666
--- /dev/null
@@ -0,0 +1,25 @@
+<?php
+declare(strict_types = 1);
+
+namespace TYPO3\CMS\Core\Routing\Aspect;
+
+/*
+ * 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!
+ */
+
+/**
+ * Used for anything that invokes (more expensive) persistence invocations.
+ * Basically used to improve performance by deferring their execution.
+ */
+interface PersistedMappableAspectInterface extends MappableAspectInterface
+{
+}
index dd1fab3..d82e36b 100644 (file)
@@ -46,7 +46,7 @@ use TYPO3\CMS\Frontend\Page\PageRepository;
  *
  * @internal might change its options in the future, be aware that there might be modifications.
  */
-class PersistedPatternMapper implements StaticMappableAspectInterface
+class PersistedPatternMapper implements PersistedMappableAspectInterface, StaticMappableAspectInterface
 {
     use SiteLanguageAwareTrait;
 
diff --git a/typo3/sysext/core/Tests/Unit/Routing/Aspect/AspectFactoryTest.php b/typo3/sysext/core/Tests/Unit/Routing/Aspect/AspectFactoryTest.php
new file mode 100644 (file)
index 0000000..d19e62d
--- /dev/null
@@ -0,0 +1,180 @@
+<?php
+declare(strict_types = 1);
+
+namespace TYPO3\CMS\Core\Tests\Unit\Routing\Enhancer;
+
+/*
+ * 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 Prophecy\Prophecy\ObjectProphecy;
+use TYPO3\CMS\Core\Routing\Aspect\AspectFactory;
+use TYPO3\CMS\Core\Routing\Aspect\AspectInterface;
+use TYPO3\CMS\Core\Routing\Aspect\PersistedMappableAspectInterface;
+use TYPO3\CMS\Core\Site\Entity\SiteLanguage;
+use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
+
+class AspectFactoryTest extends UnitTestCase
+{
+    /**
+     * @var AspectFactory
+     */
+    protected $subject;
+
+    /**
+     * @var SiteLanguage|ObjectProphecy
+     */
+    protected $languageProphecy;
+
+    /**
+     * @var string
+     */
+    protected $persistedMockClass;
+
+    /**
+     * @var string
+     */
+    protected $aspectMockClass;
+
+    protected function setUp()
+    {
+        parent::setUp();
+        $this->languageProphecy = $this->prophesize(
+            SiteLanguage::class
+        );
+        $this->persistedMockClass = $this->getMockClass(
+            PersistedMappableAspectInterface::class
+        );
+        $this->aspectMockClass = $this->getMockClass(
+            AspectInterface::class
+        );
+        $GLOBALS['TYPO3_CONF_VARS']['SYS']['routing']['aspects'] = [
+            'Persisted' => $this->persistedMockClass,
+            'Aspect' => $this->aspectMockClass,
+        ];
+        $this->subject = new AspectFactory();
+    }
+
+    protected function tearDown()
+    {
+        unset($this->subject, $this->languageProphecy);
+        parent::tearDown();
+    }
+
+    /**
+     * @test
+     */
+    public function createAspectsThrowsExceptionOnMissingType()
+    {
+        $this->expectException(\InvalidArgumentException::class);
+        $this->expectExceptionCode(1538079481);
+        $this->subject->createAspects(
+            ['a' => []],
+            $this->languageProphecy->reveal()
+        );
+    }
+
+    /**
+     * @test
+     */
+    public function createAspectsThrowsExceptionOnUnregisteredType()
+    {
+        $this->expectException(\OutOfRangeException::class);
+        $this->expectExceptionCode(1538079482);
+        $this->subject->createAspects(
+            ['a' => ['type' => 'Undefined']],
+            $this->languageProphecy->reveal()
+        );
+    }
+
+    /**
+     * @return array
+     */
+    public function aspectsDataProvider(): array
+    {
+        return [
+            'single aspect' => [
+                [
+                    'a' => ['type' => 'Aspect'],
+                ],
+                [
+                    'a' => AspectInterface::class,
+                ],
+            ],
+            'both non-persisted' => [
+                [
+                    'a' => ['type' => 'Aspect'],
+                    'b' => ['type' => 'Aspect'],
+                ],
+                [
+                    'a' => AspectInterface::class,
+                    'b' => AspectInterface::class,
+                ],
+            ],
+            'both persisted' => [
+                [
+                    'a' => ['type' => 'Persisted'],
+                    'b' => ['type' => 'Persisted'],
+                ],
+                [
+                    'a' => PersistedMappableAspectInterface::class,
+                    'b' => PersistedMappableAspectInterface::class,
+                ],
+            ],
+            // persisted shall be sorted to the end
+            'first persisted, second non-persisted' => [
+                [
+                    'a' => ['type' => 'Persisted'],
+                    'b' => ['type' => 'Aspect'],
+                ],
+                [
+                    'b' => AspectInterface::class,
+                    'a' => PersistedMappableAspectInterface::class,
+                ],
+            ],
+            // persisted shall be sorted to the end
+            'many persisted, many non-persisted' => [
+                [
+                    'a' => ['type' => 'Persisted'],
+                    'b' => ['type' => 'Aspect'],
+                    'c' => ['type' => 'Persisted'],
+                    'd' => ['type' => 'Aspect'],
+                ],
+                [
+                    'b' => AspectInterface::class,
+                    'd' => AspectInterface::class,
+                    'a' => PersistedMappableAspectInterface::class,
+                    'c' => PersistedMappableAspectInterface::class,
+                ],
+            ],
+        ];
+    }
+
+    /**
+     * @param array $settings
+     * @param string[] $expectation
+     *
+     * @test
+     * @dataProvider aspectsDataProvider
+     */
+    public function aspectsAreCreatedAndSorted(array $settings, array $expectation)
+    {
+        $aspects = $this->subject->createAspects(
+            $settings,
+            $this->languageProphecy->reveal()
+        );
+        static::assertSame(array_keys($aspects), array_keys($expectation));
+        array_walk($aspects, function ($aspect, $key) use ($expectation) {
+            static::assertInstanceOf($expectation[$key], $aspect);
+        });
+    }
+}