[TASK] Extract settings tree diff into a object and add tests

bnf · bnf · commit fc9bfac7f515 · 2025-03-25T17:01:07.000+01:00
Split the `computeSettingsDiff` into a self-constructing data object and a form-data transformation backed by the site settings factory. The diff algorithm does not depend on any site context and is therefore implemented generically. Currently missing tests are added along the way. Arrays are now transformed into settings objects as early as possible in order to reduce the amount of different arrays formats (settings map vs tree) that are passed around. Releases: main, 13.4 Resolves: #104955 Change-Id: Ifb84e8e4af41340fb60c1627d07c85bda85af2ee Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/86084 Tested-by: Andreas Kienast <akienast@scripting-base.de> Reviewed-by: Benjamin Franzke <ben@bnf.dev> Tested-by: Benjamin Franzke <ben@bnf.dev> Tested-by: Benni Mack <benni@typo3.org> Tested-by: core-ci <typo3@b13.com> Reviewed-by: Andreas Kienast <akienast@scripting-base.de> Reviewed-by: Benni Mack <benni@typo3.org>
diff --git a/typo3/sysext/backend/Classes/Controller/SiteSettingsController.php b/typo3/sysext/backend/Classes/Controller/SiteSettingsController.php
@@ -203,17 +203,18 @@ public function saveAction(ServerRequestInterface $request): ResponseInterface
             return new RedirectResponse($returnUrl ?? $overviewUrl);
         }
 
-        $changes = $this->siteSettingsService->computeSettingsDiff($site, $parsedBody['settings'] ?? []);
-        $this->siteSettingsService->writeSettings($site, $changes['settings']);
+        $newSettings = $this->siteSettingsService->createSettingsFromFormData($site, $parsedBody['settings'] ?? []);
+        $settingsDiff = $this->siteSettingsService->computeSettingsDiff($site, $newSettings);
+        $this->siteSettingsService->writeSettings($site, $settingsDiff->asArray());
 
-        if ($changes['changes'] !== [] || $changes['deletions'] !== []) {
+        if ($settingsDiff->changes !== [] || $settingsDiff->deletions !== []) {
             $this->getBackendUser()->writelog(
                 Type::SITE,
                 SettingAction::CHANGE,
                 SystemLogErrorClassification::MESSAGE,
                 null,
                 'Site settings changed for \'%s\': %s',
-                [$site->getIdentifier(), json_encode($changes)],
+                [$site->getIdentifier(), json_encode($settingsDiff)],
                 'site'
             );
 
@@ -246,8 +247,10 @@ public function dumpAction(ServerRequestInterface $request): ResponseInterface
         $specificSetting = (string)($parsedBody['specificSetting'] ?? '');
 
         $minify = $specificSetting !== '' ? false : true;
-        $changes = $this->siteSettingsService->computeSettingsDiff($site, $parsedBody['settings'] ?? [], $minify);
-        $settings = $changes['settings'];
+
+        $newSettings = $this->siteSettingsService->createSettingsFromFormData($site, $parsedBody['settings'] ?? []);
+        $settingsDiff = $this->siteSettingsService->computeSettingsDiff($site, $newSettings, $minify);
+        $settings = $settingsDiff->asArray();
         if ($specificSetting !== '') {
             $value = ArrayUtility::getValueByPath($settings, $specificSetting, '.');
             $settings = ArrayUtility::setValueByPath([], $specificSetting, $value, '.');
diff --git a/typo3/sysext/core/Classes/Settings/SettingsTree.php b/typo3/sysext/core/Classes/Settings/SettingsTree.php
@@ -0,0 +1,115 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * 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!
+ */
+
+namespace TYPO3\CMS\Core\Settings;
+
+use TYPO3\CMS\Core\Utility\ArrayUtility;
+
+/**
+ * @internal
+ */
+final readonly class SettingsTree
+{
+    /**
+     * @param string[] $changes
+     * @param string[] $deletions
+     */
+    public function __construct(
+        public array $settingsTree,
+        public array $changes,
+        public array $deletions,
+    ) {}
+
+    public function asArray(): array
+    {
+        return $this->settingsTree;
+    }
+
+    /**
+     * Calculate a new settings tree for the given $targetSettings
+     *
+     * Settings that have the same value as their default value
+     * are removed (tree is minified) if the list of default settings
+     * via $defaultSettings.
+     *
+     * @param array $currentSettingsTree Current settings tree (recursive structure)
+     *                                   In case of site settings: config/sites/…/settings.yaml
+     * @param SettingsInterface $targetSettings Target settings
+     *                                          (values as supplied via the settings editor)
+     * @param SettingsInterface $defaultSettings Default settings, without local settings tree applied.
+     *                                           In case of site settings: Combination of all settings
+     *                                           defined in settings.definitions.yaml + setting.yaml
+     *                                           from all selected sets combined
+     */
+    public static function diff(
+        array $currentSettingsTree,
+        SettingsInterface $targetSettings,
+        ?SettingsInterface $defaultSettings = null,
+    ): self {
+        // Copy existing settings from current settings tree, to keep any settings
+        // that have been present before (and are not defined in $defaultSettings)
+        // Usecase for site settings:
+        // Preserve "anonymous" v12-style site settings that have no definition in settings.definitions.yaml
+        $settingsTree = $currentSettingsTree;
+
+        // Merge target settings into current settingsTree
+        $changes = [];
+        $deletions = [];
+        foreach ($targetSettings->getIdentifiers() as $key) {
+            $value = $targetSettings->get($key);
+            if ($defaultSettings !== null && $value === $defaultSettings->get($key)) {
+                if (ArrayUtility::isValidPath($settingsTree, $key, '.')) {
+                    $settingsTree = self::removeByPathWithAncestors($settingsTree, $key, '.');
+                    $deletions[] = $key;
+                }
+                continue;
+            }
+            if (!ArrayUtility::isValidPath($settingsTree, $key, '.') ||
+                $value !== ArrayUtility::getValueByPath($settingsTree, $key, '.')
+            ) {
+                $settingsTree = ArrayUtility::setValueByPath($settingsTree, $key, $value, '.');
+                $changes[] = $key;
+            }
+        }
+
+        return new self(
+            $settingsTree,
+            $changes,
+            $deletions
+        );
+    }
+
+    private static function removeByPathWithAncestors(array $array, string $path, string $delimiter): array
+    {
+        if ($path === '' || !ArrayUtility::isValidPath($array, $path, $delimiter)) {
+            return $array;
+        }
+
+        $array = ArrayUtility::removeByPath($array, $path, $delimiter);
+        $parts = explode($delimiter, $path);
+        array_pop($parts);
+        $parentPath = implode($delimiter, $parts);
+
+        if ($parentPath !== '' && ArrayUtility::isValidPath($array, $parentPath, $delimiter)) {
+            $parent = ArrayUtility::getValueByPath($array, $parentPath, $delimiter);
+            if ($parent === []) {
+                return self::removeByPathWithAncestors($array, $parentPath, $delimiter);
+            }
+        }
+        return $array;
+    }
+}
diff --git a/typo3/sysext/core/Classes/Site/SiteSettingsFactory.php b/typo3/sysext/core/Classes/Site/SiteSettingsFactory.php
@@ -68,6 +68,21 @@ public function getSettings(string $siteIdentifier, array $siteConfiguration): S
         return $settings;
     }
 
+    /**
+     * Load settings from config/sites/{$siteIdentifier}/settings.yaml.
+     *
+     * @internal
+     */
+    public function loadLocalSettingsTree(string $siteIdentifier): ?array
+    {
+        $fileName = $this->configPath . '/' . $siteIdentifier . '/' . $this->settingsFileName;
+        if (!file_exists($fileName)) {
+            return null;
+        }
+
+        return $this->yamlFileLoader->load(GeneralUtility::fixWindowsFilePath($fileName));
+    }
+
     /**
      * Fetch the settings for a specific site and return the parsed Site Settings object.
      *
@@ -82,12 +97,7 @@ public function createSettings(array $sets = [], ?string $siteIdentifier = null,
     {
         $settingsTree = [];
         if ($siteIdentifier !== null) {
-            $fileName = $this->configPath . '/' . $siteIdentifier . '/' . $this->settingsFileName;
-            if (file_exists($fileName)) {
-                $settingsTree = $this->yamlFileLoader->load(GeneralUtility::fixWindowsFilePath($fileName));
-            } else {
-                $settingsTree = $inlineSettings;
-            }
+            $settingsTree = $this->loadLocalSettingsTree($siteIdentifier) ?? $inlineSettings;
         }
 
         return $this->composeSettings($settingsTree, $sets);
@@ -135,14 +145,12 @@ public function composeSettings(array $settingsTree, array $sets): SiteSettings
         );
     }
 
+    /**
+     * @internal
+     */
     public function createSettingsForKeys(array $settingKeys, string $siteIdentifier, array $inlineSettings = []): SiteSettings
     {
-        $fileName = $this->configPath . '/' . $siteIdentifier . '/' . $this->settingsFileName;
-        if (file_exists($fileName)) {
-            $settingsTree = $this->yamlFileLoader->load(GeneralUtility::fixWindowsFilePath($fileName));
-        } else {
-            $settingsTree = $inlineSettings;
-        }
+        $settingsTree = $this->loadLocalSettingsTree($siteIdentifier) ?? $inlineSettings;
 
         /** @var array<string, string|int|float|bool|array|null> $settingsMap */
         $settingsMap = [];
@@ -152,11 +160,37 @@ public function createSettingsForKeys(array $settingKeys, string $siteIdentifier
             }
             $settingsMap[$key] = ArrayUtility::getValueByPath($settingsTree, $key, '.');
         }
-        $flatSettings = $settingsTree === [] ? [] : ArrayUtility::flattenPlain($settingsTree);
-        return new SiteSettings(
-            settings: $settingsMap,
+
+        return SiteSettings::create(
+            settingsMap: $settingsMap,
+            settingsTree: $settingsTree,
+        );
+    }
+
+    /**
+     * @internal
+     */
+    public function createSettingsFromFormData(array $settingsMap, array $definitions): SiteSettings
+    {
+        $settingsTree = [];
+        foreach ($settingsMap as $key => $value) {
+            $definition = $definitions[$key] ?? null;
+            if ($definition === null) {
+                throw new \RuntimeException('Unexpected setting ' . $key . ' is not defined', 1724067004);
+            }
+            $type = $this->settingsTypeRegistry->get($definition->type);
+            // @todo We should collect invalid values (readonly-violation/validation-error) and report in the UI instead of ignoring them
+            if ($definition->readonly || !$type->validate($value, $definition)) {
+                $value = $definition->default;
+            }
+            $value = $type->transformValue($value, $definition);
+            $settingsMap[$key] = $value;
+            $settingsTree = ArrayUtility::setValueByPath($settingsTree, $key, $value, '.');
+        }
+
+        return SiteSettings::create(
+            settingsMap: $settingsMap,
             settingsTree: $settingsTree,
-            flatSettings: $flatSettings,
         );
     }
 
diff --git a/typo3/sysext/core/Classes/Site/SiteSettingsService.php b/typo3/sysext/core/Classes/Site/SiteSettingsService.php
@@ -24,12 +24,12 @@
 use TYPO3\CMS\Core\Messaging\FlashMessage;
 use TYPO3\CMS\Core\Messaging\FlashMessageService;
 use TYPO3\CMS\Core\Settings\SettingDefinition;
+use TYPO3\CMS\Core\Settings\SettingsTree;
 use TYPO3\CMS\Core\Settings\SettingsTypeRegistry;
 use TYPO3\CMS\Core\Site\Entity\Site;
 use TYPO3\CMS\Core\Site\Entity\SiteSettings;
 use TYPO3\CMS\Core\Site\Set\SetRegistry;
 use TYPO3\CMS\Core\Type\ContextualFeedbackSeverity;
-use TYPO3\CMS\Core\Utility\ArrayUtility;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 
 /**
@@ -78,57 +78,20 @@ public function getLocalSettings(Site $site): SiteSettings
         );
     }
 
-    public function computeSettingsDiff(Site $site, array $rawSettings, bool $minify = true): array
+    public function computeSettingsDiff(Site $site, SiteSettings $newSettings, bool $minify = true): SettingsTree
     {
-        $settings = [];
-        $localSettings = [];
+        // Settings from sets only – setting values without site-local config/sites/*/settings.yaml applied
+        $defaultSettings = $minify ? $this->siteSettingsFactory->createSettings($site->getSets(), null) : null;
 
-        $definitions = $this->getDefinitions($site);
-        foreach ($rawSettings as $key => $value) {
-            $definition = $definitions[$key] ?? null;
-            if ($definition === null) {
-                throw new \RuntimeException('Unexpected setting ' . $key . ' is not defined', 1724067004);
-            }
-            if ($definition->readonly) {
-                continue;
-            }
-            $type = $this->settingsTypeRegistry->get($definition->type);
-            $settings[$key] = $type->transformValue($value, $definition);
-        }
-
-        // Settings from sets – setting values without config/sites/*/settings.yaml applied
-        $setSettings = $this->siteSettingsFactory->createSettings($site->getSets());
         // Settings from config/sites/*/settings.yaml only (our persistence target)
-        $localSettings = $this->siteSettingsFactory->createSettingsForKeys(
-            array_map(static fn(SettingDefinition $d) => $d->key, $definitions),
-            $site->getIdentifier(),
-            $site->getRawConfiguration()['settings'] ?? []
-        );
-
-        // Read existing settings, as we *must* not remove any settings that may be present because of
-        //  * "undefined" settings that were supported since TYPO3 v12
-        //  * (temporary) inactive sets
-        $settingsTree = $localSettings->getAll();
+        $localSettings = $this->siteSettingsFactory->loadLocalSettingsTree($site->getIdentifier()) ??
+            $site->getRawConfiguration()['settings'] ?? [];
 
-        // Merge incoming settings into current settingsTree
-        $changes = [];
-        $deletions = [];
-        foreach ($settings as $key => $value) {
-            if ($minify && $value === $setSettings->get($key)) {
-                if (ArrayUtility::isValidPath($settingsTree, $key, '.')) {
-                    $settingsTree = $this->removeByPathWithAncestors($settingsTree, $key, '.');
-                    $deletions[] = $key;
-                }
-                continue;
-            }
-            $settingsTree = ArrayUtility::setValueByPath($settingsTree, $key, $value, '.');
-            $changes[] = $key;
-        }
-        return [
-            'settings' => $settingsTree,
-            'changes' => $changes,
-            'deletions' => $deletions,
-        ];
+        return SettingsTree::diff(
+            $localSettings,
+            $newSettings,
+            $defaultSettings,
+        );
     }
 
     public function writeSettings(Site $site, array $settings): void
@@ -144,7 +107,10 @@ public function writeSettings(Site $site, array $settings): void
         $this->codeCache->flush();
     }
 
-    private function getDefinitions(Site $site): array
+    /**
+     * @return array<string, SettingDefinition>
+     */
+    public function getDefinitions(Site $site): array
     {
         $sets = $this->setRegistry->getSets(...$site->getSets());
         $definitions = [];
@@ -156,26 +122,8 @@ private function getDefinitions(Site $site): array
         return $definitions;
     }
 
-    private function removeByPathWithAncestors(array $array, string $path, string $delimiter): array
+    public function createSettingsFromFormData(Site $site, array $settingsMap): SiteSettings
     {
-        if ($path === '') {
-            return $array;
-        }
-        if (!ArrayUtility::isValidPath($array, $path, $delimiter)) {
-            return $array;
-        }
-
-        $array = ArrayUtility::removeByPath($array, $path, $delimiter);
-        $parts = explode($delimiter, $path);
-        array_pop($parts);
-        $parentPath = implode($delimiter, $parts);
-
-        if ($parentPath !== '' && ArrayUtility::isValidPath($array, $parentPath, $delimiter)) {
-            $parent = ArrayUtility::getValueByPath($array, $parentPath, $delimiter);
-            if ($parent === []) {
-                return $this->removeByPathWithAncestors($array, $parentPath, $delimiter);
-            }
-        }
-        return $array;
+        return $this->siteSettingsFactory->createSettingsFromFormData($settingsMap, $this->getDefinitions($site));
     }
 }
diff --git a/typo3/sysext/core/Tests/Unit/Settings/SettingsTreeTest.php b/typo3/sysext/core/Tests/Unit/Settings/SettingsTreeTest.php
