#232 - Fix edge cases looping forever or failing with a TypeError

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.

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).

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
 ----------------

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

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.

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.

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) {

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<mixed> $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

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}'");
         }

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);

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',

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();

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<string, array{array<string, mixed>, 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<string, mixed> $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");

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.
      *

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', []);

tests/Transformer/String/SlugifyTransformerTest.php

@@ -52,6 +52,27 @@ public function testConfigureOptionsRejectsInvalidTransliterator(): void
         $this->resolveOptions($transformer, ['transliterator' => 'Not-A-Real-Transliterator']);
     }
 
+    /**
+     * @return iterable<string, array{array<string, mixed>}>
+     */
+    public static function provideInvalidOptionTypes(): iterable
+    {
+        yield 'transliterator' => [['transliterator' => 123]];
+        yield 'replace' => [['replace' => ['/a/']]];
+        yield 'separator' => [['separator' => []]];
+    }
+
+    /**
+     * @param array<string, mixed> $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());