diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b61bd84..c59e7136 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ Latest * [#221](https://github.com/cleverage/process-bundle/issues/221) Fix CsvSplitterTask: each produced file contains `max_lines` data lines (instead of `max_lines - 2`), no infinite loop with `max_lines` <= 2 (`max_lines` must now be an integer greater than 0), no header-only file at the end. Update documentation, add tests. * [#223](https://github.com/cleverage/process-bundle/issues/223) Fix CounterTask: the final count is outputted once, as `flush()` may be called several times. Document that `flush()` implementations must be idempotent. Update documentation, add tests. * [#228](https://github.com/cleverage/process-bundle/issues/228) Add the missing `setAllowedTypes()` on HashTransformer (`raw_output`), SimpleBatchTask (`batch_count`), ConditionTrait (`empty`, `not_empty`), NormalizerTask / SerializerTask / DeserializerTask (`context`) and ObjectUpdaterTask (`property_path`): a wrong type is now reported when the options are resolved. Add tests. +* [#232](https://github.com/cleverage/process-bundle/issues/232) Fix edge cases looping forever or failing with a `TypeError`: validate FileSplitterTask `max_lines` (greater than 0, also when given as input), catch any `\Throwable` in PropertySetterTask, type SlugifyTransformer options, explicit exceptions in InputFolderBrowserTask (no folder path) and `XmlFile::write()` (`saveXML()` failure). Update documentation, 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/docs/reference/tasks/file_splitter_task.md b/docs/reference/tasks/file_splitter_task.md index 761c4b5e..a8881e0f 100644 --- a/docs/reference/tasks/file_splitter_task.md +++ b/docs/reference/tasks/file_splitter_task.md @@ -25,10 +25,10 @@ next `max_lines` lines of the source file (the last file may contain fewer lines Options ------- -| Code | Type | Required | Default | Description | -|-------------|----------|:--------:|---------|-------------------------------------------| -| `file_path` | `string` | **X** | | Path of the file to split | -| `max_lines` | `int` | | `1000` | Maximum number of lines per produced file | +| Code | Type | Required | Default | Description | +|-------------|----------|:--------:|---------|-------------------------------------------------------------------| +| `file_path` | `string` | **X** | | Path of the file to split | +| `max_lines` | `int` | | `1000` | Maximum number of lines per produced file, must be greater than 0 | Examples -------- @@ -48,7 +48,7 @@ read_chunk: Notes ----- -* Values given as input are merged after option resolution, so they are not validated. +* Values given as input (`file_path`, `max_lines`) are validated like the options; other input keys are ignored. * Every line of the source file is kept, in order, including empty lines. Line content is preserved, but each line break (`\n` or `\r\n`) is written as `PHP_EOL`, and a missing line break on the last line is added. * An empty source file produces no output (the task is skipped). diff --git a/docs/reference/tasks/input_folder_browser_task.md b/docs/reference/tasks/input_folder_browser_task.md index ac146c93..9d21bddc 100644 --- a/docs/reference/tasks/input_folder_browser_task.md +++ b/docs/reference/tasks/input_folder_browser_task.md @@ -19,7 +19,8 @@ directory. The folder path is released once its files have all been iterated, so each input (the same path again or a different one) is browsed from the start. Receiving a different folder path while an iteration is in progress throws a -`\LogicException`. +`\LogicException`, and an empty input (`null`, `''`) when no folder is being browsed throws an +`\UnexpectedValueException`. Possible outputs ---------------- diff --git a/docs/reference/tasks/property_setter_task.md b/docs/reference/tasks/property_setter_task.md index 028379b2..1fcc7c83 100644 --- a/docs/reference/tasks/property_setter_task.md +++ b/docs/reference/tasks/property_setter_task.md @@ -21,7 +21,8 @@ Possible outputs The input, with the configured values set. -If a value cannot be set, the exception is set on the state (with `property` and `value` added to the error context) +If a value cannot be set (including a `\TypeError` of the property accessor, e.g. on a scalar input), the exception +is set on the state (with `property` and `value` added to the error context) and handled according to the task `error_strategy`; the remaining values are not set. Options diff --git a/docs/reference/tasks/xml_writer_task.md b/docs/reference/tasks/xml_writer_task.md index 33e8fc28..8064ed57 100644 --- a/docs/reference/tasks/xml_writer_task.md +++ b/docs/reference/tasks/xml_writer_task.md @@ -43,3 +43,4 @@ Notes ----- * The file is opened on each execution: with the default `wb` mode, each input overwrites the file. +* If the XML content cannot be generated (`\DOMDocument::saveXML()` fails) or written, a `\RuntimeException` is thrown. diff --git a/docs/reference/transformers/slugify_transformer.md b/docs/reference/transformers/slugify_transformer.md index e3441c68..063fc7b1 100644 --- a/docs/reference/transformers/slugify_transformer.md +++ b/docs/reference/transformers/slugify_transformer.md @@ -55,6 +55,6 @@ slugify: Notes ----- -An invalid `transliterator` identifier (rejected by `\Transliterator::create()`) raises an -`InvalidOptionsException` when the options are resolved, i.e. when the transformer is configured, not on the first -transformed value. +An invalid `transliterator` identifier (rejected by `\Transliterator::create()`), or an option that is not a string, +raises an `InvalidOptionsException` when the options are resolved, i.e. when the transformer is configured, not on the +first transformed value. diff --git a/src/Filesystem/XmlFile.php b/src/Filesystem/XmlFile.php index 9bb9bca8..4fecd2f1 100644 --- a/src/Filesystem/XmlFile.php +++ b/src/Filesystem/XmlFile.php @@ -67,6 +67,9 @@ public function read(): \DOMDocument public function write(\DOMDocument $dom): void { $content = $dom->saveXML(); + if (false === $content) { + throw new \RuntimeException('Could not generate the XML content'); + } $result = $this->file->fwrite($content); if (false === $result) { diff --git a/src/Task/File/FileSplitterTask.php b/src/Task/File/FileSplitterTask.php index c159478e..b455f8d3 100644 --- a/src/Task/File/FileSplitterTask.php +++ b/src/Task/File/FileSplitterTask.php @@ -98,6 +98,7 @@ protected function configureOptions(OptionsResolver $resolver): void 'max_lines' => 1000, ]); $resolver->setAllowedTypes('max_lines', ['int']); + $resolver->setAllowedValues('max_lines', static fn (int $value): bool => $value >= 1); } /** @@ -115,7 +116,11 @@ protected function getMergedOptions(ProcessState $state): array } // @var array $input - return array_merge($options, $input); + // Options given as input are validated like the task options + $resolver = new OptionsResolver(); + $this->configureOptions($resolver); + + return $resolver->resolve(array_merge($options, array_intersect_key($input, $options))); } private function stripLineBreak(string $line): string diff --git a/src/Task/File/InputFolderBrowserTask.php b/src/Task/File/InputFolderBrowserTask.php index 1283529a..6a819f01 100644 --- a/src/Task/File/InputFolderBrowserTask.php +++ b/src/Task/File/InputFolderBrowserTask.php @@ -73,6 +73,9 @@ protected function getOptions(ProcessState $state): array $this->folderPath = $folderPath; } + if (null === $this->folderPath) { + throw new \UnexpectedValueException('No folder path given as input'); + } if (!is_dir($this->folderPath)) { throw new InvalidConfigurationException("Folder path does not exists or is not a folder: '{$this->folderPath}'"); } diff --git a/src/Task/PropertySetterTask.php b/src/Task/PropertySetterTask.php index 2117b4c9..9fcb767d 100644 --- a/src/Task/PropertySetterTask.php +++ b/src/Task/PropertySetterTask.php @@ -37,7 +37,7 @@ public function execute(ProcessState $state): void foreach ($options['values'] as $key => $value) { try { $this->accessor->setValue($input, $key, $value); - } catch (\Exception $e) { + } catch (\Throwable $e) { $state->addErrorContextValue('property', $key); $state->addErrorContextValue('value', $value); $state->setException($e); diff --git a/src/Transformer/String/SlugifyTransformer.php b/src/Transformer/String/SlugifyTransformer.php index 22dcfd64..7b682905 100644 --- a/src/Transformer/String/SlugifyTransformer.php +++ b/src/Transformer/String/SlugifyTransformer.php @@ -56,6 +56,9 @@ public function configureOptions(OptionsResolver $resolver): void 'separator' => '_', ] ); + $resolver->setAllowedTypes('transliterator', ['string']); + $resolver->setAllowedTypes('replace', ['string']); + $resolver->setAllowedTypes('separator', ['string']); $resolver->setNormalizer( 'transliterator', diff --git a/tests/Filesystem/XmlFileTest.php b/tests/Filesystem/XmlFileTest.php index 81a1fa2c..cae50e9a 100644 --- a/tests/Filesystem/XmlFileTest.php +++ b/tests/Filesystem/XmlFileTest.php @@ -105,6 +105,21 @@ public function testReadRestoresLibxmlErrorHandling(): void } } + public function testWriteThrowsWhenTheXmlCannotBeGenerated(): void + { + $dom = new class extends \DOMDocument { + public function saveXML(?\DOMNode $node = null, int $options = 0): string|false + { + return false; + } + }; + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Could not generate the XML content'); + + (new XmlFile($this->path, 'wb'))->write($dom); + } + public function testWriteThenRead(): void { $dom = new \DOMDocument(); diff --git a/tests/Task/File/FileSplitterTaskTest.php b/tests/Task/File/FileSplitterTaskTest.php index 1f0a48c2..8bc84f7f 100644 --- a/tests/Task/File/FileSplitterTaskTest.php +++ b/tests/Task/File/FileSplitterTaskTest.php @@ -22,6 +22,7 @@ use CleverAge\ProcessBundle\Task\File\FileSplitterTask; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; +use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException; #[\PHPUnit\Framework\Attributes\CoversClass(FileSplitterTask::class)] #[\PHPUnit\Framework\Attributes\UsesClass(SplFile::class)] @@ -102,6 +103,31 @@ public function testFilePathAndMaxLinesCanBeGivenAsInput(): void $this->assertSame(['a'.\PHP_EOL.'b'.\PHP_EOL, 'c'.\PHP_EOL], $chunks); } + /** + * @return iterable, mixed}> + */ + public static function provideInvalidMaxLines(): iterable + { + yield 'zero as option' => [['max_lines' => 0], null]; + yield 'negative as option' => [['max_lines' => -1], null]; + yield 'zero as input' => [[], ['max_lines' => 0]]; + yield 'string as input' => [[], ['max_lines' => 'abc']]; + } + + /** + * @param array $options + */ + #[DataProvider('provideInvalidMaxLines')] + public function testInvalidMaxLinesIsRejected(array $options, mixed $input): void + { + $filePath = $this->createSourceFile("a\nb\n"); + + // max_lines lower than 1 used to loop forever, a string one to throw a TypeError + $this->expectException(InvalidOptionsException::class); + + $this->runTask(new FileSplitterTask(), ['file_path' => $filePath, ...$options], $input); + } + public function testTaskCanBeReusedAfterIteration(): void { $filePath = $this->createSourceFile("a\nb\nc\n"); diff --git a/tests/Task/File/FolderBrowserTaskTest.php b/tests/Task/File/FolderBrowserTaskTest.php index caa40369..fc839f15 100644 --- a/tests/Task/File/FolderBrowserTaskTest.php +++ b/tests/Task/File/FolderBrowserTaskTest.php @@ -97,6 +97,18 @@ public function testInputFolderBrowserBrowsesAnotherFolderAfterAnEmptyOne(): voi self::assertSame([$this->tmpDir.'/dirB/b1.txt'], $this->iterate($task, $state, $this->tmpDir.'/dirB')); } + public function testInputFolderBrowserRequiresAFolderPathAsInput(): void + { + $task = new InputFolderBrowserTask(new NullLogger()); + $state = $this->createState([]); + $task->initialize($state); + + $this->expectException(\UnexpectedValueException::class); + $this->expectExceptionMessage('No folder path given as input'); + + $this->iterate($task, $state); + } + /** * Mimics the ProcessManager loop over an iterable task and returns the non-skipped outputs. * diff --git a/tests/Task/PropertySetterTaskTest.php b/tests/Task/PropertySetterTaskTest.php index d528d2d7..d428a645 100644 --- a/tests/Task/PropertySetterTaskTest.php +++ b/tests/Task/PropertySetterTaskTest.php @@ -67,6 +67,16 @@ public function testFailureKeepsOriginalExceptionWithErrorContext(mixed $value): self::assertNull($state->getOutput()); } + public function testScalarInputFailureKeepsErrorContext(): void + { + // The PropertyAccessor throws a \TypeError (not an \Exception) on a scalar input + $state = $this->execute(['[name]' => 'Foo'], 'not an array'); + + self::assertInstanceOf(\TypeError::class, $state->getException()); + self::assertSame(['property' => '[name]', 'value' => 'Foo'], $state->getErrorContext()); + self::assertNull($state->getOutput()); + } + private function execute(array $values, mixed $input): ProcessState { $processConfiguration = new ProcessConfiguration('test', []); diff --git a/tests/Transformer/String/SlugifyTransformerTest.php b/tests/Transformer/String/SlugifyTransformerTest.php index be620304..aaae8159 100644 --- a/tests/Transformer/String/SlugifyTransformerTest.php +++ b/tests/Transformer/String/SlugifyTransformerTest.php @@ -52,6 +52,27 @@ public function testConfigureOptionsRejectsInvalidTransliterator(): void $this->resolveOptions($transformer, ['transliterator' => 'Not-A-Real-Transliterator']); } + /** + * @return iterable}> + */ + public static function provideInvalidOptionTypes(): iterable + { + yield 'transliterator' => [['transliterator' => 123]]; + yield 'replace' => [['replace' => ['/a/']]]; + yield 'separator' => [['separator' => []]]; + } + + /** + * @param array $options + */ + #[\PHPUnit\Framework\Attributes\DataProvider('provideInvalidOptionTypes')] + public function testConfigureOptionsRejectsInvalidOptionTypes(array $options): void + { + $this->expectException(InvalidOptionsException::class); + + $this->resolveOptions(new SlugifyTransformer(), $options); + } + public function testGetCodeReturnsCorrectCode(): void { $this->assertSame('slugify', (new SlugifyTransformer())->getCode());