Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Workflow] DeprecateEvent::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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

lyrixx
Copy link
Member

@lyrixxlyrixx commentedApr 10, 2025
edited
Loading

QA
Branch?7.3
Bug fix?kind of
New feature?no
Deprecations?yes
IssuesFix#59748
LicenseMIT
+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

useSymfony\Component\DependencyInjection\ServiceLocator;useSymfony\Component\DependencyInjection\Attribute\AutowireLocator;useSymfony\Component\Workflow\Attribute\AsTransitionListener;useSymfony\Component\Workflow\Event\TransitionEvent;class GenericListener   {publicfunction__construct(           #[AutowireLocator('workflow','name')]privateServiceLocator$workflows       ) {       }       #[AsTransitionListener()]publicfunctiondoSomething(TransitionEvent$event):void       {$workflow =$this->workflows->get($event->getWorkflowName());       }   }

smnandre and OskarStark reacted with thumbs up emoji
Copy link
Member

@nicolas-grekasnicolas-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.

@lyrixxlyrixxforce-pushed theworkflow-deprecate-event-get-workflow branch 4 times, most recently from0dd99b3 to09c3175CompareApril 10, 2025 14:48
@stof
Copy link
Member

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 usingtrigger_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 usingsymfony/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.

lyrixx and GromNaN reacted with thumbs up emoji

@lyrixxlyrixxforce-pushed theworkflow-deprecate-event-get-workflow branch from09c3175 to5c483fbCompareApril 11, 2025 08:51
@lyrixxlyrixx requested a review fromstofApril 11, 2025 10:16
@lyrixxlyrixxforce-pushed theworkflow-deprecate-event-get-workflow branch from5c483fb to178a01eCompareApril 11, 2025 17:52
@ro0NL
Copy link
Contributor

ro0NL commentedApr 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 fixgetWorkflow instead? using the same solution (leveraging the actual DI target object)

@lyrixx
Copy link
MemberAuthor

cant we fixgetWorkflow 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

ro0NL reacted with thumbs up emoji

@lyrixxlyrixxforce-pushed theworkflow-deprecate-event-get-workflow branch from178a01e to595711cCompareApril 11, 2025 18:04
@lyrixxlyrixxforce-pushed theworkflow-deprecate-event-get-workflow branch from595711c to20fbc9cCompareApril 11, 2025 18:06
@nicolas-grekas
Copy link
Member

Thank you@lyrixx.

@nicolas-grekasnicolas-grekas merged commitf2357a0 intosymfony:7.3Apr 14, 2025
6 of 11 checks passed
@lyrixxlyrixx deleted the workflow-deprecate-event-get-workflow branchApril 14, 2025 12:56
@fabpotfabpot mentioned this pull requestMay 2, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@alexandre-dauboisalexandre-dauboisalexandre-daubois approved these changes

@stofstofAwaiting requested review from stof

Assignees
No one assigned
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

[8]ページ先頭

©2009-2025 Movatter.jp