[ISSUE/1227] parentTheme config field inheritance, add changelog

[BrocksiNet]

1. Why is this change necessary?

It is useless to copy the same config fields accross themes.
Why can I set a parentThemeId when it is not using the inheritance (confusing).

2. What does this change do, exactly?

It allows to inherit the config fields from a parent theme

3. Describe each step to reproduce the issue or behaviour.

Create two themes, add one theme as parentTheme via API, try to build the childTheme after that (there will be also a core bug)
Create some config fields ... there is no inheritance from parent to child.

4. Please link to the relevant issues (if any).

See #1227 and #507

5. Checklist

• I have run the core tests without any error
• I have squashed any insignificant commits
• I have created a changelog file with all necessary information about my changes
• I have written or adjusted the documentation according to my changes
• This change has comments for package types, values, functions, and non-obvious lines of code
• I have read the contribution requirements and fulfil them.

[NiklasLimberg]

Hey @bjoern-flagbit could you please fill out the changelog and add a test based on the scenario in the Issue?

[BrocksiNet]

Hey @bjoern-flagbit could you please fill out the changelog and add a test based on the scenario in the Issue?

Changelog file is there. How detailed you need the test base scenario because in some parts it is already described at point 3 (also in the linked issues).

[NiklasLimberg]

@bjoern-flagbit Jes it's described in the Issue, but we need it as a Unit Test to ensure that this doesn't break in the future again

[BrocksiNet]

Hmm the main problem is that the existing test is not failing in both cases. So for this someone need to check why the current test was not failing and then we can run the test again on the changed version. Will think about this, but will take some time.

[BrocksiNet]

@NiklasLimberg there is a general problem. Because: \Shopware\Storefront\Test\Theme\ThemeServiceTest is testing \Shopware\Storefront\Theme\StorefrontPluginConfiguration\StorefrontPluginConfiguration with the function getThemeConfig. So this is not really a ThemeServiceTest. This makes me sad now.

[J-Rahe]

hey @bjoern-flagbit, @bneumann97 should come back to you regarding this.

[BrocksiNet]

If you wanna I can for sure provide a test for loadConfigByName and getThemeConfiguration.
This I guess would be useful, but for sure some other peps would be faster in writting unit-tests then me 😆

[dithom]

+1 We need this! Sadly im also not good at writing unit test 😅

[rvveber]

+1

[rconcrown]

+1 Would be useful!

[BrocksiNet]

I researched a bit more about the test and I was wrong at the beginning so the ThemeServiceTest is testing getPluginConfiguration from ThemeService. There was just also another function with the same name.

I tried to reproduce the core error but this seams to me at the moment not possible because there is always an mocked object that will not run into this error. Need more time for this. 😿

[bneumann97]

Hey @bjoern-flagbit, thanks for your contribution 💙 Something seems to be wrong with your code style, Please run the PHP CS-Fixer on your branch. The tests are part of our contribution guideline, so we do need a test for this to accept the PR

[BrocksiNet]

Fixed the code style but not the conflict.

This commit 4925bcb makes the test also useless 🥱

[shopwareBot]

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-16911

Please use this issue to track the state of your pull request.

[NiklasLimberg]

Hi @bjoern-flagbit,
are u still working on it?
This seems to be still missing a test, therefore it's still incomplete and i'll close this in about 2 days

[NiklasLimberg]

Ok the 2 days are up i'll close it

[BrocksiNet]

This problem is still present in Shopware Version 6.4.7.0. Tested today.

How you can reproduce this:

1. Create a new theme
2. Add some theme config (https://developer.shopware.com/docs/guides/plugins/themes/theme-configuration)
3. Duplicate the new created theme via administration
4. Check if the config field is there (and yes it should be because it was copied into the new db entry)
5. Now add a second config field to the parent theme (the theme you created first)
6. Check the parent theme if the second config field is there ... sure it is reading theme.json ... so everything fine
7. Now check the duplicated theme (child theme) ... you will not find the new config field (second one) ... only the config fields that where copied before ...

The suggested changes where reading also the parent theme config from file base and merging them together. Maybe need to update this PR. If I have time for this, would be cool.

[BrocksiNet]

Here is a patch File you can use with Shopware 6.4.7.0 if you wanna use this feature:

--- /Theme/ThemeService.php
+++ /Theme/ThemeService.php
@@ -150,51 +150,45 @@
      */
     public function getThemeConfiguration(string $themeId, bool $translate, Context $context): array
     {
-        $criteria = new Criteria();
-        $criteria->setTitle('theme-service::load-config');
-
-        $criteria->addFilter(new MultiFilter(
-            MultiFilter::CONNECTION_OR,
-            [
-                new EqualsFilter('technicalName', StorefrontPluginRegistry::BASE_THEME_NAME),
-                new EqualsFilter('id', $themeId),
-            ]
-        ));
-
-        $themes = $this->themeRepository->search($criteria, $context);
+        $baseThemeSearchResult = $this->getThemeByName(StorefrontPluginRegistry::BASE_THEME_NAME, $context);
+        /** @var ThemeEntity $baseTheme */
+        $baseTheme = $baseThemeSearchResult->first();
+        $baseThemeConfig = $this->mergeStaticConfig($baseTheme);

-        $theme = $themes->get($themeId);
+        $childThemeSearchResult = $this->getThemeById($themeId, $context);
+        /** @var ThemeEntity $childTheme */
+        $childTheme = $childThemeSearchResult->first();
+        $childThemeConfig = $this->mergeStaticConfig($childTheme);

-        /** @var ThemeEntity|null $theme */
-        if (!$theme) {
-            throw new InvalidThemeException($themeId);
+        $parentThemeId = $childTheme->getParentThemeId();
+        if (null !== $parentThemeId) {
+            /** @var ThemeEntity $parentTheme */
+            $parentThemeSearchResult = $this->getThemeById($parentThemeId, $context);
+            $parentTheme = $parentThemeSearchResult->first();
+            $parentThemeConfig = $this->mergeStaticConfig($parentTheme);
         }

-        /** @var ThemeEntity $baseTheme */
-        $baseTheme = $themes->filter(function (ThemeEntity $theme) {
-            return $theme->getTechnicalName() === StorefrontPluginRegistry::BASE_THEME_NAME;
-        })->first();
+        if (isset($parentThemeConfig)) {
+            $baseThemeConfig = array_replace_recursive($baseThemeConfig, $parentThemeConfig);
+        }

-        $baseThemeConfig = $this->mergeStaticConfig($baseTheme);
+        $themeConfig = array_replace_recursive($baseThemeConfig, $childThemeConfig);

         $themeConfigFieldFactory = new ThemeConfigFieldFactory();
         $configFields = [];

-        $configuredTheme = $this->mergeStaticConfig($theme);
-        $themeConfig = array_replace_recursive($baseThemeConfig, $configuredTheme);
-
         foreach ($themeConfig['fields'] as $name => $item) {
             $configFields[$name] = $themeConfigFieldFactory->create($name, $item);
         }

-        $configFields = json_decode((string) json_encode($configFields), true);
+        $configFields = json_decode((string)json_encode($configFields), true);

-        $labels = array_replace_recursive($baseTheme->getLabels() ?? [], $theme->getLabels() ?? []);
+        $labels = array_replace_recursive($baseTheme->getLabels() ?? [], $childTheme->getLabels() ?? []);
         if ($translate && !empty($labels)) {
             $configFields = $this->translateLabels($configFields, $labels);
         }

-        $helpTexts = array_replace_recursive($baseTheme->getHelpTexts() ?? [], $theme->getHelpTexts() ?? []);
+        $helpTexts = array_replace_recursive($baseTheme->getHelpTexts() ?? [], $childTheme->getHelpTexts() ?? []);
         if ($translate && !empty($helpTexts)) {
             $configFields = $this->translateHelpTexts($configFields, $helpTexts);
         }
@@ -245,6 +239,36 @@
         return $outputStructure;
     }

+    private function getThemeByName(
+        string $themeName,
+               $context
+    ): \Shopware\Core\Framework\DataAbstractionLayer\Search\EntitySearchResult {
+        $criteria = new Criteria();
+        $criteria->setTitle('theme-service::load-config::by-name');
+
+        $criteria->addFilter(new EqualsFilter(
+            'technicalName',
+            $themeName
+        ));
+
+        return $this->themeRepository->search($criteria, $context);
+    }
+
+    private function getThemeById(
+        string $themeId,
+               $context
+    ): \Shopware\Core\Framework\DataAbstractionLayer\Search\EntitySearchResult {
+        $criteria = new Criteria();
+        $criteria->setTitle('theme-service::load-config::by-id');
+
+        $criteria->addFilter(new EqualsFilter(
+            'id',
+            $themeId
+        ));
+
+        return $this->themeRepository->search($criteria, $context);
+    }
+
     private function mergeStaticConfig(ThemeEntity $theme): array
     {
         $configuredTheme = [];

--- /Theme/ConfigLoader/DatabaseConfigLoader.php
+++ /Theme/ConfigLoader/DatabaseConfigLoader.php
@@ -146,9 +146,13 @@

             \assert(\is_string($parentTheme->getTechnicalName()));

-            return $this->extensionRegistry
+            $parentPluginConfig = $this->extensionRegistry
                 ->getConfigurations()
                 ->getByTechnicalName($parentTheme->getTechnicalName());
+
+            if ($parentPluginConfig !== null) {
+                return $parentPluginConfig;
+            }
         }

         return $this->extensionRegistry

[BrocksiNet]

Why this feature is needed?

At the moment it is not possible for Shopware to load new config fields from parentThemes from file level.

So when a parentTheme get updates and new config fields ... your childTheme can not use them.

[ssltg]

Hi @bjoern-flagbit
thank you for your contribution. But this behaviour will already be available with the 6.4.8.1 release.
From then on, every Change to a parent theme will effect child themes as well.

[BrocksiNet]

@ssltg Did you also fixed the Bug in DatabaseConfigLoader ? 😉

[BrocksiNet]

I can confirm that this is now working in core. I tested with Shopware Version 6.4.10.1
Also when you add new config files in theme.json on file level.

Good job. Thx. 👍