Skip to content

[Messenger] Consume a PSR-14 dispatcher for dispatching events #45967

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

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Apr 7, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets Part of #45963
License MIT
Doc PR N/A

As @Crell said in #45963, requiring the event dispatcher passed to those classes to be an implementation of Symfony's contract was too restrictive. We can easily consume a PSR-14 dispatcher here.

This PR obviously only fixes the low hanging fruit. The commands coming with the messenger component register event subscribers on the dispatcher, in a way that we need the actual EventDispatcher component for.

@carsonbot carsonbot added this to the 5.4 milestone Apr 7, 2022
@derrabus derrabus force-pushed the bugfix/messenger-psr-ed branch 2 times, most recently from 3fcc3d5 to c3eedbf Compare April 7, 2022 08:37
@derrabus derrabus force-pushed the bugfix/messenger-psr-ed branch from c3eedbf to a7794bb Compare April 7, 2022 09:02
@derrabus derrabus force-pushed the bugfix/messenger-psr-ed branch from a7794bb to 5e04bf4 Compare April 7, 2022 10:15
@@ -18,6 +18,7 @@
"require": {
"php": ">=7.2.5",
"psr/log": "^1|^2|^3",
"psr/event-dispatcher": "^1",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically only required if using the queue parts of the library, I think. If it's purely a sync message bus it's not required. That suggests it may be better as a suggests/dev.

PSR packages are small enough that it doesn't really matter, but it's worth mentioning as event-dispatcher wasn't in a first-level require. I'm comfortable either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was, because we could have Symfony's EventDispatcher contracts v1 installed which extended the PSR-14 interface conditionally (= only if it's present), mainly because of PHP 7.1 compatibility (PSR-14 requires PHP 7.2).

I have pushed a new version that should work even in that combination, which allowed me to remove the dependency again.

@derrabus derrabus force-pushed the bugfix/messenger-psr-ed branch from 5e04bf4 to a1213b0 Compare April 7, 2022 21:47
@fabpot
Copy link
Member

fabpot commented Apr 8, 2022

That's a new feature, and as such, it should target 6.1 (we merged the same for Mailer as a new feature as well).

@derrabus derrabus changed the base branch from 5.4 to 6.1 April 8, 2022 08:44
@derrabus derrabus force-pushed the bugfix/messenger-psr-ed branch from f1169bb to 78603d0 Compare April 8, 2022 08:44
@derrabus derrabus added Feature and removed Bug labels Apr 8, 2022
@derrabus
Copy link
Member Author

derrabus commented Apr 8, 2022

That's a new feature, and as such, it should target 6.1 (we merged the same for Mailer as a new feature as well).

All right. That makes it a lot easier. 😅

@derrabus derrabus force-pushed the bugfix/messenger-psr-ed branch from 78603d0 to f500c7a Compare April 8, 2022 08:54
@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 6.1 Apr 11, 2022
@nicolas-grekas
Copy link
Member

Thank you @derrabus.

@nicolas-grekas nicolas-grekas merged commit 8b680f0 into symfony:6.1 Apr 11, 2022
@derrabus derrabus deleted the bugfix/messenger-psr-ed branch April 11, 2022 17:17
@fabpot fabpot mentioned this pull request Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants