From 704498f88f05bc3d965d727842bab9d4cc0c454c Mon Sep 17 00:00:00 2001 From: Nicolas Joubert Date: Mon, 28 Sep 2026 12:04:17 +0200 Subject: [PATCH 1/2] chore(cleanup) #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. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 1 + src/Task/ColumnAggregatorTask.php | 3 +- src/Task/FilterTask.php | 2 +- src/Task/Process/ProcessExecutorTask.php | 3 +- src/Task/RowAggregatorTask.php | 7 +- .../Date/DateFormatTransformer.php | 3 +- src/Transformer/DefaultTransformer.php | 2 +- .../ExpressionLanguageMapTransformer.php | 3 +- .../RecursivePropertySetterTransformer.php | 19 ++-- src/Transformer/RulesTransformer.php | 2 +- src/Transformer/String/ImplodeTransformer.php | 1 - src/Transformer/String/SprintfTransformer.php | 1 - src/Transformer/String/TrimTransformer.php | 2 - src/Transformer/TransformerTrait.php | 2 +- .../ExpressionLanguageMapTransformerTest.php | 70 ++++++++++++++ tests/Transformer/RulesTransformerTest.php | 91 +++++++++++++++++++ .../String/ImplodeTransformerTest.php | 2 +- 17 files changed, 187 insertions(+), 27 deletions(-) create mode 100644 tests/Transformer/ExpressionLanguageMapTransformerTest.php create mode 100644 tests/Transformer/RulesTransformerTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e918fb94..3f9ccb96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Latest * [#192](https://github.com/cleverage/process-bundle/issues/192) Fix CommandRunnerTask: only pass the `options` option to `Process::setOptions()`, support string `commandline` through `Process::fromShellCommandline()`, validate option types. Update documentation, add tests. * [#194](https://github.com/cleverage/process-bundle/issues/194) Fix ProcessLauncherTask: the `process_options` normalizer returned an array despite its scalar return type, so the task always failed with a `TypeError`. Update documentation, add tests. * [#189](https://github.com/cleverage/process-bundle/issues/189) Fix EventDispatcherTask: dispatch the event under the `event_name` option (regression since v4.0). `event_name` is now optional: when `null` (default), the event is dispatched under its class name. 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. diff --git a/src/Task/ColumnAggregatorTask.php b/src/Task/ColumnAggregatorTask.php index d08bfe06..fea605fd 100644 --- a/src/Task/ColumnAggregatorTask.php +++ b/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 { diff --git a/src/Task/FilterTask.php b/src/Task/FilterTask.php index 290ba13e..1bbb3b37 100644 --- a/src/Task/FilterTask.php +++ b/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 diff --git a/src/Task/Process/ProcessExecutorTask.php b/src/Task/Process/ProcessExecutorTask.php index 6775e6c5..64a0424c 100644 --- a/src/Task/Process/ProcessExecutorTask.php +++ b/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 { diff --git a/src/Task/RowAggregatorTask.php b/src/Task/RowAggregatorTask.php index c43317d3..7e5c85be 100644 --- a/src/Task/RowAggregatorTask.php +++ b/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 { diff --git a/src/Transformer/Date/DateFormatTransformer.php b/src/Transformer/Date/DateFormatTransformer.php index e60324a0..ba95e3d0 100644 --- a/src/Transformer/Date/DateFormatTransformer.php +++ b/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: diff --git a/src/Transformer/DefaultTransformer.php b/src/Transformer/DefaultTransformer.php index 96a847be..83628fe9 100644 --- a/src/Transformer/DefaultTransformer.php +++ b/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 { diff --git a/src/Transformer/ExpressionLanguageMapTransformer.php b/src/Transformer/ExpressionLanguageMapTransformer.php index 0356e9a3..186231e7 100644 --- a/src/Transformer/ExpressionLanguageMapTransformer.php +++ b/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; diff --git a/src/Transformer/Object/RecursivePropertySetterTransformer.php b/src/Transformer/Object/RecursivePropertySetterTransformer.php index b42c1df7..9bf7c0d4 100644 --- a/src/Transformer/Object/RecursivePropertySetterTransformer.php +++ b/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; diff --git a/src/Transformer/RulesTransformer.php b/src/Transformer/RulesTransformer.php index 7ef7d4e2..c9e5fbbd 100644 --- a/src/Transformer/RulesTransformer.php +++ b/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; } diff --git a/src/Transformer/String/ImplodeTransformer.php b/src/Transformer/String/ImplodeTransformer.php index e005c67c..7257ac13 100644 --- a/src/Transformer/String/ImplodeTransformer.php +++ b/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'); } diff --git a/src/Transformer/String/SprintfTransformer.php b/src/Transformer/String/SprintfTransformer.php index 7013c72b..a3263615 100644 --- a/src/Transformer/String/SprintfTransformer.php +++ b/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'); } diff --git a/src/Transformer/String/TrimTransformer.php b/src/Transformer/String/TrimTransformer.php index 95627dd6..e2d97819 100644 --- a/src/Transformer/String/TrimTransformer.php +++ b/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; diff --git a/src/Transformer/TransformerTrait.php b/src/Transformer/TransformerTrait.php index 4bea4f87..c63c1d17 100644 --- a/src/Transformer/TransformerTrait.php +++ b/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); diff --git a/tests/Transformer/ExpressionLanguageMapTransformerTest.php b/tests/Transformer/ExpressionLanguageMapTransformerTest.php new file mode 100644 index 00000000..3554235e --- /dev/null +++ b/tests/Transformer/ExpressionLanguageMapTransformerTest.php @@ -0,0 +1,70 @@ +resolveOptions($transformer); + + $this->assertSame('answer', $transformer->transform(42, $options)); + } + + /** + * @return iterable + */ + 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 + */ + private function resolveOptions(ExpressionLanguageMapTransformer $transformer): array + { + $resolver = new OptionsResolver(); + $transformer->configureOptions($resolver); + + return $resolver->resolve([ + 'map' => [ + ['condition' => 'data === 42', 'output' => '"answer"'], + ], + ]); + } +} diff --git a/tests/Transformer/RulesTransformerTest.php b/tests/Transformer/RulesTransformerTest.php new file mode 100644 index 00000000..67d1a20c --- /dev/null +++ b/tests/Transformer/RulesTransformerTest.php @@ -0,0 +1,91 @@ +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 $options + * + * @return array + */ + private function resolveOptions(RulesTransformer $transformer, array $options): array + { + $resolver = new OptionsResolver(); + $transformer->configureOptions($resolver); + + return $resolver->resolve($options); + } +} diff --git a/tests/Transformer/String/ImplodeTransformerTest.php b/tests/Transformer/String/ImplodeTransformerTest.php index d15c75e0..32ecfd83 100644 --- a/tests/Transformer/String/ImplodeTransformerTest.php +++ b/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)); From 7b7d7e7c345d8abefb9b5b6d687a454aeb353b62 Mon Sep 17 00:00:00 2001 From: Nicolas Joubert Date: Mon, 28 Sep 2026 13:59:04 +0200 Subject: [PATCH 2/2] test #201 Declare the classes used by the new tests (UsesClass), fixing risky tests when coverage is enabled Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/Transformer/RulesTransformerTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/Transformer/RulesTransformerTest.php b/tests/Transformer/RulesTransformerTest.php index 67d1a20c..d2499509 100644 --- a/tests/Transformer/RulesTransformerTest.php +++ b/tests/Transformer/RulesTransformerTest.php @@ -23,6 +23,8 @@ #[\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