From ca846b5fbf22650b0156032f9129174e23558f43 Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Sun, 5 Feb 2023 19:51:08 +0100 Subject: [PATCH] 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 | 9 ++- config/packages/security.yaml | 2 - config/services_test.yaml | 7 +- phpstan.neon | 30 -------- src/Configuration/ConfigLoaderInterface.php | 13 ++-- src/Configuration/ConfigurationService.php | 72 +++++++++++++++++++ src/Configuration/SystemConfiguration.php | 14 ++-- .../SystemConfigurationController.php | 8 ++- src/Form/Type/MailType.php | 2 +- src/Repository/ConfigurationRepository.php | 63 +++------------- tests/Configuration/TestConfigLoader.php | 17 ++--- tests/Controller/ControllerBaseTest.php | 13 ++-- tests/Controller/TimesheetControllerTest.php | 16 +---- tests/phpstan.neon | 10 --- 14 files changed, 119 insertions(+), 157 deletions(-) create mode 100644 src/Configuration/ConfigurationService.php diff --git a/config/packages/cache.yaml b/config/packages/cache.yaml index 4aab94ae..8fd91582 100644 --- a/config/packages/cache.yaml +++ b/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 diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 08a7a06f..6d2585b9 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -1,6 +1,4 @@ security: - enable_authenticator_manager: true - password_hashers: App\Entity\User: auto diff --git a/config/services_test.yaml b/config/services_test.yaml index cd4520c3..ba0ae9f7 100644 --- a/config/services_test.yaml +++ b/config/services_test.yaml @@ -7,12 +7,7 @@ services: public: true App\Configuration\SystemConfiguration: - arguments: ['@App\Repository\ConfigurationRepository', "%kimai.config%"] - - App\Repository\ConfigurationRepository: - class: Doctrine\ORM\EntityRepository - factory: ['@doctrine.orm.entity_manager', getRepository] - arguments: ['App\Entity\Configuration'] + arguments: ['@App\Configuration\ConfigurationService', "%kimai.config%"] # required for the importer command test App\Repository\UserRepository: diff --git a/phpstan.neon b/phpstan.neon index 652edd80..df749584 100644 --- a/phpstan.neon +++ b/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 diff --git a/src/Configuration/ConfigLoaderInterface.php b/src/Configuration/ConfigLoaderInterface.php index 3a723759..efa480e7 100644 --- a/src/Configuration/ConfigLoaderInterface.php +++ b/src/Configuration/ConfigLoaderInterface.php @@ -9,18 +9,13 @@ namespace App\Configuration; -use App\Entity\Configuration; - +/** + * @internal + */ interface ConfigLoaderInterface { /** - * @param string $name - * @return ?Configuration - */ - public function getConfiguration(string $name): ?Configuration; - - /** - * @return Configuration[] + * @return array */ public function getConfigurations(): array; } diff --git a/src/Configuration/ConfigurationService.php b/src/Configuration/ConfigurationService.php new file mode 100644 index 00000000..3a0bdbd7 --- /dev/null +++ b/src/Configuration/ConfigurationService.php @@ -0,0 +1,72 @@ + + */ + private static array $cacheAll = []; + private static bool $initialized = false; + + public function __construct(private ConfigurationRepository $configurationRepository, private CacheInterface $cache) + { + } + + /** + * @return array + */ + 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(); + } +} diff --git a/src/Configuration/SystemConfiguration.php b/src/Configuration/SystemConfiguration.php index a3039e81..09f08825 100644 --- a/src/Configuration/SystemConfiguration.php +++ b/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) { - $this->set($configuration->getName(), $configuration->getValue()); + foreach ($this->repository->getConfigurations() as $key => $value) { + $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 + * @return array> */ public function getSamlRolesMapping(): array { @@ -227,7 +227,7 @@ final class SystemConfiguration } /** - * @return array + * @return array|bool> */ public function getSamlConnection(): array { @@ -235,7 +235,7 @@ final class SystemConfiguration } /** - * @return array + * @return array> */ public function getSamlAttributeMapping(): array { diff --git a/src/Controller/SystemConfigurationController.php b/src/Controller/SystemConfigurationController.php index 962d9d1e..5c2cfe19 100644 --- a/src/Controller/SystemConfigurationController.php +++ b/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); diff --git a/src/Form/Type/MailType.php b/src/Form/Type/MailType.php index 20c4b603..87153792 100644 --- a/src/Form/Type/MailType.php +++ b/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']) ], ]); } diff --git a/src/Repository/ConfigurationRepository.php b/src/Repository/ConfigurationRepository.php index 92b807e7..a0d9f516 100644 --- a/src/Repository/ConfigurationRepository.php +++ b/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 + * @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 - */ - 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 */ public function getConfigurations(): array { - $this->prefillCache(); + $query = $this->createQueryBuilder('s')->select('s.name')->addSelect('s.value')->getQuery(); + /** @var array> $result */ + $result = $query->getArrayResult(); - return array_values(self::$cacheAll); - } - - public function getConfiguration(string $name): ?Configuration - { - $this->prefillCache(); - - if (!\array_key_exists($name, self::$cacheAll)) { - return null; + $all = []; + foreach ($result as $row) { + $all[$row['name']] = $row['value']; } - return self::$cacheAll[$name]; + return $all; } - public function saveSystemConfiguration(SystemConfiguration $model) + public function saveSystemConfiguration(SystemConfiguration $model): void { $em = $this->getEntityManager(); $em->beginTransaction(); @@ -120,7 +81,5 @@ class ConfigurationRepository extends EntityRepository implements ConfigLoaderIn $em->rollback(); throw $ex; } - - $this->clearCache(); } } diff --git a/tests/Configuration/TestConfigLoader.php b/tests/Configuration/TestConfigLoader.php index 25c2ac01..17490483 100644 --- a/tests/Configuration/TestConfigLoader.php +++ b/tests/Configuration/TestConfigLoader.php @@ -18,29 +18,22 @@ use App\Entity\Configuration; class TestConfigLoader implements ConfigLoaderInterface { /** - * @var Configuration[] + * @var array */ - private array $configs; + private array $configs = []; /** * @param Configuration[] $configs */ public function __construct(array $configs) { - $this->configs = $configs; - } - - public function getConfiguration(string $name): ?Configuration - { - if (!\array_key_exists($name, $this->configs)) { - return null; + foreach ($configs as $config) { + $this->configs[$config->getName()] = $config->getValue(); } - - return $this->configs[$name]; } /** - * @return Configuration[] + * @return array */ public function getConfigurations(): array { diff --git a/tests/Controller/ControllerBaseTest.php b/tests/Controller/ControllerBaseTest.php index fcb305d9..62c7a95c 100644 --- a/tests/Controller/ControllerBaseTest.php +++ b/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 */ - $repository = self::getContainer()->get(ConfigurationRepository::class); - $repository->clearCache(); + /** @var ConfigurationService $service */ + $service = self::getContainer()->get(ConfigurationService::class); + $service->clearCache(); } protected function getClientForAuthenticatedUser(string $role = User::ROLE_USER): HttpKernelBrowser diff --git a/tests/Controller/TimesheetControllerTest.php b/tests/Controller/TimesheetControllerTest.php index b99833da..33b413d7 100644 --- a/tests/Controller/TimesheetControllerTest.php +++ b/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 */ - $repository = $this->getEntityManager()->getRepository(Configuration::class); - $config = new Configuration(); - $config->setName('timesheet.rules.allow_overbooking_budget'); - $config->setValue(false); - $repository->saveConfiguration($config); + $this->setSystemConfiguration('timesheet.rules.allow_overbooking_budget', false); $this->assertHasValidationError( $client, @@ -496,12 +489,7 @@ class TimesheetControllerTest extends ControllerBaseTest $response = $client->getResponse(); $this->assertTrue($response->isSuccessful()); - /** @var ConfigurationRepository $repository */ - $repository = $this->getEntityManager()->getRepository(Configuration::class); - $config = new Configuration(); - $config->setName('timesheet.rules.allow_zero_duration'); - $config->setValue(false); - $repository->saveConfiguration($config); + $this->setSystemConfiguration('timesheet.rules.allow_zero_duration', false); $this->assertHasValidationError( $client, diff --git a/tests/phpstan.neon b/tests/phpstan.neon index 9f335e79..900e6fd6 100644 --- a/tests/phpstan.neon +++ b/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