Skip to content

[FrameworkBundle][Messenger] Add AsRoutedMessage attribute #49143

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
Closed
wants to merge 1 commit into from
Closed

[FrameworkBundle][Messenger] Add AsRoutedMessage attribute #49143

wants to merge 1 commit into from

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Jan 28, 2023 •
edited
Loading

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets NA
License MIT
Doc PR Todo

This PR add a new #[AsRoutedMessage] attribute. This allows to configure a transport on a message class directly, without having to write this in config files.

It seems that a try has been made in #41179, but unfortunately, there is no news for a few months now 😕. This is an alternate solution using a compile-time solution whereas the existing PR is using a runtime solution.

The attribute can be used as follow:

use Symfony\Component\Messenger\Attribute\AsRoutedMessage;

#[AsRoutedMessage(transports: ['async'])]
class SmsMessage
{
    private $content;

    public function __construct(string $content)
    {
        $this->content = $content;
    }

    public function getContent(): string
    {
        return $this->content;
    }
}

On a behavior side, routings defined thanks to this attribute are merged with the ones defined in the configuration.

Finally, I'm not a 100% sure about the attribute naming. Opinons on it are very welcomed!

@carsonbot carsonbot added this to the 6.3 milestone Jan 28, 2023
@derrabus
Copy link
Member

Thank you. I think, this feature is certainly useful.

What bothers me about the implementation is that we need to register the message classes as services. Given that they should be stupid data containers, having them as services feels wrong. The fact that the pass ignores the service definitions of the message classes entirely shows me that we're abusing the DI container as a descovery mechanism.

@alexandre-daubois
Copy link
Member Author

@derrabus agree. Should I add an exception if the message class is not a defined service? I understand this is not an ideal solution. I guess this type of "abuse" has already been raised and has no "easy" solution for now 😕 ?

@derrabus
Copy link
Member

Should I add an exception if the message class is not a defined service?

Please don't. 🙈

@alexandre-daubois
Copy link
Member Author

Rebased 👍

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@kbond
Copy link
Member

kbond commented Oct 2, 2023

@alexandre-daubois, any thoughts on this concern? I believe it still exists in this PR.

@kbond
Copy link
Member

kbond commented Oct 2, 2023

I'm also not a fan of messages as services. If I understand #51718 correctly, it includes a system to read/parse a set of namespaces for attributes during warmup. Could the same thing be applied here? Could we have a generalized system for this? I've had the need for such a thing myself in the past.

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@fabpot
Copy link
Member

fabpot commented Feb 4, 2024

Closing as we certainly don't want messages as services.

@fabpot fabpot closed this Feb 4, 2024
@alexandre-daubois alexandre-daubois mentioned this pull request Jun 24, 2024
Closed
This was referenced Jun 24, 2024
Merged
Closed
nicolas-grekas added a commit that referenced this pull request Jun 28, 2024
@nicolas-grekas
feature #57507 [Messenger] Introduce #[AsMessage] attribute for mes…
27be27a 
…sage routing (pounard)

This PR was merged into the 7.2 branch.

Discussion
----------

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

| 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:

* #33912 where the discussion started, 5 years ago.
* #49143 closed PR that was doing the same, but at compile time, rejected because the actual doctrine is that messages should never be services.
* #41179 is stilled opened, and awaiting for modifications, but it was written for an earlier version of Symfony and is inactive for a year or so, yet messenger code has evolved since, and this PR will never merge as-is, it requires to be fully rewrote, reason why I opened this new one.

Commits
-------

d652842 feature #57506 [Messenger] introduce AsMessage attribute for message routing
symfony-splitter pushed a commit to symfony/messenger that referenced this pull request Jun 28, 2024
@nicolas-grekas
feature #57507 [Messenger] Introduce #[AsMessage] attribute for mes…
7d6afa4 
…sage routing (pounard)

This PR was merged into the 7.2 branch.

Discussion
----------

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

| 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:

* symfony/symfony#33912 where the discussion started, 5 years ago.
* symfony/symfony#49143 closed PR that was doing the same, but at compile time, rejected because the actual doctrine is that messages should never be services.
* symfony/symfony#41179 is stilled opened, and awaiting for modifications, but it was written for an earlier version of Symfony and is inactive for a year or so, yet messenger code has evolved since, and this PR will never merge as-is, it requires to be fully rewrote, reason why I opened this new one.

Commits
-------

d65284239f feature #57506 [Messenger] introduce AsMessage attribute for message routing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus left review comments

Assignees
No one assigned
Labels
Feature FrameworkBundle Messenger Status: Needs Review
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
6 participants
@alexandre-daubois @derrabus @kbond @fabpot @nicolas-grekas @carsonbot