#203 - Fix ProcessState::addErrorContextValue() TypeError on empty iterables and PropertySetterTask

CHANGELOG.md

@@ -13,6 +13,7 @@ Latest
 * [#199](https://github.com/cleverage/process-bundle/issues/199) Fix PregFilterTransformer: an array `replacement` is no longer cast to the string `"Array"`, and requires an array `pattern`. Update documentation, add tests.
 * [#200](https://github.com/cleverage/process-bundle/issues/200) Fix GenericTransformer: a contextual option declared with `required: false` and no default can now be used; its placeholders are replaced by `null` when omitted. Update documentation, add tests.
 * [#202](https://github.com/cleverage/process-bundle/issues/202) Fix XmlReaderTask: throw an explicit `\UnexpectedValueException` on an empty file or invalid XML (with the libxml error messages) instead of a `ValueError` or a silent empty `\DOMDocument`. Update documentation, add tests.
+* [#203](https://github.com/cleverage/process-bundle/issues/203) Fix `ProcessState::addErrorContextValue()`: accept any value type, so that iterable tasks on an empty iterable are skipped and PropertySetterTask reports the original exception instead of a `TypeError`. 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/03-custom_tasks.md

@@ -82,8 +82,8 @@ Sometimes, when you execute a task, you need to change how the process continues
   `error_strategy` is then applied (throwing an exception from `execute` has the same effect)
 * `ProcessState::setErrorOutput($value)`: send a value to the error branch of your workflow (the tasks listed in
   `error_outputs`)
-* `ProcessState::addErrorContextValue($key, $value)` / `removeErrorContext($key)`: add information to the log record
-  written when an error occurs
+* `ProcessState::addErrorContextValue($key, $value)` / `removeErrorContext($key)`: add information (any value) to the
+  log record written when an error occurs
 
 ## Options
 

docs/reference/tasks/property_setter_task.md

@@ -22,9 +22,7 @@ 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)
-and handled according to the task `error_strategy`; the remaining values are not set. Note that only `string`, `int`
-and `array` values can be added to the error context: for other value types (`bool`, `float`, `null`, objects), a
-`\TypeError` is raised instead of the original exception (it is still handled according to `error_strategy`).
+and handled according to the task `error_strategy`; the remaining values are not set.
 
 Options
 -------

docs/reference/tasks/split_join_line_task.md

@@ -19,7 +19,8 @@ Possible outputs
 ----------------
 
 `array`: one line per split value, iterated in the order of `split_columns`. Each line contains all the original columns
-except the `split_columns`, plus the `join_column` holding the split value (as a string).
+except the `split_columns`, plus the `join_column` holding the split value (as a string). If no line is produced (empty
+`split_columns`), the task is skipped.
 
 Options
 -------

docs/reference/tasks/yaml_reader_task.md

@@ -43,5 +43,5 @@ Notes
 -----
 
 * The root of the file must be a mapping or a sequence, otherwise an `\InvalidArgumentException` is thrown (e.g. for an
-  empty file). An empty root mapping or sequence (`{}` or `[]`) raises a `\TypeError`.
+  empty file). An empty root mapping or sequence (`{}` or `[]`) produces no output: the task is skipped.
 * The current root key is added to the error context of the process as `iterator_key`.

src/Model/ProcessState.php

@@ -201,7 +201,7 @@ public function setErrorContext(array $errorContext): void
         $this->errorContext = $errorContext;
     }
 
-    public function addErrorContextValue(string|int $key, string|int|array $value): void
+    public function addErrorContextValue(string|int $key, mixed $value): void
     {
         $this->errorContext[$key] = $value;
     }

src/Task/AbstractIterableOutputTask.php

@@ -30,9 +30,8 @@ public function execute(ProcessState $state): void
     {
         $this->handleIteratorFromInput($state);
 
-        $state->addErrorContextValue('iterator_key', $this->iterator->key());
-
         if ($this->iterator->valid()) {
+            $state->addErrorContextValue('iterator_key', $this->iterator->key());
             $state->setOutput($this->iterator->current());
         } else {
             $state->setSkipped(true);

tests/Model/ProcessStateTest.php

@@ -0,0 +1,68 @@
+<?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\Model;
+
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(ProcessState::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
+class ProcessStateTest extends TestCase
+{
+    /**
+     * @return iterable<string, array{mixed}>
+     */
+    public static function errorContextValueProvider(): iterable
+    {
+        yield 'string' => ['foo'];
+        yield 'int' => [42];
+        yield 'array' => [['foo' => 'bar']];
+        yield 'bool' => [true];
+        yield 'float' => [1.5];
+        yield 'null' => [null];
+        yield 'object' => [new \stdClass()];
+    }
+
+    #[DataProvider('errorContextValueProvider')]
+    public function testAddErrorContextValueAcceptsAnyValue(mixed $value): void
+    {
+        $state = $this->createState();
+
+        $state->addErrorContextValue('key', $value);
+
+        self::assertSame(['key' => $value], $state->getErrorContext());
+    }
+
+    public function testRemoveErrorContext(): void
+    {
+        $state = $this->createState();
+        $state->addErrorContextValue('kept', 'foo');
+        $state->addErrorContextValue(0, null);
+
+        $state->removeErrorContext(0);
+
+        self::assertSame(['kept' => 'foo'], $state->getErrorContext());
+    }
+
+    private function createState(): ProcessState
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+
+        return new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+    }
+}

tests/Task/AbstractIterableOutputTaskTest.php

@@ -0,0 +1,116 @@
+<?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\Task;
+
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Model\AbstractConfigurableTask;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use CleverAge\ProcessBundle\Task\AbstractIterableOutputTask;
+use CleverAge\ProcessBundle\Task\ConstantIterableOutputTask;
+use CleverAge\ProcessBundle\Task\InputIteratorTask;
+use CleverAge\ProcessBundle\Task\SplitJoinLineTask;
+use PHPUnit\Framework\TestCase;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(AbstractIterableOutputTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ConstantIterableOutputTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(InputIteratorTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(SplitJoinLineTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(AbstractConfigurableTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
+class AbstractIterableOutputTaskTest extends TestCase
+{
+    public function testEmptyConstantIterableIsSkipped(): void
+    {
+        $task = new ConstantIterableOutputTask();
+        $state = $this->createState(['output' => []]);
+        $task->initialize($state);
+
+        $task->execute($state);
+
+        self::assertTrue($state->isSkipped());
+        self::assertNull($state->getException());
+        self::assertSame([], $state->getErrorContext());
+        self::assertFalse($task->next($state));
+    }
+
+    public function testEmptyInputIteratorIsSkipped(): void
+    {
+        $task = new InputIteratorTask();
+        $state = $this->createState([], []);
+        $task->initialize($state);
+
+        $task->execute($state);
+
+        self::assertTrue($state->isSkipped());
+        self::assertSame([], $state->getErrorContext());
+        self::assertFalse($task->next($state));
+    }
+
+    public function testIterationSetsIteratorKeyInErrorContext(): void
+    {
+        $task = new InputIteratorTask();
+        $state = $this->createState([], ['a' => 'foo', 'b' => 'bar']);
+        $task->initialize($state);
+
+        $outputs = [];
+        $keys = [];
+        do {
+            $state->setSkipped(false);
+            $task->execute($state);
+            $outputs[] = $state->getOutput();
+            $keys[] = $state->getErrorContext()['iterator_key'] ?? null;
+        } while ($task->next($state));
+
+        self::assertSame(['foo', 'bar'], $outputs);
+        self::assertSame(['a', 'b'], $keys);
+        self::assertSame([], $state->getErrorContext());
+
+        // A new input starts a new iteration cycle; an empty one is skipped
+        $state->setInput([]);
+        $task->execute($state);
+        self::assertTrue($state->isSkipped());
+        self::assertSame([], $state->getErrorContext());
+    }
+
+    public function testSplitJoinLineWithoutSplitColumnIsSkipped(): void
+    {
+        $task = new SplitJoinLineTask();
+        $state = $this->createState(['split_columns' => [], 'join_column' => 'value'], ['name' => 'Item1']);
+        $task->initialize($state);
+
+        $task->execute($state);
+
+        self::assertTrue($state->isSkipped());
+        self::assertSame([], $state->getErrorContext());
+    }
+
+    private function createState(array $options, mixed $input = null): ProcessState
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+        $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+        $state->setContextualOptionResolver(new ContextualOptionResolver());
+        $state->setContext([]);
+        $state->setTaskConfiguration(new TaskConfiguration('iterate', AbstractIterableOutputTask::class, $options));
+        $state->setInput($input);
+
+        return $state;
+    }
+}

tests/Task/PropertySetterTaskTest.php

@@ -0,0 +1,85 @@
+<?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\Task;
+
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Model\AbstractConfigurableTask;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use CleverAge\ProcessBundle\Task\PropertySetterTask;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException;
+use Symfony\Component\PropertyAccess\PropertyAccess;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(PropertySetterTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(AbstractConfigurableTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
+class PropertySetterTaskTest extends TestCase
+{
+    public function testValuesAreSet(): void
+    {
+        $state = $this->execute(['[status]' => 'imported', '[enabled]' => true], ['name' => 'Foo']);
+
+        self::assertNull($state->getException());
+        self::assertSame(['name' => 'Foo', 'status' => 'imported', 'enabled' => true], $state->getOutput());
+    }
+
+    /**
+     * @return iterable<string, array{mixed}>
+     */
+    public static function valueProvider(): iterable
+    {
+        yield 'string' => ['foo'];
+        yield 'int' => [42];
+        yield 'array' => [['foo']];
+        yield 'bool' => [true];
+        yield 'float' => [1.5];
+        yield 'null' => [null];
+    }
+
+    #[DataProvider('valueProvider')]
+    public function testFailureKeepsOriginalExceptionWithErrorContext(mixed $value): void
+    {
+        // A property path (not an index) cannot be written to an array
+        $state = $this->execute(['enabled' => $value], ['name' => 'Foo']);
+
+        self::assertInstanceOf(NoSuchPropertyException::class, $state->getException());
+        self::assertSame(['property' => 'enabled', 'value' => $value], $state->getErrorContext());
+        self::assertNull($state->getOutput());
+    }
+
+    private function execute(array $values, mixed $input): ProcessState
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+        $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+        $state->setContextualOptionResolver(new ContextualOptionResolver());
+        $state->setContext([]);
+        $state->setTaskConfiguration(new TaskConfiguration('set', PropertySetterTask::class, ['values' => $values]));
+        $state->setInput($input);
+
+        $task = new PropertySetterTask(new NullLogger(), PropertyAccess::createPropertyAccessor());
+        $task->initialize($state);
+        $task->execute($state);
+
+        return $state;
+    }
+}