That's a bit strange to me, I'm not sure we ever did that. Usually tag attributes are used in a compiler pass. Shouldn't we do the same here? What's the more extended use case for this?

That's a bit strange to me, I'm not sure we ever did that. Usually tag attributes are used in a compiler pass. Shouldn't we do the same here? What's the more extended use case for this?

It looks like you missed the PR description :D The example show a usage of tag attribute in a compiler pass

And the use case is described in the PR description too. But to go a bit further

class Kernelextends BaseKernelimplements CompilerPassInterface{use MicroKernelTrait;publicfunctionprocess(ContainerBuilder$container)    {$classesToWorkflow = [];foreach ($container->findTaggedServiceIds('workflow.workflow')as$id =>$attributes) {$config =$container->getDefinition($id)->getTag('workflow')[0];foreach ($config['supports'] ?? []as$class) {$classesToWorkflow[$class] =newReference($id);            }        }$container->register(MyRegistry::class, MyRegistry::class)            ->setArguments([$classesToWorkflow])        ;    }}

Note

1. I did not tested this very example
2. Instead of an array of service, it would be better to use a service locator, but that's not the point here

Doesn't your registry duplicate the code we have in theRegistry class? Shouldn't we revert making it internal instead?
I'm challenging this because it feel strange to me to use tags for what you do here (I'm not sure we have any similar example).

I have a big issue with the registry: People use it to get one workflow. In the documentation we used to tell to do :

$workflowRegistry->get($subject)->apply($subject,'foobar');

And this is wrong: people must inject the right Workflow directly (law of demeter, performance, etc)

That's why I decided to make it internal initially.

The documentation has been updated since (I just checked, and there is stillone occurrence, cc@alexandre-daubois)

But there is still one issue. If you have generic controller / service where you cannot know in advance which workflow to use, you have to use a ServiceLocator. There are all tagged, by name, so it should be easy...

BUT, sometime, (cfthis discussion) the name is not enough.

With this PR, people will have full power to build their own registry. It'll be faster, cleaner, and build exactly for each use case.

Even with the current Registry and itsWorkflowSupportStrategyInterface you can't build something totally dynamic, since you miss some information about the configuration

Mouais, I'm not convinced.
The docs tell people the best way.
The Registry is the entry point for more powerful features.
If people want to shoot themselves in the foot, that's life.
So, I don't see why we need to have yet another way. I understand that one is userland and the other is internal, but still. If we're talking about edge cases, just using the current Registry should be enough, right?

I disagree. Registry needs instantiated workflow to work. It means no lazy loading.

Then you can't accesssupports configuration option.

That's two pain points that show the current way to work doesn't work well.

Rebased, and conflict fixed

Hello, This PR is ready and I have rebased it.

@fabpot I understand your concern. But I have a real use case where I need to fetch a workflow dynamically based on some entity properties. We'll have a LOT of workflows - basically one by client (B to B to C), and everyClient lays on the same entity. I could use the registry, but there is no lazy loading, so it will degrade performance.

Finally, the diff is very minimal, and does not really impact the framework. Everything is thrown after the compilation. I don't really how it could hurt 🤓

The issue is that if we start exposing this config on the attribute with the intent of allowing projects to read it, we must provide BC on the structure of this config (with no way at all to ever deprecate anything in it as it is not accessed through a dedicated API in which we can trigger deprecations)

Hmm, Got it 👍🏼 We could document that the shape is not BC?

@lyrixx but then, how would you use it ?

Basically with something like this:

# config/packages/workflow.yamlframework:workflows:my_workflow_name:metadata:features:[repair, wash, dry]

// src/Kernel.phpclass Kernelextends BaseKernelimplements CompilerPassInterface{use MicroKernelTrait;publicfunctionprocess(ContainerBuilder$container)    {$configurations = [];foreach ($container->findTaggedServiceIds('workflow')as$id =>$config) {$configurations[$id] =$config[0]['config'];        }$container            ->getDefinition(Selector::class)            ->setArgument('$configurations',$configurations)        ;    }}

<?php// src/Workflow/Selector.phpnamespaceApp\Workflow;usePsr\Container\ContainerInterface;useSymfony\Component\DependencyInjection\Attribute\TaggedLocator;class Selector{publicfunction__construct(        #[TaggedLocator('workflow')]privatereadonlyContainerInterface$container,privatereadonlyarray$configurations,    ) {    }publicfunctiongetWorkflow(/** Article $article */)    {foreach ($this->configurationsas$id =>$config) {// $features = $article->getWorkflowFeatures();$featuresRequired = ['repair','wash','dry'];sort($featuresRequired);$featuresAvailable =$config['metadata']['features'] ?? [];sort($featuresAvailable);if ($featuresRequired ===$featuresAvailable) {return$this->container->get($id);            }        }thrownew \RuntimeException('No workflow found');    }}

and finally

publicfunction index(Selector$selector,Article$article):Response    {$w =$selector->getWorkflow($article);dd($w);

Would you accept this PR if I inject in the tagonly the workflow metadata? It'll be enough.

Would you accept this PR if I inject in the tagonly the workflow metadata? It'll be enough.

cc@fabpot

PR rebased, and updated