[TASK] Resolve Todos in InheritancesResolverService 48/54948/4
authorrschueppel <rs@davitec.de>
Wed, 6 Dec 2017 16:44:00 +0000 (17:44 +0100)
committerChristian Kuhn <lolli@schwarzbu.ch>
Thu, 8 Feb 2018 22:56:04 +0000 (23:56 +0100)
Add some comments to understand InheritanceResolverService class.
Minor refactoring of duplicated code.
Add Test for the doc example.

Resolves: #83238
Releases: master, 8.7
Change-Id: I3aed06a76b609ef78641530a19e3379f202c84ec
Reviewed-on: https://review.typo3.org/54948
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Reviewed-by: Mathias Schreiber <mathias.schreiber@typo3.com>
Tested-by: Mathias Schreiber <mathias.schreiber@typo3.com>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
typo3/sysext/form/Classes/Mvc/Configuration/InheritancesResolverService.php
typo3/sysext/form/Tests/Unit/Mvc/Configuration/InheritancesResolverServiceTest.php

index 0ab7993..c561afe 100644 (file)
@@ -16,7 +16,6 @@ namespace TYPO3\CMS\Form\Mvc\Configuration;
  */
 
 use TYPO3\CMS\Core\Utility\ArrayUtility;
-use TYPO3\CMS\Core\Utility\Exception\MissingArrayPathException;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Extbase\Object\ObjectManager;
 use TYPO3\CMS\Form\Mvc\Configuration\Exception\CycleInheritancesException;
@@ -24,6 +23,38 @@ use TYPO3\CMS\Form\Mvc\Configuration\Exception\CycleInheritancesException;
 /**
  * Resolve declared inheritances within an configuration array
  *
+ * Basic concept:
+ * - Take a large YAML config and replace the key '__inheritance' by the referenced YAML partial (of the same config file)
+ * - Maybe also override some keys of the referenced partial
+ * - Avoid endless loop by reference cycles
+ *
+ * e.g.
+ * ---------------------
+ *
+ * Form:
+ *  part1:
+ *    key1: value1
+ *    key2: value2
+ *    key3: value3
+ *  part2:
+ *    __inheritance:
+ *      10: Form.part1
+ *    key2: another_value
+ *
+ * will result in:
+ * ---------------------
+ *
+ * Form:
+ *  part1:
+ *    key1: value1
+ *    key2: value2
+ *    key3: value3
+ *  part2:
+ *    key1: value1
+ *    key2: another_value
+ *    key3: value3
+ *
+ * ---------------------
  * Scope: frontend / backend
  * @internal
  */
@@ -52,7 +83,7 @@ class InheritancesResolverService
     protected $inheritanceStack = [];
 
     /**
-     * Needed to park a configuration path for cyclically inheritances
+     * Needed to buffer a configuration path for cyclically inheritances
      * detection while inheritances for this path is ongoing.
      *
      * @var string
@@ -122,9 +153,14 @@ class InheritancesResolverService
     /**
      * Resolve all inheritances within a configuration.
      *
-     * @todo: More description
-     * @param array $configuration
-     * @param array $pathStack
+     * Takes a YAML config mapped to associative array $configuration
+     * - replace all findings of key '__inheritance' recursively
+     * - perform a deep search in config by iteration, thus check for endless loop by reference cycle
+     *
+     * Return the completed configuration.
+     *
+     * @param array $configuration - a mapped YAML configuration (full or partial)
+     * @param array $pathStack - an identifier for YAML key as array (Form.part1.key => {Form, part1, key})
      * @param bool $setInheritancePathToCkeck
      * @return array
      */
@@ -134,43 +170,36 @@ class InheritancesResolverService
         bool $setInheritancePathToCkeck = true
     ): array {
         foreach ($configuration as $key => $values) {
+            //add current key to pathStack
             $pathStack[] = $key;
             $path = implode('.', $pathStack);
 
+            //check endless loop for current path
             $this->throwExceptionIfCycleInheritances($path, $path);
+
+            //overwrite service property 'inheritancePathToCheck' with current path
             if ($setInheritancePathToCkeck) {
                 $this->inheritancePathToCkeck = $path;
             }
 
+            //if value of subnode is an array, perform a deep search iteration step
             if (is_array($configuration[$key])) {
                 if (isset($configuration[$key][self::INHERITANCE_OPERATOR])) {
-                    try {
-                        $inheritances = ArrayUtility::getValueByPath(
-                            $this->referenceConfiguration,
-                            $path . '.' . self::INHERITANCE_OPERATOR,
-                            '.'
-                        );
-                    } catch (MissingArrayPathException $exception) {
-                        $inheritances = null;
-                    }
+                    $inheritances = $this->getValueByPath($this->referenceConfiguration, $path . '.' . self::INHERITANCE_OPERATOR);
 
+                    //and replace the __inheritance operator by the respective partial
                     if (is_array($inheritances)) {
                         $inheritedConfigurations = $this->resolveInheritancesRecursive($inheritances);
-
-                        $configuration[$key] = array_replace_recursive(
-                            $inheritedConfigurations,
-                            $configuration[$key]
-                        );
+                        $configuration[$key] = array_replace_recursive($inheritedConfigurations, $configuration[$key]);
                     }
 
+                    //remove the inheritance operator from configuration
                     unset($configuration[$key][self::INHERITANCE_OPERATOR]);
                 }
 
                 if (!empty($configuration[$key])) {
-                    $configuration[$key] = $this->resolve(
-                        $configuration[$key],
-                        $pathStack
-                    );
+                    // resolve subnode of YAML config
+                    $configuration[$key] = $this->resolve($configuration[$key], $pathStack);
                 }
             }
             array_pop($pathStack);
@@ -182,7 +211,8 @@ class InheritancesResolverService
     /**
      * Additional helper for the resolve method.
      *
-     * @todo: More description
+     * Takes all inheritances (an array of YAML paths), and check them for endless loops
+     *
      * @param array $inheritances
      * @return array
      * @throws CycleInheritancesException
@@ -193,15 +223,7 @@ class InheritancesResolverService
         $inheritedConfigurations = [];
         foreach ($inheritances as $inheritancePath) {
             $this->throwExceptionIfCycleInheritances($inheritancePath, $inheritancePath);
-            try {
-                $inheritedConfiguration = ArrayUtility::getValueByPath(
-                    $this->referenceConfiguration,
-                    $inheritancePath,
-                    '.'
-                );
-            } catch (MissingArrayPathException $exception) {
-                $inheritedConfiguration = null;
-            }
+            $inheritedConfiguration = $this->getValueByPath($this->referenceConfiguration, $inheritancePath);
 
             if (
                 isset($inheritedConfiguration[self::INHERITANCE_OPERATOR])
@@ -250,56 +272,23 @@ class InheritancesResolverService
     /**
      * Throw an exception if a cycle is detected.
      *
-     * @todo: More description
      * @param string $path
      * @param string $pathToCheck
      * @throws CycleInheritancesException
      */
     protected function throwExceptionIfCycleInheritances(string $path, string $pathToCheck)
     {
-        try {
-            $configuration = ArrayUtility::getValueByPath(
-                $this->referenceConfiguration,
-                $path,
-                '.'
-            );
-        } catch (MissingArrayPathException $exception) {
-            $configuration = null;
-        }
+        $configuration = $this->getValueByPath($this->referenceConfiguration, $path);
 
         if (isset($configuration[self::INHERITANCE_OPERATOR])) {
-            try {
-                $inheritances = ArrayUtility::getValueByPath(
-                    $this->referenceConfiguration,
-                    $path . '.' . self::INHERITANCE_OPERATOR,
-                    '.'
-                );
-            } catch (MissingArrayPathException $exception) {
-                $inheritances = null;
-            }
+            $inheritances = $this->getValueByPath($this->referenceConfiguration, $path . '.' . self::INHERITANCE_OPERATOR);
 
             if (is_array($inheritances)) {
                 foreach ($inheritances as $inheritancePath) {
-                    try {
-                        $configuration = ArrayUtility::getValueByPath(
-                            $this->referenceConfiguration,
-                            $inheritancePath,
-                            '.'
-                        );
-                    } catch (MissingArrayPathException $exception) {
-                        $configuration = null;
-                    }
+                    $configuration = $this->getValueByPath($this->referenceConfiguration, $inheritancePath);
 
                     if (isset($configuration[self::INHERITANCE_OPERATOR])) {
-                        try {
-                            $_inheritances = ArrayUtility::getValueByPath(
-                                $this->referenceConfiguration,
-                                $inheritancePath . '.' . self::INHERITANCE_OPERATOR,
-                                '.'
-                            );
-                        } catch (MissingArrayPathException $exception) {
-                            $_inheritances = null;
-                        }
+                        $_inheritances = $this->getValueByPath($this->referenceConfiguration, $inheritancePath . '.' . self::INHERITANCE_OPERATOR);
 
                         foreach ($_inheritances as $_inheritancePath) {
                             if (strpos($pathToCheck, $_inheritancePath) === 0) {
@@ -350,4 +339,22 @@ class InheritancesResolverService
         }
         return $result;
     }
+
+    /**
+     * Check the given array representation of a YAML config for the given path and return it's value / sub-array.
+     * If path is not found, return null;
+     *
+     * @param array $config
+     * @param string $path
+     * @param string $delimiter
+     * @return string|array|null
+     */
+    protected function getValueByPath(array $config, string $path, string $delimiter = '.')
+    {
+        try {
+            return ArrayUtility::getValueByPath($config, $path, $delimiter);
+        } catch (\RuntimeException $exception) {
+            return null;
+        }
+    }
 }
index fa73a7b..208e37b 100644 (file)
@@ -34,6 +34,46 @@ class InheritancesResolverServiceTest extends \TYPO3\TestingFramework\Core\Unit\
 
     /**
      * @test
+     * Test for the explicit example in service class comment
+     */
+    public function getDocExampleInheritance()
+    {
+        $input = [
+            'Form' => [
+                'part1' => [
+                    'key1' => 'value1',
+                    'key2' => 'value2',
+                    'key3' => 'value3',
+                ],
+                'part2' => [
+                    '__inheritances' => [
+                        10 => 'Form.part1',
+                    ],
+                    'key2' => 'another_value'
+                ],
+            ],
+        ];
+
+        $expected = [
+            'Form' => [
+                'part1' => [
+                    'key1' => 'value1',
+                    'key2' => 'value2',
+                    'key3' => 'value3',
+                ],
+                'part2' => [
+                    'key1' => 'value1',
+                    'key2' => 'another_value',
+                    'key3' => 'value3'
+                ],
+            ],
+        ];
+
+        $this->assertSame($expected, $this->subject->reset()->setReferenceConfiguration($input)->getResolvedConfiguration($input));
+    }
+
+    /**
+     * @test
      */
     public function getMergedConfigurationSimpleInheritance()
     {