#201 - Fix minor defects

CHANGELOG.md

@@ -19,6 +19,7 @@ Latest
 * [#206](https://github.com/cleverage/process-bundle/issues/206) Fix AdvancedStatCounterTask, IterableBatchTask, ConditionTrait and ColumnAggregatorTask edge cases: log the first counted execution with correct counts, accept a `null` `batch_count` (only flush at the end), allow scalar inputs in conditions (`''` path on the whole value), aggregate `null` column values. Update documentation, add tests.
 * [#207](https://github.com/cleverage/process-bundle/issues/207) Fix JsonStreamReaderTask / JsonStreamWriterTask: throw an explicit `\UnexpectedValueException` when a line decodes to a scalar, create the missing parent directory when writing. Update documentation, add tests.
 * [#208](https://github.com/cleverage/process-bundle/issues/208) Fix FolderBrowserTask, InputFolderBrowserTask, CsvReaderTask and LineReaderTask (and their `Input*` variants): reset the state at the end of the iteration, so that a following input is read from its beginning. Update documentation, add tests.
+* [#201](https://github.com/cleverage/process-bundle/issues/201) Fix minor defects: error messages of TransformerTrait, RulesTransformer and ExpressionLanguageMapTransformer, useless `setRequired()` in ImplodeTransformer and SprintfTransformer, stray namespace in TrimTransformer, wrong or missing docblocks. Add tests.
 
 ## Deprecated
 * [#189](https://github.com/cleverage/process-bundle/issues/189) EventDispatcherTask: when `event_name` is set, listening to `CleverAge\ProcessBundle\Event\EventDispatcherTaskEvent` is deprecated (the event is still dispatched under its class name, with an `E_USER_DEPRECATED` error, if it has listeners). Listen to the configured `event_name` instead: the BC layer will be removed in v6.0.

src/Task/ColumnAggregatorTask.php

@@ -22,7 +22,8 @@
 use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
 
 /**
- * @todo   @vclavreul describe this task
+ * For each configured column, collect the input rows that contain this column (and match the optional condition), and
+ * output all the groups once all previous tasks are resolved.
  */
 class ColumnAggregatorTask extends AbstractConfigurableTask implements BlockingTaskInterface
 {

src/Task/FilterTask.php

@@ -21,7 +21,7 @@
 
 /**
  * Skip inputs under given matching conditions
- * - equality is softly checked
+ * - equality is strictly checked
  * - unexisting key is the same as null.
  */
 class FilterTask extends AbstractConfigurableTask

src/Task/Process/ProcessExecutorTask.php

@@ -23,7 +23,8 @@
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /**
- * Execute one or many processes while chaining inputs in a iterable way.
+ * Execute a process for each input: the input is passed to the process entry point, and the output of its end point
+ * becomes the task output.
  */
 class ProcessExecutorTask extends AbstractConfigurableTask
 {

src/Task/RowAggregatorTask.php

@@ -21,10 +21,9 @@
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /**
- * Wait for defined inputs before passing an aggregated output.
- * Should have been a BlockingTask, but due to limitations in the current model, it's a hack using skips and finalize.
- *
- * @see README.md:Known issues
+ * Group input rows sharing the same value for the "aggregate_by" column. Each group is made of the first received row
+ * (without the "aggregate_columns"), plus a sub-array under "aggregation_key" listing the "aggregate_columns" values of
+ * every row of the group. The groups are output once all previous tasks are resolved.
  */
 class RowAggregatorTask extends AbstractConfigurableTask implements BlockingTaskInterface
 {

src/Transformer/Date/DateFormatTransformer.php

@@ -17,8 +17,7 @@
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /**
- * Transformer aiming to take a date as an input (object or string) and format it according to options.
- * In input it takes any value understood by \DateTime.
+ * Transformer aiming to take a date object (\DateTimeInterface) as an input and format it according to options.
  *
  * @example in YML config
  * transformers:

src/Transformer/DefaultTransformer.php

@@ -16,7 +16,7 @@
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /**
- * @todo vclavreul comment this class
+ * Return a default value when the input is falsy, otherwise return the input unchanged.
  */
 class DefaultTransformer implements ConfigurableTransformerInterface
 {

src/Transformer/ExpressionLanguageMapTransformer.php

@@ -79,7 +79,8 @@ public function transform(mixed $value, array $options = []): mixed
             return $value;
         }
         if (!$options['ignore_missing']) {
-            throw new \UnexpectedValueException("No expression accepting value '{$value}' in map");
+            $printableValue = \is_scalar($value) || $value instanceof \Stringable ? (string) $value : get_debug_type($value);
+            throw new \UnexpectedValueException("No expression accepting value '{$printableValue}' in map");
         }
 
         return null;

src/Transformer/Object/RecursivePropertySetterTransformer.php

@@ -20,7 +20,8 @@
 use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
 
 /**
- * Read a property from the input value and return it.
+ * Read an iterable from the input, then set one or more properties on each of its items, using values read from the
+ * input itself.
  */
 class RecursivePropertySetterTransformer implements ConfigurableTransformerInterface
 {
@@ -44,26 +45,26 @@ public function transform(mixed $value, array $options = []): mixed
             throw new TransformerException($options['iterator']);
         }
 
-        $protertiesToSet = [];
+        $propertiesToSet = [];
         foreach ($options['set_properties'] as $propertyName => $propertyValuePath) {
-            $protertiesValue = null;
+            $propertyValue = null;
             if (!$options['ignore_missing'] || $this->accessor->isReadable($value, $propertyValuePath)) {
-                $protertiesValue = $this->accessor->getValue($value, $propertyValuePath);
-                if (null === $protertiesValue && !$options['ignore_null']) {
+                $propertyValue = $this->accessor->getValue($value, $propertyValuePath);
+                if (null === $propertyValue && !$options['ignore_null']) {
                     throw new TransformerException($propertyValuePath);
                 }
             }
-            $protertiesToSet[$propertyName] = $protertiesValue;
+            $propertiesToSet[$propertyName] = $propertyValue;
         }
 
         foreach ($iterable as &$item) {
-            foreach ($protertiesToSet as $protertyName => $propertyValue) {
+            foreach ($propertiesToSet as $propertyPath => $propertyValue) {
                 try {
-                    $this->accessor->setValue($item, $protertyName, $propertyValue);
+                    $this->accessor->setValue($item, $propertyPath, $propertyValue);
                 } catch (NoSuchPropertyException $e) {
                     if ($item instanceof \stdClass) {
                         $item = (object) array_merge((array) $item, [
-                            $protertyName => $propertyValue,
+                            $propertyPath => $propertyValue,
                         ]);
                     } else {
                         throw $e;

src/Transformer/RulesTransformer.php

@@ -76,7 +76,7 @@ public function configureOptions(OptionsResolver $resolver): void
             foreach ($rules as $rule) {
                 if ($rule['default']) {
                     if ($hasFoundDefault) {
-                        throw new \InvalidArgumentException('Rules set cannot have more than 2 default rules');
+                        throw new \InvalidArgumentException('Rules set cannot have more than one default rule');
                     }
                     $hasFoundDefault = true;
                 }

src/Transformer/String/ImplodeTransformer.php

@@ -23,7 +23,6 @@ class ImplodeTransformer implements ConfigurableTransformerInterface
 {
     public function configureOptions(OptionsResolver $resolver): void
     {
-        $resolver->setRequired('separator');
         $resolver->setDefault('separator', '|');
         $resolver->setAllowedTypes('separator', 'string');
     }

src/Transformer/String/SprintfTransformer.php

@@ -40,7 +40,6 @@ public function getCode(): string
      */
     public function configureOptions(OptionsResolver $resolver): void
     {
-        $resolver->setRequired('format');
         $resolver->setDefault('format', '%s');
         $resolver->setAllowedTypes('format', 'string');
     }

src/Transformer/String/TrimTransformer.php

@@ -11,8 +11,6 @@
  * file that was distributed with this source code.
  */
 
-namespace Transformer;
-
 namespace CleverAge\ProcessBundle\Transformer\String;
 
 use CleverAge\ProcessBundle\Transformer\ConfigurableTransformerInterface;

src/Transformer/TransformerTrait.php

@@ -39,7 +39,7 @@ public function normalizeTransformers(Options $options, array $transformers): ar
                 $transformer->configureOptions($transformerOptionsResolver);
                 $transformerOptions = $transformerOptionsResolver->resolve($transformerOptions);
             } elseif (!empty($transformerOptions)) {
-                throw new \InvalidArgumentException("Transformer {${$origTransformerCode}} should not have options");
+                throw new \InvalidArgumentException("Transformer {$origTransformerCode} should not have options");
             }
 
             $closure = static fn ($value) => $transformer->transform($value, $transformerOptions);

tests/Transformer/ExpressionLanguageMapTransformerTest.php

@@ -0,0 +1,70 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/ProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\ProcessBundle\Tests\Transformer;
+
+use CleverAge\ProcessBundle\Transformer\ExpressionLanguageMapTransformer;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
+use Symfony\Component\OptionsResolver\OptionsResolver;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(ExpressionLanguageMapTransformer::class)]
+class ExpressionLanguageMapTransformerTest extends TestCase
+{
+    public function testTransform(): void
+    {
+        $transformer = new ExpressionLanguageMapTransformer(new ExpressionLanguage());
+        $options = $this->resolveOptions($transformer);
+
+        $this->assertSame('answer', $transformer->transform(42, $options));
+    }
+
+    /**
+     * @return iterable<string, array{mixed, string}>
+     */
+    public static function missingValueProvider(): iterable
+    {
+        yield 'scalar' => [3, "No expression accepting value '3' in map"];
+        yield 'array' => [[3], "No expression accepting value 'array' in map"];
+        yield 'object' => [new \stdClass(), "No expression accepting value 'stdClass' in map"];
+        yield 'null' => [null, "No expression accepting value 'null' in map"];
+    }
+
+    #[DataProvider('missingValueProvider')]
+    public function testMissingValueMessage(mixed $value, string $expectedMessage): void
+    {
+        $transformer = new ExpressionLanguageMapTransformer(new ExpressionLanguage());
+        $options = $this->resolveOptions($transformer);
+
+        $this->expectException(\UnexpectedValueException::class);
+        $this->expectExceptionMessage($expectedMessage);
+
+        $transformer->transform($value, $options);
+    }
+
+    /**
+     * @return array<string, mixed>
+     */
+    private function resolveOptions(ExpressionLanguageMapTransformer $transformer): array
+    {
+        $resolver = new OptionsResolver();
+        $transformer->configureOptions($resolver);
+
+        return $resolver->resolve([
+            'map' => [
+                ['condition' => 'data === 42', 'output' => '"answer"'],
+            ],
+        ]);
+    }
+}

tests/Transformer/RulesTransformerTest.php

@@ -0,0 +1,93 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/ProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\ProcessBundle\Tests\Transformer;
+
+use CleverAge\ProcessBundle\Registry\TransformerRegistry;
+use CleverAge\ProcessBundle\Transformer\Array\ArrayLastTransformer;
+use CleverAge\ProcessBundle\Transformer\RulesTransformer;
+use CleverAge\ProcessBundle\Transformer\TransformerTrait;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
+use Symfony\Component\OptionsResolver\OptionsResolver;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(RulesTransformer::class)]
+#[\PHPUnit\Framework\Attributes\CoversTrait(TransformerTrait::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TransformerRegistry::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ArrayLastTransformer::class)]
+class RulesTransformerTest extends TestCase
+{
+    public function testTransform(): void
+    {
+        $transformer = $this->createTransformer();
+        $options = $this->resolveOptions($transformer, [
+            'rules_set' => [
+                ['condition' => 'value == "foo"', 'constant' => 'is foo'],
+                ['default' => true, 'constant' => 'is not foo'],
+            ],
+        ]);
+
+        $this->assertSame('is foo', $transformer->transform('foo', $options));
+        $this->assertSame('is not foo', $transformer->transform('bar', $options));
+    }
+
+    public function testRulesSetCannotHaveMoreThanOneDefaultRule(): void
+    {
+        $transformer = $this->createTransformer();
+
+        $this->expectException(\InvalidArgumentException::class);
+        $this->expectExceptionMessage('Rules set cannot have more than one default rule');
+
+        $this->resolveOptions($transformer, [
+            'rules_set' => [
+                ['default' => true, 'constant' => 'first'],
+                ['default' => true, 'constant' => 'second'],
+            ],
+        ]);
+    }
+
+    public function testNonConfigurableTransformerCannotHaveOptions(): void
+    {
+        $transformer = $this->createTransformer();
+
+        $this->expectException(\InvalidArgumentException::class);
+        $this->expectExceptionMessage('Transformer array_last should not have options');
+
+        $this->resolveOptions($transformer, [
+            'rules_set' => [
+                ['default' => true, 'transformers' => ['array_last' => ['foo' => 'bar']]],
+            ],
+        ]);
+    }
+
+    private function createTransformer(): RulesTransformer
+    {
+        $registry = new TransformerRegistry();
+        $registry->addTransformer(new ArrayLastTransformer());
+
+        return new RulesTransformer($registry, new ExpressionLanguage());
+    }
+
+    /**
+     * @param array<string, mixed> $options
+     *
+     * @return array<string, mixed>
+     */
+    private function resolveOptions(RulesTransformer $transformer, array $options): array
+    {
+        $resolver = new OptionsResolver();
+        $transformer->configureOptions($resolver);
+
+        return $resolver->resolve($options);
+    }
+}

tests/Transformer/String/ImplodeTransformerTest.php

@@ -66,7 +66,7 @@ public function testConfigureOptions(): void
 
         $transformer->configureOptions($resolver);
 
-        $this->assertTrue($resolver->isRequired('separator'));
+        $this->assertFalse($resolver->isRequired('separator'));
 
         $resolvedOptions = $resolver->resolve();
         $this->assertEquals(['separator'], array_keys($resolvedOptions));