Skip to content

[Workflow] Deprecate Event::getWorkflow() method #60195

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 14, 2025
Merged

[Workflow] Deprecate Event::getWorkflow() method #60195

merged 1 commit into from
Apr 14, 2025
+90 −4

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Apr 10, 2025 •
edited
Loading

Q A
Branch? 7.3
Bug fix? kind of
New feature? no
Deprecations? yes
Issues Fix #59748
License MIT 
+use Symfony\Component\DependencyInjection\Attribute\Target;
 
 class ContinueToState3 implements EventSubscriberInterface
 {
+    public function __construct(
+        #[Target('your_workflow_name')]
+        private readonly WorkflowInterface $workflow,
+    ) {
+    }
 
     public static function getSubscribedEvents(): array
     {
         return [
             'workflow.your_workflow_name.completed.to_state2' => ['terminateOrder', \PHP_INT_MIN],
         ];
     }
 
     public function terminateOrder(Event $event): void
     {
         $subject = $event->getSubject();
-        if ($event->getWorkflow()->workflow->can($subject, 'to_state3')) {
-            $event->getWorkflow()->apply($subject, 'to_state3');
+        if ($this->workflow->can($subject, 'to_state3')) {
+            $this->workflow->apply($subject, 'to_state3');
         }
     }
 }

If one has a listener able to run on many workflow, it need to inject a locator

   use Symfony\Component\DependencyInjection\ServiceLocator;
   use Symfony\Component\DependencyInjection\Attribute\AutowireLocator;
   use Symfony\Component\Workflow\Attribute\AsTransitionListener;
   use Symfony\Component\Workflow\Event\TransitionEvent;

   class GenericListener
   {
       public function __construct(
           #[AutowireLocator('workflow', 'name')]
           private ServiceLocator $workflows
       ) {
       }

       #[AsTransitionListener()]
       public function doSomething(TransitionEvent $event): void
       {
           $workflow = $this->workflows->get($event->getWorkflowName());
       }
   }

@carsonbot carsonbot added this to the 7.3 milestone Apr 10, 2025
nicolas-grekas
nicolas-grekas reviewed Apr 10, 2025
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like my comment about #[Deprecated] would apply to the few other cases where we use it.
Alternatively, we could ban it from the codebase. It's not better than the annotation anyway.
We should maybe postpone using it to Symfony 8+, where we won't have to duplicate the runtime notice for PHP <8.4.

@lyrixx lyrixx force-pushed the workflow-deprecate-event-get-workflow branch 4 times, most recently from 0dd99b3 to 09c3175 Compare April 10, 2025 14:48
@stof
Copy link
Member

stof commented Apr 10, 2025

Alternatively, we could ban it from the codebase. It's not better than the annotation anyway.

The benefit of the native attribute is that PHP will report the deprecation in the place doing the call, not in the method being deprecated, so it gives a better location for the warning (in our system using trigger_deprecation, identifying the most useful location requires investigating the trace, which is not displayed by default by PHP for warnings IIRC, even though it is included when using symfony/error-handler).
But I agree we can delay its usage in our codebase until Symfony 8.x when we won't need a fallback anymore.

stof
stof reviewed Apr 11, 2025
View reviewed changes
@lyrixx lyrixx force-pushed the workflow-deprecate-event-get-workflow branch from 09c3175 to 5c483fb Compare April 11, 2025 08:51
@lyrixx lyrixx requested a review from stof April 11, 2025 10:16
@lyrixx lyrixx force-pushed the workflow-deprecate-event-get-workflow branch from 5c483fb to 178a01e Compare April 11, 2025 17:52
@ro0NL
Copy link
Contributor

ro0NL commented Apr 11, 2025 •
edited
Loading 

$this->workflows->get($event->getWorkflowName());

im wondering if we can avoid $this and $event mixing to get a workflow, eg. is the deprecation really legit?

cant we fix getWorkflow instead? using the same solution (leveraging the actual DI target object)

@lyrixx
Copy link
Member Author

lyrixx commented Apr 11, 2025

cant we fix getWorkflow instead? using the same solution (leveraging the actual DI target object)

I means injecting a locator in every workflow. IMHO this is not the respectability of the workflow to do that.

I already thought about using a custom event dispatcher, but a bit overkill IMHO.

More over, having a generic listener is really rare, so this use case is really limited

@lyrixx lyrixx force-pushed the workflow-deprecate-event-get-workflow branch from 178a01e to 595711c Compare April 11, 2025 18:04
@lyrixx lyrixx force-pushed the workflow-deprecate-event-get-workflow branch from 595711c to 20fbc9c Compare April 11, 2025 18:06
@nicolas-grekas
Copy link
Member

Thank you @lyrixx.

@nicolas-grekas nicolas-grekas merged commit f2357a0 into symfony:7.3 Apr 14, 2025
6 of 11 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Apr 14, 2025
Closed
@lyrixx lyrixx deleted the workflow-deprecate-event-get-workflow branch April 14, 2025 12:56
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@alexandre-daubois alexandre-daubois alexandre-daubois approved these changes

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Deprecation Status: Needs Review Workflow
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

[Workflow][Profiler] No information from chainned transition
6 participants
@lyrixx @stof @ro0NL @nicolas-grekas @alexandre-daubois @carsonbot