From 07c1ed37438147d3780f180a1ce5771cbfe2a7d4 Mon Sep 17 00:00:00 2001 From: Nicolas Joubert Date: Thu, 22 Oct 2026 17:18:39 +0200 Subject: [PATCH 1/2] #339 - Reject a shared service referenced by several tasks of the same process when the container is compiled: all these tasks ran with the same instance, without error. Update documentation, add tests. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 3 ++ docs/03-custom_tasks.md | 3 +- docs/05-good_practices.md | 4 +- .../Compiler/TaskLocatorCompilerPass.php | 22 +++++++- .../Compiler/TaskLocatorCompilerPassTest.php | 52 +++++++++++++++++++ 5 files changed, 79 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b33c604..0cb6db27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ Latest ------ +## Changes +* [#339](https://github.com/cleverage/process-bundle/issues/339) A shared service referenced by several tasks of the same process is rejected when the container is compiled (`InvalidArgumentException` naming the service, the tasks and the process): all these tasks ran with the same instance, initialized with the options of each task in turn, without error. Declare the task services with `shared: false` (applied by the autoconfiguration of `TaskInterface` since 5.4). Update documentation, add tests. + v5.4.1 ----- diff --git a/docs/03-custom_tasks.md b/docs/03-custom_tasks.md index 386cd043..72bca29a 100644 --- a/docs/03-custom_tasks.md +++ b/docs/03-custom_tasks.md @@ -20,7 +20,8 @@ design them like any other service, with only small specificities: Most of the time you don't want a task declared in a process to share some data with another declaration using the same service, or even with another instance of your process. Since a task is a service, by default, Symfony will only create one instance (keeping values in every attribute of your class). With `shared: false`, a new instance is created -each time a task configuration is initialized. +each time a task configuration is initialized. A shared service referenced by several tasks of the same process is +rejected when the container is compiled. With autoconfiguration (`autoconfigure: true`, set in the `_defaults` of the Symfony skeleton), the bundle applies these settings to every service implementing `TaskInterface`: `shared: false` and a `monolog.logger` tag on the diff --git a/docs/05-good_practices.md b/docs/05-good_practices.md index 9ff7c731..ebe20410 100644 --- a/docs/05-good_practices.md +++ b/docs/05-good_practices.md @@ -19,8 +19,8 @@ in several places, declare it once as a [generic transformer](reference/03-gener Task services should be `shared: false`: a task often keeps state in its properties (an open file, a buffer, a counter...). A shared service would share this state between two tasks using the same service -in a process, or between two executions of a process in the same PHP process (sub-processes, workers, tests). See -[service declaration](03-custom_tasks.md#service-declaration). +in a process (this is rejected when the container is compiled), or between two executions of a process in the same PHP +process (sub-processes, workers, tests). See [service declaration](03-custom_tasks.md#service-declaration). ## Split big workflows into small processes diff --git a/src/DependencyInjection/Compiler/TaskLocatorCompilerPass.php b/src/DependencyInjection/Compiler/TaskLocatorCompilerPass.php index aa8fa2ab..3d6ba77a 100644 --- a/src/DependencyInjection/Compiler/TaskLocatorCompilerPass.php +++ b/src/DependencyInjection/Compiler/TaskLocatorCompilerPass.php @@ -42,20 +42,38 @@ public function process(ContainerBuilder $container): void $configurations = $container->getParameterBag()->resolveValue($processConfigurationRegistry->getArgument(0)); $services = []; - foreach (\is_array($configurations) ? $configurations : [] as $configuration) { - foreach ($configuration['tasks'] ?? [] as $task) { + foreach (\is_array($configurations) ? $configurations : [] as $processCode => $configuration) { + $taskCodesByService = []; + foreach ($configuration['tasks'] ?? [] as $taskCode => $task) { $id = ltrim((string) ($task['service'] ?? ''), '@'); // Unknown services keep failing at runtime, with the same message if ('' !== $id && $container->has($id)) { $services[$id] = new Reference($id); + $taskCodesByService[$id][] = (string) $taskCode; } } + $this->checkSharedServices($container, (string) $processCode, $taskCodesByService); } $container->getDefinition('cleverage_process.registry.task') ->replaceArgument(0, ServiceLocatorTagPass::register($container, $services)); } + /** + * A shared service referenced by several tasks of a process would run all of them with the same instance: the + * options of the last initialization would apply to every task, and their states would be mixed. + * + * @param array> $taskCodesByService + */ + private function checkSharedServices(ContainerBuilder $container, string $processCode, array $taskCodesByService): void + { + foreach ($taskCodesByService as $id => $taskCodes) { + if (\count($taskCodes) > 1 && $container->findDefinition($id)->isShared()) { + throw new InvalidArgumentException(\sprintf('The service "%s" is used by the tasks "%s" of the process "%s", but it is shared: all these tasks would run with the same instance (and the options of one of them). Declare the service with "shared: false".', $id, implode('", "', $taskCodes), $processCode)); + } + } + } + /** * Sets the "service" of the tasks configured with a "task" code. The configurations are not resolved (parameters), * only the "service" key is set. diff --git a/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php b/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php index 75825e9c..3144a432 100644 --- a/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php +++ b/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php @@ -134,6 +134,37 @@ public function testInvalidTaskCodeAttributeThrows(): void (new TaskLocatorCompilerPass())->process($container); } + public function testSharedServiceUsedBySeveralTasksOfAProcessThrows(): void + { + $container = $this->createContainerWithTasks(['test.process' => ['first' => 'app_task', 'second' => 'app_task']]); + $container->setDefinition('app.task', (new Definition(DummyTask::class))->addTag('cleverage.task', ['code' => 'app_task'])); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('The service "app.task" is used by the tasks "first", "second" of the process "test.process", but it is shared: all these tasks would run with the same instance (and the options of one of them). Declare the service with "shared: false".'); + + (new TaskLocatorCompilerPass())->process($container); + } + + public function testNonSharedServiceCanBeUsedBySeveralTasksOfAProcess(): void + { + $container = $this->createContainerWithTasks(['test.process' => ['first' => 'app_task', 'second' => 'app_task']]); + $container->setDefinition('app.task', (new Definition(DummyTask::class))->setShared(false)->addTag('cleverage.task', ['code' => 'app_task'])); + + (new TaskLocatorCompilerPass())->process($container); + + $this->addToAssertionCount(1); + } + + public function testSharedServiceCanBeUsedOnceByEachProcess(): void + { + $container = $this->createContainerWithTasks(['test.first' => ['task' => 'app_task'], 'test.second' => ['task' => 'app_task']]); + $container->setDefinition('app.task', (new Definition(DummyTask::class))->addTag('cleverage.task', ['code' => 'app_task'])); + + (new TaskLocatorCompilerPass())->process($container); + + $this->addToAssertionCount(1); + } + public function testNothingIsDoneWithoutTheRegistries(): void { $container = new ContainerBuilder(); @@ -150,6 +181,27 @@ private function createContainerWithTask(array $task): ContainerBuilder { $container = new ContainerBuilder(); (new CleverAgeProcessExtension())->load([['configurations' => ['test.process' => ['tasks' => ['my_task' => $task]]]]], $container); + + return $this->clearTaskTags($container); + } + + /** + * @param array> $taskCodesByProcess task code by task, for each process + */ + private function createContainerWithTasks(array $taskCodesByProcess): ContainerBuilder + { + $configurations = []; + foreach ($taskCodesByProcess as $processCode => $taskCodes) { + $configurations[$processCode]['tasks'] = array_map(static fn (string $code): array => ['task' => $code], $taskCodes); + } + $container = new ContainerBuilder(); + (new CleverAgeProcessExtension())->load([['configurations' => $configurations]], $container); + + return $this->clearTaskTags($container); + } + + private function clearTaskTags(ContainerBuilder $container): ContainerBuilder + { // Only the tasks of the test foreach (array_keys($container->findTaggedServiceIds('cleverage.task')) as $id) { $container->getDefinition($id)->clearTag('cleverage.task'); From 1c58e59411783bc7e5c3b6113cb41a03406eb421 Mon Sep 17 00:00:00 2001 From: Nicolas Joubert Date: Thu, 22 Oct 2026 17:20:29 +0200 Subject: [PATCH 2/2] #339 - Fix Rector Co-Authored-By: Claude Opus 5.5 (1M context) --- .../Compiler/TaskLocatorCompilerPassTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php b/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php index 3144a432..a5a10342 100644 --- a/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php +++ b/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php @@ -152,7 +152,7 @@ public function testNonSharedServiceCanBeUsedBySeveralTasksOfAProcess(): void (new TaskLocatorCompilerPass())->process($container); - $this->addToAssertionCount(1); + self::assertInstanceOf(Reference::class, $container->getDefinition('cleverage_process.registry.task')->getArgument(0)); } public function testSharedServiceCanBeUsedOnceByEachProcess(): void @@ -162,7 +162,7 @@ public function testSharedServiceCanBeUsedOnceByEachProcess(): void (new TaskLocatorCompilerPass())->process($container); - $this->addToAssertionCount(1); + self::assertInstanceOf(Reference::class, $container->getDefinition('cleverage_process.registry.task')->getArgument(0)); } public function testNothingIsDoneWithoutTheRegistries(): void