Skip to content

[Messenger] Introduce #[AsMessage] attribute for message routing #57507

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
Jun 28, 2024
Merged

[Messenger] Introduce #[AsMessage] attribute for message routing #57507

merged 1 commit into from
Jun 28, 2024

Conversation

pounard
Copy link
Contributor

@pounard pounard commented Jun 24, 2024 •
edited by nicolas-grekas
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #57506
License MIT

Basic implementation of #57506.

  • Adds the Symfony\Component\Messenger\Attribute\AsMessage attribute, with the $transport parameter which can be string or array.
  • Implement runtime routing in Symfony\Component\Messenger\Transport\Sender\SendersLocator.

Rationale:

  • Messages are not services, it cannot be computed during container compilation.
  • Reflection is very fast, it shouldn't be significant for performances, yet I don't have measured it yet.
  • YAML configuration and Symfony\Component\Messenger\Stamp\TransportNamesStamp will always override the attribute values, allowing users to change hardcoded routing on a per-environment basis.
  • This is the simplest implementation I could think of for discussion.

Links and references:

@carsonbot carsonbot added this to the 7.2 milestone Jun 24, 2024
@carsonbot carsonbot changed the title feature #57506 [Messenger] introduce AsMessage attribute for message … [Messenger] feature #57506 introduce AsMessage attribute for message … Jun 24, 2024
@pounard pounard changed the title [Messenger] feature #57506 introduce AsMessage attribute for message … [Messenger] feature #57506 introduce #[AsMessage] attribute for message … Jun 24, 2024
@pounard
Copy link
Contributor Author

pounard commented Jun 24, 2024

All tests failures seems unrelated, and seem happen in conjunction with some Doctrine ORM versions.

@nicolas-grekas nicolas-grekas changed the title [Messenger] feature #57506 introduce #[AsMessage] attribute for message … [Messenger] Introduce #[AsMessage] attribute for message routing Jun 24, 2024
nicolas-grekas
nicolas-grekas reviewed Jun 24, 2024
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.

Works for me, here are some minor comments.

@pounard pounard force-pushed the 57506-messenger-as-message branch 2 times, most recently from a024c4c to 0e42fda Compare June 24, 2024 18:57
@pounard
Copy link
Contributor Author

pounard commented Jun 24, 2024 •
edited
Loading

@nicolas-grekas @yceruto all done, thanks for the review.

@pounard
Copy link
Contributor Author

pounard commented Jun 24, 2024 •
edited
Loading

Same unrelated failures again in Symfony\Bridge\Doctrine\Tests\ManagerRegistryTest.

@pounard pounard mentioned this pull request Jun 24, 2024
Closed
@OskarStark OskarStark changed the title [Messenger] Introduce #[AsMessage] attribute for message routing [Messenger] Introduce #[AsMessage] attribute for message routing Jun 25, 2024
nicolas-grekas
nicolas-grekas reviewed Jun 25, 2024
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.

small rebase needed

@pounard pounard force-pushed the 57506-messenger-as-message branch from 3ad89ad to d652842 Compare June 25, 2024 08:26
@pounard
Copy link
Contributor Author

pounard commented Jun 25, 2024

@nicolas-grekas

small rebase needed

Did it, CI failures are still related to doctrine.

nicolas-grekas
nicolas-grekas approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your answers. Works for me.

@pounard
Copy link
Contributor Author

pounard commented Jun 25, 2024

Thanks for your answers. Works for me.

I'd like to thank you for your time, I appreciate it very much.

@pounard
Copy link
Contributor Author

pounard commented Jun 26, 2024 •
edited
Loading

@nicolas-grekas Will this be merged? I need to know in order to continue experimenting/proposing new features for the messenger.

@nicolas-grekas
Copy link
Member

I'd like to wait a bit in case anyone else would like to review, but eventually I think it should be merged yes.

@pounard
Copy link
Contributor Author

pounard commented Jun 26, 2024

@nicolas-grekas in the meantime, I'd like to have your global impressions about #57536 if you can find a few minutes for it.

kbond
kbond reviewed Jun 26, 2024
View reviewed changes
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

I like it!

The attributes are retrieved at runtime. As questioned in #41179 (comment), is this a problem?

@pounard
Copy link
Contributor Author

pounard commented Jun 26, 2024 •
edited
Loading

The attributes are retrieved at runtime. As questioned in #41179 (comment), is this a problem?

I don't think so, reflection is fast and it probably will be insignificant compared to the command execution itself.

EDIT: here we are at dispatch, so it actually will not be executed, yet it is still way faster than the database insert that will be issued if the transport is doctrine-based, or anyother tcp round-trip if it is another type of broker.

@kbond
Copy link
Member

kbond commented Jun 26, 2024

I don't think so, reflection is fast and it probably will be insignificant compared to the command execution itself.

I think this also but wanted to confirm.

@nicolas-grekas
Copy link
Member

Thank you @pounard.

@nicolas-grekas nicolas-grekas merged commit 27be27a into symfony:7.2 Jun 28, 2024
5 of 10 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Jun 28, 2024
Closed
@pounard
Copy link
Contributor Author

pounard commented Jun 28, 2024

Thanks!

@pounard pounard mentioned this pull request Jun 28, 2024
Merged
@valtzu valtzu mentioned this pull request Aug 4, 2024
Closed
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 16, 2024
@javiereguiluz
minor #20003 [Messenger] Document #[AsMessage] attribute (pounard)
ffcb916 
This PR was merged into the 7.2 branch.

Discussion
----------

[Messenger] Document `#[AsMessage]` attribute

Fixes #20002

Document the newly merged `#[AsMessage]` attribute symfony/symfony#57507

Commits
-------

a381536 [Messenger] document the #[AsMessage] attribute
@fabpot fabpot mentioned this pull request Oct 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbond kbond kbond approved these changes

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

@yceruto yceruto yceruto approved these changes

Assignees
No one assigned
Labels
Feature Messenger Status: Reviewed
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.

[Messenger] Add the #[AsMessage] attribute
5 participants
@pounard @nicolas-grekas @kbond @yceruto @carsonbot