random improvements (#5506)

* fix order of destroying form elements * API documentation - fixes #1949 * make command path independent * use PhpSubprocess to execute migrations * allow to configure optional SAML attributes * fix deprecation * unify wording of exported state - fixes #5392
2025-05-28 13:34:01 +02:00
parent 81107377f4
commit 0420eb27c7
19 changed files with 150 additions and 63 deletions
--- a/assets/js/forms/KimaiTimesheetForm.js
+++ b/assets/js/forms/KimaiTimesheetForm.js
@@ -40,16 +40,18 @@ export default class KimaiTimesheetForm extends KimaiFormPlugin {

        if (this._beginTime !== undefined) {
            this._beginTime.removeEventListener('change', this._beginListener);
-            delete this._beginTime;
+            delete this._beginListener;
            this._beginTime.removeEventListener('blur', this._beginBlurListener);
            delete this._beginBlurListener;
+            delete this._beginTime;
        }

        if (this._endTime !== undefined) {
            this._endTime.removeEventListener('change', this._endListener);
-            delete this._endTime;
+            delete this._endListener;
            this._endTime.removeEventListener('blur', this._endBlurListener);
            delete this._endBlurListener;
+            delete this._endTime;
        }

        if (this._duration !== undefined) {
--- a/phpstan.neon
+++ b/phpstan.neon
@@ -145,7 +145,7 @@ parameters:

        -
            message: "#^Cannot call method find\\(\\) on Symfony\\\\Component\\\\Console\\\\Application\\|null\\.$#"
-            count: 2
+            count: 1
            path: src/Command/AbstractBundleInstallerCommand.php

        -
@@ -423,11 +423,6 @@ parameters:
            count: 1
            path: src/Command/ReloadCommand.php

-        -
-            message: "#^Cannot call method find\\(\\) on Symfony\\\\Component\\\\Console\\\\Application\\|null\\.$#"
-            count: 1
-            path: src/Command/ResetDevelopmentCommand.php
-
        -
            message: "#^Access to an undefined property SimpleXMLElement\\|false\\:\\:\\$file\\.$#"
            count: 1
--- a/public/build/app.4f0fd3e5.js
+++ b/public/build/app.4f0fd3e5.js
--- a/public/build/app.f98f56cc.js
+++ b/public/build/app.f98f56cc.js
--- a/public/build/app.f98f56cc.js.LICENSE.txt
+++ b/public/build/app.f98f56cc.js.LICENSE.txt
--- a/public/build/entrypoints.json
+++ b/public/build/entrypoints.json
@@ -3,7 +3,7 @@
    "app": {
      "js": [
        "/build/runtime.6c399d29.js",
-        "/build/app.4f0fd3e5.js"
+        "/build/app.f98f56cc.js"
      ],
      "css": [
        "/build/app.0416ea92.css"
@@ -72,7 +72,7 @@
  },
  "integrity": {
    "/build/runtime.6c399d29.js": "sha384-/rm616f12czi8l/27GvWXtb3g608vJZf2XTUKxqCRI4tsa2vUHP+BW90edTok5zC",
-    "/build/app.4f0fd3e5.js": "sha384-p1bPGT8nZ43+h+77R5zqIOwLx0RIESeYGq9x/3FxDd5V43nw2aF6263pbj9oa57M",
+    "/build/app.f98f56cc.js": "sha384-YWrfaZO6uvTBk94Q/zlqG56vaP68xEWYD1YunerAX1rMJcRMFSzi8fp7TVGeJm78",
    "/build/app.0416ea92.css": "sha384-JAIO6+B/vmV8IlpsmQ+zkfO4JsvdKTMsZZaDg6EMlZxkMZ5K3cpEK0mo24D/Wgkh",
    "/build/app-rtl.7a875ca7.js": "sha384-T7gLI61h9dGeMgzo63vKu4GiDOeLPct9zSUHrceNbhSwIdUmSSNoZ1+d7fKhJJ4/",
    "/build/app-rtl.0848906b.css": "sha384-O26Xw3P/NSea5iT6lt5v2RaZ6+jP06hf9vYD2pUJbkFSCLGU1iGXnobP25dWjUs/",
--- a/public/build/manifest.json
+++ b/public/build/manifest.json
@@ -1,6 +1,6 @@
 {
  "build/app.css": "/build/app.0416ea92.css",
-  "build/app.js": "/build/app.4f0fd3e5.js",
+  "build/app.js": "/build/app.f98f56cc.js",
  "build/app-rtl.css": "/build/app-rtl.0848906b.css",
  "build/app-rtl.js": "/build/app-rtl.7a875ca7.js",
  "build/export-pdf.css": "/build/export-pdf.d8a6c23b.css",
--- a/src/Command/AbstractBundleInstallerCommand.php
+++ b/src/Command/AbstractBundleInstallerCommand.php
@@ -18,6 +18,7 @@ use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Style\SymfonyStyle;
 use Symfony\Component\Filesystem\Exception\FileNotFoundException;
+use Symfony\Component\Process\PhpSubprocess;

 /**
 * Extend this class if you have a plugin that requires installation steps.
@@ -36,8 +37,7 @@ abstract class AbstractBundleInstallerCommand extends Command
    }

    /**
-     * If your bundle ships assets, that need to be available in the public/ directory,
-     * then overwrite this method and return: <true>.
+     * Return <true> if your bundle ships assets for the public/ directory.
     */
    protected function hasAssets(): bool
    {
@@ -189,13 +189,28 @@ abstract class AbstractBundleInstallerCommand extends Command
        // prevent windows from breaking
        $config = str_replace('/', DIRECTORY_SEPARATOR, $config);

-        $command = $this->getApplication()->find('doctrine:migrations:migrate');
-        $cmdInput = new ArrayInput(['--allow-no-migration' => true, '--configuration' => $config]);
-        $cmdInput->setInteractive(false);
-        if (0 !== $command->run($cmdInput, $output)) {
+        $process = new PhpSubprocess(
+            [
+                'bin/console',
+                'doctrine:migrations:migrate',
+                '--allow-no-migration',
+                '--no-interaction',
+                '--configuration=' . $config
+            ],
+            $this->getRootDirectory()
+        );
+        $process->run();
+
+        if (!$process->isSuccessful()) {
+            $io->error('Failed to install bundle database: ' . PHP_EOL . $config);
+            $io->error($process->getErrorOutput());
            throw new \Exception('Problem occurred while executing migrations.');
        }

+        if ($io->isVerbose()) {
+            $io->write($process->getOutput());
+        }
+
        $io->writeln('');
    }
 }
--- a/src/Command/AbstractResetCommand.php
+++ b/src/Command/AbstractResetCommand.php
@@ -116,7 +116,7 @@ abstract class AbstractResetCommand extends Command
        }

        try {
-            $command = $this->getApplication()->find('doctrine:query:sql');
+            $command = $this->getApplication()->find('dbal:run-sql');
            $command->run(new ArrayInput(['sql' => 'DROP TABLE IF EXISTS migration_versions']), $output);
        } catch (Exception $ex) {
            $io->error('Failed to drop migration_versions table: ' . $ex->getMessage());
@@ -125,7 +125,7 @@ abstract class AbstractResetCommand extends Command
        }

        try {
-            $command = $this->getApplication()->find('doctrine:query:sql');
+            $command = $this->getApplication()->find('dbal:run-sql');
            $command->run(new ArrayInput(['sql' => 'DROP TABLE IF EXISTS kimai2_sessions']), $output);
        } catch (Exception $ex) {
            $io->error('Failed to drop kimai2_sessions table: ' . $ex->getMessage());
--- a/src/Command/PluginCommand.php
+++ b/src/Command/PluginCommand.php
@@ -26,7 +26,8 @@ final class PluginCommand extends Command
 {
    public function __construct(
        private readonly PluginManager $pluginManager,
-        private readonly PackageManager $packageManager
+        private readonly PackageManager $packageManager,
+        private readonly string $projectDirectory
    )
    {
        parent::__construct();
@@ -69,17 +70,16 @@ final class PluginCommand extends Command
                }
            }

-            // using getApplication()->find('doctrine:migrations:migrate') does NOT work here
-            // because the Doctrine command can only be executed once
-            // if run more than once it fails with a "Container is frozen" exception
-
-            $process = new PhpSubprocess([
-                'bin/console',
-                'doctrine:migrations:migrate',
-                '--allow-no-migration',
-                '--no-interaction',
-                '--configuration=' . $config
-            ]);
+            $process = new PhpSubprocess(
+                [
+                    'bin/console',
+                    'doctrine:migrations:migrate',
+                    '--allow-no-migration',
+                    '--no-interaction',
+                    '--configuration=' . $config
+                ],
+                $this->projectDirectory
+            );
            $process->run();

            if (!$process->isSuccessful()) {
--- a/src/Command/ResetDevelopmentCommand.php
+++ b/src/Command/ResetDevelopmentCommand.php
@@ -10,9 +10,10 @@
 namespace App\Command;

 use Symfony\Component\Console\Attribute\AsCommand;
-use Symfony\Component\Console\Input\ArrayInput;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Console\Style\SymfonyStyle;
+use Symfony\Component\Process\PhpSubprocess;

 /**
 * Command used to execute all the basic application bootstrapping AFTER "composer install" was executed.
@@ -24,16 +25,36 @@ use Symfony\Component\Console\Output\OutputInterface;
 #[AsCommand(name: 'kimai:reset:dev', description: 'Resets the "development" environment')]
 final class ResetDevelopmentCommand extends AbstractResetCommand
 {
-    public function __construct(string $kernelEnvironment)
+    public function __construct(string $kernelEnvironment, private readonly string $projectDirectory)
    {
        parent::__construct($kernelEnvironment);
    }

    protected function loadData(InputInterface $input, OutputInterface $output): void
    {
-        $command = $this->getApplication()->find('doctrine:fixtures:load');
-        $cmdInput = new ArrayInput([]);
-        $cmdInput->setInteractive(false);
-        $command->run($cmdInput, $output);
+        $io = new SymfonyStyle($input, $output);
+
+        $io->writeln('Importing fixtures, this will take a while ... please be patient ...');
+
+        $process = new PhpSubprocess(
+            [
+                'bin/console',
+                'doctrine:fixtures:load',
+                '--no-interaction',
+            ],
+            $this->projectDirectory
+        );
+        $process->run();
+
+        if (!$process->isSuccessful()) {
+            $io->error('Failed to load fixtures');
+            $io->error($process->getErrorOutput());
+        } else {
+            if ($io->isVerbose()) {
+                $io->write($process->getOutput());
+            } else {
+                $io->success('Fixtures loaded');
+            }
+        }
    }
 }
--- a/src/Form/API/TimesheetApiEditForm.php
+++ b/src/Form/API/TimesheetApiEditForm.php
@@ -27,7 +27,11 @@ final class TimesheetApiEditForm extends TimesheetEditForm
            return;
        }

-        $builder->add('billable', BillableType::class);
+        $builder->add('billable', BillableType::class, [
+            'documentation' => [
+                'description' => 'If true, this timesheet will be flagged as billable'
+            ]
+        ]);

        $builder->addEventListener(
            FormEvents::PRE_SUBMIT,
@@ -56,6 +60,7 @@ final class TimesheetApiEditForm extends TimesheetEditForm
            $builder->remove('metaFields');
        }

+        // TODO this is only a quick fix, see bugs reports
        if ($builder->has('duration')) {
            $builder->remove('duration');
        }
@@ -64,11 +69,6 @@ final class TimesheetApiEditForm extends TimesheetEditForm
            $builder->get('user')->setRequired(false);
        }

-        // TODO this is only a quick fix, see bugs reports
-        if ($builder->has('duration')) {
-            $builder->remove('duration');
-        }
-
        if ($builder->has('tags')) {
            $builder->remove('tags');
            // @deprecated for BC reasons here, arrays will be supported in 2.0
@@ -82,6 +82,10 @@ final class TimesheetApiEditForm extends TimesheetEditForm
    {
        $builder->add('begin', DateTimeApiType::class, array_merge($dateTimeOptions, [
            'label' => 'begin',
+            'required' => false,
+            'documentation' => [
+                'description' => 'If no begin date-time is set, the users current timestamp will be used'
+            ]
        ]));
    }

@@ -90,6 +94,9 @@ final class TimesheetApiEditForm extends TimesheetEditForm
        $builder->add('end', DateTimeApiType::class, array_merge($dateTimeOptions, [
            'label' => 'end',
            'required' => false,
+            'documentation' => [
+                'description' => 'If no end date-time is set, the timesheet will be running'
+            ]
        ]));
    }

--- a/src/Form/MultiUpdate/TimesheetMultiUpdate.php
+++ b/src/Form/MultiUpdate/TimesheetMultiUpdate.php
@@ -183,8 +183,8 @@ final class TimesheetMultiUpdate extends AbstractType
                'label' => 'mark_as_exported',
                'required' => false,
                'choices' => [
-                    'entryState.exported' => true,
-                    'entryState.not_exported' => false
+                    'yes' => true,
+                    'no' => false
                ]
            ]);
        }
--- a/src/Form/TimesheetEditForm.php
+++ b/src/Form/TimesheetEditForm.php
@@ -43,7 +43,10 @@ class TimesheetEditForm extends AbstractType
 {
    use FormTrait;

-    public function __construct(private CustomerRepository $customers, private SystemConfiguration $systemConfiguration)
+    public function __construct(
+        private readonly CustomerRepository $customers,
+        private readonly SystemConfiguration $systemConfiguration
+    )
    {
    }

@@ -400,14 +403,17 @@ class TimesheetEditForm extends AbstractType
        }

        $builder->add('exported', YesNoType::class, [
-            'label' => 'exported'
+            'label' => 'exported',
+            'documentation' => [
+                'description' => 'If true, this timesheet will be flagged as being exported'
+            ]
        ]);
    }

    protected function addBillable(FormBuilderInterface $builder, array $options): void
    {
        if ($options['include_billable']) {
-            $builder->add('billableMode', TimesheetBillableType::class, []);
+            $builder->add('billableMode', TimesheetBillableType::class);
        }

        $builder->addModelTransformer(new CallbackTransformer(
--- a/src/Saml/SamlProvider.php
+++ b/src/Saml/SamlProvider.php
@@ -97,6 +97,9 @@ final class SamlProvider
            $field = $mapping['kimai'];
            $attribute = $mapping['saml'];
            $value = $this->getPropertyValue($token, $attribute);
+            if ($value === null) {
+                continue;
+            }
            $setter = 'set' . ucfirst($field);
            if (method_exists($user, $setter)) {
                $user->$setter($value);
@@ -117,7 +120,7 @@ final class SamlProvider
        $user->setAuth(User::AUTH_SAML);
    }

-    private function getPropertyValue(SamlLoginAttributes $token, $attribute): string
+    private function getPropertyValue(SamlLoginAttributes $token, $attribute): ?string
    {
        $results = [];
        $attributes = $token->getAttributes();
@@ -129,11 +132,16 @@ final class SamlProvider
            }
            if ($part[0] === '$') {
                $key = substr($part, 1);
-                if (!\array_key_exists($key, $attributes)) {
+                $optional = false;
+                if ($key[0] === '$') {
+                    $key = substr($key, 1);
+                    $optional = true;
+                }
+                if (!$optional && !\array_key_exists($key, $attributes)) {
                    throw new \RuntimeException('Missing SAML attribute in response: ' . $key);
                }

-                if (\is_array($attributes[$key]) && isset($attributes[$key][0])) {
+                if (\array_key_exists($key, $attributes) && \is_array($attributes[$key]) && isset($attributes[$key][0])) {
                    $results[] = $attributes[$key][0];
                }
            } else {
@@ -145,6 +153,6 @@ final class SamlProvider
            return implode(' ', $results);
        }

-        return $attribute;
+        return null;
    }
 }
--- a/templates/export/renderer/default.html.twig
+++ b/templates/export/renderer/default.html.twig
@@ -275,6 +275,10 @@
        {
            text-align: right;
        }
+        .items th.column-exported
+        {
+            text-align: center;
+        }
        .table>thead:first-child>tr:first-child>th {
            padding-right: 5px;
        }
@@ -668,12 +672,8 @@
                                {{ entry.description|desc2html }}
                            {% endif %}
                        </td>
-                        <td class="column-exported" {% if not columns.exported %}style="display: none"{% endif %}>
-                            {% if entry.exported %}
-                                {{ 'entryState.exported'|trans }}
-                            {% else %}
-                                {{ 'entryState.not_exported'|trans }}
-                            {% endif %}
+                        <td class="column-exported text-center" {% if not columns.exported %}style="display: none"{% endif %}>
+                            {{ widgets.label_boolean(entry.exported) }}
                        </td>
                        <td class="column-tags" {% if not columns.tags %}style="display: none"{% endif %}>
                            {% if entry.tags is defined and entry.tags is not empty %}
--- a/tests/Command/PluginCommandTest.php
+++ b/tests/Command/PluginCommandTest.php
@@ -40,9 +40,10 @@ class PluginCommandTest extends KernelTestCase

    private function getCommandTester(array $plugins, array $options = []): CommandTester
    {
+        $projectDirectory = __DIR__ . '/../../';
        $kernel = self::bootKernel();
        $this->application = new Application($kernel);
-        $this->application->add(new PluginCommand(new PluginManager($plugins), new PackageManager(__DIR__ . '/../../')));
+        $this->application->add(new PluginCommand(new PluginManager($plugins), new PackageManager($projectDirectory), $projectDirectory));

        $command = $this->application->find('kimai:plugins');
        $commandTester = new CommandTester($command);
--- a/tests/Command/ResetDevelopmentCommandTest.php
+++ b/tests/Command/ResetDevelopmentCommandTest.php
@@ -23,7 +23,7 @@ class ResetDevelopmentCommandTest extends KernelTestCase
    {
        $kernel = self::bootKernel();
        $application = new Application($kernel);
-        $application->add(new ResetDevelopmentCommand('dev'));
+        $application->add(new ResetDevelopmentCommand('dev', __DIR__ . '/../../'));

        self::assertTrue($application->has('kimai:reset:dev'));
        $command = $application->find('kimai:reset:dev');
@@ -32,7 +32,7 @@ class ResetDevelopmentCommandTest extends KernelTestCase

    public function testCommandNameIsNotEnabledInProd(): void
    {
-        $sut = new ResetDevelopmentCommand('prod');
+        $sut = new ResetDevelopmentCommand('prod', __DIR__ . '/../../');
        self::assertFalse($sut->isEnabled());
    }
 }
--- a/tests/Saml/SamlProviderTest.php
+++ b/tests/Saml/SamlProviderTest.php
@@ -122,4 +122,36 @@ class SamlProviderTest extends TestCase
        $sut = $this->getSamlProvider(null, $user);
        $sut->findUser($token);
    }
+
+    public function testAuthenticateNotThrowsOnOptionalAttribute(): void
+    {
+        $mapping = [
+            'mapping' => [
+                ['saml' => '$Chicken', 'kimai' => 'alias'],
+                ['saml' => '$$Email', 'kimai' => 'title'],
+            ],
+            'roles' => [
+                'attribute' => '',
+                'mapping' => []
+            ]
+        ];
+
+        $user = new User();
+        $user->setAuth(User::AUTH_SAML);
+        $user->setUserIdentifier('foo1@example.com');
+        $user->setTitle('I will not be overwritten');
+
+        $token = new SamlLoginAttributes();
+        $token->setUserIdentifier($user->getUserIdentifier());
+        $token->setAttributes([
+            'Chicken' => ['foo@example.com'],
+        ]);
+
+        $sut = $this->getSamlProvider($mapping, $user);
+        $tokenUser = $sut->findUser($token);
+        self::assertSame($user, $tokenUser);
+        self::assertTrue($tokenUser->isSamlUser());
+        self::assertEquals('foo@example.com', $tokenUser->getAlias());
+        self::assertEquals('I will not be overwritten', $tokenUser->getTitle());
+    }
 }