2.0 RC 1 - new ConfigurationService (#3810)

* use html5 email validation
* remove static cache seed, for compatibility with non default (file based) caches
* added caching ConfigurationService, removed use of doctrine result cache
* simplify configuration API

config/packages/cache.yaml

@@ -1,11 +1,10 @@
 framework:
     cache:
-        # Unique name of your app: used to compute stable namespaces for cache keys.
-        prefix_seed: "kimai"
-
-        # Redis
         #app: cache.adapter.redis
-        #default_redis_provider: redis://localhost
+        #default_redis_provider: redis://127.0.0.1:6379
+
+        #app: cache.adapter.memcached
+        #default_memcached_provider: 'memcached://localhost'
 
         # APCu (not recommended with heavy random-write workloads as memory fragmentation can cause perf issues)
         #app: cache.adapter.apcu

config/packages/security.yaml

@@ -1,6 +1,4 @@
 security:
-    enable_authenticator_manager: true
-
     password_hashers:
         App\Entity\User: auto
 

config/services_test.yaml

@@ -7,12 +7,7 @@ services:
         public: true
 
     App\Configuration\SystemConfiguration:
-        arguments: ['@App\Repository\ConfigurationRepository', "%kimai.config%"]
+        arguments: ['@App\Configuration\ConfigurationService', "%kimai.config%"]
-
-    App\Repository\ConfigurationRepository:
-        class:     Doctrine\ORM\EntityRepository
-        factory:   ['@doctrine.orm.entity_manager', getRepository]
-        arguments: ['App\Entity\Configuration']
 
     # required for the importer command test
     App\Repository\UserRepository:

phpstan.neon

@@ -915,36 +915,16 @@ parameters:
             count: 1
             path: src/Configuration/SystemConfiguration.php
 
-        -
-            message: "#^Offset \\(int\\|string\\) might not exist on array\\|null\\.$#"
-            count: 1
-            path: src/Configuration/SystemConfiguration.php
-
-        -
-            message: "#^Parameter \\#1 \\$array of function array_filter expects array, array\\|null given\\.$#"
-            count: 2
-            path: src/Configuration/SystemConfiguration.php
-
         -
             message: "#^Parameter \\#1 \\$key of method App\\\\Configuration\\\\SystemConfiguration\\:\\:set\\(\\) expects string, mixed given\\.$#"
             count: 1
             path: src/Configuration/SystemConfiguration.php
 
-        -
-            message: "#^Parameter \\#1 \\$key of method App\\\\Configuration\\\\SystemConfiguration\\:\\:set\\(\\) expects string, string\\|null given\\.$#"
-            count: 1
-            path: src/Configuration/SystemConfiguration.php
-
         -
             message: "#^Parameter \\#1 \\$string of function trim expects string, bool\\|float\\|int\\|string given\\.$#"
             count: 1
             path: src/Configuration/SystemConfiguration.php
 
-        -
-            message: "#^Parameter \\#2 \\$array of function array_key_exists expects array, array\\|null given\\.$#"
-            count: 3
-            path: src/Configuration/SystemConfiguration.php
-
         -
             message: "#^Method App\\\\Controller\\\\AbstractController\\:\\:createFormForGetRequest\\(\\) has parameter \\$data with no type specified\\.$#"
             count: 1
@@ -1615,11 +1595,6 @@ parameters:
             count: 3
             path: src/Controller/SystemConfigurationController.php
 
-        -
-            message: "#^Parameter \\#1 \\$model of method App\\\\Repository\\\\ConfigurationRepository\\:\\:saveSystemConfiguration\\(\\) expects App\\\\Form\\\\Model\\\\SystemConfiguration, mixed given\\.$#"
-            count: 1
-            path: src/Controller/SystemConfigurationController.php
-
         -
             message: "#^Parameter \\#1 \\$value of method App\\\\Form\\\\Model\\\\Configuration\\:\\:setValue\\(\\) expects bool\\|int\\|object\\|string\\|null, bool\\|float\\|int\\|string given\\.$#"
             count: 1
@@ -6360,11 +6335,6 @@ parameters:
             count: 1
             path: src/Repository/BookmarkRepository.php
 
-        -
-            message: "#^Method App\\\\Repository\\\\ConfigurationRepository\\:\\:saveSystemConfiguration\\(\\) has no return type specified\\.$#"
-            count: 1
-            path: src/Repository/ConfigurationRepository.php
-
         -
             message: "#^Cannot call method getSearchFields\\(\\) on App\\\\Utils\\\\SearchTerm\\|null\\.$#"
             count: 1

src/Configuration/ConfigLoaderInterface.php

@@ -9,18 +9,13 @@
 
 namespace App\Configuration;
 
-use App\Entity\Configuration;
+/**
-
+ * @internal
+ */
 interface ConfigLoaderInterface
 {
     /**
-     * @param string $name
+     * @return array<string, string|null>
-     * @return ?Configuration
-     */
-    public function getConfiguration(string $name): ?Configuration;
-
-    /**
-     * @return Configuration[]
      */
     public function getConfigurations(): array;
 }

src/Configuration/ConfigurationService.php

@@ -0,0 +1,72 @@
+<?php
+
+/*
+ * This file is part of the Kimai time-tracking app.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Configuration;
+
+use App\Entity\Configuration;
+use App\Form\Model\SystemConfiguration;
+use App\Repository\ConfigurationRepository;
+use Symfony\Contracts\Cache\CacheInterface;
+use Symfony\Contracts\Cache\ItemInterface;
+
+final class ConfigurationService implements ConfigLoaderInterface
+{
+    /**
+     * @var array<string, string|null>
+     */
+    private static array $cacheAll = [];
+    private static bool $initialized = false;
+
+    public function __construct(private ConfigurationRepository $configurationRepository, private CacheInterface $cache)
+    {
+    }
+
+    /**
+     * @return array<string, string|null>
+     */
+    public function getConfigurations(): array
+    {
+        if (self::$initialized === true) {
+            return self::$cacheAll;
+        }
+
+        self::$cacheAll = $this->cache->get('configurations', function (ItemInterface $item) {
+            $item->expiresAfter(86400); // one day
+
+            return $this->configurationRepository->getConfigurations();
+        });
+
+        self::$initialized = true;
+
+        return self::$cacheAll;
+    }
+
+    public function getConfiguration(string $name): ?Configuration
+    {
+        return $this->configurationRepository->findOneBy(['name' => $name]);
+    }
+
+    public function clearCache(): void
+    {
+        $this->cache->delete('configurations');
+        self::$initialized = false;
+    }
+
+    public function saveConfiguration(Configuration $configuration): void
+    {
+        $this->configurationRepository->saveConfiguration($configuration);
+        $this->clearCache();
+    }
+
+    public function saveSystemConfiguration(SystemConfiguration $model): void
+    {
+        $this->configurationRepository->saveSystemConfiguration($model);
+        $this->clearCache();
+    }
+}

src/Configuration/SystemConfiguration.php

@@ -13,7 +13,7 @@ final class SystemConfiguration
 {
     private bool $initialized = false;
 
-    public function __construct(private ConfigLoaderInterface $repository, private ?array $settings)
+    public function __construct(private ConfigLoaderInterface $repository, private array $settings = [])
     {
     }
 
@@ -23,8 +23,8 @@ final class SystemConfiguration
             return;
         }
 
-        foreach ($this->repository->getConfigurations() as $configuration) {
+        foreach ($this->repository->getConfigurations() as $key => $value) {
-            $this->set($configuration->getName(), $configuration->getValue());
+            $this->set($key, $value);
         }
 
         $this->initialized = true;
@@ -35,7 +35,6 @@ final class SystemConfiguration
      *
      * If no key is given to the method, the entire array will be replaced.
      *
-     * @see https://github.com/divineomega/array_undot
      * @param string $key
      * @param mixed $value
      * @return void
@@ -70,6 +69,7 @@ final class SystemConfiguration
     /**
      * This method should be avoided if possible, use plain keys instead.
      *
+     * @see https://github.com/divineomega/array_undot
      * @param string $key
      * @return array
      */
@@ -219,7 +219,7 @@ final class SystemConfiguration
     }
 
     /**
-     * @return array<mixed>
+     * @return array<int, array<'saml'|'kimai', string>>
      */
     public function getSamlRolesMapping(): array
     {
@@ -227,7 +227,7 @@ final class SystemConfiguration
     }
 
     /**
-     * @return array<mixed>
+     * @return array<string, array<mixed>|bool>
      */
     public function getSamlConnection(): array
     {
@@ -235,7 +235,7 @@ final class SystemConfiguration
     }
 
     /**
-     * @return array<mixed>
+     * @return array<int, array<'saml'|'kimai', string>>
      */
     public function getSamlAttributeMapping(): array
     {

src/Controller/SystemConfigurationController.php

@@ -9,6 +9,7 @@
 
 namespace App\Controller;
 
+use App\Configuration\ConfigurationService;
 use App\Configuration\SystemConfiguration;
 use App\Event\SystemConfigurationEvent;
 use App\Form\Model\Configuration;
@@ -30,7 +31,6 @@ use App\Form\Type\TimezoneType;
 use App\Form\Type\TrackingModeType;
 use App\Form\Type\WeekDaysType;
 use App\Form\Type\YesNoType;
-use App\Repository\ConfigurationRepository;
 use App\Timesheet\LockdownService;
 use App\Utils\PageSetup;
 use App\Validator\Constraints\ColorChoices;
@@ -61,7 +61,7 @@ use Symfony\Component\Validator\Constraints\Regex;
 #[IsGranted('system_configuration')]
 final class SystemConfigurationController extends AbstractController
 {
-    public function __construct(private EventDispatcherInterface $eventDispatcher, private ConfigurationRepository $repository, private SystemConfiguration $systemConfiguration, private LockdownService $lockdownService)
+    public function __construct(private EventDispatcherInterface $eventDispatcher, private ConfigurationService $repository, private SystemConfiguration $systemConfiguration, private LockdownService $lockdownService)
     {
     }
 
@@ -142,7 +142,9 @@ final class SystemConfigurationController extends AbstractController
 
         if ($form->isSubmitted() && $form->isValid()) {
             try {
-                $this->repository->saveSystemConfiguration($form->getData());
+                /** @var SystemConfigurationModel $saveModel */
+                $saveModel = $form->getData();
+                $this->repository->saveSystemConfiguration($saveModel);
                 $this->flashSuccess('action.update.success');
             } catch (\Exception $ex) {
                 $this->handleFormUpdateException($ex, $form);

src/Form/Type/MailType.php

@@ -21,7 +21,7 @@ final class MailType extends AbstractType
         $resolver->setDefaults([
             'label' => 'email',
             'constraints' => [
-                new Email(['mode' => 'loose'])
+                new Email(['mode' => 'html5'])
             ],
         ]);
     }

src/Repository/ConfigurationRepository.php

@@ -9,7 +9,6 @@
 
 namespace App\Repository;
 
-use App\Configuration\ConfigLoaderInterface;
 use App\Entity\Configuration;
 use App\Form\Model\SystemConfiguration;
 use Doctrine\ORM\EntityRepository;
@@ -17,74 +16,36 @@ use Doctrine\ORM\Exception\ORMException;
 
 /**
  * @extends EntityRepository<Configuration>
+ * @internal use App\Configuration\ConfigurationService instead
  * @final
  */
-class ConfigurationRepository extends EntityRepository implements ConfigLoaderInterface
+class ConfigurationRepository extends EntityRepository
 {
-    private const CACHE_KEY = 'ConfigurationRepository_All';
-    /**
-     * @var array<string, Configuration>
-     */
-    private static array $cacheAll = [];
-    private static bool $initialized = false;
-
-    public function clearCache(): void
-    {
-        self::$cacheAll = [];
-        self::$initialized = false;
-
-        $cache = $this->getEntityManager()->getConfiguration()->getResultCache();
-        if ($cache !== null && $cache->hasItem(self::CACHE_KEY)) {
-            $cache->deleteItem(self::CACHE_KEY);
-        }
-    }
-
-    private function prefillCache(): void
-    {
-        if (self::$initialized === true) {
-            return;
-        }
-
-        $query = $this->createQueryBuilder('s')->getQuery();
-        $query->enableResultCache(86400, self::CACHE_KEY);
-
-        $configs = $query->getResult();
-        foreach ($configs as $config) {
-            self::$cacheAll[$config->getName()] = $config;
-        }
-        self::$initialized = true;
-    }
-
     public function saveConfiguration(Configuration $configuration): void
     {
         $entityManager = $this->getEntityManager();
         $entityManager->persist($configuration);
         $entityManager->flush();
-        $this->clearCache();
     }
 
     /**
-     * @return Configuration[]
+     * @return array<string, string>
      */
     public function getConfigurations(): array
     {
-        $this->prefillCache();
+        $query = $this->createQueryBuilder('s')->select('s.name')->addSelect('s.value')->getQuery();
+        /** @var array<int, array<'name'|'value', string>> $result */
+        $result = $query->getArrayResult();
 
-        return array_values(self::$cacheAll);
+        $all = [];
+        foreach ($result as $row) {
+            $all[$row['name']] = $row['value'];
         }
 
-    public function getConfiguration(string $name): ?Configuration
+        return $all;
-    {
-        $this->prefillCache();
-
-        if (!\array_key_exists($name, self::$cacheAll)) {
-            return null;
     }
 
-        return self::$cacheAll[$name];
+    public function saveSystemConfiguration(SystemConfiguration $model): void
-    }
-
-    public function saveSystemConfiguration(SystemConfiguration $model)
     {
         $em = $this->getEntityManager();
         $em->beginTransaction();
@@ -120,7 +81,5 @@ class ConfigurationRepository extends EntityRepository implements ConfigLoaderIn
             $em->rollback();
             throw $ex;
         }
-
-        $this->clearCache();
     }
 }

tests/Configuration/TestConfigLoader.php

@@ -18,29 +18,22 @@ use App\Entity\Configuration;
 class TestConfigLoader implements ConfigLoaderInterface
 {
     /**
-     * @var Configuration[]
+     * @var array<string, string|null>
      */
-    private array $configs;
+    private array $configs = [];
 
     /**
      * @param Configuration[] $configs
      */
     public function __construct(array $configs)
     {
-        $this->configs = $configs;
+        foreach ($configs as $config) {
+            $this->configs[$config->getName()] = $config->getValue();
         }
-
-    public function getConfiguration(string $name): ?Configuration
-    {
-        if (!\array_key_exists($name, $this->configs)) {
-            return null;
-        }
-
-        return $this->configs[$name];
     }
 
     /**
-     * @return Configuration[]
+     * @return array<string, string|null>
      */
     public function getConfigurations(): array
     {

tests/Controller/ControllerBaseTest.php

@@ -9,11 +9,11 @@
 
 namespace App\Tests\Controller;
 
+use App\Configuration\ConfigurationService;
 use App\DataFixtures\UserFixtures;
 use App\Entity\Configuration;
 use App\Entity\User;
 use App\Form\Type\DateRangeType;
-use App\Repository\ConfigurationRepository;
 use App\Repository\UserRepository;
 use App\Tests\KernelTestTrait;
 use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
@@ -89,9 +89,10 @@ abstract class ControllerBaseTest extends WebTestCase
 
     protected function setSystemConfiguration(string $name, $value): void
     {
-        $repository = self::getContainer()->get(ConfigurationRepository::class);
+        /** @var ConfigurationService $repository */
+        $repository = self::getContainer()->get(ConfigurationService::class);
 
-        $entity = $repository->findOneBy(['name' => $name]);
+        $entity = $repository->getConfiguration($name);
         if ($entity === null) {
             $entity = new Configuration();
             $entity->setName($name);
@@ -103,9 +104,9 @@ abstract class ControllerBaseTest extends WebTestCase
 
     protected function clearConfigCache()
     {
-        /** @var ConfigurationRepository $repository */
+        /** @var ConfigurationService $service */
-        $repository = self::getContainer()->get(ConfigurationRepository::class);
+        $service = self::getContainer()->get(ConfigurationService::class);
-        $repository->clearCache();
+        $service->clearCache();
     }
 
     protected function getClientForAuthenticatedUser(string $role = User::ROLE_USER): HttpKernelBrowser

tests/Controller/TimesheetControllerTest.php

@@ -10,11 +10,9 @@
 namespace App\Tests\Controller;
 
 use App\Entity\Activity;
-use App\Entity\Configuration;
 use App\Entity\Timesheet;
 use App\Entity\TimesheetMeta;
 use App\Entity\User;
-use App\Repository\ConfigurationRepository;
 use App\Repository\TagRepository;
 use App\Tests\DataFixtures\ActivityFixtures;
 use App\Tests\DataFixtures\TagFixtures;
@@ -442,12 +440,7 @@ class TimesheetControllerTest extends ControllerBaseTest
         $response = $client->getResponse();
         $this->assertTrue($response->isSuccessful());
 
-        /** @var ConfigurationRepository $repository */
+        $this->setSystemConfiguration('timesheet.rules.allow_overbooking_budget', false);
-        $repository = $this->getEntityManager()->getRepository(Configuration::class);
-        $config = new Configuration();
-        $config->setName('timesheet.rules.allow_overbooking_budget');
-        $config->setValue(false);
-        $repository->saveConfiguration($config);
 
         $this->assertHasValidationError(
             $client,
@@ -496,12 +489,7 @@ class TimesheetControllerTest extends ControllerBaseTest
         $response = $client->getResponse();
         $this->assertTrue($response->isSuccessful());
 
-        /** @var ConfigurationRepository $repository */
+        $this->setSystemConfiguration('timesheet.rules.allow_zero_duration', false);
-        $repository = $this->getEntityManager()->getRepository(Configuration::class);
-        $config = new Configuration();
-        $config->setName('timesheet.rules.allow_zero_duration');
-        $config->setValue(false);
-        $repository->saveConfiguration($config);
 
         $this->assertHasValidationError(
             $client,

tests/phpstan.neon

@@ -2867,11 +2867,6 @@ parameters:
             count: 4
             path: Controller/CalendarControllerTest.php
 
-        -
-            message: "#^Cannot call method findOneBy\\(\\) on object\\|null\\.$#"
-            count: 1
-            path: Controller/ControllerBaseTest.php
-
         -
             message: "#^Cannot call method getRepository\\(\\) on object\\|null\\.$#"
             count: 1
@@ -2887,11 +2882,6 @@ parameters:
             count: 1
             path: Controller/ControllerBaseTest.php
 
-        -
-            message: "#^Cannot call method saveConfiguration\\(\\) on object\\|null\\.$#"
-            count: 1
-            path: Controller/ControllerBaseTest.php
-
         -
             message: "#^Method App\\\\Tests\\\\Controller\\\\ControllerBaseTest\\:\\:assert404\\(\\) has no return type specified\\.$#"
             count: 1