Skip to content

Commit b59aa59

Browse files
njoubert-cleveragenjoubert-cleverage
authored
Merge pull request #214 from cleverage/203
#203 - Fix ProcessState::addErrorContextValue() TypeError on empty iterables and PropertySetterTask
2 parents ac53294 + 7603a4f commit b59aa59

10 files changed

Lines changed: 278 additions & 10 deletions

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ Latest
1313
* [#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.
1414
* [#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.
1515
* [#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.
16+
* [#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.
1617

1718
## Deprecated
1819
* [#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‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,8 @@ Sometimes, when you execute a task, you need to change how the process continues
8282
`error_strategy` is then applied (throwing an exception from `execute` has the same effect)
8383
* `ProcessState::setErrorOutput($value)`: send a value to the error branch of your workflow (the tasks listed in
8484
`error_outputs`)
85-
* `ProcessState::addErrorContextValue($key, $value)` / `removeErrorContext($key)`: add information to the log record
86-
written when an error occurs
85+
* `ProcessState::addErrorContextValue($key, $value)` / `removeErrorContext($key)`: add information (any value) to the
86+
log record written when an error occurs
8787

8888
## Options
8989

‎docs/reference/tasks/property_setter_task.md‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,7 @@ Possible outputs
2222
The input, with the configured values set.
2323

2424
If a value cannot be set, the exception is set on the state (with `property` and `value` added to the error context)
25-
and handled according to the task `error_strategy`; the remaining values are not set. Note that only `string`, `int`
26-
and `array` values can be added to the error context: for other value types (`bool`, `float`, `null`, objects), a
27-
`\TypeError` is raised instead of the original exception (it is still handled according to `error_strategy`).
25+
and handled according to the task `error_strategy`; the remaining values are not set.
2826

2927
Options
3028
-------

‎docs/reference/tasks/split_join_line_task.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ Possible outputs
1919
----------------
2020

2121
`array`: one line per split value, iterated in the order of `split_columns`. Each line contains all the original columns
22-
except the `split_columns`, plus the `join_column` holding the split value (as a string).
22+
except the `split_columns`, plus the `join_column` holding the split value (as a string). If no line is produced (empty
23+
`split_columns`), the task is skipped.
2324

2425
Options
2526
-------

‎docs/reference/tasks/yaml_reader_task.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,5 +43,5 @@ Notes
4343
-----
4444
4545
* The root of the file must be a mapping or a sequence, otherwise an `\InvalidArgumentException` is thrown (e.g. for an
46-
empty file). An empty root mapping or sequence (`{}` or `[]`) raises a `\TypeError`.
46+
empty file). An empty root mapping or sequence (`{}` or `[]`) produces no output: the task is skipped.
4747
* The current root key is added to the error context of the process as `iterator_key`.

‎src/Model/ProcessState.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ public function setErrorContext(array $errorContext): void
201201
$this->errorContext = $errorContext;
202202
}
203203

204-
public function addErrorContextValue(string|int $key, string|int|array $value): void
204+
public function addErrorContextValue(string|int $key, mixed $value): void
205205
{
206206
$this->errorContext[$key] = $value;
207207
}

‎src/Task/AbstractIterableOutputTask.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,8 @@ public function execute(ProcessState $state): void
3030
{
3131
$this->handleIteratorFromInput($state);
3232

33-
$state->addErrorContextValue('iterator_key', $this->iterator->key());
34-
3533
if ($this->iterator->valid()) {
34+
$state->addErrorContextValue('iterator_key', $this->iterator->key());
3635
$state->setOutput($this->iterator->current());
3736
} else {
3837
$state->setSkipped(true);

‎tests/Model/ProcessStateTest.php‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/*
6+
* This file is part of the CleverAge/ProcessBundle package.
7+
*
8+
* Copyright (c) Clever-Age
9+
*
10+
* For the full copyright and license information, please view the LICENSE
11+
* file that was distributed with this source code.
12+
*/
13+
14+
namespace CleverAge\ProcessBundle\Tests\Model;
15+
16+
use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
17+
use CleverAge\ProcessBundle\Model\ProcessHistory;
18+
use CleverAge\ProcessBundle\Model\ProcessState;
19+
use PHPUnit\Framework\Attributes\DataProvider;
20+
use PHPUnit\Framework\TestCase;
21+
22+
#[\PHPUnit\Framework\Attributes\CoversClass(ProcessState::class)]
23+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
24+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
25+
class ProcessStateTest extends TestCase
26+
{
27+
/**
28+
* @return iterable<string, array{mixed}>
29+
*/
30+
public static function errorContextValueProvider(): iterable
31+
{
32+
yield 'string' => ['foo'];
33+
yield 'int' => [42];
34+
yield 'array' => [['foo' => 'bar']];
35+
yield 'bool' => [true];
36+
yield 'float' => [1.5];
37+
yield 'null' => [null];
38+
yield 'object' => [new \stdClass()];
39+
}
40+
41+
#[DataProvider('errorContextValueProvider')]
42+
public function testAddErrorContextValueAcceptsAnyValue(mixed $value): void
43+
{
44+
$state = $this->createState();
45+
46+
$state->addErrorContextValue('key', $value);
47+
48+
self::assertSame(['key' => $value], $state->getErrorContext());
49+
}
50+
51+
public function testRemoveErrorContext(): void
52+
{
53+
$state = $this->createState();
54+
$state->addErrorContextValue('kept', 'foo');
55+
$state->addErrorContextValue(0, null);
56+
57+
$state->removeErrorContext(0);
58+
59+
self::assertSame(['kept' => 'foo'], $state->getErrorContext());
60+
}
61+
62+
private function createState(): ProcessState
63+
{
64+
$processConfiguration = new ProcessConfiguration('test', []);
65+
66+
return new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
67+
}
68+
}

‎tests/Task/AbstractIterableOutputTaskTest.php‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/*
6+
* This file is part of the CleverAge/ProcessBundle package.
7+
*
8+
* Copyright (c) Clever-Age
9+
*
10+
* For the full copyright and license information, please view the LICENSE
11+
* file that was distributed with this source code.
12+
*/
13+
14+
namespace CleverAge\ProcessBundle\Tests\Task;
15+
16+
use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
17+
use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
18+
use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
19+
use CleverAge\ProcessBundle\Model\AbstractConfigurableTask;
20+
use CleverAge\ProcessBundle\Model\ProcessHistory;
21+
use CleverAge\ProcessBundle\Model\ProcessState;
22+
use CleverAge\ProcessBundle\Task\AbstractIterableOutputTask;
23+
use CleverAge\ProcessBundle\Task\ConstantIterableOutputTask;
24+
use CleverAge\ProcessBundle\Task\InputIteratorTask;
25+
use CleverAge\ProcessBundle\Task\SplitJoinLineTask;
26+
use PHPUnit\Framework\TestCase;
27+
28+
#[\PHPUnit\Framework\Attributes\CoversClass(AbstractIterableOutputTask::class)]
29+
#[\PHPUnit\Framework\Attributes\UsesClass(ConstantIterableOutputTask::class)]
30+
#[\PHPUnit\Framework\Attributes\UsesClass(InputIteratorTask::class)]
31+
#[\PHPUnit\Framework\Attributes\UsesClass(SplitJoinLineTask::class)]
32+
#[\PHPUnit\Framework\Attributes\UsesClass(AbstractConfigurableTask::class)]
33+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
34+
#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
35+
#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
36+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
37+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
38+
class AbstractIterableOutputTaskTest extends TestCase
39+
{
40+
public function testEmptyConstantIterableIsSkipped(): void
41+
{
42+
$task = new ConstantIterableOutputTask();
43+
$state = $this->createState(['output' => []]);
44+
$task->initialize($state);
45+
46+
$task->execute($state);
47+
48+
self::assertTrue($state->isSkipped());
49+
self::assertNull($state->getException());
50+
self::assertSame([], $state->getErrorContext());
51+
self::assertFalse($task->next($state));
52+
}
53+
54+
public function testEmptyInputIteratorIsSkipped(): void
55+
{
56+
$task = new InputIteratorTask();
57+
$state = $this->createState([], []);
58+
$task->initialize($state);
59+
60+
$task->execute($state);
61+
62+
self::assertTrue($state->isSkipped());
63+
self::assertSame([], $state->getErrorContext());
64+
self::assertFalse($task->next($state));
65+
}
66+
67+
public function testIterationSetsIteratorKeyInErrorContext(): void
68+
{
69+
$task = new InputIteratorTask();
70+
$state = $this->createState([], ['a' => 'foo', 'b' => 'bar']);
71+
$task->initialize($state);
72+
73+
$outputs = [];
74+
$keys = [];
75+
do {
76+
$state->setSkipped(false);
77+
$task->execute($state);
78+
$outputs[] = $state->getOutput();
79+
$keys[] = $state->getErrorContext()['iterator_key'] ?? null;
80+
} while ($task->next($state));
81+
82+
self::assertSame(['foo', 'bar'], $outputs);
83+
self::assertSame(['a', 'b'], $keys);
84+
self::assertSame([], $state->getErrorContext());
85+
86+
// A new input starts a new iteration cycle; an empty one is skipped
87+
$state->setInput([]);
88+
$task->execute($state);
89+
self::assertTrue($state->isSkipped());
90+
self::assertSame([], $state->getErrorContext());
91+
}
92+
93+
public function testSplitJoinLineWithoutSplitColumnIsSkipped(): void
94+
{
95+
$task = new SplitJoinLineTask();
96+
$state = $this->createState(['split_columns' => [], 'join_column' => 'value'], ['name' => 'Item1']);
97+
$task->initialize($state);
98+
99+
$task->execute($state);
100+
101+
self::assertTrue($state->isSkipped());
102+
self::assertSame([], $state->getErrorContext());
103+
}
104+
105+
private function createState(array $options, mixed $input = null): ProcessState
106+
{
107+
$processConfiguration = new ProcessConfiguration('test', []);
108+
$state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
109+
$state->setContextualOptionResolver(new ContextualOptionResolver());
110+
$state->setContext([]);
111+
$state->setTaskConfiguration(new TaskConfiguration('iterate', AbstractIterableOutputTask::class, $options));
112+
$state->setInput($input);
113+
114+
return $state;
115+
}
116+
}

‎tests/Task/PropertySetterTaskTest.php‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/*
6+
* This file is part of the CleverAge/ProcessBundle package.
7+
*
8+
* Copyright (c) Clever-Age
9+
*
10+
* For the full copyright and license information, please view the LICENSE
11+
* file that was distributed with this source code.
12+
*/
13+
14+
namespace CleverAge\ProcessBundle\Tests\Task;
15+
16+
use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
17+
use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
18+
use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
19+
use CleverAge\ProcessBundle\Model\AbstractConfigurableTask;
20+
use CleverAge\ProcessBundle\Model\ProcessHistory;
21+
use CleverAge\ProcessBundle\Model\ProcessState;
22+
use CleverAge\ProcessBundle\Task\PropertySetterTask;
23+
use PHPUnit\Framework\Attributes\DataProvider;
24+
use PHPUnit\Framework\TestCase;
25+
use Psr\Log\NullLogger;
26+
use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException;
27+
use Symfony\Component\PropertyAccess\PropertyAccess;
28+
29+
#[\PHPUnit\Framework\Attributes\CoversClass(PropertySetterTask::class)]
30+
#[\PHPUnit\Framework\Attributes\UsesClass(AbstractConfigurableTask::class)]
31+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
32+
#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
33+
#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
34+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
35+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
36+
class PropertySetterTaskTest extends TestCase
37+
{
38+
public function testValuesAreSet(): void
39+
{
40+
$state = $this->execute(['[status]' => 'imported', '[enabled]' => true], ['name' => 'Foo']);
41+
42+
self::assertNull($state->getException());
43+
self::assertSame(['name' => 'Foo', 'status' => 'imported', 'enabled' => true], $state->getOutput());
44+
}
45+
46+
/**
47+
* @return iterable<string, array{mixed}>
48+
*/
49+
public static function valueProvider(): iterable
50+
{
51+
yield 'string' => ['foo'];
52+
yield 'int' => [42];
53+
yield 'array' => [['foo']];
54+
yield 'bool' => [true];
55+
yield 'float' => [1.5];
56+
yield 'null' => [null];
57+
}
58+
59+
#[DataProvider('valueProvider')]
60+
public function testFailureKeepsOriginalExceptionWithErrorContext(mixed $value): void
61+
{
62+
// A property path (not an index) cannot be written to an array
63+
$state = $this->execute(['enabled' => $value], ['name' => 'Foo']);
64+
65+
self::assertInstanceOf(NoSuchPropertyException::class, $state->getException());
66+
self::assertSame(['property' => 'enabled', 'value' => $value], $state->getErrorContext());
67+
self::assertNull($state->getOutput());
68+
}
69+
70+
private function execute(array $values, mixed $input): ProcessState
71+
{
72+
$processConfiguration = new ProcessConfiguration('test', []);
73+
$state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
74+
$state->setContextualOptionResolver(new ContextualOptionResolver());
75+
$state->setContext([]);
76+
$state->setTaskConfiguration(new TaskConfiguration('set', PropertySetterTask::class, ['values' => $values]));
77+
$state->setInput($input);
78+
79+
$task = new PropertySetterTask(new NullLogger(), PropertyAccess::createPropertyAccessor());
80+
$task->initialize($state);
81+
$task->execute($state);
82+
83+
return $state;
84+
}
85+
}

0 commit comments

Comments
 (0)