From 30d9eedeca554c7bd94de7adc4c6fa9f7ecb8662 Mon Sep 17 00:00:00 2001 From: Nicolas Joubert Date: Fri, 9 Oct 2026 12:02:26 +0200 Subject: [PATCH] fix(task) #143 Type the remaining untyped task options: split_character of CsvWriterTask and SplitJoinLineTask must be a string, write_headers of CsvWriterTask and log_empty_lines of CsvReaderTask are cast to bool Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 1 + docs/reference/tasks/csv_reader_task.md | 2 +- docs/reference/tasks/csv_writer_task.md | 2 +- src/Task/File/Csv/CsvReaderTask.php | 3 ++ src/Task/File/Csv/CsvWriterTask.php | 3 ++ src/Task/SplitJoinLineTask.php | 1 + tests/OptionAllowedTypesTest.php | 45 ++++++++++++++++++++++++- 7 files changed, 54 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c761d9a1..f88789dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ Latest * [#243](https://github.com/cleverage/process-bundle/issues/243) Fix RecursivePropertySetterTransformer: a `\stdClass` item without the property was replaced by a copy in the output, so the input object was not modified; the property is now added to the item itself. Update documentation, add tests. * [#242](https://github.com/cleverage/process-bundle/issues/242) Fix InputFileReaderTask: an input that is not a non-empty string (e.g. `null`) throws an explicit `\UnexpectedValueException` (`No file path given as input`) instead of a PHP warning followed by a `TypeError`. Update documentation, add tests. * [#244](https://github.com/cleverage/process-bundle/issues/244) Fix MappingTransformer: a missing target property of a `\stdClass` destination (`initial_value` or `keep_input`) threw `Property '...' is not writable`, it is now added when the target is a simple property name (nested paths still throw). Update documentation, add tests. +* [#143](https://github.com/cleverage/process-bundle/issues/143) Type the remaining untyped task options: `split_character` of CsvWriterTask and SplitJoinLineTask must be a `string` (a wrong type used to fail later with a `TypeError`), `write_headers` of CsvWriterTask and `log_empty_lines` of CsvReaderTask are cast to `bool` (any value used to be evaluated as a boolean, so it is still accepted). Update documentation, add tests. v5.1 ----- diff --git a/docs/reference/tasks/csv_reader_task.md b/docs/reference/tasks/csv_reader_task.md index 664321f2..4e85c2cc 100644 --- a/docs/reference/tasks/csv_reader_task.md +++ b/docs/reference/tasks/csv_reader_task.md @@ -35,7 +35,7 @@ Options | `escape` | `string` | | `\` | CSV escape character | | `headers` | `array\|null` | | `null` | Static list of CSV headers. If `null`, headers are read from the first line of the file; otherwise the first line is read as data | | `mode` | `string` | | `rb` | File open mode (see [fopen mode parameter](https://www.php.net/manual/en/function.fopen.php)) | -| `log_empty_lines` | `bool` | | `false` | Log a warning when a line cannot be read (empty line) | +| `log_empty_lines` | `bool` | | `false` | Log a warning when a line cannot be read (empty line); cast to `bool` | Examples -------- diff --git a/docs/reference/tasks/csv_writer_task.md b/docs/reference/tasks/csv_writer_task.md index 22c08eb6..c4ffb382 100644 --- a/docs/reference/tasks/csv_writer_task.md +++ b/docs/reference/tasks/csv_writer_task.md @@ -34,7 +34,7 @@ Options | `headers` | `array\|null` | | `null` | Static list of CSV headers. If `null`, the keys of the first input are used | | `mode` | `string` | | `wb` | File open mode (see [fopen mode parameter](https://www.php.net/manual/en/function.fopen.php)) | | `split_character` | `string` | | `\|` | Used to implode array values | -| `write_headers` | `bool` | | `true` | Write the headers as first line, only if the file is empty (useful with an append `mode`) | +| `write_headers` | `bool` | | `true` | Write the headers as first line, only if the file is empty (useful with an append `mode`); cast to `bool` | Examples -------- diff --git a/src/Task/File/Csv/CsvReaderTask.php b/src/Task/File/Csv/CsvReaderTask.php index f976ddca..69a54211 100644 --- a/src/Task/File/Csv/CsvReaderTask.php +++ b/src/Task/File/Csv/CsvReaderTask.php @@ -17,6 +17,7 @@ use CleverAge\ProcessBundle\Model\IterableTaskInterface; use CleverAge\ProcessBundle\Model\ProcessState; use Psr\Log\LoggerInterface; +use Symfony\Component\OptionsResolver\Options; use Symfony\Component\OptionsResolver\OptionsResolver; /** @@ -105,5 +106,7 @@ protected function configureOptions(OptionsResolver $resolver): void $resolver->setDefaults([ 'log_empty_lines' => false, ]); + // Any value used to be evaluated as a boolean: cast it instead of rejecting it + $resolver->setNormalizer('log_empty_lines', static fn (Options $options, mixed $value): bool => (bool) $value); } } diff --git a/src/Task/File/Csv/CsvWriterTask.php b/src/Task/File/Csv/CsvWriterTask.php index 8c150a12..2e36324c 100644 --- a/src/Task/File/Csv/CsvWriterTask.php +++ b/src/Task/File/Csv/CsvWriterTask.php @@ -52,6 +52,9 @@ protected function configureOptions(OptionsResolver $resolver): void 'split_character' => '|', 'write_headers' => true, ]); + $resolver->setAllowedTypes('split_character', ['string']); + // Any value used to be evaluated as a boolean: cast it instead of rejecting it + $resolver->setNormalizer('write_headers', static fn (Options $options, mixed $value): bool => (bool) $value); $resolver->setNormalizer( 'file_path', diff --git a/src/Task/SplitJoinLineTask.php b/src/Task/SplitJoinLineTask.php index 9b6a614a..91eb06a1 100644 --- a/src/Task/SplitJoinLineTask.php +++ b/src/Task/SplitJoinLineTask.php @@ -40,6 +40,7 @@ protected function configureOptions(OptionsResolver $resolver): void $resolver->setDefaults([ 'split_character' => ',', ]); + $resolver->setAllowedTypes('split_character', ['string']); } protected function initializeIterator(ProcessState $state): \Iterator diff --git a/tests/OptionAllowedTypesTest.php b/tests/OptionAllowedTypesTest.php index 67d8777f..53e51e80 100644 --- a/tests/OptionAllowedTypesTest.php +++ b/tests/OptionAllowedTypesTest.php @@ -19,17 +19,21 @@ use CleverAge\ProcessBundle\Model\AbstractConfigurableTask; use CleverAge\ProcessBundle\Model\ProcessHistory; use CleverAge\ProcessBundle\Model\ProcessState; +use CleverAge\ProcessBundle\Task\File\Csv\CsvReaderTask; +use CleverAge\ProcessBundle\Task\File\Csv\CsvWriterTask; use CleverAge\ProcessBundle\Task\ObjectUpdaterTask; use CleverAge\ProcessBundle\Task\Serialization\DeserializerTask; use CleverAge\ProcessBundle\Task\Serialization\NormalizerTask; use CleverAge\ProcessBundle\Task\Serialization\SerializerTask; use CleverAge\ProcessBundle\Task\SimpleBatchTask; +use CleverAge\ProcessBundle\Task\SplitJoinLineTask; use CleverAge\ProcessBundle\Transformer\Array\ArrayFilterTransformer; use CleverAge\ProcessBundle\Transformer\ConditionTrait; use CleverAge\ProcessBundle\Transformer\ConfigurableTransformerInterface; use CleverAge\ProcessBundle\Transformer\String\HashTransformer; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; +use Psr\Log\NullLogger; use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException; use Symfony\Component\OptionsResolver\OptionsResolver; use Symfony\Component\PropertyAccess\PropertyAccess; @@ -47,6 +51,9 @@ #[\PHPUnit\Framework\Attributes\CoversClass(SerializerTask::class)] #[\PHPUnit\Framework\Attributes\CoversClass(DeserializerTask::class)] #[\PHPUnit\Framework\Attributes\CoversClass(ObjectUpdaterTask::class)] +#[\PHPUnit\Framework\Attributes\CoversClass(CsvReaderTask::class)] +#[\PHPUnit\Framework\Attributes\CoversClass(CsvWriterTask::class)] +#[\PHPUnit\Framework\Attributes\CoversClass(SplitJoinLineTask::class)] #[\PHPUnit\Framework\Attributes\UsesClass(AbstractConfigurableTask::class)] #[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)] #[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)] @@ -107,6 +114,8 @@ public static function invalidTaskOptionsProvider(): iterable yield 'serializer context' => [SerializerTask::class, ['format' => 'json', 'context' => 'groups']]; yield 'deserializer context' => [DeserializerTask::class, ['type' => 'array', 'format' => 'json', 'context' => 'groups']]; yield 'object updater property_path' => [ObjectUpdaterTask::class, ['property_path' => ['name']]]; + yield 'csv writer split_character' => [CsvWriterTask::class, ['file_path' => 'file.csv', 'split_character' => 1]]; + yield 'split join line split_character' => [SplitJoinLineTask::class, ['split_columns' => [], 'join_column' => 'value', 'split_character' => [',']]]; } /** @@ -133,6 +142,35 @@ public static function validTaskOptionsProvider(): iterable yield 'deserializer context' => [DeserializerTask::class, ['type' => 'array', 'format' => 'json', 'context' => []]]; yield 'object updater string property_path' => [ObjectUpdaterTask::class, ['property_path' => 'name']]; yield 'object updater PropertyPath property_path' => [ObjectUpdaterTask::class, ['property_path' => new PropertyPath('name')]]; + yield 'csv writer split_character' => [CsvWriterTask::class, ['file_path' => 'file.csv', 'split_character' => ';']]; + yield 'split join line split_character' => [SplitJoinLineTask::class, ['split_columns' => [], 'join_column' => 'value', 'split_character' => ';']]; + } + + /** + * @return iterable, array, string, bool}> + */ + public static function booleanTaskOptionsProvider(): iterable + { + yield 'csv reader log_empty_lines true' => [CsvReaderTask::class, ['file_path' => 'file.csv', 'log_empty_lines' => true], 'log_empty_lines', true]; + yield 'csv reader log_empty_lines 1' => [CsvReaderTask::class, ['file_path' => 'file.csv', 'log_empty_lines' => 1], 'log_empty_lines', true]; + yield 'csv reader log_empty_lines empty string' => [CsvReaderTask::class, ['file_path' => 'file.csv', 'log_empty_lines' => ''], 'log_empty_lines', false]; + yield 'csv writer write_headers false' => [CsvWriterTask::class, ['file_path' => 'file.csv', 'write_headers' => false], 'write_headers', false]; + yield 'csv writer write_headers 0' => [CsvWriterTask::class, ['file_path' => 'file.csv', 'write_headers' => 0], 'write_headers', false]; + yield 'csv writer write_headers yes' => [CsvWriterTask::class, ['file_path' => 'file.csv', 'write_headers' => 'yes'], 'write_headers', true]; + } + + /** + * Boolean options used to accept any value evaluated as a boolean: it is cast instead of being rejected. + * + * @param class-string $class + * @param array $options + */ + #[DataProvider('booleanTaskOptionsProvider')] + public function testBooleanTaskOptionIsCast(string $class, array $options, string $option, bool $expected): void + { + [$task, $state] = $this->initializeTask($class, $options); + + self::assertSame($expected, (new \ReflectionMethod($task, 'getOption'))->invoke($task, $state, $option)); } /** @@ -167,12 +205,15 @@ private function resolveTransformerOptions(string $class, array $options): array /** * @param class-string $class * @param array $options + * + * @return array{AbstractConfigurableTask, ProcessState} */ - private function initializeTask(string $class, array $options): void + private function initializeTask(string $class, array $options): array { $task = match ($class) { NormalizerTask::class, SerializerTask::class, DeserializerTask::class => new $class(new Serializer()), ObjectUpdaterTask::class => new ObjectUpdaterTask(PropertyAccess::createPropertyAccessor()), + CsvReaderTask::class => new CsvReaderTask(new NullLogger()), default => new $class(), }; @@ -182,5 +223,7 @@ private function initializeTask(string $class, array $options): void $state->setContext([]); $state->setTaskConfiguration(new TaskConfiguration('task', $class, $options)); $task->initialize($state); + + return [$task, $state]; } }