#305 - Bundle best practices: residues of the review

CHANGELOG.md

@@ -2,6 +2,7 @@ Latest
 ------
 
 ## Changes
+* [#305](https://github.com/cleverage/process-bundle/issues/305) Bundle best practices, residues: the generic transformers are no longer autowired (explicit arguments). `ProcessHelpCommand` receives the `TaskRegistry` instead of the service container (passing a container is deprecated). Fetching from the service container a task of the application made public by the `TaskInterface` autoconfiguration is deprecated (`container.private` tag, new internal `PublicTaskDeprecationCompilerPass`); the tasks made public by the application itself are not concerned. Trigger the missing deprecation notices: `InputAggregatorTask`, `CheckSerializerCompilerPass`, `errors` task configuration key (use `error_outputs`). `TaskConfiguration::getErrors()` triggers a single notice on PHP 8.4+. `CleverAgeProcessBundle` is marked `@final since 5.4`. Update documentation, add tests.
 * [#299](https://github.com/cleverage/process-bundle/issues/299) Require `symfony/polyfill-php83`, `symfony/polyfill-php84` and `symfony/polyfill-php85`: the functions of PHP 8.3 to 8.5 can be used while supporting PHP 8.2. Rector targets PHP 8.2 (`withPhpVersion()`, `withPhpSets()` read from `composer.json`) and applies the rules using a newer function when its polyfill is required: `ProcessConfiguration` uses `array_any()` again; `array_first()` replaces `reset()` / `current()` in `ProcessConfiguration::getMainTask()`, `ConstraintLoader` and `ProcessHelpCommand` (no internal array pointer, same results).
 * [#279](https://github.com/cleverage/process-bundle/issues/279) Define the public API (Symfony BC promise): the classes not meant to be extended are marked `@final since 5.4` (the engine: `ProcessManager`, `ProcessConfigurationRegistry`, `TransformerRegistry`, `ContextualOptionResolver`; the models: `ProcessState`, `ProcessHistory`, `SubprocessInstance`, the configurations, the events, the `Filesystem` files; the concrete tasks and transformers), they will be `final` in v6.0: extending them triggers a deprecation with the Symfony `DebugClassLoader`. The abstract classes, interfaces, traits, exceptions and the file tasks extended by other tasks (`FileReaderTask`, `LineReaderTask`, `FolderBrowserTask`, `CsvReaderTask`, `InputCsvReaderTask`, `Filesystem\CsvResource`) stay open. `TaskRegistry`, new in this version, is `final`. The infrastructure classes are marked `@internal`: `CleverAgeProcessExtension`, `Configuration`, the commands, `DataQueueEventListener`, `PhpFunctionProvider`, the Monolog processors, `ProcessLogger` and `TaskLogger`. Update documentation.
 * [#269](https://github.com/cleverage/process-bundle/issues/269) Composer: `symfony/process`, `symfony/stopwatch`, `symfony/var-dumper` and `symfony/monolog-bundle` are no longer required, they are suggested (applications using them must require them). Without `symfony/process` (`CommandRunnerTask`, `ProcessLauncherTask`) or `symfony/stopwatch` (`StopwatchTask`), the container still compiles and these tasks fail with an explicit message (`UnavailableServicesCompilerPass`); without `symfony/var-dumper`, nothing is dumped (as before); without `symfony/monolog-bundle`, the `cleverage_process*` channels and processors are not registered and the default logger is used. `monolog/monolog` stays required (`Logger\*Processor`). Remove the unused `symfony/monolog-bridge` dependency. `ProcessConfiguration` no longer uses `array_any()` (PHP 8.4), whose polyfill was only installed by the removed packages. Add a `conflict` with `cleverage/doctrine-process-bundle`, `cleverage/soap-process-bundle` and `cleverage/ui-process-bundle` `<3.3`, which use the MonologBundle without requiring it. Update documentation, add tests.

config/services/command.yaml

@@ -24,5 +24,4 @@ services:
             - { name: console.command }
         arguments:
             - '@cleverage_process.registry.process_configuration'
-            - '@service_container'
             - '@cleverage_process.registry.task'

docs/03-custom_tasks.md

@@ -25,7 +25,10 @@ each time a task configuration is initialized.
 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
 `cleverage_process_task` channel (see [logging](#logging)), as well as `public: true`, only kept for the deprecated
-fallback on the service container. A setting explicitly defined on the service wins (e.g. `shared: true`).
+fallback on the service container: fetching such a task directly from the service container is deprecated since 5.4
+(the task will be private in v6.0), unless the application makes it public itself (`public: true` on the service, in
+`_defaults` or `_instanceof`, or `#[Autoconfigure(public: true)]`). A setting explicitly defined on the service wins
+(e.g. `shared: true`).
 
 ```yaml
 # config/services.yaml

docs/05-good_practices.md

@@ -71,8 +71,7 @@ See [errors and skips](04-advanced_workflow.md#errors-and-skips).
 
 When a task has several `outputs`, each output is fully processed one after the other, in the configured order, but
 this is an implementation detail. If a task needs the result of another branch, make the dependency explicit in the
-graph (e.g. with a blocking task or an [InputAggregatorTask](reference/tasks/input_aggregator_task.md)) rather than
-relying on the order in which branches are executed.
+graph (e.g. with a blocking task) rather than relying on the order in which branches are executed.
 
 ## Use the context for runtime parameters
 

docs/reference/02-task_definition.md

@@ -54,7 +54,8 @@ are replaced by the process [contextual values](../01-quick_start.md#contextual-
 **error_outputs**: optional list of following tasks, receiving the error output of this task (by default its input when
 an error occurs). It can be a simple string. See [errors and skips](../04-advanced_workflow.md#errors-and-skips).
 
-**errors**: deprecated alias of `error_outputs`. Defining both on the same task throws an exception.
+**errors**: **deprecated since 5.4, removed in v6.0**, use `error_outputs` (same behaviour). Defining both on the same
+task throws an exception.
 
 **error_strategy**: optional, either *skip* or *stop*. Defines if the process continues with the next input or stops
 when the task fails. When not defined, the global `default_error_strategy` is used (*stop* by default).

docs/reference/tasks/input_aggregator_task.md

@@ -1,7 +1,8 @@
 InputAggregatorTask
 ===================
 
-**Deprecated**: this class is marked `@deprecated` (too error-prone, should be refactored as a blocking task).
+**Deprecated since v5.4, removed in v6.0**: this task is too error-prone (it should be refactored as a blocking task).
+An `E_USER_DEPRECATED` error is triggered when it is instantiated.
 
 Accumulates inputs coming from several parent tasks. The task is skipped until an input has been received from every
 parent declared in `input_codes`; it then outputs an array of all received inputs, indexed by their destination key,

src/CleverAgeProcessBundle.php

@@ -13,12 +13,17 @@
 
 namespace CleverAge\ProcessBundle;
 
+use CleverAge\ProcessBundle\DependencyInjection\Compiler\PublicTaskDeprecationCompilerPass;
 use CleverAge\ProcessBundle\DependencyInjection\Compiler\TaskLocatorCompilerPass;
 use CleverAge\ProcessBundle\DependencyInjection\Compiler\TransformerLocatorCompilerPass;
 use CleverAge\ProcessBundle\DependencyInjection\Compiler\UnavailableServicesCompilerPass;
+use Symfony\Component\DependencyInjection\Compiler\PassConfig;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\HttpKernel\Bundle\Bundle;
 
+/**
+ * @final since 5.4
+ */
 class CleverAgeProcessBundle extends Bundle
 {
     /**
@@ -31,6 +36,8 @@ public function build(ContainerBuilder $container): void
         $container->addCompilerPass(new UnavailableServicesCompilerPass());
         $container->addCompilerPass(new TransformerLocatorCompilerPass());
         $container->addCompilerPass(new TaskLocatorCompilerPass());
+        // Before ResolveInstanceofConditionalsPass (priority 100)
+        $container->addCompilerPass(new PublicTaskDeprecationCompilerPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 110);
     }
 
     #[\Override]

src/Command/ProcessHelpCommand.php

@@ -64,22 +64,28 @@ class ProcessHelpCommand extends Command
 
     protected TaskRegistry $taskRegistry;
 
+    /**
+     * @deprecated since 5.4, only set when a container is passed to the constructor, will be removed in v6.0
+     */
+    protected ?ContainerInterface $container = null;
+
     public function __construct(
         protected ProcessConfigurationRegistry $processConfigRegistry,
-        protected ContainerInterface $container,
-        ?TaskRegistry $taskRegistry = null,
+        TaskRegistry|ContainerInterface $taskRegistry,
     ) {
         parent::__construct();
-        if (!$taskRegistry instanceof TaskRegistry) {
+        if ($taskRegistry instanceof ContainerInterface) {
             trigger_deprecation(
                 'cleverage/process-bundle',
                 '5.4',
-                'Not passing a "%s" to "%s::__construct()" is deprecated: the tasks are fetched from the service container. The argument will be required in v6.0.',
-                TaskRegistry::class,
+                'Passing a service container to "%s::__construct()" is deprecated, pass a "%s" instead: the tasks are fetched from the service container. Only a "%s" will be accepted in v6.0.',
                 self::class,
+                TaskRegistry::class,
+                TaskRegistry::class,
             );
+            $this->container = $taskRegistry;
             // Same behaviour as before the task registry: any service of the container
-            $taskRegistry = new TaskRegistry($this->container, $this->container);
+            $taskRegistry = new TaskRegistry($taskRegistry, $taskRegistry);
         }
         $this->taskRegistry = $taskRegistry;
     }

src/Configuration/TaskConfiguration.php

@@ -49,6 +49,9 @@ class TaskConfiguration
 
     protected bool $inErrorBranch = false;
 
+    /**
+     * @deprecated not used, will be removed in v6.0
+     */
     protected bool $logErrors;
 
     /**
@@ -67,7 +70,7 @@ public function __construct(
         protected string $errorStrategy = self::STRATEGY_SKIP,
         protected string $logLevel = LogLevel::CRITICAL,
     ) {
-        $this->logErrors = LogLevel::DEBUG !== $logLevel; // @deprecated, remove me in next version
+        $this->logErrors = LogLevel::DEBUG !== $logLevel;
     }
 
     public function getCode(): string
@@ -126,9 +129,10 @@ public function getOutputs(): array
     }
 
     /**
+     * @deprecated since 2.0.4, use getErrorOutputs() instead, will be removed in v6.0
+     *
      * @return list<string>
      */
-    #[\Deprecated(message: 'Use getErrorOutputs method instead')]
     public function getErrors(): array
     {
         trigger_deprecation('cleverage/process-bundle', '2.0.4', 'The "%s()" method is deprecated, use "getErrorOutputs()" instead.', __METHOD__);

src/DependencyInjection/CleverAgeProcessExtension.php

@@ -43,7 +43,8 @@ public function load(array $configs, ContainerBuilder $container): void
 
         $container->registerForAutoconfiguration(ProcessStopCheckerInterface::class)
             ->addTag('cleverage_process.stop_checker');
-        // Tasks are fetched from the container by the process manager, and keep state between executions
+        // Tasks keep state between executions. Public for the deprecated fallback of the TaskRegistry on the service
+        // container: their direct access is deprecated (PublicTaskDeprecationCompilerPass), private in v6.0
         $container->registerForAutoconfiguration(TaskInterface::class)
             ->setPublic(true)
             ->setShared(false)
@@ -77,7 +78,6 @@ static function (ChildDefinition $definition, AsTransformer $attribute): void {
         // Automatic transformer creation from config
         foreach ($config['generic_transformers'] as $transformerCode => $transformerConfig) {
             $transformerDefinition = new Definition(GenericTransformer::class);
-            $transformerDefinition->setAutowired(true);
             $transformerDefinition->setPublic(false);
             $transformerDefinition->setArguments([
                 new Reference('cleverage_process.context.contextual_option_resolver'),

src/DependencyInjection/Compiler/CheckSerializerCompilerPass.php

@@ -21,13 +21,18 @@
 /**
  * Check the presence of the serializer (required for this bundle), and help the user to set it.
  *
- * @deprecated no longer used by the bundle, which compiles without the serializer: the services depending on it
- *             fail when they are used (see UnavailableServicesCompilerPass). Will be removed in v6.0.
+ * @deprecated since 5.4, no longer used by the bundle, which compiles without the serializer: the services depending
+ *             on it fail when they are used (see UnavailableServicesCompilerPass). Will be removed in v6.0.
  */
 class CheckSerializerCompilerPass implements CompilerPassInterface
 {
     final public const MSG = 'The Symfony serializer component do not seem enabled, consider toggling framework.serializer.enable (see https://symfony.com/doc/current/reference/configuration/framework.html#reference-serializer-enabled)';
 
+    public function __construct()
+    {
+        trigger_deprecation('cleverage/process-bundle', '5.4', 'The "%s" class is deprecated, it will be removed in v6.0.', self::class);
+    }
+
     public function process(ContainerBuilder $container): void
     {
         if (!$container->has('serializer') && !$container->has(DenormalizerInterface::class)) {

src/DependencyInjection/Compiler/PublicTaskDeprecationCompilerPass.php

@@ -0,0 +1,64 @@
+<?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\DependencyInjection\Compiler;
+
+use CleverAge\ProcessBundle\Model\TaskInterface;
+use Symfony\Component\DependencyInjection\Attribute\Autoconfigure;
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
+/**
+ * The autoconfiguration of TaskInterface makes the tasks of the application public, for the deprecated fallback of
+ * the TaskRegistry on the service container: fetching them directly from the container is deprecated (container.private
+ * tag, as for the tasks of the bundle), they will be private in v6.0. The tasks made public by the application itself
+ * (service, "_defaults" or "_instanceof" configuration, #[Autoconfigure] attribute) are not concerned.
+ *
+ * Runs before ResolveInstanceofConditionalsPass, which merges the autoconfiguration into the definitions.
+ *
+ * @internal
+ */
+final class PublicTaskDeprecationCompilerPass implements CompilerPassInterface
+{
+    public function process(ContainerBuilder $container): void
+    {
+        foreach ($container->getDefinitions() as $id => $definition) {
+            if (!$definition->isAutoconfigured()
+                || $definition->isAbstract()
+                || $definition->isSynthetic()
+                || $definition->hasTag('container.private')
+                || isset($definition->getChanges()['public'])) {
+                continue;
+            }
+            // Same resolution as ResolveClassPass for the services whose id is the class
+            $class = $container->getParameterBag()->resolveValue($definition->getClass() ?? $id);
+            if (!\is_string($class)
+                || !($reflectionClass = $container->getReflectionClass($class, false)) instanceof \ReflectionClass
+                || !$reflectionClass->implementsInterface(TaskInterface::class)) {
+                continue;
+            }
+            foreach ($definition->getInstanceofConditionals() as $type => $instanceof) {
+                if (isset($instanceof->getChanges()['public']) && is_a($class, $type, true)) {
+                    continue 2;
+                }
+            }
+            foreach ($reflectionClass->getAttributes(Autoconfigure::class, \ReflectionAttribute::IS_INSTANCEOF) as $attribute) {
+                if (null !== $attribute->newInstance()->public) {
+                    continue 2;
+                }
+            }
+
+            $definition->addTag('container.private', ['package' => 'cleverage/process-bundle', 'version' => '5.4']);
+        }
+    }
+}

src/DependencyInjection/Configuration.php

@@ -210,8 +210,16 @@ protected function appendTaskConfigDefinition(NodeBuilder $definition): void
             ->defaultValue(LogLevel::CRITICAL);
 
         foreach (['outputs', 'errors', 'error_outputs'] as $nodeName) {
-            $definition->arrayNode($nodeName)
-                ->beforeNormalization()
+            $node = $definition->arrayNode($nodeName);
+            if ('errors' === $nodeName) {
+                $node->info('Deprecated, use "error_outputs"')
+                    ->setDeprecated(
+                        'cleverage/process-bundle',
+                        '5.4',
+                        'The "%node%" option of the task "%path%" is deprecated, use "error_outputs" instead. It will be removed in v6.0.'
+                    );
+            }
+            $node->beforeNormalization()
                 ->ifString()
                 ->then(static fn ($item): array => [$item])->end()
                 ->scalarPrototype();

src/Task/InputAggregatorTask.php

@@ -22,10 +22,15 @@
  * Should have been a BlockingTask, but due to limitations in the current model, it's a hack using skips and finalize.
  *
  * @see        README.md:Known issues
- * @deprecated It's way too much error prone - should be refactored as a blocking task
+ * @deprecated since 5.4, it's way too much error prone - should be refactored as a blocking task, will be removed in v6.0
  */
 class InputAggregatorTask extends AbstractConfigurableTask
 {
+    public function __construct()
+    {
+        trigger_deprecation('cleverage/process-bundle', '5.4', 'The "%s" task is deprecated: it is too error-prone. It will be removed in v6.0.', self::class);
+    }
+
     /**
      * @var array<string, mixed>
      */

tests/Command/ProcessHelpCommandTest.php

@@ -27,10 +27,10 @@
 #[UsesClass(TaskRegistry::class)]
 class ProcessHelpCommandTest extends TestCase
 {
-    public function testWithoutTaskRegistryIsDeprecated(): void
+    public function testWithContainerIsDeprecated(): void
     {
         self::assertSame(
-            ['Since cleverage/process-bundle 5.4: Not passing a "'.TaskRegistry::class.'" to "'.ProcessHelpCommand::class.'::__construct()" is deprecated: the tasks are fetched from the service container. The argument will be required in v6.0.'],
+            ['Since cleverage/process-bundle 5.4: Passing a service container to "'.ProcessHelpCommand::class.'::__construct()" is deprecated, pass a "'.TaskRegistry::class.'" instead: the tasks are fetched from the service container. Only a "'.TaskRegistry::class.'" will be accepted in v6.0.'],
             $this->collectDeprecations(static fn (): ProcessHelpCommand => new ProcessHelpCommand(new ProcessConfigurationRegistry([], 'stop'), new Container()))
         );
     }
@@ -39,7 +39,7 @@ public function testWithTaskRegistryIsNotDeprecated(): void
     {
         self::assertSame(
             [],
-            $this->collectDeprecations(static fn (): ProcessHelpCommand => new ProcessHelpCommand(new ProcessConfigurationRegistry([], 'stop'), new Container(), new TaskRegistry(new ServiceLocator([]))))
+            $this->collectDeprecations(static fn (): ProcessHelpCommand => new ProcessHelpCommand(new ProcessConfigurationRegistry([], 'stop'), new TaskRegistry(new ServiceLocator([]))))
         );
     }
 

tests/Configuration/TaskConfigurationTest.php

@@ -37,9 +37,8 @@ public function testGetErrorsIsDeprecated(): void
         }
 
         self::assertSame(['error'], $errors);
-        // PHP 8.4+ also triggers its own deprecation for the #[\Deprecated] attribute
-        self::assertContains(
-            'Since cleverage/process-bundle 2.0.4: The "CleverAge\ProcessBundle\Configuration\TaskConfiguration::getErrors()" method is deprecated, use "getErrorOutputs()" instead.',
+        self::assertSame(
+            ['Since cleverage/process-bundle 2.0.4: The "CleverAge\ProcessBundle\Configuration\TaskConfiguration::getErrors()" method is deprecated, use "getErrorOutputs()" instead.'],
             $deprecations
         );
     }