-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[Workflow] Document the fact workflows are tagged #21429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Pinging @lyrixx in case he has some time to review this change. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of this PR.
Workflows can also be injected thanks to their name and the | ||
:class:`Symfony\\Component\\DependencyInjection\\Attribute\\Target` | ||
attribute:: | ||
**(2) Use the ``#[Target]`` attribute** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO the target is much much better. What about putting this one before the "Use a specific argument name" ?
workflow.rst
Outdated
class MyClass | ||
{ | ||
public function __construct( | ||
#[AutowireLocator('blog_publishing', 'user_registration')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 This does not work, does't it?
According to the PHPDoc :
1. A tag name or an explicit list of service ids
2. The name of the attribute that defines the key referencing each service in the locator
You can use the full service name: <workflow_type>.<worflow_name>
. Example state_machine.blog_publishing
(Note: I did not check if it's a state machine or a workflow, in your example)
But this this is kinda useless IMHO.
The whole point it to inject them all, and get lazy loading:
public function __construct(
#[AutowireLocator('workflow')] // <-- this is not the workflow name, but the tag name
private ServiceLocator $workflows,
) {
}
#[Route(path: '/foobar')]
public function foobar()
{
dd($this->workflows->get('workflow.article')); // <-- this is the service ID. See above
}
But, IMHO, this is a bit boring to prefix by workflow.
or state_machine.
That's why I prefer:
public function __construct(
// This is the tag name
// / This is index attribute, see https://github.com/symfony/symfony/blob/74fc896a8e13930a27037f7c97c76ca353ae1a51/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1201-L1202
// / /
#[AutowireLocator('workflow', 'name')]
private ServiceLocator $workflows,
) {
}
#[Route(path: '/foobar')]
public function foobar()
{
dd($this->workflows->get('article')); // Now this is the workflow name (without any prefix)
dd($this->workflows->get('task')); // Same same
}
And, if you wan to get only workflow, or only state machines:
public function __construct(
#[AutowireLocator('workflow.workflow', 'name')]
private ServiceLocator $workflows,
#[AutowireLocator('workflow.state_machine', 'name')]
private ServiceLocator $stateMachines,
) {
}
#[Route(path: '/foobar')]
public function foobar()
{
dd($this->workflows, $this->stateMachines);
}

All my examples work, and have been tested with https://github.com/lyrixx/SFLive-Paris2016-Workflow ; I also tested all feature, and they are available in 6.4 too
EDITS
- I'm not sure it actually works. I'm investigating !
- it do works on 6.4, but not on 7.4. So there is a bug somewhere. Will fix it!
- okay, no bugs. I need ALL the app to be in 7.4 (even the DIC to get symfony/symfony@8b79ff5)
Grég, thanks for such a fantastic review 🙇 I made all the changes and fixes that you pointed out. Thanks! |
ac95e29
to
fd05b83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍🏼
I still prefer the "target" option over the "specific argument name", but it's good to me
Yes, Let's do something. Let's change this in 8.0 branch after the Symfony 8.0 release and let's show only the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
Thanks for taking care of this PR!
d9d3f3f
to
809fb8f
Compare
Fixes #20876.
This makes the same changes we did in #21382 when explaining the
#[Target]
attribute and then it addresses the issue explained in #20876.