improve color chooser and name validation (#2622)

config/services.yaml

@@ -242,6 +242,6 @@ services:
         arguments: ['App\Entity\Invoice']
 
     App\Repository\BookmarkRepository:
-        class:     \Repository\BookmarkRepository
+        class:     App\Repository\BookmarkRepository
         factory:   ['@doctrine.orm.entity_manager', getRepository]
         arguments: ['App\Entity\Bookmark']

src/Configuration/StringAccessibleConfigTrait.php

@@ -17,6 +17,10 @@ trait StringAccessibleConfigTrait
      * @var array
      */
     protected $settings;
+    /**
+     * @var array
+     */
+    protected $original;
     /**
      * @var ConfigLoaderInterface
      */
@@ -29,7 +33,7 @@ trait StringAccessibleConfigTrait
     public function __construct(ConfigLoaderInterface $repository, array $settings)
     {
         $this->repository = $repository;
-        $this->settings = $settings;
+        $this->original = $this->settings = $settings;
     }
 
     /**
@@ -80,6 +84,17 @@ trait StringAccessibleConfigTrait
      */
     abstract protected function getPrefix(): string;
 
+    /**
+     * @param string $key
+     * @return mixed
+     */
+    public function default(string $key)
+    {
+        $key = $this->prepareSearchKey($key);
+
+        return $this->get($key, $this->original);
+    }
+
     /**
      * @param string $key
      * @return mixed

src/Configuration/SystemConfiguration.php

@@ -367,32 +367,13 @@ class SystemConfiguration implements SystemBundleConfiguration
         return (int) $this->find('theme.autocomplete_chars');
     }
 
-    public function getThemeColorChoices(): ?array
+    public function getThemeColorChoices(): ?string
     {
         $config = $this->find('theme.color_choices');
-        if (empty($config)) {
+        if (!empty($config)) {
-            return null;
+            return $config;
-        }
-        $config = explode(',', $config);
-
-        $colors = [];
-        foreach ($config as $item) {
-            if (empty($item)) {
-                continue;
-            }
-            $item = explode('|', $item);
-            $key = $item[0];
-            $value = $key;
-            if (\count($item) > 1) {
-                $value = $item[1];
-            }
-
-            if (empty($key)) {
-                $key = $value;
-            }
-            $colors[$key] = $value;
         }
 
-        return array_unique($colors);
+        return $this->default('theme.color_choices');
     }
 }

src/DependencyInjection/Configuration.php

@@ -429,7 +429,7 @@ class Configuration implements ConfigurationInterface
                 ->end()
                 ->scalarNode('color_choices')
                     ->defaultValue(implode(',', [
-                        Constants::SOFTWARE . '|' . Constants::DEFAULT_COLOR, 'Silver|#c0c0c0', 'Gray|#808080', 'Black|#000000',
+                        'Silver|#c0c0c0', 'Gray|#808080', 'Black|#000000',
                         'Maroon|#800000', 'Brown|#a52a2a', 'Red|#ff0000', 'Orange|#ffa500',
                         'Gold|#ffd700', 'Yellow|#ffff00', 'Peach|#ffdab9', 'Khaki|#f0e68c',
                         'Olive|#808000', 'Lime|#00ff00', 'Jelly|#9acd32', 'Green|#008000', 'Teal|#008080',

src/Form/Type/ColorChoiceType.php

@@ -66,9 +66,12 @@ class ColorChoiceType extends AbstractType implements DataTransformerInterface
 
         if ($this->isLimitedColors()) {
             $choices = [];
-            foreach ($this->systemConfiguration->getThemeColorChoices() as $name => $color) {
+            $colors = $this->convertStringToColorArray($this->systemConfiguration->getThemeColorChoices());
+
+            foreach ($colors as $name => $color) {
                 $choices[$name] = $color;
             }
+
             $options['choices'] = $choices;
             $options['search'] = false;
             $options['attr']['data-renderer'] = 'color';
@@ -77,6 +80,37 @@ class ColorChoiceType extends AbstractType implements DataTransformerInterface
         $resolver->setDefaults($options);
     }
 
+    private function convertStringToColorArray(string $config): array
+    {
+        $config = explode(',', $config);
+
+        $colors = [];
+        foreach ($config as $item) {
+            if (empty($item)) {
+                continue;
+            }
+            $item = explode('|', $item);
+            $key = $item[0];
+            $value = $key;
+
+            if (\count($item) > 1) {
+                $value = $item[1];
+            }
+
+            if (empty($key)) {
+                $key = $value;
+            }
+
+            if ($value === Constants::DEFAULT_COLOR) {
+                continue;
+            }
+
+            $colors[$key] = $value;
+        }
+
+        return array_unique($colors);
+    }
+
     /**
      * {@inheritdoc}
      */

src/Validator/Constraints/ColorChoices.php

@@ -22,5 +22,6 @@ class ColorChoices extends Constraint
     ];
 
     public $message = 'The given value {{ value }} is not a valid hexadecimal color.';
-    public $invalidNameMessage = 'The given value {{ name }} is not a valid color name for {{ color }}. Allowed are {{ max }} characters, given {{ count }}.';
+    public $invalidNameMessage = 'The given value {{ name }} is not a valid color name for {{ color }}. Allowed are {{ max }} alpha-numerical characters, including minus and space.';
+    public $maxLength = 20;
 }

src/Validator/Constraints/ColorChoicesValidator.php

@@ -57,12 +57,15 @@ class ColorChoicesValidator extends ConstraintValidator
                 return;
             }
 
-            if (!\is_string($name) || 1 !== preg_match('/^[0-9a-zA-Z]{1,10}$/i', $name)) {
+            $name = str_replace(['-', ' '], '', $name);
+            $length = mb_strlen($name);
+
+            if (!\is_string($name) || $length > $constraint->maxLength || !ctype_alnum($name)) {
                 $this->context->buildViolation($constraint->invalidNameMessage)
                     ->setParameter('{{ name }}', $this->formatValue($name))
                     ->setParameter('{{ color }}', $this->formatValue($code))
-                    ->setParameter('{{ max }}', $this->formatValue(10))
+                    ->setParameter('{{ max }}', $this->formatValue($constraint->maxLength))
-                    ->setParameter('{{ count }}', $this->formatValue(\strlen($name)))
+                    ->setParameter('{{ count }}', $this->formatValue($length))
                     ->setCode(ColorChoices::COLOR_CHOICES_NAME_ERROR)
                     ->addViolation();
             }

tests/Configuration/SystemConfigurationTest.php

@@ -136,7 +136,7 @@ class SystemConfigurationTest extends TestCase
         $this->assertEquals(99, $sut->find('timesheet.active_entries.hard_limit'));
         $this->assertTrue($sut->find('theme.colors_limited'));
         $this->assertTrue($sut->isThemeColorsLimited());
-        $this->assertEquals(['Maroon' => '#800000', 'Brown' => '#a52a2a', 'Red' => '#ff0000', 'Orange' => '#ffa500', '#ffffff' => '#ffffff', '#000000' => '#000000'], $sut->getThemeColorChoices());
+        $this->assertEquals('Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,#ffffff,,|#000000', $sut->getThemeColorChoices());
     }
 
     public function testDefaultWithLoader()
@@ -148,6 +148,7 @@ class SystemConfigurationTest extends TestCase
         $this->assertEquals(7, $sut->find('timesheet.active_entries.hard_limit'));
         $this->assertFalse($sut->isSamlActive());
         $this->assertFalse($sut->find('theme.colors_limited'));
+        $this->assertEquals('Europe/London', $sut->default('defaults.customer.timezone'));
     }
 
     public function testDefaultWithMixedConfigs()
@@ -160,7 +161,7 @@ class SystemConfigurationTest extends TestCase
         ]);
         $this->assertFalse($sut->find('timesheet.rules.allow_future_times'));
         $this->assertTrue($sut->isSamlActive());
-        $this->assertNull($sut->getThemeColorChoices());
+        $this->assertEquals('Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,#ffffff,,|#000000', $sut->getThemeColorChoices());
         $this->assertEquals('2020-03-27', $sut->getFinancialYearStart());
     }
 

tests/DependencyInjection/AppExtensionTest.php

@@ -175,7 +175,7 @@ class AppExtensionTest extends TestCase
                     'background_color' => '#d2d6de',
                 ],
                 'colors_limited' => true,
-                'color_choices' => 'Kimai|#d2d6de,Silver|#c0c0c0,Gray|#808080,Black|#000000,Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,Gold|#ffd700,Yellow|#ffff00,Peach|#ffdab9,Khaki|#f0e68c,Olive|#808000,Lime|#00ff00,Jelly|#9acd32,Green|#008000,Teal|#008080,Aqua|#00ffff,LightBlue|#add8e6,DeepSky|#00bfff,Dodger|#1e90ff,Blue|#0000ff,Navy|#000080,Purple|#800080,Fuchsia|#ff00ff,Violet|#ee82ee,Rose|#ffe4e1,Lavender|#E6E6FA'
+                'color_choices' => 'Silver|#c0c0c0,Gray|#808080,Black|#000000,Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,Gold|#ffd700,Yellow|#ffff00,Peach|#ffdab9,Khaki|#f0e68c,Olive|#808000,Lime|#00ff00,Jelly|#9acd32,Green|#008000,Teal|#008080,Aqua|#00ffff,LightBlue|#add8e6,DeepSky|#00bfff,Dodger|#1e90ff,Blue|#0000ff,Navy|#000080,Purple|#800080,Fuchsia|#ff00ff,Violet|#ee82ee,Rose|#ffe4e1,Lavender|#E6E6FA'
             ],
             'kimai.theme.select_type' => 'selectpicker',
             'kimai.theme.show_about' => true,

tests/DependencyInjection/ConfigurationTest.php

@@ -371,7 +371,7 @@ class ConfigurationTest extends TestCase
                     'background_color' => '#d2d6de'
                 ],
                 'colors_limited' => true,
-                'color_choices' => 'Kimai|#d2d6de,Silver|#c0c0c0,Gray|#808080,Black|#000000,Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,Gold|#ffd700,Yellow|#ffff00,Peach|#ffdab9,Khaki|#f0e68c,Olive|#808000,Lime|#00ff00,Jelly|#9acd32,Green|#008000,Teal|#008080,Aqua|#00ffff,LightBlue|#add8e6,DeepSky|#00bfff,Dodger|#1e90ff,Blue|#0000ff,Navy|#000080,Purple|#800080,Fuchsia|#ff00ff,Violet|#ee82ee,Rose|#ffe4e1,Lavender|#E6E6FA'
+                'color_choices' => 'Silver|#c0c0c0,Gray|#808080,Black|#000000,Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,Gold|#ffd700,Yellow|#ffff00,Peach|#ffdab9,Khaki|#f0e68c,Olive|#808000,Lime|#00ff00,Jelly|#9acd32,Green|#008000,Teal|#008080,Aqua|#00ffff,LightBlue|#add8e6,DeepSky|#00bfff,Dodger|#1e90ff,Blue|#0000ff,Navy|#000080,Purple|#800080,Fuchsia|#ff00ff,Violet|#ee82ee,Rose|#ffe4e1,Lavender|#E6E6FA'
             ],
             'industry' => [
                 'translation' => null,

tests/Validator/Constraints/ColorChoicesValidatorTest.php

@@ -32,6 +32,8 @@ class ColorChoicesValidatorTest extends ConstraintValidatorTestCase
         yield ['#000aaa'];
         yield ['#fffaaa'];
         yield ['Foo|#fffaaa,|#fffaaa,#fffaaa,Bar|#fffaaa,'];
+        yield ['Fo o - sdsd|#fffaaa'];
+        yield ['abcdefghijklmnopqrst|#fffaaa'];
         yield [''];
         yield [null];
     }
@@ -56,9 +58,9 @@ class ColorChoicesValidatorTest extends ConstraintValidatorTestCase
 
     public function getInvalidColors()
     {
-        yield ['sdf sdf|#000000', null, 'sdf sdf', '#000000'];
+        yield ['sdf_sdf|#000000', null, 'sdf_sdf', '#000000'];
         yield ['sdfghjklöß.|#aaabbb', null, 'sdfghjklöß.', '#aaabbb'];
-        yield ['abcdefghijklmn|#aaabbb', null, 'abcdefghijklmn', '#aaabbb'];
+        yield ['abcdefghijklmnopqrstu|#aaabbb', null, 'abcdefghijklmnopqrstu', '#aaabbb'];
         yield ['string', 'string', null];
         yield ['000', '000', null];
         yield ['aaa', 'aaa', null];
@@ -94,10 +96,10 @@ class ColorChoicesValidatorTest extends ConstraintValidatorTestCase
         }
 
         if (null !== $invalidName) {
-            $this->buildViolation('The given value {{ name }} is not a valid color name for {{ color }}. Allowed are {{ max }} characters, given {{ count }}.')
+            $this->buildViolation('The given value {{ name }} is not a valid color name for {{ color }}. Allowed are {{ max }} alpha-numerical characters, including minus and space.')
                 ->setParameter('{{ color }}', '"' . ($invalidNameCode ?? $color) . '"')
-                ->setParameter('{{ max }}', '10')
+                ->setParameter('{{ max }}', (string) $constraint->maxLength)
-                ->setParameter('{{ count }}', (string) \strlen($invalidName))
+                ->setParameter('{{ count }}', (string) mb_strlen($invalidName))
                 ->setParameter('{{ name }}', '"' . $invalidName . '"')
                 ->setCode(ColorChoices::COLOR_CHOICES_NAME_ERROR)
                 ->assertRaised();