NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Workflow] Add EventNameTrait to compute event name strings in subscribers#54344

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:7.1fromsqurious:workflow/improve-event-names-dx

Mar 21, 2024

Merged

[Workflow] Add EventNameTrait to compute event name strings in subscribers#54344

fabpot merged 1 commit intosymfony:7.1fromsqurious:workflow/improve-event-names-dx

Mar 21, 2024

Conversation

Copy link

Contributor

squrious commentedMar 20, 2024

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	N/A
License	MIT

Hello!

Using the event dispatcher, we usually use event's class name to configure the event to listen to. For workflow, we
still have to use raw strings:

class WorkflowPostSubscriberimplements EventSubscriberInterface{publicstaticfunctiongetSubscribedEvents():array    {return ['workflow.post.entered.published' =>'onPublishedEntered',                ];    }}

Using class names is more clear about what event we use (easier to know which event to use in the listener). Even if we already have attributes to define event listeners, the event subscriber way could be improved.

Proposal

This PR adds a trait to improve DX when using workflow events in event subscribers.

class WorkflowPostSubscriberimplements EventSubscriberInterface{publicstaticfunctiongetSubscribedEvents():array    {return [            PublishedEvent::get(workflowName:'post', placeName:'entered') =>'onPublishedEntered',                ];    }}

For a better DX, theEventNameTrait provides two methods:getNameForPlace andgetNameForTransition, so the second
argument of::get and its PHPDoc are consistent with the event type.

In event classes, it is used like:

class EnterEventextends Event{use EventNameTrait {        use getNameForPlaceaspublic get;    }// ...}

Cheers!

squrious requested a review fromlyrixx as acode owner

March 20, 2024 09:00

carsonbot added Status: Needs Review Feature Workflow labels

Mar 20, 2024

carsonbot added this to the7.1 milestone

Mar 20, 2024

squrious force-pushed theworkflow/improve-event-names-dx branch from62740c8 to3fb63f1Compare

March 20, 2024 09:02

stof reviewed

Mar 20, 2024

View reviewed changes

src/Symfony/Component/Workflow/Event/EventNameTrait.phpShow resolvedHide resolved

src/Symfony/Component/Workflow/Event/EventNameTrait.php OutdatedShow resolvedHide resolved

Copy link

Member

lyrixx commentedMar 20, 2024

Did you see#51210?

Copy link

ContributorAuthor

squrious commentedMar 20, 2024

Did you see#51210?

Yes! But as event subscribers allow to register events too, shouldn't we take care of their DX too? Or maybe I could leverage the existing code for this?

Copy link

Member

lyrixx commentedMar 20, 2024

IMHO, event listener are better thank event subscriber now.

But I think we can do something for the subscriber too. I'll review it

lyrixx reviewed

Mar 20, 2024

View reviewed changes

src/Symfony/Component/Workflow/Event/EventNameTrait.php OutdatedShow resolvedHide resolved

Copy link

Member

derrabus commentedMar 20, 2024

But as event subscribers allow to register events too, shouldn't we take care of their DX too?

No. I'd rather get rid of subscribers, tbh. From my POV, the only reason subscribers still exist, is the amount of busy work we would force upon downstream code if we removed them. Regarding DX improvements, I'd keep our efforts focused on attribute-based listeners.

Copy link

Member

lyrixx commentedMar 20, 2024

@derrabus with SF full stack, it's easy to register listener, but without the framework, it's harder. I think it's still better to merge this PR once ready